[SPv2] Flatten property trees in PaintRecordBuilder into a single display list.

[SPv2] Flatten property trees in PaintRecordBuilder into a single cc::DisplayItemList.

This has the effect of making SPv2 no longer depend at all on paired display
items, which have no effect on the SPv2 compositing path.

As a result, updated all call sites of PaintRecordBuilder to not use
paired display items, and instead simply use canvas or GraphicsContext
commands.

This will allow us to disable paired display items in (blink-side) SPv2 in
a future CL.

BUG=665259
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2751433002
Cr-Commit-Position: refs/heads/master@{#460277}
Committed: https://chromium.googlesource.com/chromium/src/+/f249b473be53eb04af58e4fa38ce896bba643946

[chrishtr, 2017-03-13 16:59:52]

Description was changed from

==========
none

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

to

==========
none

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

[chrishtr, 2017-03-13 16:59:53]

Description was changed from

==========
none

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

to

==========
none

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-03-13 17:00:52]

Description was changed from

==========
none

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[SPv2] Flatten property trees in PaintRecordBuilder into a single display list.

BUG=665259
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-03-27 23:17:37]

Description was changed from

==========
[SPv2] Flatten property trees in PaintRecordBuilder into a single display list.

BUG=665259
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[SPv2] Flatten property trees in PaintRecordBuilder into a single display list.

Update all callsites to not have any extra paired display items either.

BUG=665259
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-03-27 23:18:25]

Description was changed from

==========
[SPv2] Flatten property trees in PaintRecordBuilder into a single display list.

Update all callsites to not have any extra paired display items either.

BUG=665259
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[SPv2] Flatten property trees in PaintRecordBuilder into a single display list.

Updated all SVG callsites to not have any extra paired display items either.

BUG=665259
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-03-27 23:18:30]

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

[commit-bot: I haz the power, 2017-03-27 23:19:19]

Dry run: 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-03-28 00:11:30]

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

[commit-bot: I haz the power, 2017-03-28 00:11:31]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[chrishtr, 2017-03-28 02:06:11]

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

[commit-bot: I haz the power, 2017-03-28 02:06:49]

Dry run: 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-03-28 03:03:49]

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

[commit-bot: I haz the power, 2017-03-28 03:03:50]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[chrishtr, 2017-03-28 03:48:40]

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

[commit-bot: I haz the power, 2017-03-28 03:49:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chrishtr, 2017-03-28 03:54:57]

Description was changed from

==========
[SPv2] Flatten property trees in PaintRecordBuilder into a single display list.

Updated all SVG callsites to not have any extra paired display items either.

BUG=665259
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[SPv2] Flatten property trees in PaintRecordBuilder into a single
cc::DisplayItemList.

This has the effect of making SPv2 no longer depend at all on paired display
items, which have no effect on the SPv2 compositing path.

As a result, updated all call sites of PaintRecordBuilder to not use
paired display items, and instead simply use canvas or GraphicsContext
commands.

This will allow us to disable paired display items in (blink-side) SPv2 in
a future CL.

BUG=665259
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-03-28 03:57:49]

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

[chrishtr, 2017-03-28 03:57:51]

https://codereview.chromium.org/2751433002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right):

https://codereview.chromium.org/2751433002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:367:
paintRecordForCurrentFrame(floatBounds, url), SkShader::kRepeat_TileMode,
In this call site, the previous code applied no transform because
the src and dest bounds were the same. Also, I believe that there is an
implicit clip to the bounds passed to MakePictureShader, so the clip
that used to be applied also is no longer relevant.

[commit-bot: I haz the power, 2017-03-28 04:44:41]

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

[commit-bot: I haz the power, 2017-03-28 04:44:41]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[chrishtr, 2017-03-28 19:26:51]

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

[commit-bot: I haz the power, 2017-03-28 19:27:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chrishtr, 2017-03-28 19:28:00]

Updated to fix a couple of small bugs in the printing code and add some
TestExpectations
files for subpixel adjustments to printed output. Patchset 14 has the diffs for
those,
and I added one example to the latest patchset.

[pdr., 2017-03-28 21:01:31]

