Refactor to send iframe resize messages directly from layout thread to constellation #15129

cynicaldevil · 2017-01-20T19:45:12Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14682.

r? @jdm
passing tests:
tests/wpt/mozilla/tests/css/matchMedia.html, tests/wpt/mozilla/tests/mozilla/window_resize_not_triggered_on_load.html, tests/wpt/mozilla/tests/mozilla/iframe/resize_after_load.html, tests/wpt/mozilla/tests/css/meta_viewport_resize.html

This change is

highfive · 2017-01-20T19:45:16Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/constellation.rs
@fitzgen: components/script_traits/lib.rs, components/script_traits/lib.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs
@KiChjang: components/script_traits/lib.rs, components/script_traits/lib.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs

cynicaldevil · 2017-01-20T19:49:30Z

components/layout_thread/lib.rs

+                    DisplayItem::Iframe(ref iframe) => {
+                        let size = Size2D::new((*item).bounds().size.width.to_f32_px(),
+                                               (*item).bounds().size.height.to_f32_px());
+                        debug!("new iframe size: {:?}", size);


Left this debug message because I thought it would be helpful, but can remove if its unneeded.

Rather than iterating over the complete display list every time we build one, we should send this message when we create an iframe display item instead: https://dxr.mozilla.org/servo/rev/1f76aa6ef7ec7db0a0c40223874a72e8f4059deb/components/layout/display_list_builder.rs#1441

In fact, we may want to consider batching up all of the iframe sizes that are discovered and sending them in a single message once the display list has been created, rather than one by one.

heh, I had thought of this but forgot all about it when I couldn't get those tests to pass....

jdm

Great work! @jrmuizel will be pleased by this change.

jdm · 2017-01-20T20:17:18Z

components/constellation/constellation.rs

@@ -1089,6 +1083,12 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
            FromLayoutMsg::ChangeRunningAnimationsState(pipeline_id, animation_state) => {
                self.handle_change_running_animations_state(pipeline_id, animation_state)
            }
+            // The compositor discovered the size of a subframe. This needs to be reflected by all


s/The compositor/Layout/

jdm · 2017-01-20T20:17:18Z

components/layout_thread/lib.rs

+                    DisplayItem::Iframe(ref iframe) => {
+                        let size = Size2D::new((*item).bounds().size.width.to_f32_px(),
+                                               (*item).bounds().size.height.to_f32_px());
+                        debug!("new iframe size: {:?}", size);


Rather than iterating over the complete display list every time we build one, we should send this message when we create an iframe display item instead: https://dxr.mozilla.org/servo/rev/1f76aa6ef7ec7db0a0c40223874a72e8f4059deb/components/layout/display_list_builder.rs#1441

jdm · 2017-01-20T20:17:18Z

components/layout_thread/lib.rs

+                            warn!("Layout resize to constellation failed ({}).", e);
+                        }
+                    },
+                    _ => debug!("other!"),


This looks unnecessary :)

jdm · 2017-01-20T20:17:18Z

components/constellation/constellation.rs

