Fix RenderLayer::collectLayers logic bug.

Fix RenderLayer::collectLayers logic bug.

In collectLayers, if a layer is normal flow only, we will not add it
to the z-order lists. The trouble is that
RenderLayer::shouldBeNormalFlowOnly depends on 
RenderLayer::needsCompositedScrolling. This means that the result of
collectLayers depends on the opt-in decision of other layers,
something which must not happen: when we are determining opt-in, we
must never depend on the opt-in decision for another layer. This CL
makes this function opt-in agnostic when it needs to be.

R=jchaffraix

BUG=238282
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151665

[hartmanng, 2013-05-06 21:42:42]

Hey Julien, this patch fixes the logic issue we uncovered in
https://codereview.chromium.org/13427009/, and re-enables the relevant layout
tests.

Please take a look.

[shawnsingh, 2013-05-07 00:47:42]

Glenn suggested I could also take a look here.  So in addition to whatever
Julien reviews, here are some additional comments.  Can you please describe the
exact bug that occurs which this patch is fixing?  I guess I'm more out of
context than I realized on this.

https://codereview.chromium.org/14999005/diff/11001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/14999005/diff/11001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:5772: case DoNotForceCompositedScrolling:
It might be more robust to future code changes to also add a default: here, and
let the DoNotForceCompositedScrolling fall through to that default case.  I'm
not sure if there's some style reason not to do it, though?

https://codereview.chromium.org/14999005/diff/11001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:5877: const bool
needsCompositedScrollingGivenForceMode = ((forceMode ==
ForceCompositedScrollingOff) || !needsCompositedScrolling());
I think this variable name is a little unclear -- there's a phrase inside of the
phrase which seems like it could be interpreted as "bool
needsCompositedScrolling" ... at which point total confusion ensues.

https://codereview.chromium.org/14999005/diff/11001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/14999005/diff/11001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.h:828: void
rebuildZOrderLists(OwnPtr<Vector<RenderLayer*> >&, OwnPtr<Vector<RenderLayer*>
>&, const RenderLayer* layerToForceAsStackingContainer = 0,
ForceNeedsCompositedScrollingMode = DoNotForceCompositedScrolling);
I would feel more comfortable with an identifier name here, i.e.
"ForceNeedsCompositedScrollingMode mode = DoNotForceCompositedScrolling"
(mode could be replaced with a better name, too, perhaps)

Same for the other 2 instances of this default argument.

[shawnsingh, 2013-05-07 00:50:03]

On 2013/05/07 00:47:42, shawnsingh wrote:
> Can you please describe the
> exact bug that occurs which this patch is fixing?

Um, to clarify, I meant that you should modify the patch description, so that
your explanation also goes into the CL as well as to me.

[hartmanng, 2013-05-13 19:27:05]

Thanks for the comments Shawn! I was away much of last week, so I've been slow
to respond.

The issue description has been updated (let me know if it could still use
clearing up) and I've responded to your comments.

PTAL

https://codereview.chromium.org/14999005/diff/11001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/14999005/diff/11001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:5772: case DoNotForceCompositedScrolling:
On 2013/05/07 00:47:42, shawnsingh wrote:
> It might be more robust to future code changes to also add a default: here,
and
> let the DoNotForceCompositedScrolling fall through to that default case.  I'm
> not sure if there's some style reason not to do it, though?

Based on https://codereview.chromium.org/14458008/#msg5, I get the feeling that
default: clauses are frowned upon in this area of the code, but I could be
interpreting that incorrectly. jchaffraix, can you clarify?

https://codereview.chromium.org/14999005/diff/11001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:5877: const bool
needsCompositedScrollingGivenForceMode = ((forceMode ==
ForceCompositedScrollingOff) || !needsCompositedScrolling());
On 2013/05/07 00:47:42, shawnsingh wrote:
> I think this variable name is a little unclear -- there's a phrase inside of
the
> phrase which seems like it could be interpreted as "bool
> needsCompositedScrolling" ... at which point total confusion ensues.

Good point, it is a bit confusing. I was trying to make it specific to
distinguish it from the needsCompositedScrolling() value - the idea is that we
only want the "real" needsCompositedScrolling if we're not forcing composited
scrolling off.

Do you think this would be better as just "bool needsCompositedScrolling"? I may
be adding unnecessary complexity here.

For now I've called it "shouldNeedCompositedScrolling", but I'm not sure that's
quite right either.

