Begin tracking why layout is invalidated

Begin tracking why layout is invalidated

This is the first of a few patches to track the reasons for calls to
LayoutObject::setNeedsLayout in order to improve the causes shown in
devtools. This patch adds invalidation reasons to most calls of
setNeedsLayout. Future patches will remove Unknown causes and track
additional layout invalidations (e.g., positioned layout movement).

Screenshot:
http://pr.gg/changeCanvasSize.png

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

[pdr., 2015-03-23 02:38:15]

pdr@chromium.org changed reviewers:
+ esprehn@chromium.org, kouhei@chromium.org

[kouhei (in TOK), 2015-03-23 04:24:19]

lgtm

[Erik Dahlström (inactive), 2015-03-23 09:17:16]

ed@opera.com changed reviewers:
+ ed@opera.com

[Erik Dahlström (inactive), 2015-03-23 09:17:17]

https://codereview.chromium.org/1025213002/diff/1/Source/core/svg/SVGClipPath...
File Source/core/svg/SVGClipPathElement.cpp (right):

https://codereview.chromium.org/1025213002/diff/1/Source/core/svg/SVGClipPath...
Source/core/svg/SVGClipPathElement.cpp:67:
object->setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::Unknown);
Should be LayoutInvalidationReason::ChildChanged, no?

[esprehn, 2015-03-23 20:19:44]

I have some concerns that if you go through an n^2 layout (which is pretty easy
with a flex box or grid) this will flood you with layout change notifications, I
guess that would just fill the buffer?

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutBl...
File Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutBl...
Source/core/layout/LayoutBlock.cpp:1733:
r->setNeedsLayout(LayoutInvalidationReason::AncestorMoved, MarkOnlyThis);
Just like with setNeedsStyleRecalc I'd prefer the argument for "reason" was
last. The enums and class name are so long it pushes the important part of the
code way off to the side.

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutBl...
File Source/core/layout/LayoutBlockFlowLine.cpp (right):

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutBl...
Source/core/layout/LayoutBlockFlowLine.cpp:1698:
setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::Unknown,
MarkOnlyThis);
This is from inside an already running layout

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutCo...
File Source/core/layout/LayoutCounter.cpp (right):

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutCo...
Source/core/layout/LayoutCounter.cpp:422:
setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation(LayoutInvalidationReason::Unknown);
this is from counters changing, you need another type like CountersChanged

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutGr...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:1111:
setNeedsLayout(LayoutInvalidationReason::Unknown);
GridChanged ?

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutMe...
File Source/core/layout/LayoutMenuList.cpp (right):

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutMe...
Source/core/layout/LayoutMenuList.cpp:200:
setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation(LayoutInvalidationReason::Unknown);
StyleChange

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutMu...
File Source/core/layout/LayoutMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutMu...
Source/core/layout/LayoutMultiColumnFlowThread.cpp:534:
previousColumnBox->setNeedsLayout(LayoutInvalidationReason::Unknown);
ColumnsChanged

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutOb...
File Source/core/layout/LayoutObjectChildList.cpp (right):

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObjectChildList.cpp:68:
oldChild->setNeedsLayoutAndPrefWidthsRecalc(LayoutInvalidationReason::DomChanged);
This doesn't mean the dom changed, it could just mean stuff became display none,
or other reasons like the parent decided children were now allowed.

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObjectChildList.cpp:159:
newChild->setNeedsLayoutAndPrefWidthsRecalc(LayoutInvalidationReason::DomChanged);
This doesn't mean the dom changed, it could just mean stuff went from display
none -> not none or other reasons.

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutTa...
File Source/core/layout/LayoutTable.h (right):

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutTa...
Source/core/layout/LayoutTable.h:230:
setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::Unknown);
TableChanged ?

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutTa...
File Source/core/layout/LayoutTableCol.cpp (right):

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutTa...
Source/core/layout/LayoutTableCol.cpp:84:
setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation(LayoutInvalidationReason::Unknown);
AttributeChanged ? This is when the span attr changes.

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/SubtreeL...
File Source/core/layout/SubtreeLayoutScope.cpp (right):

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/SubtreeL...
Source/core/layout/SubtreeLayoutScope.cpp:58:
descendant->setNeedsLayout(LayoutInvalidationReason::Unknown,
MarkContainerChain, this);
This can happen under an n^2 layout.

https://codereview.chromium.org/1025213002/diff/1/Source/core/paint/Deprecate...
File Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/1025213002/diff/1/Source/core/paint/Deprecate...
Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:844:
box().setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::Unknown);
OverflowChanged or better ScrollbarsChanged

[pdr., 2015-03-24 03:56:19]

Thanks for the reviews! Updated.

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutBl...
File Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutBl...
Source/core/layout/LayoutBlock.cpp:1733:
r->setNeedsLayout(LayoutInvalidationReason::AncestorMoved, MarkOnlyThis);
On 2015/03/23 at 20:19:43, esprehn wrote:
> Just like with setNeedsStyleRecalc I'd prefer the argument for "reason" was
last. The enums and class name are so long it pushes the important part of the
code way off to the side.

setNeedsLayout takes 2 optional arguments so if we put the reason last, all
callsites will need to list the default values for the optional arguments.

