From 6a46eef5cd85bacefe0a5dc383833df5076d34fa Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Tue, 11 Nov 2014 08:20:37 -0800 Subject: [PATCH 1/4] Update FrameTree pipelines when appropriate Instead of simply creating a new FrameTree when an iframe starts a load, update the existing FrameTree's pipeline. This prevents the FrameTree from accumulating many extra children. --- components/compositing/constellation.rs | 152 +++++++++++++-------- components/layout/wrapper.rs | 4 +- components/msg/constellation_msg.rs | 2 +- components/script/dom/htmliframeelement.rs | 63 ++++----- 4 files changed, 126 insertions(+), 95 deletions(-) diff --git a/components/compositing/constellation.rs b/components/compositing/constellation.rs index 5bc565b7f9a..0024d516f43 100644 --- a/components/compositing/constellation.rs +++ b/components/compositing/constellation.rs @@ -98,7 +98,7 @@ struct FrameTree { /// The ID of this frame. pub id: FrameId, /// The pipeline for this frame. - pub pipeline: Rc, + pub pipeline: RefCell>, /// The parent frame's pipeline. pub parent: RefCell>>, /// A vector of child frames. @@ -112,7 +112,7 @@ impl FrameTree { -> FrameTree { FrameTree { id: id, - pipeline: pipeline, + pipeline: RefCell::new(pipeline.clone()), parent: RefCell::new(parent_pipeline), children: RefCell::new(vec!()), has_compositor_layer: Cell::new(false), @@ -165,7 +165,7 @@ pub struct FrameTreeDiff { impl FrameTree { fn to_sendable(&self) -> SendableFrameTree { SendableFrameTree { - pipeline: self.pipeline.to_sendable(), + pipeline: self.pipeline.borrow().to_sendable(), children: self.children .borrow() .iter() @@ -178,18 +178,24 @@ impl FrameTree { trait FrameTreeTraversal { fn contains(&self, id: PipelineId) -> bool; fn find(&self, id: PipelineId) -> Option; + fn find_with_subpage_id(&self, id: Option) -> Option>; fn replace_child(&self, id: PipelineId, new_child: Self) -> ReplaceResult; fn iter(&self) -> FrameTreeIterator; } impl FrameTreeTraversal for Rc { fn contains(&self, id: PipelineId) -> bool { - self.iter().any(|frame_tree| id == frame_tree.pipeline.id) + self.iter().any(|frame_tree| id == frame_tree.pipeline.borrow().id) } /// Returns the frame tree whose key is id fn find(&self, id: PipelineId) -> Option> { - self.iter().find(|frame_tree| id == frame_tree.pipeline.id) + self.iter().find(|frame_tree| id == frame_tree.pipeline.borrow().id) + } + + /// Returns the frame tree whose subpage is id + fn find_with_subpage_id(&self, id: Option) -> Option> { + self.iter().find(|frame_tree| id == frame_tree.pipeline.borrow().subpage_id) } /// Replaces a node of the frame tree in place. Returns the node that was removed or the @@ -198,7 +204,7 @@ impl FrameTreeTraversal for Rc { for frame_tree in self.iter() { let mut children = frame_tree.children.borrow_mut(); let child = children.iter_mut() - .find(|child| child.frame_tree.pipeline.id == id); + .find(|child| child.frame_tree.pipeline.borrow().id == id); match child { Some(child) => { *new_child.parent.borrow_mut() = child.frame_tree.parent.borrow().clone(); @@ -293,7 +299,7 @@ impl NavigationContext { /// Loads a new set of page frames, returning all evicted frame trees fn load(&mut self, frame_tree: Rc, compositor_proxy: &mut CompositorProxy) -> Vec> { - debug!("navigating to {}", frame_tree.pipeline.id); + debug!("navigating to {}", frame_tree.pipeline.borrow().id); let evicted = replace(&mut self.next, vec!()); match self.current.take() { Some(current) => self.previous.push(current), @@ -334,7 +340,7 @@ impl NavigationContext { self.current = Some(new_frame.clone()); compositor_proxy.send(CompositorMsg::ChangePageLoadData( new_frame.id, - new_frame.pipeline.load_data.clone())); + new_frame.pipeline.borrow().load_data.clone())); } } @@ -463,11 +469,12 @@ impl Constellation { debug!("constellation got frame rect message"); self.handle_frame_rect_msg(pipeline_id, subpage_id, Rect::from_untyped(&rect)); } - ConstellationMsg::ScriptLoadedURLInIFrame(url, source_pipeline_id, subpage_id, sandbox) => { + ConstellationMsg::ScriptLoadedURLInIFrame(url, source_pipeline_id, new_subpage_id, old_subpage_id, sandbox) => { debug!("constellation got iframe URL load message"); self.handle_script_loaded_url_in_iframe_msg(url, source_pipeline_id, - subpage_id, + new_subpage_id, + old_subpage_id, sandbox); } ConstellationMsg::SetCursor(cursor) => self.handle_set_cursor_msg(cursor), @@ -550,12 +557,12 @@ impl Constellation { loop { let idx = self.pending_frames.iter().position(|pending| { - pending.after.pipeline.id == pipeline_id + pending.after.pipeline.borrow().id == pipeline_id }); match idx { Some(idx) => { debug!("removing pending frame change for failed pipeline"); - self.pending_frames[idx].after.pipeline.force_exit(); + self.pending_frames[idx].after.pipeline.borrow().force_exit(); self.compositor_proxy.send(CompositorMsg::PaintTaskExited(old_pipeline.id)); self.pending_frames.remove(idx); }, @@ -607,7 +614,7 @@ impl Constellation { // Returns true if a child frame tree's subpage id matches the given subpage id let subpage_eq = |child_frame_tree: & &mut ChildFrameTree| { - child_frame_tree.frame_tree.pipeline. + child_frame_tree.frame_tree.pipeline.borrow(). subpage_id.expect("Constellation: child frame does not have a subpage id. This should not be possible.") == subpage_id @@ -668,7 +675,7 @@ impl Constellation { device_pixel_ratio: ScaleFactor) { child_frame_tree.rect = Some(rect); // NOTE: work around borrowchk issues - let pipeline = &child_frame_tree.frame_tree.pipeline; + let pipeline = &*child_frame_tree.frame_tree.pipeline.borrow(); if !already_sent.contains(&pipeline.id) { if is_active { let ScriptControlChan(ref script_chan) = pipeline.script_chan; @@ -688,31 +695,60 @@ impl Constellation { } } + fn update_child_pipeline(frame_tree: Rc, + new_pipeline: Rc, + old_subpage_id: SubpageId) { + let existing_tree = match frame_tree.find_with_subpage_id(Some(old_subpage_id)) { + Some(existing_tree) => existing_tree.clone(), + None => panic!("Tried to update non-existing frame tree with pipeline={} subpage={}", + new_pipeline.id, + old_subpage_id), + }; + + *existing_tree.pipeline.borrow_mut() = new_pipeline; + } + + fn create_or_update_child_pipeline(&mut self, + frame_tree: Rc, + new_pipeline: Rc, + new_rect: Option>, + old_subpage_id: Option) { + match old_subpage_id { + Some(old_subpage_id) => + Constellation::update_child_pipeline(frame_tree.clone(), new_pipeline, old_subpage_id), + None => { + let new_frame_tree = + Rc::new(FrameTree::new(self.get_next_frame_id(), new_pipeline, + Some(frame_tree.pipeline.borrow().clone()))); + frame_tree.children.borrow_mut().push(ChildFrameTree::new(new_frame_tree, + new_rect)); + } + } + } // The script task associated with pipeline_id has loaded a URL in an iframe via script. This // will result in a new pipeline being spawned and a frame tree being added to - // source_pipeline_id's frame tree's children. This message is never the result of a page - // navigation. + // containing_page_pipeline_id's frame tree's children. This message is never the result of a + // page navigation. fn handle_script_loaded_url_in_iframe_msg(&mut self, url: Url, - source_pipeline_id: PipelineId, - subpage_id: SubpageId, + containing_page_pipeline_id: PipelineId, + new_subpage_id: SubpageId, + old_subpage_id: Option, sandbox: IFrameSandboxState) { // Start by finding the frame trees matching the pipeline id, // and add the new pipeline to their sub frames. - let frame_trees = self.find_all(source_pipeline_id); + let frame_trees = self.find_all(containing_page_pipeline_id); if frame_trees.is_empty() { panic!("Constellation: source pipeline id of ScriptLoadedURLInIFrame is not in navigation context, nor is it in a pending frame. This should be impossible."); } - let next_pipeline_id = self.get_next_pipeline_id(); - // Compare the pipeline's url to the new url. If the origin is the same, // then reuse the script task in creating the new pipeline - let source_pipeline = self.pipelines.get(&source_pipeline_id).expect("Constellation: - source Id of ScriptLoadedURLInIFrame does have an associated pipeline in + let source_pipeline = self.pipelines.get(&containing_page_pipeline_id).expect("Constellation: + source Id of ScriptLoadedURLInIFrameMsg does have an associated pipeline in constellation. This should be impossible.").clone(); let source_url = source_pipeline.load_data.url.clone(); @@ -730,21 +766,20 @@ impl Constellation { None }; + let new_frame_pipeline_id = self.get_next_pipeline_id(); let pipeline = self.new_pipeline( - next_pipeline_id, - Some(subpage_id), + new_frame_pipeline_id, + Some(new_subpage_id), script_pipeline, LoadData::new(url) ); - let rect = self.pending_sizes.remove(&(source_pipeline_id, subpage_id)); + let rect = self.pending_sizes.remove(&(containing_page_pipeline_id, new_subpage_id)); for frame_tree in frame_trees.iter() { - let next_frame_id = self.get_next_frame_id(); - frame_tree.children.borrow_mut().push(ChildFrameTree::new( - Rc::new(FrameTree::new(next_frame_id, - pipeline.clone(), - Some(source_pipeline.clone()))), - rect)); + self.create_or_update_child_pipeline(frame_tree.clone(), + pipeline.clone(), + rect, + old_subpage_id); } self.pipelines.insert(pipeline.id, pipeline); } @@ -778,7 +813,7 @@ impl Constellation { // changes would be overridden by changing the subframe associated with source_id. let parent = source_frame.parent.clone(); - let subpage_id = source_frame.pipeline.subpage_id; + let subpage_id = source_frame.pipeline.borrow().subpage_id; let next_pipeline_id = self.get_next_pipeline_id(); let next_frame_id = self.get_next_frame_id(); let pipeline = self.new_pipeline(next_pipeline_id, subpage_id, None, load_data); @@ -805,7 +840,7 @@ impl Constellation { } else { let old = self.current_frame().as_ref().unwrap(); for frame in old.iter() { - frame.pipeline.revoke_paint_permission(); + frame.pipeline.borrow().revoke_paint_permission(); } } self.navigation_context.forward(&mut *self.compositor_proxy) @@ -817,7 +852,7 @@ impl Constellation { } else { let old = self.current_frame().as_ref().unwrap(); for frame in old.iter() { - frame.pipeline.revoke_paint_permission(); + frame.pipeline.borrow().revoke_paint_permission(); } } self.navigation_context.back(&mut *self.compositor_proxy) @@ -825,7 +860,7 @@ impl Constellation { }; for frame in destination_frame.iter() { - frame.pipeline.load(); + frame.pipeline.borrow().load(); } self.grant_paint_permission(destination_frame, NavigationType::Navigate); @@ -838,9 +873,9 @@ impl Constellation { fn handle_key_msg(&self, key: Key, state: KeyState, mods: KeyModifiers) { self.current_frame().as_ref().map(|frame| { - let ScriptControlChan(ref chan) = frame.pipeline.script_chan; + let ScriptControlChan(ref chan) = frame.pipeline.borrow().script_chan; chan.send(ConstellationControlMsg::SendEvent( - frame.pipeline.id, CompositorEvent::KeyEvent(key, state, mods))); + frame.pipeline.borrow().id, CompositorEvent::KeyEvent(key, state, mods))); }); } @@ -871,7 +906,7 @@ impl Constellation { // If it is not found, it simply means that this pipeline will not receive // permission to paint. let pending_index = self.pending_frames.iter().rposition(|frame_change| { - frame_change.after.pipeline.id == pipeline_id + frame_change.after.pipeline.borrow().id == pipeline_id }); match pending_index { Some(pending_index) => { @@ -897,7 +932,7 @@ impl Constellation { frame not contained in the current frame. This is a bug"); for frame in to_revoke.iter() { - frame.pipeline.revoke_paint_permission(); + frame.pipeline.borrow().revoke_paint_permission(); } // If to_add is not the root frame, then replace revoked_frame with it. @@ -907,8 +942,8 @@ impl Constellation { { if to_add.parent.borrow().is_some() { debug!("Constellation: replacing {} with {} in {}", - revoke_id, to_add.pipeline.id, - next_frame_tree.pipeline.id); + revoke_id, to_add.pipeline.borrow().id, + next_frame_tree.pipeline.borrow().id); flag = true; } } @@ -921,7 +956,7 @@ impl Constellation { // Add to_add to parent's children, if it is not the root let parent = &to_add.parent; for parent in parent.borrow().iter() { - let subpage_id = to_add.pipeline.subpage_id + let subpage_id = to_add.pipeline.borrow().subpage_id .expect("Constellation: Child frame's subpage id is None. This should be impossible."); let rect = self.pending_sizes.remove(&(parent.id, subpage_id)); @@ -945,14 +980,14 @@ impl Constellation { let mut already_seen = HashSet::new(); for frame_tree in self.current_frame().iter() { debug!("constellation sending resize message to active frame"); - let pipeline = &frame_tree.pipeline; + let pipeline = &*frame_tree.pipeline.borrow();; let ScriptControlChan(ref chan) = pipeline.script_chan; let _ = chan.send_opt(ConstellationControlMsg::Resize(pipeline.id, new_size)); already_seen.insert(pipeline.id); } for frame_tree in self.navigation_context.previous.iter() .chain(self.navigation_context.next.iter()) { - let pipeline = &frame_tree.pipeline; + let pipeline = &*frame_tree.pipeline.borrow(); if !already_seen.contains(&pipeline.id) { debug!("constellation sending resize message to inactive frame"); let ScriptControlChan(ref chan) = pipeline.script_chan; @@ -967,9 +1002,10 @@ impl Constellation { let frame_tree = &change.after; if frame_tree.parent.borrow().is_none() { debug!("constellation sending resize message to pending outer frame ({})", - frame_tree.pipeline.id); - let ScriptControlChan(ref chan) = frame_tree.pipeline.script_chan; - let _ = chan.send_opt(ConstellationControlMsg::Resize(frame_tree.pipeline.id, new_size)); + frame_tree.pipeline.borrow().id); + let ScriptControlChan(ref chan) = frame_tree.pipeline.borrow().script_chan; + let _ = chan.send_opt(ConstellationControlMsg::Resize( + frame_tree.pipeline.borrow().id, new_size)); } } @@ -981,15 +1017,15 @@ impl Constellation { // TODO(tkuehn): should only exit once per unique script task, // and then that script task will handle sub-exits for frame_tree in frame_tree.iter() { - frame_tree.pipeline.exit(PipelineExitType::PipelineOnly); - self.compositor_proxy.send(CompositorMsg::PaintTaskExited(frame_tree.pipeline.id)); - self.pipelines.remove(&frame_tree.pipeline.id); + frame_tree.pipeline.borrow().exit(PipelineExitType::PipelineOnly); + self.compositor_proxy.send(CompositorMsg::PaintTaskExited(frame_tree.pipeline.borrow().id)); + self.pipelines.remove(&frame_tree.pipeline.borrow().id); } } fn handle_evicted_frames(&mut self, evicted: Vec>) { for frame_tree in evicted.into_iter() { - if !self.navigation_context.contains(frame_tree.pipeline.id) { + if !self.navigation_context.contains(frame_tree.pipeline.borrow().id) { self.close_pipelines(frame_tree); } else { let frames = frame_tree.children.borrow().iter() @@ -1030,7 +1066,7 @@ impl Constellation { let mut iter = frame_tree.iter(); for frame in iter { frame.has_compositor_layer.set(true); - frame.pipeline.grant_paint_permission(); + frame.pipeline.borrow().grant_paint_permission(); } } Err(()) => {} // message has been discarded, probably shutting down @@ -1043,7 +1079,7 @@ impl Constellation { -> Option<(ChildFrameTree, Rc)> { for child in frame_tree.children.borrow().iter() { let child_frame_tree = child.frame_tree.clone(); - if child.frame_tree.pipeline.id == child_pipeline_id { + if child.frame_tree.pipeline.borrow().id == child_pipeline_id { return Some((ChildFrameTree::new(child_frame_tree, child.rect), frame_tree.clone())); } @@ -1070,13 +1106,13 @@ impl Constellation { }; if child.frame_tree.has_compositor_layer.get() { - child.frame_tree.pipeline.grant_paint_permission(); + child.frame_tree.pipeline.borrow().grant_paint_permission(); return; } let sendable_frame_tree_diff = FrameTreeDiff { - parent_pipeline: parent.pipeline.to_sendable(), - pipeline: child.frame_tree.pipeline.to_sendable(), + parent_pipeline: parent.pipeline.borrow().to_sendable(), + pipeline: child.frame_tree.pipeline.borrow().to_sendable(), rect: child.rect, }; @@ -1085,7 +1121,7 @@ impl Constellation { match port.recv_opt() { Ok(()) => { child.frame_tree.has_compositor_layer.set(true); - child.frame_tree.pipeline.grant_paint_permission(); + child.frame_tree.pipeline.borrow().grant_paint_permission(); } Err(()) => {} // The message has been discarded, we are probably shutting down. } diff --git a/components/layout/wrapper.rs b/components/layout/wrapper.rs index e11587710fa..98567dbcecd 100644 --- a/components/layout/wrapper.rs +++ b/components/layout/wrapper.rs @@ -144,8 +144,8 @@ pub trait TLayoutNode { Some(elem) => elem, None => panic!("not an iframe element!") }; - let size = (*iframe_element.unsafe_get()).size().unwrap(); - (*size.pipeline_id(), *size.subpage_id()) + ((*iframe_element.unsafe_get()).containing_page_pipeline_id().unwrap(), + (*iframe_element.unsafe_get()).subpage_id().unwrap()) } } diff --git a/components/msg/constellation_msg.rs b/components/msg/constellation_msg.rs index cbd6062f724..5cd2a0cce39 100644 --- a/components/msg/constellation_msg.rs +++ b/components/msg/constellation_msg.rs @@ -203,7 +203,7 @@ pub enum Msg { LoadComplete, FrameRect(PipelineId, SubpageId, Rect), LoadUrl(PipelineId, LoadData), - ScriptLoadedURLInIFrame(Url, PipelineId, SubpageId, IFrameSandboxState), + ScriptLoadedURLInIFrame(Url, PipelineId, SubpageId, Option, IFrameSandboxState), Navigate(NavigationDirection), PainterReady(PipelineId), ResizedWindow(WindowSizeData), diff --git a/components/script/dom/htmliframeelement.rs b/components/script/dom/htmliframeelement.rs index 683c18c678a..66d0f824fa2 100644 --- a/components/script/dom/htmliframeelement.rs +++ b/components/script/dom/htmliframeelement.rs @@ -20,7 +20,7 @@ use dom::node::{Node, NodeHelpers, NodeTypeId, window_from_node}; use dom::urlhelper::UrlHelper; use dom::virtualmethods::VirtualMethods; use dom::window::Window; -use page::IterablePage; +use page::{IterablePage, Page}; use servo_msg::constellation_msg::{PipelineId, SubpageId, ConstellationChan}; use servo_msg::constellation_msg::IFrameSandboxState::{IFrameSandboxed, IFrameUnsandboxed}; @@ -44,7 +44,8 @@ enum SandboxAllowance { #[dom_struct] pub struct HTMLIFrameElement { htmlelement: HTMLElement, - size: Cell>, + subpage_id: Cell>, + containing_page_pipeline_id: Cell>, sandbox: Cell>, } @@ -54,31 +55,12 @@ impl HTMLIFrameElementDerived for EventTarget { } } -#[jstraceable] -#[privatize] -#[deriving(Copy)] -pub struct IFrameSize { - pipeline_id: PipelineId, - subpage_id: SubpageId, -} - -impl IFrameSize { - #[inline] - pub fn pipeline_id<'a>(&'a self) -> &'a PipelineId { - &self.pipeline_id - } - - #[inline] - pub fn subpage_id<'a>(&'a self) -> &'a SubpageId { - &self.subpage_id - } -} - pub trait HTMLIFrameElementHelpers { fn is_sandboxed(self) -> bool; fn get_url(self) -> Option; /// http://www.whatwg.org/html/#process-the-iframe-attributes fn process_the_iframe_attributes(self); + fn generate_new_subpage_id(self, page: &Page) -> (SubpageId, Option); } impl<'a> HTMLIFrameElementHelpers for JSRef<'a, HTMLIFrameElement> { @@ -100,6 +82,13 @@ impl<'a> HTMLIFrameElementHelpers for JSRef<'a, HTMLIFrameElement> { }) } + fn generate_new_subpage_id(self, page: &Page) -> (SubpageId, Option) { + let old_subpage_id = self.subpage_id.get(); + let subpage_id = page.get_next_subpage_id(); + self.subpage_id.set(Some(subpage_id)); + (subpage_id, old_subpage_id) + } + fn process_the_iframe_attributes(self) { let url = match self.get_url() { Some(url) => url.clone(), @@ -112,19 +101,19 @@ impl<'a> HTMLIFrameElementHelpers for JSRef<'a, HTMLIFrameElement> { IFrameUnsandboxed }; - // Subpage Id let window = window_from_node(self).root(); let window = window.r(); let page = window.page(); - let subpage_id = page.get_next_subpage_id(); + let (new_subpage_id, old_subpage_id) = self.generate_new_subpage_id(page); - self.size.set(Some(IFrameSize { - pipeline_id: page.id, - subpage_id: subpage_id, - })); + self.containing_page_pipeline_id.set(Some(page.id)); let ConstellationChan(ref chan) = page.constellation_chan; - chan.send(ConstellationMsg::ScriptLoadedURLInIFrame(url, page.id, subpage_id, sandboxed)); + chan.send(ConstellationMsg::ScriptLoadedURLInIFrame(url, + page.id, + new_subpage_id, + old_subpage_id, + sandboxed)); } } @@ -132,7 +121,8 @@ impl HTMLIFrameElement { fn new_inherited(localName: DOMString, prefix: Option, document: JSRef) -> HTMLIFrameElement { HTMLIFrameElement { htmlelement: HTMLElement::new_inherited(HTMLElementTypeId::HTMLIFrameElement, localName, prefix, document), - size: Cell::new(None), + subpage_id: Cell::new(None), + containing_page_pipeline_id: Cell::new(None), sandbox: Cell::new(None), } } @@ -144,8 +134,13 @@ impl HTMLIFrameElement { } #[inline] - pub fn size(&self) -> Option { - self.size.get() + pub fn containing_page_pipeline_id(&self) -> Option { + self.containing_page_pipeline_id.get() + } + + #[inline] + pub fn subpage_id(&self) -> Option { + self.subpage_id.get() } } @@ -171,11 +166,11 @@ impl<'a> HTMLIFrameElementMethods for JSRef<'a, HTMLIFrameElement> { } fn GetContentWindow(self) -> Option> { - self.size.get().and_then(|size| { + self.subpage_id.get().and_then(|subpage_id| { let window = window_from_node(self).root(); let children = window.page().children.borrow(); let child = children.iter().find(|child| { - child.subpage_id.unwrap() == size.subpage_id + child.subpage_id.unwrap() == subpage_id }); child.and_then(|page| { page.frame.borrow().as_ref().map(|frame| { From f1d82cf82c9fcbfdf0e40363588fd4ca0f708a3a Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Fri, 14 Nov 2014 10:31:28 -0800 Subject: [PATCH 2/4] Add FrameTree::add_child This helper method simplifies a few lines. --- components/compositing/constellation.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/components/compositing/constellation.rs b/components/compositing/constellation.rs index 0024d516f43..6e2d1380aa9 100644 --- a/components/compositing/constellation.rs +++ b/components/compositing/constellation.rs @@ -118,6 +118,10 @@ impl FrameTree { has_compositor_layer: Cell::new(false), } } + + fn add_child(&self, new_child: ChildFrameTree) { + self.children.borrow_mut().push(new_child); + } } #[deriving(Clone)] @@ -720,8 +724,7 @@ impl Constellation { let new_frame_tree = Rc::new(FrameTree::new(self.get_next_frame_id(), new_pipeline, Some(frame_tree.pipeline.borrow().clone()))); - frame_tree.children.borrow_mut().push(ChildFrameTree::new(new_frame_tree, - new_rect)); + frame_tree.add_child(ChildFrameTree::new(new_frame_tree, new_rect)); } } } @@ -963,8 +966,7 @@ impl Constellation { let parent = next_frame_tree.find(parent.id).expect( "Constellation: pending frame has a parent frame that is not active. This is a bug."); - parent.children.borrow_mut().push(ChildFrameTree::new(to_add.clone(), - rect)); + parent.add_child(ChildFrameTree::new(to_add.clone(), rect)); } } } From 19f57008da833e8a8b0bf401c960c271e305970e Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Fri, 14 Nov 2014 10:33:50 -0800 Subject: [PATCH 3/4] Notify compositor when a FrameTree changes pipelines When the constellation change a FrameTree pipeline, it should send a notification to the compositor, so that it can negotiate buffers from the correct render task. This commit also migrates away from the generalized FrameTreeUpdateMsg to ones specific to the situation at hand. That turned out to be an unnecessary abstraction. --- components/compositing/compositor.rs | 55 +++++++++++++++++++---- components/compositing/compositor_task.rs | 15 ++++--- components/compositing/constellation.rs | 52 +++++++++++---------- components/compositing/headless.rs | 6 ++- 4 files changed, 89 insertions(+), 39 deletions(-) diff --git a/components/compositing/compositor.rs b/components/compositing/compositor.rs index a2d86d9d5a5..df0d505ad4e 100644 --- a/components/compositing/compositor.rs +++ b/components/compositing/compositor.rs @@ -5,7 +5,7 @@ use compositor_layer::{CompositorData, CompositorLayer, WantsScrollEventsFlag}; use compositor_task::{CompositorEventListener, CompositorProxy, CompositorReceiver}; use compositor_task::{CompositorTask, LayerProperties, Msg}; -use constellation::{FrameId, FrameTreeDiff, SendableFrameTree}; +use constellation::{FrameId, SendableFrameTree}; use pipeline::CompositionPipeline; use scrolling::ScrollingTimerProxy; use windowing; @@ -289,9 +289,15 @@ impl IOCompositor { self.send_viewport_rects_for_all_layers(); } - (Msg::FrameTreeUpdate(frame_tree_diff, response_channel), + (Msg::ChangeLayerPipelineAndRemoveChildren(old_pipeline, new_pipeline, response_channel), ShutdownState::NotShuttingDown) => { - self.update_frame_tree(&frame_tree_diff); + self.handle_change_layer_pipeline_and_remove_children(old_pipeline, new_pipeline); + response_channel.send(()); + } + + (Msg::CreateRootLayerForPipeline(parent_pipeline, pipeline, rect, response_channel), + ShutdownState::NotShuttingDown) => { + self.handle_create_root_layer_for_pipeline(parent_pipeline, pipeline, rect); response_channel.send(()); } @@ -534,8 +540,41 @@ impl IOCompositor { return root_layer; } - fn update_frame_tree(&mut self, frame_tree_diff: &FrameTreeDiff) { - let pipeline_id = frame_tree_diff.parent_pipeline.id; + fn handle_change_layer_pipeline_and_remove_children(&mut self, + old_pipeline: CompositionPipeline, + new_pipeline: CompositionPipeline) { + let root_layer = match self.find_pipeline_root_layer(old_pipeline.id) { + Some(root_layer) => root_layer, + None => { + debug!("Ignoring ChangeLayerPipelineAndRemoveChildren message for pipeline ({}) shutting down.", + old_pipeline.id); + return; + } + }; + + root_layer.clear_all_tiles(self); + root_layer.children().clear(); + + let new_pipeline_id = new_pipeline.id; + self.get_or_create_pipeline_details(new_pipeline_id).pipeline = Some(new_pipeline); + } + + fn handle_create_root_layer_for_pipeline(&mut self, + parent_pipeline: CompositionPipeline, + new_pipeline: CompositionPipeline, + frame_rect: Option>) { + let root_layer = self.create_root_layer_for_pipeline_and_rect(&new_pipeline, frame_rect); + match frame_rect { + Some(ref frame_rect) => { + *root_layer.masks_to_bounds.borrow_mut() = true; + + let frame_rect = frame_rect.to_untyped(); + *root_layer.bounds.borrow_mut() = Rect::from_untyped(&frame_rect); + } + None => {} + } + + let pipeline_id = parent_pipeline.id; let parent_layer = match self.find_pipeline_root_layer(pipeline_id) { Some(root_layer) => root_layer, None => { @@ -544,9 +583,7 @@ impl IOCompositor { return; } }; - parent_layer.add_child( - self.create_root_layer_for_pipeline_and_rect(&frame_tree_diff.pipeline, - frame_tree_diff.rect)); + parent_layer.add_child(root_layer); } fn find_pipeline_root_layer(&self, pipeline_id: PipelineId) -> Option>> { @@ -683,7 +720,7 @@ impl IOCompositor { Some(ref layer) => { layer.bounds.borrow_mut().origin = Point2D::from_untyped(&new_origin) } - None => panic!("Compositor received SetLayerOrigin for nonexistent layer"), + None => panic!("Compositor received SetLayerOrigin for nonexistent layer: {}", pipeline_id), }; self.send_buffer_requests_for_all_layers(); diff --git a/components/compositing/compositor_task.rs b/components/compositing/compositor_task.rs index aa4424e2214..3ff651da281 100644 --- a/components/compositing/compositor_task.rs +++ b/components/compositing/compositor_task.rs @@ -5,7 +5,7 @@ //! Communication with the compositor task. pub use windowing; -pub use constellation::{FrameId, SendableFrameTree, FrameTreeDiff}; +pub use constellation::{FrameId, SendableFrameTree}; use compositor; use headless; @@ -13,15 +13,17 @@ use windowing::{WindowEvent, WindowMethods}; use azure::azure_hl::{SourceSurfaceMethods, Color}; use geom::point::Point2D; -use geom::rect::Rect; +use geom::rect::{Rect, TypedRect}; use geom::size::Size2D; use layers::platform::surface::{NativeCompositingGraphicsContext, NativeGraphicsMetadata}; use layers::layers::LayerBufferSet; +use pipeline::CompositionPipeline; use servo_msg::compositor_msg::{Epoch, LayerId, LayerMetadata, ReadyState}; use servo_msg::compositor_msg::{PaintListener, PaintState, ScriptListener, ScrollPolicy}; use servo_msg::constellation_msg::{ConstellationChan, LoadData, PipelineId}; use servo_msg::constellation_msg::{Key, KeyState, KeyModifiers}; use servo_util::cursor::Cursor; +use servo_util::geometry::PagePx; use servo_util::memory::MemoryProfilerChan; use servo_util::time::TimeProfilerChan; use std::comm::{channel, Sender, Receiver}; @@ -206,8 +208,10 @@ pub enum Msg { PaintMsgDiscarded, /// Replaces the current frame tree, typically called during main frame navigation. SetFrameTree(SendableFrameTree, Sender<()>, ConstellationChan), - /// Sends an updated version of the frame tree. - FrameTreeUpdate(FrameTreeDiff, Sender<()>), + /// Requests the compositor to create a root layer for a new frame. + CreateRootLayerForPipeline(CompositionPipeline, CompositionPipeline, Option>, Sender<()>), + /// Requests the compositor to change a root layer's pipeline and remove all child layers. + ChangeLayerPipelineAndRemoveChildren(CompositionPipeline, CompositionPipeline, Sender<()>), /// The load of a page has completed. LoadComplete, /// Indicates that the scrolling timeout with the given starting timestamp has happened and a @@ -237,8 +241,9 @@ impl Show for Msg { Msg::ChangePageTitle(..) => write!(f, "ChangePageTitle"), Msg::ChangePageLoadData(..) => write!(f, "ChangePageLoadData"), Msg::PaintMsgDiscarded(..) => write!(f, "PaintMsgDiscarded"), - Msg::FrameTreeUpdate(..) => write!(f, "FrameTreeUpdate"), Msg::SetFrameTree(..) => write!(f, "SetFrameTree"), + Msg::CreateRootLayerForPipeline(..) => write!(f, "CreateRootLayerForPipeline"), + Msg::ChangeLayerPipelineAndRemoveChildren(..) => write!(f, "ChangeLayerPipelineAndRemoveChildren"), Msg::LoadComplete => write!(f, "LoadComplete"), Msg::ScrollTimeout(..) => write!(f, "ScrollTimeout"), Msg::KeyEvent(..) => write!(f, "KeyEvent"), diff --git a/components/compositing/constellation.rs b/components/compositing/constellation.rs index 6e2d1380aa9..e1fa00c2e82 100644 --- a/components/compositing/constellation.rs +++ b/components/compositing/constellation.rs @@ -156,16 +156,6 @@ enum ReplaceResult { OriginalNode(Rc), } -/// A struct that triggers the addition of a new frame to a previously existing frame tree. -pub struct FrameTreeDiff { - /// The parent pipeline of the new frame. - pub parent_pipeline: CompositionPipeline, - /// The pipeline of the new frame itself. - pub pipeline: CompositionPipeline, - /// The frame rect of the new frame used for positioning its compositor layer. - pub rect: Option>, -} - impl FrameTree { fn to_sendable(&self) -> SendableFrameTree { SendableFrameTree { @@ -699,7 +689,8 @@ impl Constellation { } } - fn update_child_pipeline(frame_tree: Rc, + fn update_child_pipeline(&mut self, + frame_tree: Rc, new_pipeline: Rc, old_subpage_id: SubpageId) { let existing_tree = match frame_tree.find_with_subpage_id(Some(old_subpage_id)) { @@ -709,7 +700,21 @@ impl Constellation { old_subpage_id), }; - *existing_tree.pipeline.borrow_mut() = new_pipeline; + let old_pipeline = existing_tree.pipeline.borrow().clone(); + *existing_tree.pipeline.borrow_mut() = new_pipeline.clone(); + + // If we have not yet sent this frame to the compositor for layer creation, we don't + // need to inform the compositor of updates to the pipeline. + if !existing_tree.has_compositor_layer.get() { + return; + } + + let (chan, port) = channel(); + self.compositor_proxy.send(CompositorMsg::ChangeLayerPipelineAndRemoveChildren( + old_pipeline.to_sendable(), + new_pipeline.to_sendable(), + chan)); + let _ = port.recv_opt(); } fn create_or_update_child_pipeline(&mut self, @@ -719,12 +724,13 @@ impl Constellation { old_subpage_id: Option) { match old_subpage_id { Some(old_subpage_id) => - Constellation::update_child_pipeline(frame_tree.clone(), new_pipeline, old_subpage_id), + self.update_child_pipeline(frame_tree.clone(), new_pipeline, old_subpage_id), None => { - let new_frame_tree = - Rc::new(FrameTree::new(self.get_next_frame_id(), new_pipeline, - Some(frame_tree.pipeline.borrow().clone()))); - frame_tree.add_child(ChildFrameTree::new(new_frame_tree, new_rect)); + let child_tree = Rc::new( + FrameTree::new(self.get_next_frame_id(), + new_pipeline, + Some(frame_tree.pipeline.borrow().clone()))); + frame_tree.add_child(ChildFrameTree::new(child_tree, new_rect)); } } } @@ -1112,14 +1118,12 @@ impl Constellation { return; } - let sendable_frame_tree_diff = FrameTreeDiff { - parent_pipeline: parent.pipeline.borrow().to_sendable(), - pipeline: child.frame_tree.pipeline.borrow().to_sendable(), - rect: child.rect, - }; - let (chan, port) = channel(); - self.compositor_proxy.send(CompositorMsg::FrameTreeUpdate(sendable_frame_tree_diff, chan)); + self.compositor_proxy.send(CompositorMsg::CreateRootLayerForPipeline( + parent.pipeline.borrow().to_sendable(), + child.frame_tree.pipeline.borrow().to_sendable(), + child.rect, + chan)); match port.recv_opt() { Ok(()) => { child.frame_tree.has_compositor_layer.set(true); diff --git a/components/compositing/headless.rs b/components/compositing/headless.rs index 8b58d727292..86188636260 100644 --- a/components/compositing/headless.rs +++ b/components/compositing/headless.rs @@ -90,7 +90,11 @@ impl CompositorEventListener for NullCompositor { response_chan.send(()); } - Msg::FrameTreeUpdate(_, response_channel) => { + Msg::ChangeLayerPipelineAndRemoveChildren(_, _, response_channel) => { + response_channel.send(()); + } + + Msg::CreateRootLayerForPipeline(_, _, _, response_channel) => { response_channel.send(()); } From efdc758dd7bb983f81737a927b1724bacecd5aea Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Mon, 17 Nov 2014 10:59:43 -0800 Subject: [PATCH 4/4] Only send SetLayerRect messages for compositor-backed frames Only send SetLayerRect when we know that a frame has a compositor layer. If it doesn't, we wait until we send the new layer to the compositor. --- components/compositing/constellation.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/components/compositing/constellation.rs b/components/compositing/constellation.rs index e1fa00c2e82..fca671550b1 100644 --- a/components/compositing/constellation.rs +++ b/components/compositing/constellation.rs @@ -626,9 +626,10 @@ impl Constellation { match children.iter_mut().find(|child| subpage_eq(child)) { None => {} Some(child) => { + let has_compositor_layer = child.frame_tree.has_compositor_layer.get(); update_child_rect(child, rect, - true, + has_compositor_layer, &mut already_sent, &mut self.compositor_proxy, self.window_size.device_pixel_ratio)