Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upHave ContentBox(es)Queries consult the flow tree #3857
Conversation
hoppipolla-critic-bot
commented
Oct 30, 2014
|
Critic review: https://critic.hoppipolla.co.uk/r/3033 This is an external review system which you may optionally use for the code review of your pull request. In order to help critic track your changes, please do not make in-place history rewrites (e.g. via |
| } | ||
|
|
||
| self.process_content_box_request(data.content_box_request, &mut layout_root, &mut rw_data); | ||
| self.process_content_boxes_request(data.content_boxes_request, &mut layout_root, &mut rw_data); |
This comment has been minimized.
This comment has been minimized.
pcwalton
Oct 31, 2014
Contributor
It's a little strange that we have content_box_request, content_boxes_request, and so forth, instead of an enum specifying which one script wants. Does it have to be done this way?
This comment has been minimized.
This comment has been minimized.
mrobinson
Oct 31, 2014
Author
Member
Not at all. I reimplemented this using an enum and everything is much cleaner. Thanks for the suggestion!
|
|
||
| pub fn content_boxes_query(&self, content_boxes_request: TrustedNodeAddress) -> Vec<Rect<Au>> { | ||
| self.flush_layout(ReflowForScriptQuery, None, Some(content_boxes_request)); | ||
| self.join_layout(); //FIXME: is this necessary, or is layout_rpc's mutex good enough? |
This comment has been minimized.
This comment has been minimized.
| @@ -1499,3 +1505,13 @@ bitflags! { | |||
| static IntrinsicInlineSizeIncludesSpecified = 0x08, | |||
| } | |||
| } | |||
|
|
|||
| /// A top-down fragment bounds traversal. | |||
| pub trait FragmentBoundsTraversal { | |||
This comment has been minimized.
This comment has been minimized.
pcwalton
Oct 31, 2014
Contributor
Usually I call "traversals" things that target the flow tree, since fragments are just flat lists.
This comment has been minimized.
This comment has been minimized.
|
Can we get rid of |
|
I don't think we can eliminate it yet. Hit box and mouse over testing is still done using the display list and I think a flow tree implementation is more difficult, because of z-index considerations. The paint order is encoded more readily in the display list than the flow tree, though admittedly I don't understand the mechanism ATM. |
|
That shouldn't matter, because you can't mouse-over an element that has no display items. (Gecko does mouseover and hit box testing using the display list and has no |
|
Oh, in that case, I can try removing it in a followup commit. It will be nice to remove that from the display list. |
|
Sounds good for a followup. (BTW, on the maze solver pseudo-DL items outnumber real DL items 2-1—that's why I'm keen to remove it) :) |
This comment has been minimized.
This comment has been minimized.
|
r=pcwalton |
This comment has been minimized.
This comment has been minimized.
|
saw approval from pcwalton |
This comment has been minimized.
This comment has been minimized.
|
merging mrobinson/servo/queries = 2d72f00 into auto |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
all tests pass: |
This comment has been minimized.
This comment has been minimized.
|
fast-forwarding master to auto = c9089c4 |
mrobinson commentedOct 30, 2014
Instead of looking at the display tree, have ContentBox(es)Query consult
the flow tree. This allow optimizing away parts of the display tree
later. To do this we need to be more careful about how we send reflow
requests, only querying the flow tree when possible.
Fixes #3790.