An option is to introduce two different versions of these functions:
void setNeedsLayout(LayoutInvalidationReasonForTracing reason) {
setNeedsLayout([default marking behavior], [default layout scope], reason); }
void setNeedsLayout(MarkingBehavior, SubtreeLayoutScope*,
LayoutInvalidationReasonForTracing)

I think this makes the code more confusing than the argument order. If possible
I'd like to avoid making the reason optional which will force callers of
setNeedsLayout to keep the reasons up to date. Any other ideas?

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutBl...
File Source/core/layout/LayoutBlockFlowLine.cpp (right):

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutBl...
Source/core/layout/LayoutBlockFlowLine.cpp:1698:
setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::Unknown,
MarkOnlyThis);
On 2015/03/23 at 20:19:43, esprehn wrote:
> This is from inside an already running layout

This should be okay. We have a similar situation with style recalcs which can
occur during style recalc.

The layout invalidations are pretty much only useful with devtools which keeps
handles to nodes for us. On the javascript side we can group the invalidations
however, but your point is a good one about having too many causes for any given
layout. I've switched the implementation in setNeedsLayout() to only record an
invalidation if it is the first call to setNeedsLayout. A web developer trying
to remove layouts of a given node may need to make several passes until all
causes are removed.

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutMe...
File Source/core/layout/LayoutMenuList.cpp (right):

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutMe...
Source/core/layout/LayoutMenuList.cpp:200:
setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation(LayoutInvalidationReason::Unknown);
On 2015/03/23 at 20:19:44, esprehn wrote:
> StyleChange

This can be called for other reasons such as updateFromElement() so I went with
"MenuWidthChanged" instead of StyleChange.

I also noticed we shouldn't be setting layout here and filed
http://crbug.com/469976

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutOb...
File Source/core/layout/LayoutObjectChildList.cpp (right):

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObjectChildList.cpp:68:
oldChild->setNeedsLayoutAndPrefWidthsRecalc(LayoutInvalidationReason::DomChanged);
On 2015/03/23 at 20:19:44, esprehn wrote:
> This doesn't mean the dom changed, it could just mean stuff became display
none, or other reasons like the parent decided children were now allowed.

Good catch.

I updated this to be "RemovedFromLayout" which isn't the most descriptive name
but I think it will make sense in context.

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/SubtreeL...
File Source/core/layout/SubtreeLayoutScope.cpp (right):

https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/SubtreeL...
Source/core/layout/SubtreeLayoutScope.cpp:58:
descendant->setNeedsLayout(LayoutInvalidationReason::Unknown,
MarkContainerChain, this);
I'll update SubtreeLayoutScope::setNeedsLayout() to take a reason in a followup.

[pdr., 2015-03-26 17:45:47]

Ping

[pdr., 2015-03-29 00:59:53]

On 2015/03/24 at 03:56:19, pdr wrote:
>
https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/SubtreeL...
> Source/core/layout/SubtreeLayoutScope.cpp:58:
descendant->setNeedsLayout(LayoutInvalidationReason::Unknown,
MarkContainerChain, this);
> I'll update SubtreeLayoutScope::setNeedsLayout() to take a reason in a
followup.

I went ahead and added this to the latest patch. This patch is a bit unwieldy
already so I'd like to add the rest (setChildNeedsLayout, etc) in a few small
followups.

[esprehn, 2015-03-29 04:40:50]

On 2015/03/29 at 00:59:53, pdr wrote:
> On 2015/03/24 at 03:56:19, pdr wrote:
> >
https://codereview.chromium.org/1025213002/diff/1/Source/core/layout/SubtreeL...
> > Source/core/layout/SubtreeLayoutScope.cpp:58:
descendant->setNeedsLayout(LayoutInvalidationReason::Unknown,
MarkContainerChain, this);
> > I'll update SubtreeLayoutScope::setNeedsLayout() to take a reason in a
followup.
> 
> I went ahead and added this to the latest patch. This patch is a bit unwieldy
already so I'd like to add the rest (setChildNeedsLayout, etc) in a few small
followups.

I'll review this Monday, sorry perf took all my time.

[esprehn, 2015-04-01 04:40:23]

lgtm, I'm not a huge fan of this being the first argument since it pushes the
behavioral part of the code way off to the side (and doesn't match
setNeedsStyleRecalc). Ideally we'd make the arguments to these methods required
first, then make this the second argument (and also required). But we can do
that later, lets get this feature shipped.

[pdr., 2015-04-02 01:49:24]

On 2015/04/01 at 04:40:23, esprehn wrote:
> lgtm, I'm not a huge fan of this being the first argument since it pushes the
behavioral part of the code way off to the side (and doesn't match
setNeedsStyleRecalc). Ideally we'd make the arguments to these methods required
first, then make this the second argument (and also required). But we can do
that later, lets get this feature shipped.

Thanks! Rebased, now off to the CQ.

[pdr., 2015-04-02 01:49:29]

The CQ bit was checked by pdr@chromium.org

[pdr., 2015-04-02 01:49:30]

The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org,
esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1025213002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-02 01:49:47]

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

[commit-bot: I haz the power, 2015-04-02 03:34:16]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192989