[webkit-reviews] review requested: [Bug 109302] Simplify logic for automatically opting into composited scrolling : [Attachment 190760] Patch

[bugzilla-daemon at webkit.org]

Glenn Hartmann <hartmanng at chromium.org> has asked  for review:
Bug 109302: Simplify logic for automatically opting into composited scrolling
https://bugs.webkit.org/show_bug.cgi?id=109302

Attachment 190760: Patch
https://bugs.webkit.org/attachment.cgi?id=190760&action=review

------- Additional Comments from Glenn Hartmann <hartmanng at chromium.org>
(In reply to comment #13)
> (From update of attachment 190587 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=190587&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +	     change in stacking order. This should be more intuitive and more
> 
> I think it would be helpful to add one more sentence just before the last
sentence, explaining what the code does after this comparison.	something like
"if the two lists are in fact the same, then we know it is safe to promote the
layer to be a stacking container."

done.


> Especially for a CL of this size (but also in general when not trivial),
WebKit folks appreciate having some brief text to explain all the changes
associated with each item on this list.  You can look through changelogs to see
examples of where patches have done this.  I think that wil also help whoever
the final reviewer will be.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:155
> > +	 , m_canBePromotedToStackingcontainer(false)
> 
> let's use capital C on "container"

done.


> > Source/WebCore/rendering/RenderLayer.cpp:540
> > +static const RenderLayer* getStackingOrderElementAt(const
Vector<RenderLayer*>* posZOrderList, const Vector<RenderLayer*>* negZOrderList,
const bool fromLeft, const size_t index)
> 
> This type of bool is definitely a place where enum makes more sense.	On the
other hand, this is a minor trivial private helper function, so I don't know
what actual reviewers would want.  I would still vote for enum, declared
privately immediately above here.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:563
> > +// two sets of lists.
> 
> I don't know where, perhaps this comment is a good place, but I feel like the
code needs an example in comments, that illustrates why this heavy-handed
robust approach is being taken.  i.e. the example you and Ian have told me 2-3
times now and I keep forgetting that shows how stacking order can break in ways
that are tricky and error-prone to catch otherwise, that motivates the approach
in this function.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:564
> > +void RenderLayer::updateCanBeStackingContainer(RenderLayer*
ancestorStackingContext)
> 
> What if we have an ancestorStackingContainer before an
ancestorStackingContext?  Wouldn't we need to use the ancestorStackingContainer
instead?

Using stacking containers could cause problems here, since we're in the middle
of figuring out what layers can (and can't) be stacking containers.

Here's an example Ian gave me of why this could be problematic:
Say we have a stacking context with a child that WAS a stacking container, and
a grand child that wasn't. If we use the child as the ancestorStacking* for the
grand child and then later it turns out the that child won't actually opt in
(or will opt out), then we're in trouble.


> > Source/WebCore/rendering/RenderLayer.cpp:619
> > +	 // Insert this into the posZOrderistBeforePromote directly after the
> > +	 // positioned ancestor, if there is one. Otherwise, add it to the very
> > +	 // beginning.
> 
> I think I'm misunderstanding something, why do we need to insert "this" into
the before list?  I can take a look again after hearing your explanation =

If the layer isn't positioned, it won't appear in posZOrderListBeforePromote.
Since we want to eventually compare the paint order between the pre-promote
list and the post-promote list, it's very convenient to have "this" in
posZOrderListBeforePromote in its correct paint-order position. It just
simplifies the logic later when we're actually performing the two passes
through both lists to ensure paint order hasn't changed. I've added a bit to
the comment here, hopefully that helps clarify the reasoning.


> > Source/WebCore/rendering/RenderLayer.cpp:648
> > +	 // Scan over the lists twice - the first time from left-to-right and
the
> > +	 // second from right-to-left - each time stopping when we hit this in
> > +	 // the after-promote list. We check along the way for any
inconsistencies
> > +	 // between the two lists that could cause a change in paint order if
we
> > +	 // promote.
> > +	 for (int pass = 0; pass < 2; pass++) {
> 
> Are we certain that we don't need to check the middle part of the layer lists
here, too?  i.e. the layers that get shifted from the ancestor container to the
current layer... do we care/know for certain that the ordering of those layers
will not change?

Yes, the order of the children should not change with respect to each other
because that order is determined by the tree order and z-order, which should be
completely unaffected by promotion. The only possible paint-order change is
when the children move with respect to their parent who is getting promoted, or
if the children move with respect to the other non-children (ie, if the
children are non-contiguous).


> I think the comment here is explaining the implementation, which is OK, but
it might help better to explain the concept.  For example something like "The
layer lists can conceptually be divided into 3 parts:  (a) before the new
stacking container, (b) part of the new stacking container, and (c) after the
new stacking container.  We actually only need to check (a) and (c), because
<insert reason here>.  To do this, we take two passes over the lists, one from
the beginning, the other from the end.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:699
> > +	     updateCanBeStackingContainer(ancestorStackingContext);
> 
> See one of my questions above - if we do need to use
ancestorStackingContainers instead, I guess it would be a few lines additional
code here that potentially changes the RenderLayer that's passed down the
recursion.  Does that seem right?

Same answer as before. It wouldn't be hard to add in, but it could have
correctness implications.


> > Source/WebCore/rendering/RenderLayer.cpp:1987
> > +	     updateIsNormalFlowOnly();
> 
> Looking at the code before applying this patch, it looks like something
similar to this would have been necessary for correctness in the first place,
no?  Otherwise m_isNormalFlowOnly would not have been updated when
needsCompositedScrolling changed.  Was the previous code correct, by somehow
calling styleChanged()?  If so, what is the new case that requires adding this
explicitly?

Yes, this really is necessary for correctness here, with or without the rest of
this patch. It's just that we got lucky and never ran into the border cases
before working on this patch.


> > Source/WebCore/rendering/RenderLayer.cpp:5391
> > +	 bool shouldUpdateCanBeStackingContainer =
acceleratedCompositingForOverflowScrollEnabled() && isStackingContext() &&
(m_zOrderListsDirty || m_normalFlowListDirty);
> 
> Again, related to my previous question about ancestorStackingContext vs
ancestorStackingContainer.   If we did need to use ancestorStackingContainer,
we should probably be saying (isStackingContext() || isStackingContainer())
here, right?

Same answer as above.