Very nice patch. Just nits because it's all I could find.

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:1383: #
missing code to account for CompositingRecorder in SPv2 mode, likely).
Nit:
# (Likely missing code to account for CompositingRecorder)

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:1142: crbug.com/665259
fast/borders/border-inner-bleed.html [ NeedsRebaseline ]
What are these new SPV1 differences from?

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:110: // Converts from bounds
in CSS space to device space based on the given
Can we DCHECK in the paired display item code that they are not used if spv2 is
on?

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:138:
transform.scale(deviceScaleFactor * pageScaleFactor,
supernit: transform.scale(deviceScaleFactor * pageScaleFactor) and save a line!

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGFilterPainter.cpp (right):

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGFilterPainter.cpp:33:
m_paintController->updateCurrentPaintChunkProperties(nullptr, state);
Can you use PropertyTreeState::root here?

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.cpp
(right):

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.cpp:96:
Vector<const PaintChunk*> pointerPaintChunks;
To prevent growing and copying this vector, please add:
pointerPaintChunks.reserveInitialCapacity(paintChunks().size());

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintRecordBuilder.cpp
(right):

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintRecordBuilder.cpp:31:
m_paintController->updateCurrentPaintChunkProperties(
Some of the comments in PaintRecordBuilder.h seem a little stale now. Can you do
a quick pass to make sure they are correct?

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h
(right):

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h:32:
PropertyTreeState(const PropertyTreeState& other) = default;
I haven't seen this before but just read about it and it is cool. Is it needed
though?

[commit-bot: I haz the power, 2017-03-28 21:15:04]

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

[commit-bot: I haz the power, 2017-03-28 21:15:05]

Dry run: CQ has no permission to schedule in bucket
master.tryserver.chromium.linux

[chrishtr, 2017-03-28 21:43:24]

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:1383: #
missing code to account for CompositingRecorder in SPv2 mode, likely).
On 2017/03/28 at 21:01:30, pdr. wrote:
> Nit:
> # (Likely missing code to account for CompositingRecorder)

Done.

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:1142: crbug.com/665259
fast/borders/border-inner-bleed.html [ NeedsRebaseline ]
On 2017/03/28 at 21:01:30, pdr. wrote:
> What are these new SPV1 differences from?

They are because we are using methods on GraphicsContext or PaintCanvas
directly, rather than relying on replay through an intermediate data structure,
which changes floating-point output somehow.

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:110: // Converts from bounds
in CSS space to device space based on the given
On 2017/03/28 at 21:01:30, pdr. wrote:
> Can we DCHECK in the paired display item code that they are not used if spv2
is on?

Yes, but even with this patch they are still used, just not in
PaintRecordBuilder use-cases. I'd like to defer this to a follow-on patch
which will actually disable them.

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:138:
transform.scale(deviceScaleFactor * pageScaleFactor,
On 2017/03/28 at 21:01:30, pdr. wrote:
> supernit: transform.scale(deviceScaleFactor * pageScaleFactor) and save a
line!

Done.

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGFilterPainter.cpp (right):

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGFilterPainter.cpp:33:
m_paintController->updateCurrentPaintChunkProperties(nullptr, state);
On 2017/03/28 at 21:01:30, pdr. wrote:
> Can you use PropertyTreeState::root here?

Done. Also surveyed the code and converted a few more other cases

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.cpp
(right):

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.cpp:96:
Vector<const PaintChunk*> pointerPaintChunks;
On 2017/03/28 at 21:01:30, pdr. wrote:
> To prevent growing and copying this vector, please add:
> pointerPaintChunks.reserveInitialCapacity(paintChunks().size());

Done.

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintRecordBuilder.cpp
(right):

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintRecordBuilder.cpp:31:
m_paintController->updateCurrentPaintChunkProperties(
On 2017/03/28 at 21:01:30, pdr. wrote:
> Some of the comments in PaintRecordBuilder.h seem a little stale now. Can you
do a quick pass to make sure they are correct?

Done.

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h
(right):

https://codereview.chromium.org/2751433002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h:32:
PropertyTreeState(const PropertyTreeState& other) = default;
On 2017/03/28 at 21:01:30, pdr. wrote:
> I haven't seen this before but just read about it and it is cool. Is it needed
though?

Removed, not needed in this version of the CL.

[chrishtr, 2017-03-28 21:44:58]

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

[commit-bot: I haz the power, 2017-03-28 21:45:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chrishtr, 2017-03-28 21:45:22]

I also removed the option to replay to a different property tree state in
PaintArtifact.
I think it is unecessary, but double-checking with tests now.

[pdr., 2017-03-28 21:49:20]

LGTM, killer patch

[chrishtr, 2017-03-28 21:51:59]

The CQ bit was unchecked by chrishtr@chromium.org

[chrishtr, 2017-03-28 21:52:00]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2017-03-28 21:52:24]

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

[trchen, 2017-03-28 22:28:29]

lgtm so far. Just confirming my understanding correct.

https://codereview.chromium.org/2751433002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintRecordBuilder.h
(right):

https://codereview.chromium.org/2751433002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintRecordBuilder.h:52: //
into PropertyTreeState::root() space.
Is it true we always flatten into PropertyTreeState::root() ?
Let's say we have something like this:

<div style="transform:foo; filter:blur(1px);">
   ABC
   <img src="some.svg"/>
   XYZ
</div>

Does that implies inside the frame boundary of 'some.svg', there will be a
separate property tree based on the root nodes?

From the perspective of the root frame, is it that the image element would emit
exactly one drawing display item associated with the foo transform node and blur
effect node, where the item's contents would be the SkPicture returned from the
flatten process?

[chrishtr, 2017-03-28 22:40:16]

https://codereview.chromium.org/2751433002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintRecordBuilder.h
(right):

https://codereview.chromium.org/2751433002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintRecordBuilder.h:52: //
into PropertyTreeState::root() space.
On 2017/03/28 at 22:28:29, trchen wrote:
> Is it true we always flatten into PropertyTreeState::root() ?
> Let's say we have something like this:
> 
> <div style="transform:foo; filter:blur(1px);">
>    ABC
>    <img src="some.svg"/>
>    XYZ
> </div>
> 
> Does that implies inside the frame boundary of 'some.svg', there will be a
separate property tree based on the root nodes?

It doesn't necessarily imply that, but (almost) all use-cases have that
property. Examples include rendering SVG sub-resources (which I added
"sub" property trees for in a patch a few days ago), SVG images (which 
are in their own document, printing, etc.

I don't think it's true in general for cases like drag images output by code in
LocalFrame. I intend to follow up with another patch that handles it with
a test. But note that this is a situation in which a "subtree" of the document
is painted into a separate output that doesn't need to be later combined
with the content above it. (It's also ill-defined for subtrees that are not
stacking context roots, which has led to bugs in the past..)

Note also that until the last patchset, previous ones had the ability to pass
a non-root PropertyTreeState to PaintArtifact::replay.

> 
> From the perspective of the root frame, is it that the image element would
emit exactly one drawing display item associated with the foo transform node and
blur effect node, where the item's contents would be the SkPicture returned from
the flatten process?

Yes. It's as if it was a regular painting phase of 'foo'.

[commit-bot: I haz the power, 2017-03-29 00:04:39]

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

[commit-bot: I haz the power, 2017-03-29 00:04:40]

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

[chrishtr, 2017-03-29 00:54:55]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-03-29 00:54:55]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2751433002/#ps360001
(title: "none")

[commit-bot: I haz the power, 2017-03-29 00:56:12]

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-03-29 02:57:49]

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

[commit-bot: I haz the power, 2017-03-29 02:57:50]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)

