Clarify the difference between packed layers and stacking contexts #824
Conversation
|
@glennw r? |
|
☔ The latest upstream changes (presumably #802) made this pull request unmergeable. Please resolve the merge conflicts. |
Let's hope all of the are called |
kvark
left a comment
kvark
left a comment
There was a problem hiding this comment.
Looks pretty straightforward and clean in terms of implementation.
For the future, I'd like to remove those xf_rect.as_ref().unwrap() calls and revise the coordinate system names.
| &[]); | ||
| context.builder.push_stacking_context(layer_rect, | ||
| &ClipRegion::simple(&layer_rect), | ||
| LayerToScrollTransform::identity(), |
There was a problem hiding this comment.
Not directly related to this PR, but is it correct to still call it LayerToScrollTransform?
AFAIK, the SC's/items are attached to their reference frames, but the transformation we pass here is relative to the parent stacking context (not necessarily the scroll layer).
There was a problem hiding this comment.
@kvark, I think you are right. The naming of this is off now. There is a chance that in the future StackingContexts won't have transforms at all. It depends if we decide to make all coordinates relative to reference frames or not.
There was a problem hiding this comment.
Yep, I'd say let's not have the transformations in there. Since they only affect the initial placement of items, we should use it and forget about them, only keeping the reference frames in our sight.
mrobinson
force-pushed
the
packed-layer-id
branch
from
8870224 to
e115f89
Compare
|
@kvark, thanks for looking at this. I've pushed a new rebased version with a few other minor naming changes and a new use of Default::default(). |
| fn default() -> PackedStackingContext { | ||
| PackedStackingContext { | ||
| impl Default for PackedLayer { | ||
| fn default() -> PackedLayer { |
There was a problem hiding this comment.
I believe the semantics of Default is misused here. It's not a "default" layer, it's an empty one. Default is applicable where you could drop it if you don't care, and it's going to somewhat work. This is not the case here.
There was a problem hiding this comment.
Hrm. Okay. I'm happy to change it, but does that mean that the implementation of Default::default should change.
There was a problem hiding this comment.
Since there was already impl Default for PackedStackingContext, this is not something that has to be done in your PR. But for the future I think we should remove the impl Default and just have the same thing as an fn empty() -> PackedLayer constructor. No implementation change needed.
|
The build failure seems to be |
|
I think the build failure should be resolved now (I reverted the commit that seems to cause that) - but travis might need a rebuild? |
|
@bors-servo try |
Clarify the difference between packed layers and stacking contexts Eventually scroll layers will create packed layers as well, so this will help to keep things clear. This shouldn't change behavior, because currently packed layer ids will always be equal to the stacking context ids of their owners. <!-- 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/824) <!-- Reviewable:end -->
|
☀️ Test successful - status-travis |
Eventually scroll layers will create packed layers as well, so this will help to keep things clear. This shouldn't change behavior, because currently packed layer ids will always be equal to the stacking context ids of their owners.
mrobinson
force-pushed
the
packed-layer-id
branch
from
e115f89 to
fce5d7c
Compare
|
This is related to #742, by the way. |
|
@bors-servo r=kvark |
|
📌 Commit fce5d7c has been approved by |
Clarify the difference between packed layers and stacking contexts Eventually scroll layers will create packed layers as well, so this will help to keep things clear. This shouldn't change behavior, because currently packed layer ids will always be equal to the stacking context ids of their owners. <!-- 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/824) <!-- Reviewable:end -->
|
☀️ Test successful - status-travis |
4 participants
Eventually scroll layers will create packed layers as well, so this will
help to keep things clear. This shouldn't change behavior, because
currently packed layer ids will always be equal to the stacking context
ids of their owners.
This change is