Restore sanity to RenderLayerStackingNode::shouldBeNormalFlowOnly

Restore sanity to RenderLayerStackingNode::shouldBeNormalFlowOnly

This code evolved and grew over the years, but it seems to be trying to compute
something very simple.

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

[abarth-chromium, 2014-06-02 00:29:14]

I'm not 100% sure whether this change is correct...

[abarth-chromium, 2014-06-02 01:56:25]

https://codereview.chromium.org/307933010/diff/20001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/307933010/diff/20001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:217: Bug(abarth)
media/track/track-cue-rendering-vertical.html [ NeedsRebaseline ]
These are all visually the same.  The difference is just the order in which the
<audio> element paints.  It's now consistent with how <video> and <canvas> paint
(i.e., part of the normal flow list instead of in the positive z order list).

[abarth-chromium, 2014-06-02 17:34:21]

@jchaffraix: Would you be willing to review this CL?  vollick suggested that you
might be the best reviewer.  Thanks!

[Julien - ping for review, 2014-06-03 00:53:20]

Sorry for the delay, I like the direction of this change.

https://codereview.chromium.org/307933010/diff/50001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerStackingNode.cpp (right):

https://codereview.chromium.org/307933010/diff/50001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerStackingNode.cpp:311: return
!isStackingContext() && !renderer()->isPositioned();
Based on the comments in the file, it really looks like "normal flow" just means
overflow-only layer (which is close to being a non-self-painting-layer). We
should amend these comments else it's going to be confusing :-)

Now CSS 2.1 defines normal flow as not absolutely / fixed positioned and not
floating. I think we should use this definition here so:
- it's missing a isFloating() check.
- isPositioned() is basically position != static so it should be
isOutOfFlowPositioned() instead.
- Is the stackingContext() call needed? If so, I would like a comment.

[abarth-chromium, 2014-06-03 02:55:39]

On 2014/06/03 00:53:20, Julien Chaffraix - PST wrote:
> Sorry for the delay, I like the direction of this change.
> 
>
https://codereview.chromium.org/307933010/diff/50001/Source/core/rendering/Re...
> File Source/core/rendering/RenderLayerStackingNode.cpp (right):
> 
>
https://codereview.chromium.org/307933010/diff/50001/Source/core/rendering/Re...
> Source/core/rendering/RenderLayerStackingNode.cpp:311: return
> !isStackingContext() && !renderer()->isPositioned();
> Based on the comments in the file, it really looks like "normal flow" just
means
> overflow-only layer (which is close to being a non-self-painting-layer). We
> should amend these comments else it's going to be confusing :-)

That was the case when the code was originally introduced in 2006, but the
comment has become stale.  It's now used for many more things (e.g., canvas,
video, etc).

> Now CSS 2.1 defines normal flow as not absolutely / fixed positioned and not
> floating. I think we should use this definition here so:
> - it's missing a isFloating() check.
> - isPositioned() is basically position != static so it should be
> isOutOfFlowPositioned() instead.
> - Is the stackingContext() call needed? If so, I would like a comment.

I don't think this list corresponds to anything in CSS.  In fact, I believe
almost everything in this Vector paints incorrectly.  This data structure is
just a hack for things that are getting promoted to self-painting RenderLayers
that aren't supposed to paint after the in-flow content.  They're squeezed after
the in-flow content but before z-index 0, which is as close as we can paint them
to the correct location given the current architecture.

[abarth-chromium, 2014-06-03 03:09:47]

On 2014/06/03 02:55:39, abarth wrote:
> > Now CSS 2.1 defines normal flow as not absolutely / fixed positioned and not
> > floating. I think we should use this definition here so:
> > - it's missing a isFloating() check.
> > - isPositioned() is basically position != static so it should be
> > isOutOfFlowPositioned() instead.

Done using isFloatingOrOutOfFlowPositioned.

> > - Is the stackingContext() call needed? If so, I would like a comment.

Yes.  It's needed for things like transform that make something into a stacking
context without making it floating or positioned.  Transformed elements need to
be in the positive z-order list so they can be painted in tree order with other
z-index 0 elements.

[abarth-chromium, 2014-06-03 04:45:00]

On 2014/06/03 03:09:47, abarth wrote:
> On 2014/06/03 02:55:39, abarth wrote:
> > > Now CSS 2.1 defines normal flow as not absolutely / fixed positioned and
not
> > > floating. I think we should use this definition here so:
> > > - it's missing a isFloating() check.
> > > - isPositioned() is basically position != static so it should be
> > > isOutOfFlowPositioned() instead.
> 
> Done using isFloatingOrOutOfFlowPositioned.

Turns out that breaks a very large number of tests.  I'm going to re-upload the
earlier version that maintains the current semantics.

[abarth-chromium, 2014-06-04 00:25:34]

PTAL

[Julien - ping for review, 2014-06-06 17:31:25]

lgtm

https://codereview.chromium.org/307933010/diff/90001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerStackingNode.cpp (right):

https://codereview.chromium.org/307933010/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerStackingNode.cpp:186: // Ignore non-overflow
layers and reflections.
Let's remove / update this comment as it's not relevant anymore.

https://codereview.chromium.org/307933010/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerStackingNode.cpp:208: // Overflow layers are
just painted by their enclosing layers, so they don't get put in zorder lists.
Ditto.

https://codereview.chromium.org/307933010/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerStackingNode.cpp:309: bool
RenderLayerStackingNode::shouldBeNormalFlowOnly() const
Let's put a FIXME to rename it to something else (or better just let's do it as
part of this patch) as 'normal flow' has a meaning in CSS 2.1 that we don't
match.

[abarth-chromium, 2014-06-06 17:50:15]

The CQ bit was checked by abarth@chromium.org

[abarth-chromium, 2014-06-06 17:50:29]

Done.  Thanks!

[commit-bot: I haz the power, 2014-06-06 17:51:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/abarth@chromium.org/307933010/110001

[commit-bot: I haz the power, 2014-06-06 18:58:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7189)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10636)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11036)

[abarth-chromium, 2014-06-06 19:15:24]

The CQ bit was unchecked by abarth@chromium.org

[abarth-chromium, 2014-06-06 19:27:29]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-06-06 19:28:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/abarth@chromium.org/307933010/130001

[commit-bot: I haz the power, 2014-06-06 21:16:08]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11052)

[commit-bot: I haz the power, 2014-06-06 21:51:08]

Change committed as 175704