[chrishtr, 2017-03-29 03:19:25]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2017-03-29 03:20:12]

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-03-29 04:26:15]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1490757565421250,
"parent_rev": "5ed29ebcc4a6d9622c4f39a16c033ddac99f90a3", "commit_rev":
"f249b473be53eb04af58e4fa38ce896bba643946"}

[commit-bot: I haz the power, 2017-03-29 04:26:59]

Description was changed from

==========
[SPv2] Flatten property trees in PaintRecordBuilder into a single
cc::DisplayItemList.

This has the effect of making SPv2 no longer depend at all on paired display
items, which have no effect on the SPv2 compositing path.

As a result, updated all call sites of PaintRecordBuilder to not use
paired display items, and instead simply use canvas or GraphicsContext
commands.

This will allow us to disable paired display items in (blink-side) SPv2 in
a future CL.

BUG=665259
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[SPv2] Flatten property trees in PaintRecordBuilder into a single
cc::DisplayItemList.

This has the effect of making SPv2 no longer depend at all on paired display
items, which have no effect on the SPv2 compositing path.

As a result, updated all call sites of PaintRecordBuilder to not use
paired display items, and instead simply use canvas or GraphicsContext
commands.

This will allow us to disable paired display items in (blink-side) SPv2 in
a future CL.

BUG=665259
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2751433002
Cr-Commit-Position: refs/heads/master@{#460277}
Committed:
https://chromium.googlesource.com/chromium/src/+/f249b473be53eb04af58e4fa38ce...
==========

[commit-bot: I haz the power, 2017-03-29 04:27:01]

Committed patchset #19 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/f249b473be53eb04af58e4fa38ce...