https://codereview.chromium.org/14999005/diff/11001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/14999005/diff/11001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.h:828: void
rebuildZOrderLists(OwnPtr<Vector<RenderLayer*> >&, OwnPtr<Vector<RenderLayer*>
>&, const RenderLayer* layerToForceAsStackingContainer = 0,
ForceNeedsCompositedScrollingMode = DoNotForceCompositedScrolling);
On 2013/05/07 00:47:42, shawnsingh wrote:
> I would feel more comfortable with an identifier name here, i.e.
> "ForceNeedsCompositedScrollingMode mode = DoNotForceCompositedScrolling"
> (mode could be replaced with a better name, too, perhaps)
> 
> Same for the other 2 instances of this default argument.

Apparently this is against the style, unless it "adds information". Naming this
"mode", "acceleratedCompositingForOverflowScrollEnabledMode", or anything
similar, prompts the following style error:

The parameter name "acceleratedCompositingForOverflowScrollEnabledMode" adds no
information, so it should be removed.  [readability/parameter_name] [5]


In this case I think the type name itself is fairly descriptive (also note that
Ian changed it in a patch this depends on, it's now called
"AcceleratedCompositingForOverflowScrollEnabledMode"). Do you have a name in
mind that would make it more clear?

[shawnsingh, 2013-05-16 09:47:54]

Whew... this one is a brain twister for me.  Comments below.

https://codereview.chromium.org/14999005/diff/18001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/14999005/diff/18001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:5665: if (this ==
layerToForceAsStackingContainer) {
OK, so I think I'm gradually overcoming the confusion here.  I think the problem
is that the logic here identifies the various scenarios in an indirect way, so
reading the code doesn't quite convey the concept of what's going on. 
Conceptually, I feel there are four scenarios we would expect collectLayers to
be invoked:

1. experiment 1 seeing the z order list with the layer forced as a stacking
container
  (isStacking=true, isNormalFlow=false)

2. experiment 2 seeing the z-order list with the layer forced as NOT a stacking
container.
  (isStacking=onlyWhenStackingContext,
isNormalFlow=overrideTheNeedsCompositingCondition)

3. while building the real z-order list, telling collectLayers that it can
promote overflow scrolls to be stacking containers even if they aren't
officially stacking contexts
  (isStacking=isContextOrOverflow, isNormalFlow=computeNormally)

4. while building the real z-order list, telling collectLayers that we can only
promote this layer if it is officially a stacking context.
  (isStacking=isContext, isNormalFlow=overrideTheNeedsCompositingCondition)

so... I think we should make these four cases the outer if-statements (or
switch) of this logic.  The current if-statement is effectively using
layerToForceAsStackingContainer as the indicator of which scenario we're in,
rather than the enum.

Instead, we should use the enum to indicate which scenario we're in, and
ASSERT(layerToForceAsStackingContainer) when appropriate.

And yes, I think that means we have to add an extra state to the enum.

As for naming, I couldn't find the original definition of this enum.  Which
patch is it?  I feel the word "compositing" is a red herring here. If the enum
really is being used in other places with these names, maybe we're trying to
force two concepts into one enum.   Here is the enum I envision for
collectLayers:

layerCollectionModeEnum {
  ForceLayerToStackingContainer
  PreventLayerToStackingContainer
  OverflowScrollCanBeStackingContainers
  OnlyStackingContextsCanBeStackingContainers
}



Other than this proposed rearrangement, I think I see how the patch addresses
the original problem and it looks reasonable. =)

https://codereview.chromium.org/14999005/diff/18001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:5780: const bool
shouldNeedCompositedScrolling =
((acceleratedCompositingForOverflowScrollEnabledMode ==
AcceleratedCompositingForOverflowScrollDisabled) ||
!needsCompositedScrolling());
I think we're trying to hide actual conditional logic in an OR statement.
perhaps this should say:

bool doesNotNeedCompositedScrolling;
if ( overflowScrollCompositingMode  == Disabled )
  doesNotNeedCompositedScrolling = true;
else
  doesNotNeedCompositedScrolling = !needsCompositedScrolling();

Personally this seems much more clear and intuitive to me... =)


As a side note, I feel like the logic in this function should be grouped for
more intuitive readability, too:

bool couldBeNormalFlow = /* the first chunk of conditions */
bool hasExceptionThatCannotBeNormalFlow = isPositioned() || hasTransform ||
etc... || needsCompositedScrolling

return couldBeNormalFlow && !hasExceptionThatCannotBeNormalFlow;

