Auto merge of #9421 - jdm:iframe-painting-after-navigation-redux, r=jdm

compositing: Fix a couple of bugs that prevented iframes from painting after navigation.

The first bug was that iframes were not reflowed in their parent DOM when the child page navigated. This is fixed by simply having the constellation notify the appropriate script thread when navigation occurs.

The second bug was that the compositor was unable to adjust the pipeline for existing iframe layers, only new ones. This patch adds logic to do that.

The third bug was that we have ad-hoc reflow calls throughout script/, and we didn't trigger any reflow from the code that dispatches the `load` event for the iframe so the test for the first two issues would always time out. The second commit adds another reflow call to do that, and also bites the bullet and adds a catch-all reflow (which does nothing if there's no dirty nodes in the document) at the return to the event loop.

Closes #8081.

Extension of #9285.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9421)
<!-- Reviewable:end -->

components/compositing/compositor.rs

@@ -790,6 +790,13 @@ impl<Window: WindowMethods> IOCompositor<Window> {
 
         match self.find_layer_with_pipeline_and_layer_id(pipeline_id, properties.id) {
             Some(existing_layer) => {
+                // If this layer contains a subpage, then create the root layer for that subpage
+                // now.
+                if properties.subpage_pipeline_id.is_some() {
+                    self.create_root_layer_for_subpage_if_necessary(properties,
+                                                                    existing_layer.clone())
+                }
+
                 existing_layer.update_layer(properties);
                 true
             }
@@ -868,32 +875,9 @@ impl<Window: WindowMethods> IOCompositor<Window> {
             }
 
             // If this layer contains a subpage, then create the root layer for that subpage now.
-            if let Some(ref subpage_pipeline_id) = layer_properties.subpage_pipeline_id {
-                let subpage_layer_properties = LayerProperties {
-                    id: LayerId::null(),
-                    parent_id: None,
-                    rect: Rect::new(Point2D::zero(), layer_properties.rect.size),
-                    background_color: layer_properties.background_color,
-                    scroll_policy: ScrollPolicy::Scrollable,
-                    transform: Matrix4::identity(),
-                    perspective: Matrix4::identity(),
-                    subpage_pipeline_id: layer_properties.subpage_pipeline_id,
-                    establishes_3d_context: true,
-                    scrolls_overflow_area: true,
-                };
-
-                let wants_scroll_events = if subpage_layer_properties.scrolls_overflow_area {
-                    WantsScrollEventsFlag::WantsScrollEvents
-                } else {
-                    WantsScrollEventsFlag::DoesntWantScrollEvents
-                };
-                let subpage_layer = CompositorData::new_layer(*subpage_pipeline_id,
-                                                              subpage_layer_properties,
-                                                              wants_scroll_events,
-                                                              new_layer.tile_size);
-                *subpage_layer.masks_to_bounds.borrow_mut() = true;
-                new_layer.add_child(subpage_layer);
-                self.pending_subpages.insert(*subpage_pipeline_id);
+            if layer_properties.subpage_pipeline_id.is_some() {
+                self.create_root_layer_for_subpage_if_necessary(layer_properties,
+                                                                new_layer.clone())
             }
 
             parent_layer.add_child(new_layer.clone());
@@ -902,6 +886,46 @@ impl<Window: WindowMethods> IOCompositor<Window> {
         self.dump_layer_tree();
     }
 
+    fn create_root_layer_for_subpage_if_necessary(&mut self,
+                                                  layer_properties: LayerProperties,
+                                                  parent_layer: Rc<Layer<CompositorData>>) {
+        if parent_layer.children
+                       .borrow()
+                       .iter()
+                       .any(|child| child.extra_data.borrow().subpage_info.is_some()) {
+            return
+        }
+
+        let subpage_pipeline_id =
+            layer_properties.subpage_pipeline_id
+                            .expect("create_root_layer_for_subpage() called for non-subpage?!");
+        let subpage_layer_properties = LayerProperties {
+            id: LayerId::null(),
+            parent_id: None,
+            rect: Rect::new(Point2D::zero(), layer_properties.rect.size),
+            background_color: layer_properties.background_color,
+            scroll_policy: ScrollPolicy::Scrollable,
+            transform: Matrix4::identity(),
+            perspective: Matrix4::identity(),
+            subpage_pipeline_id: Some(subpage_pipeline_id),
+            establishes_3d_context: true,
+            scrolls_overflow_area: true,
+        };
+
+        let wants_scroll_events = if subpage_layer_properties.scrolls_overflow_area {
+            WantsScrollEventsFlag::WantsScrollEvents
+        } else {
+            WantsScrollEventsFlag::DoesntWantScrollEvents
+        };
+        let subpage_layer = CompositorData::new_layer(subpage_pipeline_id,
+                                                      subpage_layer_properties,
+                                                      wants_scroll_events,
+                                                      parent_layer.tile_size);
+        *subpage_layer.masks_to_bounds.borrow_mut() = true;
+        parent_layer.add_child(subpage_layer);
+        self.pending_subpages.insert(subpage_pipeline_id);
+    }
+
     fn send_window_size(&self) {
         let dppx = self.page_zoom * self.device_pixels_per_screen_px();
         let initial_viewport = self.window_size.as_f32() / dppx;

components/compositing/compositor_layer.rs

@@ -211,6 +211,7 @@ pub enum ScrollEventResult {
 impl CompositorLayer for Layer<CompositorData> {
     fn update_layer_except_bounds(&self, layer_properties: LayerProperties) {
         self.extra_data.borrow_mut().scroll_policy = layer_properties.scroll_policy;
+        self.extra_data.borrow_mut().subpage_info = layer_properties.subpage_pipeline_id;
         *self.transform.borrow_mut() = layer_properties.transform;
         *self.perspective.borrow_mut() = layer_properties.perspective;
 

components/compositing/constellation.rs

@@ -946,6 +946,14 @@ impl<LTF: LayoutThreadFactory, STF: ScriptThreadFactory> Constellation<LTF, STF>
                     }
                 }
 
+                if !self.pipeline_is_in_current_frame(source_id) {
+                    // Disregard this load if the navigating pipeline is not actually
+                    // active. This could be caused by a delayed navigation (eg. from
+                    // a timer) or a race between multiple navigations (such as an
+                    // onclick handler on an anchor element).
+                    return None;
+                }
+
                 self.handle_load_start_msg(&source_id);
                 // Being here means either there are no pending frames, or none of the pending
                 // changes would be overridden by changing the subframe associated with source_id.
@@ -1325,6 +1333,17 @@ impl<LTF: LayoutThreadFactory, STF: ScriptThreadFactory> Constellation<LTF, STF>
     fn handle_activate_document_msg(&mut self, pipeline_id: PipelineId) {
         debug!("Document ready to activate {:?}", pipeline_id);
 
+        if let Some(ref child_pipeline) = self.pipelines.get(&pipeline_id) {
+            if let Some(ref parent_info) = child_pipeline.parent_info {
+                if let Some(parent_pipeline) = self.pipelines.get(&parent_info.0) {
+                    let _ = parent_pipeline.script_chan
+                                           .send(ConstellationControlMsg::FramedContentChanged(
+                            parent_info.0,
+                            parent_info.1));
+                }
+            }
+        }
+
         // If this pipeline is already part of the current frame tree,
         // we don't need to do anything.
         if self.pipeline_is_in_current_frame(pipeline_id) {

components/script/dom/htmliframeelement.rs

@@ -23,9 +23,10 @@ use dom::htmlelement::HTMLElement;
 use dom::node::{Node, UnbindContext, window_from_node};
 use dom::urlhelper::UrlHelper;
 use dom::virtualmethods::VirtualMethods;
-use dom::window::Window;
+use dom::window::{ReflowReason, Window};
 use js::jsapi::{JSAutoCompartment, JSAutoRequest, RootedValue, JSContext, MutableHandleValue};
 use js::jsval::{UndefinedValue, NullValue};
+use layout_interface::ReflowQueryType;
 use msg::constellation_msg::{ConstellationChan};
 use msg::constellation_msg::{NavigationDirection, PipelineId, SubpageId};
 use page::IterablePage;
@@ -34,6 +35,7 @@ use script_traits::{IFrameLoadInfo, MozBrowserEvent, ScriptMsg as ConstellationM
 use std::ascii::AsciiExt;
 use std::cell::Cell;
 use string_cache::Atom;
+use style::context::ReflowGoal;
 use url::Url;
 use util::prefs;
 use util::str::{DOMString, LengthOrPercentageOrAuto};
@@ -211,6 +213,10 @@ impl HTMLIFrameElement {
         let window = window_from_node(self);
         self.upcast::<EventTarget>().fire_simple_event("load", GlobalRef::Window(window.r()));
         // TODO Step 5 - unset child document `mut iframe load` flag
+
+        window.reflow(ReflowGoal::ForDisplay,
+                      ReflowQueryType::NoQuery,
+                      ReflowReason::IFrameLoadEvent);
     }
 }
 

components/script/dom/window.rs

@@ -107,6 +107,9 @@ pub enum ReflowReason {
     ImageLoaded,
     RequestAnimationFrame,
     WebFontLoaded,
+    FramedContentChanged,
+    IFrameLoadEvent,
+    MissingExplicitReflow,
 }
 
 pub type ScrollPoint = Point2D<Au>;
@@ -1425,6 +1428,9 @@ fn debug_reflow_events(goal: &ReflowGoal, query_type: &ReflowQueryType, reason:
         ReflowReason::ImageLoaded => "\tImageLoaded",
         ReflowReason::RequestAnimationFrame => "\tRequestAnimationFrame",
         ReflowReason::WebFontLoaded => "\tWebFontLoaded",
+        ReflowReason::FramedContentChanged => "\tFramedContentChanged",
+        ReflowReason::IFrameLoadEvent => "\tIFrameLoadEvent",
+        ReflowReason::MissingExplicitReflow => "\tMissingExplicitReflow",
     });
 
     println!("{}", debug_msg);

components/script/script_thread.rs

@@ -1028,6 +1028,14 @@ impl ScriptThread {
                     window.reflow(ReflowGoal::ForDisplay,
                                   ReflowQueryType::NoQuery,
                                   ReflowReason::ImageLoaded);
+                } else {
+                    // Reflow currently happens when explicitly invoked by code that
+                    // knows the document could have been modified. This should really
+                    // be driven by the compositor on an as-needed basis instead, to
+                    // minimize unnecessary work.
+                    window.reflow(ReflowGoal::ForDisplay,
+                                  ReflowQueryType::NoQuery,
+                                  ReflowReason::MissingExplicitReflow);
                 }
             }
         }
@@ -1131,6 +1139,8 @@ impl ScriptThread {
             ConstellationControlMsg::DispatchFrameLoadEvent {
                 target: pipeline_id, parent: containing_id } =>
                 self.handle_frame_load_event(containing_id, pipeline_id),
+            ConstellationControlMsg::FramedContentChanged(containing_pipeline_id, subpage_id) =>
+                self.handle_framed_content_changed(containing_pipeline_id, subpage_id),
             ConstellationControlMsg::ReportCSSError(pipeline_id, filename, line, column, msg) =>
                 self.handle_css_error_reporting(pipeline_id, filename, line, column, msg),
         }
@@ -1487,6 +1497,22 @@ impl ScriptThread {
         }
     }
 
+    fn handle_framed_content_changed(&self,
+                                     parent_pipeline_id: PipelineId,
+                                     subpage_id: SubpageId) {
+        let borrowed_page = self.root_page();
+        let page = borrowed_page.find(parent_pipeline_id).unwrap();
+        let doc = page.document();
+        let frame_element = doc.find_iframe(subpage_id);
+        if let Some(ref frame_element) = frame_element {
+            frame_element.upcast::<Node>().dirty(NodeDamage::OtherNodeDamage);
+            let window = page.window();
+            window.reflow(ReflowGoal::ForDisplay,
+                          ReflowQueryType::NoQuery,
+                          ReflowReason::FramedContentChanged);
+        }
+    }
+
     /// Handles a mozbrowser event, for example see:
     /// https://developer.mozilla.org/en-US/docs/Web/Events/mozbrowserloadstart
     fn handle_mozbrowser_event_msg(&self,

components/script_traits/lib.rs

@@ -143,6 +143,8 @@ pub enum ConstellationControlMsg {
         /// The pipeline that contains a frame loading the target pipeline.
         parent: PipelineId
     },
+    /// Notifies a parent frame that one of its child frames is now active.
+    FramedContentChanged(PipelineId, SubpageId),
     /// Report an error from a CSS parser for the given pipeline
     ReportCSSError(PipelineId, String, u32, u32, String),
 }

tests/wpt/mozilla/meta/MANIFEST.json

@@ -1688,6 +1688,18 @@
             "url": "/_mozilla/css/iframe/multiple_external.html"
           }
         ],
+        "css/iframe/navigation.html": [
+          {
+            "path": "css/iframe/navigation.html",
+            "references": [
+              [
+                "/_mozilla/css/iframe/navigation_ref.html",
+                "=="
+              ]
+            ],
+            "url": "/_mozilla/css/iframe/navigation.html"
+          }
+        ],
         "css/iframe/overflow.html": [
           {
             "path": "css/iframe/overflow.html",
@@ -7768,6 +7780,18 @@
           "url": "/_mozilla/css/iframe/multiple_external.html"
         }
       ],
+      "css/iframe/navigation.html": [
+        {
+          "path": "css/iframe/navigation.html",
+          "references": [
+            [
+              "/_mozilla/css/iframe/navigation_ref.html",
+              "=="
+            ]
+          ],
+          "url": "/_mozilla/css/iframe/navigation.html"
+        }
+      ],
       "css/iframe/overflow.html": [
         {
           "path": "css/iframe/overflow.html",

tests/wpt/mozilla/tests/css/iframe/navigation.html

@@ -0,0 +1,28 @@
+<!DOCTYPE html>
+<html class="reftest-wait">
+<link rel=match href=navigation_ref.html>
+<style>
+iframe {
+    display: block;
+    border: 1px solid black;
+    width: 500px;
+    height: 300px;
+    margin-left: 10px;
+    margin-top: 0px;
+}
+</style>
+<body>
+<iframe src="data:text/html,Foo"></iframe>
+<script>
+var iframe = document.getElementsByTagName('iframe')[0];
+iframe.addEventListener('load', function first() {
+    iframe.removeEventListener('load', first);
+    iframe.src = "data:text/html,Hello%20world";
+    iframe.addEventListener('load', function() {
+      document.documentElement.classList.remove("reftest-wait");
+    }, false);
+}, false);
+</script>
+</body>
+</html>
+

tests/wpt/mozilla/tests/css/iframe/navigation_ref.html

@@ -0,0 +1,17 @@
+<!DOCTYPE html>
+<html>
+<style>
+iframe {
+    display: block;
+    border: 1px solid black;
+    width: 500px;
+    height: 300px;
+    margin-left: 10px;
+    margin-top: 0px;
+}
+</style>
+<body>
+<iframe src="data:text/html,Hello%20world"></iframe>
+</body>
+</html>
+