-            // early exit if that occurs.
-            match self.pipelines.get_mut(&pipeline_id) {
-                Some(pipeline) => {
+        let result = match self.pipelines.get_mut(&pipeline_id) {


let result = self.pipelines.get_mut(&pipeline_id).map(|pipeline| {

The compiler complains about having mutably borrowed self outside the closure if I use this. Besides, I can't use map here because result can be None even if pipeline is resolved, in the case where sizes are equal.

jdm · 2017-01-20T20:17:18Z

components/script_traits/script_msg.rs

@@ -32,6 +32,8 @@ use style_traits::viewport::ViewportConstraints;
 pub enum LayoutMsg {
    /// Indicates whether this pipeline is currently running animations.
    ChangeRunningAnimationsState(PipelineId, AnimationState),
+    /// Inform the constellation of the size of the viewport.


"size of the pipeline's viewport"

jdm · 2017-01-20T20:17:18Z

components/layout_thread/lib.rs

+                    DisplayItem::Iframe(ref iframe) => {
+                        let size = Size2D::new((*item).bounds().size.width.to_f32_px(),
+                                               (*item).bounds().size.height.to_f32_px());
+                        debug!("new iframe size: {:?}", size);


In fact, we may want to consider batching up all of the iframe sizes that are discovered and sending them in a single message once the display list has been created, rather than one by one.

jrmuizel · 2017-01-20T20:19:21Z

Indeed this will allow a nice cleanup.

cynicaldevil · 2017-01-22T14:13:09Z

@jdm Added new changes.

emilio · 2017-01-22T14:25:11Z

components/constellation/constellation.rs

            // frame trees in the navigation context containing the subframe.
-            FromLayoutMsg::FrameSize(pipeline_id, size) => {
+            FromLayoutMsg::FrameSize(iframe_sizes) => {


nit: rename to FrameSizes?

emilio · 2017-01-22T14:25:11Z

components/constellation/constellation.rs

-                } else {
-                    None
+                             iframe_sizes: Vec<(PipelineId, Size2D<f32>)>) {
+        for size_data in iframe_sizes {


nit: for (pipeline_id, untyped_size) in frame sizes.

Also, could we send the TypedSize2D through IPC directly? if not, why not?

emilio · 2017-01-22T14:25:11Z

components/layout_thread/lib.rs

-                        if let Err(e) = self.constellation_chan.send(msg) {
-                            warn!("Layout resize to constellation failed ({}).", e);
-                        }
+                        iframe_sizes.push(((*iframe).iframe, size));


This is still walking all the display list instead of doing it at display list building time. You should collect the sizes in build_fragment_type_specific_display_items in components/layout/display_list_builder.rs, under the Iframe branch.

For that, you probably want to add the vector of iframe sizes to DisplayListBuildState (in that same file), and send the sizes to the constellation once you're done building the display list.

emilio · 2017-01-22T14:25:11Z

components/layout_thread/lib.rs

+                }
+            }
+
+            if iframe_sizes.len() > 0 {


nit: if !iframe_sizes.is_empty().

cynicaldevil · 2017-01-22T21:26:10Z

@emilio Fixed.

emilio

The rest looks great to me, thanks for doing this @cynicaldevil! \o/

emilio · 2017-01-22T23:39:38Z

components/layout/display_list_builder.rs

@@ -105,6 +107,7 @@ pub struct DisplayListBuildState<'a> {
    /// The current scroll root id, used to keep track of state when
    /// recursively building and processing the display list.
    pub current_scroll_root_id: ScrollRootId,
+    pub iframe_sizes: Vec<(PipelineId, TypedSize2D<f32, PagePx>)>,


Can you add a comment here, in the lines of:

/// The collection of sizes of iframe display items, needed to inform the constellation about iframe resizes.

emilio · 2017-01-22T23:39:38Z

components/layout/display_list_builder.rs

@@ -1463,6 +1467,10 @@ impl FragmentDisplayListBuilding for Fragment {
                        iframe: fragment_info.pipeline_id,
                    });

+                    let size = Size2D::new((item).bounds().size.width.to_f32_px(),


nit: I believe you don't need the parenthesis around item, do you?

emilio · 2017-01-22T23:39:38Z

components/layout_thread/lib.rs

@@ -934,6 +934,14 @@ impl LayoutThread {
                        let origin = Rect::new(Point2D::new(Au(0), Au(0)), root_size);
                        build_state.root_stacking_context.bounds = origin;
                        build_state.root_stacking_context.overflow = origin;
+
+                        if !build_state.iframe_sizes.is_empty() {
+                            let msg = ConstellationMsg::FrameSizes(build_state.iframe_sizes.clone());


nit: Since the iframe sizes are not going to be useful, you can prevent the extra allocation doing the following:

let iframe_sizes = mem::replace(&mut build_state.iframe_sizes, vec![]); let msg = ConstellationMsg::FrameSizes(iframe_sizes);

You can also make that first line a method in DisplayListBuildState if you want, something along the lines of:

fn take_iframe_sizes(&mut self) -> Vec<..> { mem::replace(..) }

But given it's only used here, it's not a big deal.

emilio · 2017-01-22T23:39:56Z

@bors-servo try

bors-servo · 2017-01-22T23:40:00Z

⌛ Trying commit 5feba01 with merge 0e088be...


        Auto merge of #15129 - cynicaldevil:iframe-resize, r=<try>

@jdm

Refactor to send iframe resize messages directly from layout thread to constellation  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14682.  r? @jdm passing tests: tests/wpt/mozilla/tests/css/matchMedia.html, tests/wpt/mozilla/tests/mozilla/window_resize_not_triggered_on_load.html, tests/wpt/mozilla/tests/mozilla/iframe/resize_after_load.html, tests/wpt/mozilla/tests/css/meta_viewport_resize.html  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15129)

emilio · 2017-01-22T23:40:30Z

Also, please do squash all the commits together :)

bors-servo · 2017-01-23T00:34:27Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev


        frame-resize refactor

cynicaldevil · 2017-01-23T13:47:04Z

Fixed and squashed the commits!

emilio · 2017-01-23T14:07:42Z

components/constellation/constellation.rs

-                None => return,
+                None => None
+            };
+            if let Some(Err(e)) = result {


We could handle this below send, and early return in the None case so this looks a bit clearer, but not a big deal.

That's actually not possible, the compiler complains about self being mutably borrowed in the match line; because self needs to be mutably borrowed again for handle_send_error. I had to resort to using this method then :)

Yeah, but early-returning should make this more legible anyway :P

emilio · 2017-01-23T14:07:59Z

@bors-servo r+

bors-servo · 2017-01-23T14:08:00Z

📌 Commit bca565a has been approved by emilio

bors-servo · 2017-01-23T14:08:04Z

⌛ Testing commit bca565a with merge 7e2329e...


        Auto merge of #15129 - cynicaldevil:iframe-resize, r=emilio

@jdm

Refactor to send iframe resize messages directly from layout thread to constellation  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14682.  r? @jdm passing tests: tests/wpt/mozilla/tests/css/matchMedia.html, tests/wpt/mozilla/tests/mozilla/window_resize_not_triggered_on_load.html, tests/wpt/mozilla/tests/mozilla/iframe/resize_after_load.html, tests/wpt/mozilla/tests/css/meta_viewport_resize.html  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15129)

bors-servo · 2017-01-23T14:53:25Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev

jrmuizel · 2017-01-23T15:47:08Z

@cynicaldevil Thanks for doing this work!


        constellation: Cleanup the frame size handler.

This is a followup to servo#15129, addressing my last review comment.


        Auto merge of #15155 - emilio:frame-size-cleanup, r=cbrewster

constellation: Cleanup the frame size handler. This is a followup to #15129, addressing my last review comment. r? anyone  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15155)


        Auto merge of #15164 - mbrubeck:revert, r=emilio

@aneeshusa

Revert several changes that broke tests This is based on #15158 by @aneeshusa, with additional reverts. This reverts #15064, which is causing many tests not to run, and #15129 and #15155 which landed while tests were not running and may have caused some new failures in iframe tests.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15164)


        Auto merge of #15164 - mbrubeck:revert, r=emilio

@aneeshusa

Revert several changes that broke tests This is based on #15158 by @aneeshusa, with additional reverts. This reverts #15064, which is causing many tests not to run, and #15129 and #15155 which landed while tests were not running and may have caused some new failures in iframe tests.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15164)


        Auto merge of #15164 - mbrubeck:revert, r=emilio

@aneeshusa

Revert several changes that broke tests This is based on #15158 by @aneeshusa, with additional reverts. This reverts #15064, which is causing many tests not to run, and #15129 and #15155 which landed while tests were not running and may have caused some new failures in iframe tests.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15164)

highfive assigned jdm Jan 20, 2017

highfive added the S-awaiting-review label Jan 20, 2017

cynicaldevil reviewed Jan 20, 2017

View changes

jdm requested changes Jan 20, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Jan 20, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 22, 2017

emilio requested changes Jan 22, 2017

View changes

cynicaldevil force-pushed the cynicaldevil:iframe-resize branch from b5d95ec to 5feba01 Jan 22, 2017

emilio reviewed Jan 22, 2017

View changes

cbrewster added the S-needs-squash label Jan 23, 2017

jdm added S-needs-code-changes and removed S-awaiting-review labels Jan 23, 2017

frame-resize refactor

Loading status checks…

bca565a

cynicaldevil force-pushed the cynicaldevil:iframe-resize branch from 5feba01 to bca565a Jan 23, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 23, 2017

emilio reviewed Jan 23, 2017

View changes

highfive assigned emilio and unassigned jdm Jan 23, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Jan 23, 2017

bors-servo merged commit bca565a into servo:master Jan 23, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jan 23, 2017

cynicaldevil deleted the cynicaldevil:iframe-resize branch Jan 23, 2017

jdm mentioned this pull request Jan 23, 2017

Remove pipeline_size_changed callback servo/webrender#766

Closed

emilio mentioned this pull request Jan 23, 2017

constellation: Cleanup the frame size handler. #15155

Merged

This was referenced Jan 23, 2017

Revert "Remove -w flag now that WebRender is always used" #15158

Closed

Revert several changes that broke tests #15164

Merged

jdm mentioned this pull request Jan 24, 2017

Figure out if Servo can just communicate iframe resizes to itself instead of relying on WebRender #14682

Closed

mbrubeck mentioned this pull request Jan 24, 2017

Re revert the backed out PRs #15186

Merged

jdm mentioned this pull request Jun 7, 2017

Intermittent crash in /html/semantics/embedded-content/the-iframe-element/change_parentage.html #17208

Open

servo / servo

Join GitHub today

Refactor to send iframe resize messages directly from layout thread to constellation #15129

Refactor to send iframe resize messages directly from layout thread to constellation #15129

Conversation

cynicaldevil commented Jan 20, 2017 • edited by larsbergstrom

highfive commented Jan 20, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jrmuizel commented Jan 20, 2017

cynicaldevil commented Jan 22, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

cynicaldevil commented Jan 22, 2017

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio commented Jan 22, 2017

bors-servo commented Jan 22, 2017

emilio commented Jan 22, 2017

bors-servo commented Jan 23, 2017

cynicaldevil commented Jan 23, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

jrmuizel commented Jan 23, 2017

cynicaldevil commented Jan 20, 2017 •

edited by larsbergstrom