https://codereview.chromium.org/14999005/diff/18001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/14999005/diff/18001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.h:831: void
rebuildZOrderLists(OwnPtr<Vector<RenderLayer*> >&, OwnPtr<Vector<RenderLayer*>
>&, const RenderLayer* layerToForceAsStackingContainer = 0,
AcceleratedCompositingForOverflowScrollEnabledMode =
AcceleratedCompositingForOverflowScrollEnabled);
Actually my problem wasn't really with the naming, but the fact that this looks
like an assignment, rather than obviously being a default parameter.  It guess
it's not too big a deal, though.

[shawnsingh, 2013-05-16 09:53:23]

https://codereview.chromium.org/14999005/diff/18001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/14999005/diff/18001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:5780: const bool
shouldNeedCompositedScrolling =
((acceleratedCompositingForOverflowScrollEnabledMode ==
AcceleratedCompositingForOverflowScrollDisabled) ||
!needsCompositedScrolling());
On 2013/05/16 09:47:54, shawnsingh wrote:
> bool doesNotNeedCompositedScrolling;

Hmm, maybe this could be even a bit more intuitive by using the opposite naming:

bool needsCompositedScrolling;
if (overflowScrollCompositingMode != Disabled)
  needsCompositedScrolling = needsCompositedScrolling();
else
  needsCompositedScrolling = false;


and then when all conditions are aggregated, it will be more consistent with the
pattern there.

[hartmanng, 2013-05-20 15:09:10]

Thanks again Shawn. I think this review helped clean things up a bit and make
things easier to follow - I've added a new enum for CollectLayers, which we use
for the top-level switch, and I also cleaned up shouldNeedCompositedScrolling()
a bit as you suggested.

PTAL

https://codereview.chromium.org/14999005/diff/18001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/14999005/diff/18001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:5665: if (this ==
layerToForceAsStackingContainer) {
On 2013/05/16 09:47:54, shawnsingh wrote:
> OK, so I think I'm gradually overcoming the confusion here.  I think the
problem
> is that the logic here identifies the various scenarios in an indirect way, so
> reading the code doesn't quite convey the concept of what's going on. 
> Conceptually, I feel there are four scenarios we would expect collectLayers to
> be invoked:
> 
> 1. experiment 1 seeing the z order list with the layer forced as a stacking
> container
>   (isStacking=true, isNormalFlow=false)
> 
> 2. experiment 2 seeing the z-order list with the layer forced as NOT a
stacking
> container.
>   (isStacking=onlyWhenStackingContext,
> isNormalFlow=overrideTheNeedsCompositingCondition)
> 
> 3. while building the real z-order list, telling collectLayers that it can
> promote overflow scrolls to be stacking containers even if they aren't
> officially stacking contexts
>   (isStacking=isContextOrOverflow, isNormalFlow=computeNormally)
> 
> 4. while building the real z-order list, telling collectLayers that we can
only
> promote this layer if it is officially a stacking context.
>   (isStacking=isContext, isNormalFlow=overrideTheNeedsCompositingCondition)
> 
> so... I think we should make these four cases the outer if-statements (or
> switch) of this logic.  The current if-statement is effectively using
> layerToForceAsStackingContainer as the indicator of which scenario we're in,
> rather than the enum.
> 
> Instead, we should use the enum to indicate which scenario we're in, and
> ASSERT(layerToForceAsStackingContainer) when appropriate.
> 
> And yes, I think that means we have to add an extra state to the enum.
> 
> As for naming, I couldn't find the original definition of this enum.  Which
> patch is it?  I feel the word "compositing" is a red herring here. If the enum
> really is being used in other places with these names, maybe we're trying to
> force two concepts into one enum.

https://codereview.chromium.org/14858004/ is where this enum is defined,
specifically in Settings.h. I think you're right - we're trying to force two
concepts into this enum, so I made a new one as you suggested.

> Here is the enum I envision for
> collectLayers:
> 
> layerCollectionModeEnum {
>   ForceLayerToStackingContainer
>   PreventLayerToStackingContainer
>   OverflowScrollCanBeStackingContainers
>   OnlyStackingContextsCanBeStackingContainers
> }
> 
> 
> 
> Other than this proposed rearrangement, I think I see how the patch addresses
> the original problem and it looks reasonable. =)

I think we actually only need 3 scenarios here, we weren't really using one of
the codepaths (your "experiment 2"). Apart from that, done.

