Store previous painting clip rects on FragmentData.

Store previous painting clip rects on FragmentData.

Add a NextFragment method on FragmentData in anticipation of support
for multiple fragments.

Use this to make InvalidatePaintLayerOptimizationsIfNeeded multicol
fragment-aware.

BUG=648274
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2854493002
Cr-Commit-Position: refs/heads/master@{#468540}
Committed: https://chromium.googlesource.com/chromium/src/+/32bf53031ff05bfc394accd43c1d4f36399bf3f6

[chrishtr, 2017-04-28 22:04:45]

Description was changed from

==========
none


none


none

BUG=
==========

to

==========
none


none


none

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-04-28 22:05:44]

Description was changed from

==========
none


none


none

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Store previous painting clip rects on ObjectPaintProperties.

Use this to make InvalidatePaintLayerOptimizationsIfNeeded multicol
fragment-aware.

BUG=648274
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-04-28 22:09:57]

Description was changed from

==========
Store previous painting clip rects on ObjectPaintProperties.

Use this to make InvalidatePaintLayerOptimizationsIfNeeded multicol
fragment-aware.

BUG=648274
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Store previous painting clip rects on ObjectPaintProperties.

Add a NextFragment method on ObjectPaintProperties in anticipation of support
for multiple fragments.

Use this to make InvalidatePaintLayerOptimizationsIfNeeded multicol
fragment-aware.

BUG=648274
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-04-28 22:10:06]

The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run

[chrishtr, 2017-04-28 22:10:25]

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

[chrishtr, 2017-04-28 22:10:26]

Also working on a README file.

[commit-bot: I haz the power, 2017-04-28 22:10:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pdr., 2017-04-28 22:23:39]

Ovr'all, a real nice patch you got there. What'cha about this factorin':
  paint/ObjectPaintProperties.h -> paint/FragmentData.h
  class ObjectPaintProperties -> inner class in FragmentData called "Properties"
  Update big class comment to mention this new technology

[commit-bot: I haz the power, 2017-04-29 00:58:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-29 00:58:18]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2017-05-01 19:51:11]

The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run

[chrishtr, 2017-05-01 19:51:21]

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

[chrishtr, 2017-05-01 19:51:22]

+wkorman as pdr is out today.

[chrishtr, 2017-05-01 19:51:35]

Description was changed from

==========
Store previous painting clip rects on ObjectPaintProperties.

Add a NextFragment method on ObjectPaintProperties in anticipation of support
for multiple fragments.

Use this to make InvalidatePaintLayerOptimizationsIfNeeded multicol
fragment-aware.

BUG=648274
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Store previous painting clip rects on FragmentData.

Add a NextFragment method on FragmentData in anticipation of support
for multiple fragments.

Use this to make InvalidatePaintLayerOptimizationsIfNeeded multicol
fragment-aware.

BUG=648274
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2017-05-01 19:51:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wkorman, 2017-05-01 20:29:25]

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1830: FragmentData*
FirstFragment() {
nit: add brief documentation, and move impl to .cpp file if we can to avoid
having to rebuild the world if/when we modify this later and to make header file
more readable.

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/FragmentData.h (right):

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/FragmentData.h:30: ClipRects*
PreviousPaintingClipRects() const {
Are there other clip rects we expect to store in FragmentData that aren't
painting related? If not we could remove Painting from names.

I see we already have similar named member data and methods in PaintLayer, but I
have same question there. It's a PaintLayer, so I would implicitly default to
assuming clip rects stored or interacted with there are painting related. Do we
need painting in the names?

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right):

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:10: #include
"core/paint/ClipRects.h"
Do we still need these? No refs in this file.

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1196: // These
are neeed because we store previous painting clip rects on
neeed -> needed

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1197: //
ObjectPaintProperties.
Comment looks stale -- we're storing them in FragmentData now.

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1200:
ToLayoutBoxModelObject(object).Layer()->SupportsSubsequenceCaching()) ||
How does subsequence caching support on a layer mean it needs paint properties?
The below re: clip related properties seems answered by the comment above but
it's not immediately apparent to me how that relates to subsequence caching.

