Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delay initiating layout operations for as long as possible. #3358

Merged
merged 1 commit into from Sep 17, 2014
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Delay initiating layout operations for as long as possible.

  • Loading branch information
jdm committed Sep 16, 2014
commit 2bd93ed070e852dda57faf9954af1574060b995d
@@ -588,15 +588,15 @@ impl<'m, 'n> NodeHelpers<'m, 'n> for JSRef<'n, Node> {
let page = window.deref().page();
let addr = self.to_trusted_node_address();

let ContentBoxResponse(rect) = page.layout_rpc.content_box(addr);
let ContentBoxResponse(rect) = page.layout().content_box(addr);
rect
}

fn get_content_boxes(&self) -> Vec<Rect<Au>> {
let window = window_from_node(self).root();
let page = window.deref().page();
let addr = self.to_trusted_node_address();
let ContentBoxesResponse(rects) = page.layout_rpc.content_boxes(addr);
let ContentBoxesResponse(rects) = page.layout().content_boxes(addr);
rects
}

@@ -19,7 +19,7 @@ use dom::location::Location;
use dom::navigator::Navigator;
use dom::performance::Performance;
use dom::screen::Screen;
use layout_interface::{ReflowForDisplay, DocumentDamageLevel};
use layout_interface::{ReflowGoal, DocumentDamageLevel};
use page::Page;
use script_task::{ExitWindowMsg, FireTimerMsg, ScriptChan, TriggerLoadMsg, TriggerFragmentMsg};
use script_traits::ScriptControlChan;
@@ -77,14 +77,14 @@ impl TimerHandle {
pub struct Window {
eventtarget: EventTarget,
pub script_chan: ScriptChan,
control_chan: ScriptControlChan,
pub control_chan: ScriptControlChan,
console: Cell<Option<JS<Console>>>,
location: Cell<Option<JS<Location>>>,
navigator: Cell<Option<JS<Navigator>>>,
pub image_cache_task: ImageCacheTask,
pub active_timers: Traceable<RefCell<HashMap<TimerId, TimerHandle>>>,
next_timer_handle: Traceable<Cell<i32>>,
compositor: Untraceable<Box<ScriptListener>>,
pub compositor: Untraceable<Box<ScriptListener>>,
pub browser_context: Traceable<RefCell<Option<BrowserContext>>>,
pub page: Rc<Page>,
performance: Cell<Option<JS<Performance>>>,
@@ -355,6 +355,7 @@ impl Reflectable for Window {

pub trait WindowHelpers {
fn damage_and_reflow(&self, damage: DocumentDamageLevel);
fn flush_layout(&self, goal: ReflowGoal);
fn wait_until_safe_to_modify_dom(&self);
fn init_browser_context(&self, doc: &JSRef<Document>);
fn load_url(&self, href: DOMString);
@@ -387,11 +388,12 @@ impl<'a> WindowHelpers for JSRef<'a, Window> {
}

fn damage_and_reflow(&self, damage: DocumentDamageLevel) {
// FIXME This should probably be ReflowForQuery, not Display. All queries currently
// currently rely on the display list, which means we can't destroy it by
// doing a query reflow.
self.page().damage(damage);
self.page().reflow(ReflowForDisplay, self.control_chan.clone(), *self.compositor);
self.page().avoided_reflows.set(self.page().avoided_reflows.get() + 1);
}

fn flush_layout(&self, goal: ReflowGoal) {
self.page().flush_layout(goal);
}

fn wait_until_safe_to_modify_dom(&self) {
@@ -13,7 +13,7 @@ use dom::document::{Document, DocumentHelpers};
use dom::element::{Element, AttributeHandlers};
use dom::node::{Node, NodeHelpers};
use dom::window::Window;
use layout_interface::{DocumentDamage};
use layout_interface::{DocumentDamage, ReflowForDisplay};
use layout_interface::{DocumentDamageLevel, HitTestResponse, MouseOverResponse};
use layout_interface::{GetRPCMsg, LayoutChan, LayoutRPC};
use layout_interface::{Reflow, ReflowGoal, ReflowMsg};
@@ -56,7 +56,7 @@ pub struct Page {
pub layout_chan: Untraceable<LayoutChan>,

/// A handle to perform RPC calls into the layout, quickly.
pub layout_rpc: Untraceable<Box<LayoutRPC>>,
layout_rpc: Untraceable<Box<LayoutRPC>>,

/// The port that we will use to join layout. If this is `None`, then layout is not running.
pub layout_join_port: Untraceable<RefCell<Option<Receiver<()>>>>,
@@ -94,6 +94,9 @@ pub struct Page {

/// Number of pending reflows that were sent while layout was active.
pub pending_reflows: Cell<int>,

/// Number of unnecessary potential reflows that were skipped since the last reflow
pub avoided_reflows: Cell<int>,
}

pub struct PageIterator {
@@ -158,9 +161,31 @@ impl Page {
constellation_chan: Untraceable::new(constellation_chan),
children: Traceable::new(RefCell::new(vec!())),
pending_reflows: Cell::new(0),
avoided_reflows: Cell::new(0),
}
}

pub fn flush_layout(&self, goal: ReflowGoal) {
let damaged = self.damage.borrow().is_some();
if damaged {
let frame = self.frame();
let window = frame.get_ref().window.root();
self.reflow(goal, window.control_chan.clone(), *window.compositor);
} else {
self.avoided_reflows.set(self.avoided_reflows.get() + 1);
}
}

pub fn layout(&self) -> &LayoutRPC {
// FIXME This should probably be ReflowForQuery, not Display. All queries currently
// currently rely on the display list, which means we can't destroy it by
// doing a query reflow.
self.flush_layout(ReflowForDisplay);
self.join_layout(); //FIXME: is this necessary, or is layout_rpc's mutex good enough?

This comment has been minimized.

Copy link
@cgaebel

cgaebel Sep 16, 2014

Contributor

The mutex is good enough, and I think join_layout is racey anyhow.

This comment has been minimized.

Copy link
@jdm

jdm Sep 16, 2014

Author Member

My fear is that initiating a reflow involves sending a message the the other task, and it seems like the two tasks might race to grab the mutex, so the content task might see stale data. Am I off base here?

This comment has been minimized.

Copy link
@cgaebel

cgaebel Sep 16, 2014

Contributor

I guess that's right. But what's unfortunate is that if layout is already doing reflow (say, due to a window resize), and flush_layout triggers another reflow, layout will report "reflow done" after the first layout, and join_layout will be unblocked.

So the way you have it removes stale content conditions, but they're still possible. I don't have a good solution to this, so I guess this looks fine to me.

This comment has been minimized.

Copy link
@jdm

jdm Sep 16, 2014

Author Member

Are you sure about that? We only report that layout has been joined if we've received the reflow complete notification that corresponds with the reflow initiated by script: http://mxr.mozilla.org/servo/source/components/script/script_task.rs#517

This comment has been minimized.

Copy link
@jdm

jdm Sep 16, 2014

Author Member

Oh darn, Page's join_layout is simpler than the logic in ScriptTask. I'll file that.

let layout_rpc: &LayoutRPC = *self.layout_rpc;
layout_rpc
}

// must handle root case separately
pub fn remove(&self, id: PipelineId) -> Option<Rc<Page>> {
let remove_idx = {
@@ -322,6 +347,9 @@ impl Page {
match root.root() {
None => {},
Some(root) => {
debug!("avoided {:d} reflows", self.avoided_reflows.get());
self.avoided_reflows.set(0);

debug!("script: performing reflow for goal {:?}", goal);

// Now, join the layout so that they will see the latest changes we have made.
@@ -390,7 +418,7 @@ impl Page {
}
let root = root.unwrap();
let root: &JSRef<Node> = NodeCast::from_ref(&*root);
let address = match self.layout_rpc.hit_test(root.to_trusted_node_address(), *point) {
let address = match self.layout().hit_test(root.to_trusted_node_address(), *point) {
Ok(HitTestResponse(node_address)) => {
Some(node_address)
}
@@ -411,7 +439,7 @@ impl Page {
}
let root = root.unwrap();
let root: &JSRef<Node> = NodeCast::from_ref(&*root);
let address = match self.layout_rpc.mouse_over(root.to_trusted_node_address(), *point) {
let address = match self.layout().mouse_over(root.to_trusted_node_address(), *point) {
Ok(MouseOverResponse(node_address)) => {
Some(node_address)
}
@@ -693,6 +693,7 @@ impl ScriptTask {
// Kick off the initial reflow of the page.
debug!("kicking off initial reflow of {}", url);
document.deref().content_changed();
window.flush_layout(ReflowForDisplay);

let fragment = url.fragment.as_ref().map(|ref fragment| fragment.to_string());

@@ -722,6 +723,8 @@ impl ScriptTask {
Ok(_) => (),
Err(_) => println!("evaluate_script failed")
}

window.flush_layout(ReflowForDisplay);
}
});

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.