-
Notifications
You must be signed in to change notification settings - Fork 276
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
Some helpers and bug fixes for picture caching work. #3368
Conversation
Try run looks good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @gw3583)
webrender/src/clip_scroll_tree.rs, line 473 at r1 (raw file):
SpatialNodeType::ScrollFrame(ref info) => { // Assume not identity since it may change with scrolling. if !info.is_pipeline_root {
isn't the root scroll frame also... scrollable? (hence, non-identity)
webrender/src/display_list_flattener.rs, line 1368 at r1 (raw file):
content_size, ScrollSensitivity::ScriptAndInputEvents, true,
it would look much cleaner at call sites if this was an enum
webrender/src/display_list_flattener.rs, line 2253 at r1 (raw file):
} // It is redundant!
do you have any idea on how often this happens in the wild? just curious :)
webrender/src/util.rs, line 591 at r1 (raw file):
offset == TypedVector2D::zero() } FastTransform::Transform { transform, .. } => {
I'm a bit worried that this would semantically cause the copy of transform
, where we should be fine just comparing a reference to it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @kvark)
webrender/src/clip_scroll_tree.rs, line 473 at r1 (raw file):
Previously, kvark (Dzmitry Malyshau) wrote…
isn't the root scroll frame also... scrollable? (hence, non-identity)
It doesn't seem so - there is an implicit scroll frame being added by WR, but it doesn't seem to be used. As a follow up, I'd like to investigate removing it - I discussed this briefly with kats.
webrender/src/display_list_flattener.rs, line 1368 at r1 (raw file):
Previously, kvark (Dzmitry Malyshau) wrote…
it would look much cleaner at call sites if this was an
enum
Fixed
webrender/src/display_list_flattener.rs, line 2253 at r1 (raw file):
Previously, kvark (Dzmitry Malyshau) wrote…
do you have any idea on how often this happens in the wild? just curious :)
It happens quite often :) Gecko provides stacking contexts for groupings in quite a lot of scenarios, and definitely always at least one or two in the main scene. Handling this case for the main scene simplifies the case of picking scroll roots to act as picture caching layers.
webrender/src/util.rs, line 591 at r1 (raw file):
Previously, kvark (Dzmitry Malyshau) wrote…
I'm a bit worried that this would semantically cause the copy of
transform
, where we should be fine just comparing a reference to it
Fixed (if what you meant is to use ref
here)
@kvark Thanks, all comments addressed / fixed, I think. |
Thanks! |
📌 Commit 1bb583d has been approved by |
Some helpers and bug fixes for picture caching work. From the individual commits: * Fix debug assert due to color being out of bounds. * Store whether a scroll frame is implicitly added or not. * Add a fast path for no-op stacking contexts. * Fix texture cache eviction of tiles that are still useful. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3368) <!-- Reviewable:end -->
☀️ Test successful - status-appveyor, status-taskcluster |
…fa2bdc47c3e4 (WR PR #3368). r=kats servo/webrender#3368 Differential Revision: https://phabricator.services.mozilla.com/D13497 --HG-- extra : moz-landing-system : lando
…fa2bdc47c3e4 (WR PR #3368). r=kats servo/webrender#3368 Differential Revision: https://phabricator.services.mozilla.com/D13497
…fa2bdc47c3e4 (WR PR #3368). r=kats servo/webrender#3368 Differential Revision: https://phabricator.services.mozilla.com/D13497 UltraBlame original commit: 1a7497dad85e5674cf508b93f22bb0301e013074
…fa2bdc47c3e4 (WR PR #3368). r=kats servo/webrender#3368 Differential Revision: https://phabricator.services.mozilla.com/D13497 UltraBlame original commit: 1a7497dad85e5674cf508b93f22bb0301e013074
…fa2bdc47c3e4 (WR PR #3368). r=kats servo/webrender#3368 Differential Revision: https://phabricator.services.mozilla.com/D13497 UltraBlame original commit: 1a7497dad85e5674cf508b93f22bb0301e013074
From the individual commits:
This change is