It looks like this logic is essentially taken from here:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pre...

and the comment there provides more detail, so maybe just copying it over here
as well.

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right):

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:173: object,
context.ancestor_transformed_or_root_paint_layer, fragment,
I wondered why this member data name was so long and found:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pre...

Reading that, it looks like we only ever set and never subsequently read from
PaintLayer.ancestor_overflow_paint_layer. Can it be removed (in separate patch)?

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.h (right):

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.h:59: static void
ClearPreviousPaintingClipRectsForTesting(const LayoutObject&);
Same as earlier re: whether we need Painting in the names.

[chrishtr, 2017-05-01 21:43:54]

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1830: FragmentData*
FirstFragment() {
On 2017/05/01 at 20:29:24, wkorman wrote:
> nit: add brief documentation, and move impl to .cpp file if we can to avoid
having to rebuild the world if/when we modify this later and to make header file
more readable.

Done.

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/FragmentData.h (right):

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/FragmentData.h:30: ClipRects*
PreviousPaintingClipRects() const {
On 2017/05/01 at 20:29:24, wkorman wrote:
> Are there other clip rects we expect to store in FragmentData that aren't
painting related? If not we could remove Painting from names.
> 
> I see we already have similar named member data and methods in PaintLayer, but
I have same question there. It's a PaintLayer, so I would implicitly default to
assuming clip rects stored or interacted with there are painting related. Do we
need painting in the names?

Done.

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right):

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:10: #include
"core/paint/ClipRects.h"
On 2017/05/01 at 20:29:24, wkorman wrote:
> Do we still need these? No refs in this file.

Moved to FragmentData, where they are needed.

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1200:
ToLayoutBoxModelObject(object).Layer()->SupportsSubsequenceCaching()) ||
On 2017/05/01 at 20:29:24, wkorman wrote:
> How does subsequence caching support on a layer mean it needs paint
properties? The below re: clip related properties seems answered by the comment
above but it's not immediately apparent to me how that relates to subsequence
caching.
> 
> It looks like this logic is essentially taken from here:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pre...
> 
> and the comment there provides more detail, so maybe just copying it over here
as well.

I no longer need this code actually, now that the data is stored on
FragmentData.

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right):

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:173: object,
context.ancestor_transformed_or_root_paint_layer, fragment,
On 2017/05/01 at 20:29:24, wkorman wrote:
> I wondered why this member data name was so long and found:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pre...
> 
> Reading that, it looks like we only ever set and never subsequently read from
PaintLayer.ancestor_overflow_paint_layer. Can it be removed (in separate patch)?

Removed.

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.h (right):

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.h:59: static void
ClearPreviousPaintingClipRectsForTesting(const LayoutObject&);
On 2017/05/01 at 20:29:24, wkorman wrote:
> Same as earlier re: whether we need Painting in the names.

Done. Also changed the names of the PaintLayer methods.

[chrishtr, 2017-05-01 21:44:08]

The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 21:44:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chrishtr, 2017-05-01 21:54:39]

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right):

https://codereview.chromium.org/2854493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:173: object,
context.ancestor_transformed_or_root_paint_layer, fragment,
On 2017/05/01 at 21:43:54, chrishtr wrote:
> On 2017/05/01 at 20:29:24, wkorman wrote:
> > I wondered why this member data name was so long and found:
> > 
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pre...
> > 
> > Reading that, it looks like we only ever set and never subsequently read
from PaintLayer.ancestor_overflow_paint_layer. Can it be removed (in separate
patch)?
> 
> Removed.

I was wrong, it is needed. It is set on PaintLayer via
UpdateAncestorOverflowLayer, which is then
used by sticky constraints code.

[wkorman, 2017-05-01 21:58:10]

lgtm

