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

------- Additional Comments from Glenn Hartmann <hartmanng at chromium.org>
(In reply to comment #9)
> (From update of attachment 189137 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=189137&action=review

> > Source/WebCore/rendering/RenderLayer.cpp:-601
> > -//  And we would conclude that C could be promoted.
> 
> Removing this comment is not something I am supportive of.

That comment isn't really relevant anymore. It discusses specific details about
the old implementation, which no longer applies to this patch. If you'd like, I
can add a more thorough description of the new implementation.


> > Source/WebCore/rendering/RenderLayer.cpp:528
> > +	 return layer->isRootLayer() || layerRenderer->isPositioned() ||
layer->hasTransform();
> 
> Looks like you should be using RenderObject::canContainFixedPositionedObject
here as this should handle more cases than that (at least, flow-thread and
foreign-object**).
> 
> ** foreign-object is not actually reachable from this code as SVG objects
don't have RenderLayer.

I actually didn't write this function, just moved it further up in the file.
Not to say that it isn't a valid concern, but maybe it could be addressed in a
separate bug?


> > Source/WebCore/rendering/RenderLayer.cpp:563
> > +	 // TODO (hartmanng at chromium.org): We can early-out of this function
more
> 
> We don't put TODO in WebKit, only FIXMEs without an owner.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:581
> > +	 // 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;
> 
> I don't think I understand your comment, are you saying that because *both*
are bitfield you can't use TemporaryChange or just because the type is boolean?

TemporaryChange<bool> takes in a reference to a bool. Since
m_needsCompositedScrolling and m_isNormalFlowOnly are bitfield values,
attempting to pass either of them in to TemporaryChange<bool> as bool& will
cause a compile error.


> > Source/WebCore/rendering/RenderLayer.cpp:688
> > +// call updateCanBeStackingContainer for all descendants of
> > +// stacking context "ancestorStackingContext"
> 
> Seems like a pretty useless comment.

removed.


> It also doesn't really match the method name:
updateCan*Children*BeStackingContainers, which calls
updateCanBeStackingContainer. There is a mismatch between self / children that
makes it difficult to read and probably justify the above comment. Suggestion
for the naming: updateCanBeStackingContainerRecursively.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:1952
> > +	     updateIsNormalFlowOnly();
> 
> I don't understand the need for this. You are concerned about the z-order
lists and whether you are a stacking context.

It's true that updateCompositedScrolling() doesn't care about normal flow
information, but it could affect the normal-flow-only status of a layer if it
decides to opt in (or opt-out) since shouldBeNormalFlowOnly() makes direct use
of needsCompositedScrolling(). If we don't update the value here,
isNormalFlowOnly() may continue to return stale values.


> Also this is deeply incorrect: if your normal-flow bit was wrong, you already
called updateSelfPaintingLayer() 5 lines above which relies on this
information. You will get away when you are promoted as
needsCompositedScrolling() force the layer to be self-painting, not so much
when you are demoted.

I didn't realize that, thanks for pointing it out. I've moved the call to
updateIsNormalFlowOnly() to above the updateSelfPaintingLayer() call. This
doesn't affect the rest of the behaviour of this patch.


> > Source/WebCore/rendering/RenderLayer.h:802
> > +	 void updateCanChildrenBeStackingContainers(RenderLayer*, bool);
> 
> An API taking a bool with *no* explanation about what this boolean is
supposed to do.
> 
> Please refrain yourself from adding boolean parameters and use enum instead
for readability.

I found a way around having a bool parameter at all.