https://codereview.chromium.org/14999005/diff/18001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:5780: const bool
shouldNeedCompositedScrolling =
((acceleratedCompositingForOverflowScrollEnabledMode ==
AcceleratedCompositingForOverflowScrollDisabled) ||
!needsCompositedScrolling());
On 2013/05/16 09:53:23, shawnsingh wrote:
> On 2013/05/16 09:47:54, shawnsingh wrote:
> > bool doesNotNeedCompositedScrolling;
> 
> Hmm, maybe this could be even a bit more intuitive by using the opposite
naming:
> 
> bool needsCompositedScrolling;
> if (overflowScrollCompositingMode != Disabled)
>   needsCompositedScrolling = needsCompositedScrolling();
> else
>   needsCompositedScrolling = false;
> 
> 
> and then when all conditions are aggregated, it will be more consistent with
the
> pattern there.

Done.

https://codereview.chromium.org/14999005/diff/18001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:5780: const bool
shouldNeedCompositedScrolling =
((acceleratedCompositingForOverflowScrollEnabledMode ==
AcceleratedCompositingForOverflowScrollDisabled) ||
!needsCompositedScrolling());
On 2013/05/16 09:47:54, shawnsingh wrote:
> I think we're trying to hide actual conditional logic in an OR statement.
> perhaps this should say:
> 
> bool doesNotNeedCompositedScrolling;
> if ( overflowScrollCompositingMode  == Disabled )
>   doesNotNeedCompositedScrolling = true;
> else
>   doesNotNeedCompositedScrolling = !needsCompositedScrolling();
> 
> Personally this seems much more clear and intuitive to me... =)
> 
> 
> As a side note, I feel like the logic in this function should be grouped for
> more intuitive readability, too:
> 
> bool couldBeNormalFlow = /* the first chunk of conditions */
> bool hasExceptionThatCannotBeNormalFlow = isPositioned() || hasTransform ||
> etc... || needsCompositedScrolling
> 
> return couldBeNormalFlow && !hasExceptionThatCannotBeNormalFlow;

Done.

[shawnsingh, 2013-05-20 17:40:58]

Oh, I see now that my "experiment 2" isn't necessary.

This unofficially looks good to me!

[hartmanng, 2013-05-20 20:45:45]

Thanks! In that case, I think this patch is ready to move forward to jchaffraix
review.

Julien, Please take a look.

[Julien - ping for review, 2013-05-30 22:23:07]

https://codereview.chromium.org/14999005/diff/28001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/14999005/diff/28001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.h:1039: bool shouldBeNormalFlowOnly(bool
canNeedCompositedScrolling = true) const;
Really not a huge fan of adding a default *boolean* argument to this function,
especially since its function is unclear to most people.

Could we just split the 2 call paths between the one that doesn't require
needsCompositedScrolling() and the other one?

[hartmanng, 2013-05-31 13:55:41]

Thanks, I addressed your comment (and uploaded the new patch), PTAL

https://codereview.chromium.org/14999005/diff/28001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/14999005/diff/28001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.h:1039: bool shouldBeNormalFlowOnly(bool
canNeedCompositedScrolling = true) const;
On 2013/05/30 22:23:07, Julien Chaffraix wrote:
> Really not a huge fan of adding a default *boolean* argument to this function,
> especially since its function is unclear to most people.
> 
> Could we just split the 2 call paths between the one that doesn't require
> needsCompositedScrolling() and the other one?

Done.

[Julien - ping for review, 2013-06-03 14:41:47]

lgtm

https://codereview.chromium.org/14999005/diff/37001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/14999005/diff/37001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:5730: bool
RenderLayer::shouldBeNormalFlowOnlyWithoutCompositedScrolling() const
Nit: I would name it shouldBeNormalFlowOnlyIgnoringCompositedScrolling but YMMV.

https://codereview.chromium.org/14999005/diff/37001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:5740: const bool
hasExceptionThatCannotBeNormalFlow = renderer()->isPositioned()
Maybe better name preventsElementFromBeingNormalFlow?

[hartmanng, 2013-06-03 14:54:30]

Thanks!

https://codereview.chromium.org/14999005/diff/37001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/14999005/diff/37001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:5730: bool
RenderLayer::shouldBeNormalFlowOnlyWithoutCompositedScrolling() const
On 2013/06/03 14:41:47, Julien Chaffraix wrote:
> Nit: I would name it shouldBeNormalFlowOnlyIgnoringCompositedScrolling but
YMMV.

Done.

https://codereview.chromium.org/14999005/diff/37001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:5740: const bool
hasExceptionThatCannotBeNormalFlow = renderer()->isPositioned()
On 2013/06/03 14:41:47, Julien Chaffraix wrote:
> Maybe better name preventsElementFromBeingNormalFlow?

Done.

[commit-bot: I haz the power, 2013-06-03 14:54:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/14999005/45001

[commit-bot: I haz the power, 2013-06-03 16:13:49]

Change committed as 151665