https://codereview.chromium.org/2854493002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2854493002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1830: // Each LayoutObject
has one or more painting fragments (exactly one
Not for this change, but to consider -- fragment data is currently stored in
RareData but it sounds like it's not actually very rare any more.

Perhaps we should move it to a straight member data.

https://codereview.chromium.org/2854493002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/FragmentData.h (right):

https://codereview.chromium.org/2854493002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/FragmentData.h:37: previous_clip_rects_ =
&clip_rects;
Same comment re: RefPtr vs PassRefPtr.

https://codereview.chromium.org/2854493002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.h (right):

https://codereview.chromium.org/2854493002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.h:933: previous_clip_rects_ =
&clip_rects;
Will this do the right thing re: RefPtr? I'd think we have to pass PassRefPtr as
param for the counting to do what's intended.

My concern is re: who owns the memory allocated for the inputted clip_rects. Do
we have any assurance the caller won't deallocate but we'll keep our ref here.

But I actually couldn't find any documentation on PassRefPtr or RefPtr just now
when I looked.

https://codereview.chromium.org/2854493002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right):

https://codereview.chromium.org/2854493002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:50: // The ancestor in
the PaintLayer tree which hasa transform or is a root
has a

[chrishtr, 2017-05-01 22:40:06]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-05-01 22:40:07]

https://codereview.chromium.org/2854493002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.h (right):

https://codereview.chromium.org/2854493002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.h:933: previous_clip_rects_ =
&clip_rects;
On 2017/05/01 at 21:58:09, wkorman wrote:
> Will this do the right thing re: RefPtr? I'd think we have to pass PassRefPtr
as param for the counting to do what's intended.
> 
> My concern is re: who owns the memory allocated for the inputted clip_rects.
Do we have any assurance the caller won't deallocate but we'll keep our ref
here.
> 
> But I actually couldn't find any documentation on PassRefPtr or RefPtr just
now when I looked.

The memory is owned by the PaintLayer or FragmentData. It does work but I agree
it's a bit
weird looking. I had to research it when I first found it also.

https://codereview.chromium.org/2854493002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right):

https://codereview.chromium.org/2854493002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:50: // The ancestor in
the PaintLayer tree which hasa transform or is a root
On 2017/05/01 at 21:58:09, wkorman wrote:
> has a

Done.

[chrishtr, 2017-05-01 22:40:07]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2854493002/#ps120001
(title: "none")

[chrishtr, 2017-05-01 22:40:31]

https://codereview.chromium.org/2854493002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2854493002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1830: // Each LayoutObject
has one or more painting fragments (exactly one
On 2017/05/01 at 21:58:09, wkorman wrote:
> Not for this change, but to consider -- fragment data is currently stored in
RareData but it sounds like it's not actually very rare any more.
> 
> Perhaps we should move it to a straight member data.

FYI it's only for PaintLayers that it is set, so it is somewhat rare.

[commit-bot: I haz the power, 2017-05-01 22:40:43]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 00:42:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 00:42:36]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[chrishtr, 2017-05-02 01:00:42]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-05-02 01:00:42]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2854493002/#ps140001
(title: "none")

[commit-bot: I haz the power, 2017-05-02 01:01:19]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 02:48:06]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493686842230170,
"parent_rev": "d9978fcc021c14d5260d1abffc2d0b39e41018db", "commit_rev":
"32bf53031ff05bfc394accd43c1d4f36399bf3f6"}

[commit-bot: I haz the power, 2017-05-02 02:48:29]

Description was changed from

==========
Store previous painting clip rects on FragmentData.

Add a NextFragment method on FragmentData in anticipation of support
for multiple fragments.

Use this to make InvalidatePaintLayerOptimizationsIfNeeded multicol
fragment-aware.

BUG=648274
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Store previous painting clip rects on FragmentData.

Add a NextFragment method on FragmentData in anticipation of support
for multiple fragments.

Use this to make InvalidatePaintLayerOptimizationsIfNeeded multicol
fragment-aware.

BUG=648274
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2854493002
Cr-Commit-Position: refs/heads/master@{#468540}
Committed:
https://chromium.googlesource.com/chromium/src/+/32bf53031ff05bfc394accd43c1d...
==========

[commit-bot: I haz the power, 2017-05-02 02:48:31]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/32bf53031ff05bfc394accd43c1d...