Compute the normal flow list on the fly

Compute the normal flow list on the fly

Storing the normal flow list in DeprecatedPaintLayerStackingNode
is not necessary and there are quite some complexity in the code
for invalidating this cache.
Ideally we shouldn't need the normal flow at all. It's not a CSS
concept but arose from our implementation of stacking contexts
and the multiple responsabilities of DeprecatedPaintLayer.

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

[Julien - ping for review, 2015-06-22 15:35:28]

jchaffraix@chromium.org changed reviewers:
+ esprehn@chromium.org, ojan@chromium.org

[Julien - ping for review, 2015-06-22 15:35:29]

Concept patch: this un-expose the normal flow list, forcing people to use the
iterator. It removes the invalidation code, saves some memory probably at the
expense of some extra walking (I don't have any performance data on that
though).

Removing the list altogether is a lot more involved as it would require at least
unifying our painting code.

[dsinclair, 2015-06-24 15:22:41]

dsinclair@chromium.org changed reviewers:
+ dsinclair@chromium.org

[dsinclair, 2015-06-24 15:22:42]

https://codereview.chromium.org/1188363002/diff/20001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayer.cpp (right):

https://codereview.chromium.org/1188363002/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayer.cpp:1143: if
(!child->stackingNode()->isTreatedAsStackingContextForPainting()) {
Any reason to not have this as a single if statement?

if (!child->stackingNode()->isTreatedAsStackingContextForPainting() &&
!layoutObject->documentBeingDestroyed())
  compositor()->setNeedsCompositingUpdate(CompositingUpdateRebuildTree);

Same below.

https://codereview.chromium.org/1188363002/diff/20001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerStackingNode.cpp (right):

https://codereview.chromium.org/1188363002/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNode.cpp:229: 
nit: remove blank line

https://codereview.chromium.org/1188363002/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNode.cpp:245: if
(!layoutObject()->documentBeingDestroyed() && !layer()->isRootLayer())
Is !isRootLayer equivalent to parent()?

https://codereview.chromium.org/1188363002/diff/20001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.cpp (right):

https://codereview.chromium.org/1188363002/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.cpp:62: if
(!m_normalFlowCurrent->stackingNode()->isTreatedAsStackingContextForPainting()
&& (!m_root.layer()->reflectionInfo() ||
m_root.layer()->reflectionInfo()->reflectionLayer() != m_normalFlowCurrent)) {
Can this condition be pulled out into a local isNormalFlowChild method since
it's repeated below?

[Julien - ping for review, 2015-06-24 16:51:57]

https://codereview.chromium.org/1188363002/diff/20001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayer.cpp (right):

https://codereview.chromium.org/1188363002/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayer.cpp:1143: if
(!child->stackingNode()->isTreatedAsStackingContextForPainting()) {
On 2015/06/24 at 15:22:41, dsinclair wrote:
> Any reason to not have this as a single if statement?

None really.

> if (!child->stackingNode()->isTreatedAsStackingContextForPainting() &&
!layoutObject->documentBeingDestroyed())
>   compositor()->setNeedsCompositingUpdate(CompositingUpdateRebuildTree);
> 
> Same below.

Done and done.

https://codereview.chromium.org/1188363002/diff/20001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerStackingNode.cpp (right):

https://codereview.chromium.org/1188363002/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNode.cpp:229: 
On 2015/06/24 at 15:22:41, dsinclair wrote:
> nit: remove blank line

Removed.

https://codereview.chromium.org/1188363002/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNode.cpp:245: if
(!layoutObject()->documentBeingDestroyed() && !layer()->isRootLayer())
On 2015/06/24 at 15:22:42, dsinclair wrote:
> Is !isRootLayer equivalent to parent()?

It is and it's a lot more clearer what the intent is :)

https://codereview.chromium.org/1188363002/diff/20001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.cpp (right):

https://codereview.chromium.org/1188363002/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.cpp:62: if
(!m_normalFlowCurrent->stackingNode()->isTreatedAsStackingContextForPainting()
&& (!m_root.layer()->reflectionInfo() ||
m_root.layer()->reflectionInfo()->reflectionLayer() != m_normalFlowCurrent)) {
On 2015/06/24 at 15:22:42, dsinclair wrote:
> Can this condition be pulled out into a local isNormalFlowChild method since
it's repeated below?

I did something even better:

(!m_root.layer()->reflectionInfo() ||
m_root.layer()->reflectionInfo()->reflectionLayer() != m_normalFlowCurrent) is
equivalent to !m_normalFlowCurrent->isReflection()

so no need for a new function

https://codereview.chromium.org/1188363002/diff/20001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.h (right):

https://codereview.chromium.org/1188363002/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.h:84:
DeprecatedPaintLayer* m_normalFlowCurrent;
Renamed this variable too as Dan pointed out the name is not great.

[dsinclair, 2015-06-24 19:28:01]

lgtm

[Julien - ping for review, 2015-06-24 21:43:00]

jchaffraix@chromium.org changed reviewers:
+ chrishtr@chromium.org

[Julien - ping for review, 2015-06-24 21:43:01]

+Chris: This was reviewed but I think some extra pair of eyes would help here.

[ojan, 2015-06-24 22:43:10]

lgtm as well.

<3 simplification changes like this! I have a very small concern about
performance overhead of doing this without changing the calling code to avoid
iterating over this list many times, but I think we should move forward with
this patch anyways. If we see extra performance overhead, we can fix the calling
code then.

[chrishtr, 2015-06-24 23:07:55]

https://codereview.chromium.org/1188363002/diff/40001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.cpp (right):

https://codereview.chromium.org/1188363002/diff/40001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.cpp:70:
m_currentNormalFlowChild = m_root.layer()->firstChild();
Is this functionality actually used? I don't think stl iterators work this way.

[Julien - ping for review, 2015-06-24 23:13:57]

https://codereview.chromium.org/1188363002/diff/40001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.cpp (right):

https://codereview.chromium.org/1188363002/diff/40001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.cpp:70:
m_currentNormalFlowChild = m_root.layer()->firstChild();
On 2015/06/24 at 23:07:55, chrishtr wrote:
> Is this functionality actually used? I don't think stl iterators work this
way.

It's not used no. For consistency with the other cases that reset m_index to 0,
I reset this value.

If you prefer I can skip resetting it.

[chrishtr, 2015-06-24 23:36:30]

https://codereview.chromium.org/1188363002/diff/40001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.cpp (right):

https://codereview.chromium.org/1188363002/diff/40001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.cpp:70:
m_currentNormalFlowChild = m_root.layer()->firstChild();
On 2015/06/24 at 23:13:57, Julien Chaffraix - PST wrote:
> On 2015/06/24 at 23:07:55, chrishtr wrote:
> > Is this functionality actually used? I don't think stl iterators work this
way.
> 
> It's not used no. For consistency with the other cases that reset m_index to
0, I reset this value.
> 
> If you prefer I can skip resetting it.

It seems weird as written. Because you can call next() one more time after
getting a null return value, it'll loop around. Maybe set it to nullptr and
ASSERT that next() is never called again?

[Julien - ping for review, 2015-06-25 17:16:24]

https://codereview.chromium.org/1188363002/diff/40001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.cpp (right):

https://codereview.chromium.org/1188363002/diff/40001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.cpp:70:
m_currentNormalFlowChild = m_root.layer()->firstChild();
On 2015/06/24 at 23:36:30, chrishtr wrote:
> On 2015/06/24 at 23:13:57, Julien Chaffraix - PST wrote:
> > On 2015/06/24 at 23:07:55, chrishtr wrote:
> > > Is this functionality actually used? I don't think stl iterators work this
way.
> > 
> > It's not used no. For consistency with the other cases that reset m_index to
0, I reset this value.
> > 
> > If you prefer I can skip resetting it.
> 
> It seems weird as written. Because you can call next() one more time after
getting a null return value, it'll loop around. Maybe set it to nullptr and
ASSERT that next() is never called again?

It won't loop around as we remove normal flow children from m_remainingChildren
the line below. Where did you see that next() would return nullptr then a value?
(that would be a definite bug!)

It's more like if you re-add normal the flow list to the iterator, it would just
work (tm). This is not possible right now though so it's a moot point.

[chrishtr, 2015-06-25 17:20:49]

https://codereview.chromium.org/1188363002/diff/40001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.cpp (right):

https://codereview.chromium.org/1188363002/diff/40001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.cpp:70:
m_currentNormalFlowChild = m_root.layer()->firstChild();
On 2015/06/25 at 17:16:24, Julien Chaffraix - PST wrote:
> On 2015/06/24 at 23:36:30, chrishtr wrote:
> > On 2015/06/24 at 23:13:57, Julien Chaffraix - PST wrote:
> > > On 2015/06/24 at 23:07:55, chrishtr wrote:
> > > > Is this functionality actually used? I don't think stl iterators work
this way.
> > > 
> > > It's not used no. For consistency with the other cases that reset m_index
to 0, I reset this value.
> > > 
> > > If you prefer I can skip resetting it.
> > 
> > It seems weird as written. Because you can call next() one more time after
getting a null return value, it'll loop around. Maybe set it to nullptr and
ASSERT that next() is never called again?
> 
> It won't loop around as we remove normal flow children from
m_remainingChildren the line below. Where did you see that next() would return
nullptr then a value? (that would be a definite bug!)
> 
> It's more like if you re-add normal the flow list to the iterator, it would
just work (tm). This is not possible right now though so it's a moot point.

If you call next() and it's already at the end, then won't line 83 execute and
return nullptr?

[chrishtr, 2015-06-25 20:37:50]

lgtm

[chrishtr, 2015-06-25 20:38:19]

https://codereview.chromium.org/1188363002/diff/40001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.cpp (right):

https://codereview.chromium.org/1188363002/diff/40001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.cpp:70:
m_currentNormalFlowChild = m_root.layer()->firstChild();
On 2015/06/25 at 17:20:49, chrishtr wrote:
> On 2015/06/25 at 17:16:24, Julien Chaffraix - PST wrote:
> > On 2015/06/24 at 23:36:30, chrishtr wrote:
> > > On 2015/06/24 at 23:13:57, Julien Chaffraix - PST wrote:
> > > > On 2015/06/24 at 23:07:55, chrishtr wrote:
> > > > > Is this functionality actually used? I don't think stl iterators work
this way.
> > > > 
> > > > It's not used no. For consistency with the other cases that reset
m_index to 0, I reset this value.
> > > > 
> > > > If you prefer I can skip resetting it.
> > > 
> > > It seems weird as written. Because you can call next() one more time after
getting a null return value, it'll loop around. Maybe set it to nullptr and
ASSERT that next() is never called again?
> > 
> > It won't loop around as we remove normal flow children from
m_remainingChildren the line below. Where did you see that next() would return
nullptr then a value? (that would be a definite bug!)
> > 
> > It's more like if you re-add normal the flow list to the iterator, it would
just work (tm). This is not possible right now though so it's a moot point.
> 
> If you call next() and it's already at the end, then won't line 83 execute and
return nullptr?

I see now. (Julien explained offline). LGTM

[Julien - ping for review, 2015-06-26 00:28:49]

The CQ bit was checked by jchaffraix@chromium.org

[Julien - ping for review, 2015-06-26 00:28:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org, chrishtr@chromium.org, ojan@chromium.org
Link to the patchset: https://codereview.chromium.org/1188363002/#ps60001
(title: "Rebaselined patch.")

[commit-bot: I haz the power, 2015-06-26 00:29:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1188363002/60001

[Julien - ping for review, 2015-06-26 00:29:14]

Thanks for the reviews everyone!

[commit-bot: I haz the power, 2015-06-26 01:13:19]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197871