[webkit-reviews] review requested: [Bug 109302] Simplify logic for automatically opting into composited scrolling : [Attachment 191777] 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 191777: Patch
https://bugs.webkit.org/attachment.cgi?id=191777&action=review

------- Additional Comments from Glenn Hartmann <hartmanng at chromium.org>
(In reply to comment #17)
> (From update of attachment 191233 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=191233&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:542
> > +	 RenderLayerModelObject* layerRenderer = layer->renderer();
> > +	 return layer->isRootLayer() || layerRenderer->isPositioned() ||
layer->hasTransform();
> 
> Please add a FIXME about this being not in sync with containingBlock (and
yes, it can be handled in a follow-up bug).

done.


> > Source/WebCore/rendering/RenderLayer.cpp:545
> > +enum StackingOrderDirection {FromRight, FromLeft};
> 
> I think we would need spaces before and after '{' and '}'.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:549
> > +// from left-to-right and right-to-left.
> 
> This is not a good use of left / right. You probably think about these
directions as you read from left-to-right so you assume Vectors grow towards
the "left" but that's arbitrary.
> 
> It's also completely unrelated to the actual walking you are doing, which is
on the z-order lists so it's more appropriate to talk about background vs
foreground walking. If you don't like to use the visual naming, {
FromStartOfVector, FromEndOfVector }

done.


> > Source/WebCore/rendering/RenderLayer.cpp:609
> > +	 updateNormalFlowList();
> 
> Was the original ASSERT wrong? (AFAICT the rest of the calling code is the
same so this shouldn't be needed, unless the ASSERT was indeed wrong).
> 
> > Source/WebCore/rendering/RenderLayer.cpp:611
> > +	 ASSERT(!m_normalFlowListDirty);
> 
> This ASSERT is trivially passing as you call updateNormalFlowList() above,
did you remove the wrong one?

Fixed, the original ASSERT was correct, I put it back in and removed the call
to updateNormalFlowList().


> > Source/WebCore/rendering/RenderLayer.cpp:636
> > +	 // We can't use TemporaryChange<> here since
m_needsCompositedScrolling and
> > +	 // m_isNormalFlowOnly are both bitfields, so we have to do it the
> > +	 // old-fashioned way.
> > +	 bool oldNeedsCompositedScrolling = m_needsCompositedScrolling;
> > +	 bool oldIsNormalFlowOnly = m_isNormalFlowOnly;
> > +
> > +	 // Set the flag on the current layer, then rebuild ancestor stacking
> > +	 // context's lists. This way, we can see the exact effects that
promoting
> > +	 // this layer would cause.
> > +	 m_isNormalFlowOnly = true;
> > +	 m_needsCompositedScrolling = false;
> > +	 ancestorStackingContext->rebuildZOrderLists(StopAtStackingContainers,
posZOrderListBeforePromote, negZOrderListBeforePromote);
> > +
> > +	 m_isNormalFlowOnly = false;
> > +	 m_needsCompositedScrolling = true;
> > +	 ancestorStackingContext->rebuildZOrderLists(StopAtStackingContainers,
posZOrderListAfterPromote, negZOrderListAfterPromote);
> > +
> > +	 m_needsCompositedScrolling = oldNeedsCompositedScrolling;
> > +	 m_isNormalFlowOnly = oldIsNormalFlowOnly;
> 
> This should be in a helper function to keep this code short, like
collectBeforeAfterPromotionZOrderLists.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:643
> > +	 bool hasBackground = renderer()->hasBackground();
> 
> We usually push these booleans when we need them (ie way below).
> 
> > Source/WebCore/rendering/RenderLayer.cpp:644
> > +	 bool weArePositioned = renderer()->isPositioned();
> 
> Why don't you just use isPositioned? Or rendererIsPositioned? (a layer isn't
strictly positioned, its renderer is)
> 
> > Source/WebCore/rendering/RenderLayer.cpp:645
> > +
> 
> Also I don't know / think the 2 booleans above buy us much as they are
exactly used once.

You're right, I took out both bools completely.


> > Source/WebCore/rendering/RenderLayer.cpp:745
> > +	 for (int pass = 0; pass < 2; pass++) {
> 
> The 'for' is super artificial and would probably be more readable if replaced
by a helper function below.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:794
> > +	     if (isStackingContext())
> > +		 return;
> 
> You check that this->isStackingContext() is true here and also in
updateCanBeStackingContainer. I think we should pick a side and check once: the
easiest is here and ASSERT in updateCanBeStackingContainer.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:6023
> > +	     if (isStackingContext || isStackingContainer())
> 
> isStackingContext is checked inside isStackingContainer. Thus this should be
written:
> 
> if (isStackingContainer())
> 
> > Source/WebCore/rendering/RenderLayer.cpp:6034
> > +	     if (isStackingContext || isStackingContainer())
> 
> Ditto.

done both.