Put SVGResourceContainer subtypes into their own property trees.

Put SVGResourceContainer subtypes into their own property trees.

These objects are always painted into other places in SVG, which should ignore
any property tree state above the container.

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

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

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

Description was changed from

==========
none


none


none


none


none


none

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

to

==========
none


none


none


none


none


none

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

[chrishtr, 2017-03-21 21:32:44]

Description was changed from

==========
none


none


none


none


none


none

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

to

==========
Put SVGResourceContainer subtypes into their own property trees.

These objects are always painted into other places in SVG, which should ignore
any property tree state above the container.

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

[chrishtr, 2017-03-21 21:33:02]

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

[chrishtr, 2017-03-21 21:33:06]

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

[commit-bot: I haz the power, 2017-03-21 21:33:25]

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

[pdr., 2017-03-21 22:26:22]

I like this approach

https://codereview.chromium.org/2768453003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2768453003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1102:
resetToRootContext(context);
This re-use of setupInitialContext is clever. I think we will need to make it a
little more robust for your usecase though. For example, paintOffset,
shouldFlattenInheritedTransform, containerForAbsolutePosition, and
paintOffsetForFloat need to be reset.

[commit-bot: I haz the power, 2017-03-21 22:44:29]

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

[commit-bot: I haz the power, 2017-03-21 22:44:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chrishtr, 2017-03-21 23:29:57]

https://codereview.chromium.org/2768453003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2768453003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1102:
resetToRootContext(context);
On 2017/03/21 at 22:26:22, pdr. wrote:
> This re-use of setupInitialContext is clever. I think we will need to make it
a little more robust for your usecase though. For example, paintOffset,
shouldFlattenInheritedTransform, containerForAbsolutePosition, and
paintOffsetForFloat need to be reset.

Changed to reuse the constructor, which will get these for free.

[pdr., 2017-03-21 23:31:12]

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

https://codereview.chromium.org/2768453003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:31:
currentEffect(nullptr),
Can you write this as currentEffect(EffectPaintPropertyNode::root()) ?

[chrishtr, 2017-03-21 23:39:17]

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

https://codereview.chromium.org/2768453003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:31:
currentEffect(nullptr),
On 2017/03/21 at 23:31:12, pdr. wrote:
> Can you write this as currentEffect(EffectPaintPropertyNode::root()) ?

Done.

[pdr., 2017-03-21 23:41:05]

LGTM

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

https://codereview.chromium.org/2768453003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:32:
inputClipOfCurrentEffect(nullptr),
can you change this to inputClipOfCurrentEffect(ClipPaintPropertyNode::root())?

[chrishtr, 2017-03-21 23:43:07]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-03-21 23:43:07]

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

https://codereview.chromium.org/2768453003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:32:
inputClipOfCurrentEffect(nullptr),
On 2017/03/21 at 23:41:05, pdr. wrote:
> can you change this to
inputClipOfCurrentEffect(ClipPaintPropertyNode::root())?

Done.

[chrishtr, 2017-03-21 23:43:08]

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

[commit-bot: I haz the power, 2017-03-21 23:43:33]

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-22 00:53:40]

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

[commit-bot: I haz the power, 2017-03-22 00:53:41]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chrishtr, 2017-03-22 18:18:55]

PTAL at the rebaseline results. These changed because of the change of property
tree state.

However, invalidation of SVG resource container subtrees is a bit of a strange
snowflake,
because they don't have any specific raster location on screen. When they
change, invalidation
of their embedding location happens via SVGResourcesCache. Therefore, the actual
rects
for these objects don't really matter and also don't really make any sense.

I investigated stopping paint invalidation for them for this reason, but this
doesn't work
cleanly, because:

a. These resources still need to get their display item clients invalidated when
they change
b. I'd have to add a bit to the paint invalidation tree walk saying "don't
raster invalidate
the subtree since I'm already in an SVG resource container". I'm not sure this
situation is
common enough to optimize for.

(b) might still be a good idea, but I'd prefer to pursue that in a followup.

[chrishtr, 2017-03-22 18:19:54]

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

[commit-bot: I haz the power, 2017-03-22 18:20: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-22 20:15:56]

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

[commit-bot: I haz the power, 2017-03-22 20:15:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chrishtr, 2017-03-22 23:45:36]

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

[chrishtr, 2017-03-22 23:45:37]

Changed my mind. I added the ability to force "no raster invalidation" for a
subtree
contained within a LayoutSVGResourceContainer.

Also, I observed that in some cases declarative content is an
LayoutSVGHiddenContainer, e.g.

<symbol id="symbol">
  <rect stroke-width="1px" fill="url(#Pattern)" stroke="navy" width="400"
height="400"/>
</symbol>

Should this CL use LayoutSVGHiddenContainer instead of
LayoutSVGResourceContainer?

[pdr., 2017-03-23 17:25:29]

On 2017/03/22 at 23:45:37, chrishtr wrote:
> Changed my mind. I added the ability to force "no raster invalidation" for a
subtree
> contained within a LayoutSVGResourceContainer.
> 
> Also, I observed that in some cases declarative content is an
LayoutSVGHiddenContainer, e.g.
> 
> <symbol id="symbol">
>   <rect stroke-width="1px" fill="url(#Pattern)" stroke="navy" width="400"
height="400"/>
> </symbol>
> 
> Should this CL use LayoutSVGHiddenContainer instead of
LayoutSVGResourceContainer?

Yeah, I think we should treat symbol like marker.

[pdr., 2017-03-23 17:25:36]

https://codereview.chromium.org/2768453003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintInvalidator.h (right):

https://codereview.chromium.org/2768453003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidator.h:57:
ForcedSubtreeNoRasterInvalidation = 1 << 6,
Can you add a comment describing this a little?

[Xianzhu, 2017-03-23 17:54:16]

lgtm.

https://codereview.chromium.org/2768453003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):

https://codereview.chromium.org/2768453003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:3339:
"refY='5' markerUnits='strokeWidth'>"
Nit: indent the continuation.

https://codereview.chromium.org/2768453003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:3347:
"marker-mid='url(#markerMiddle)' fill='none' stroke-width='8px' "
Nit: Ditto

[chrishtr, 2017-03-23 22:52:54]

https://codereview.chromium.org/2768453003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintInvalidator.h (right):

https://codereview.chromium.org/2768453003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidator.h:57:
ForcedSubtreeNoRasterInvalidation = 1 << 6,
On 2017/03/23 at 17:25:36, pdr. wrote:
> Can you add a comment describing this a little?

Done.

https://codereview.chromium.org/2768453003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):

https://codereview.chromium.org/2768453003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:3339:
"refY='5' markerUnits='strokeWidth'>"
On 2017/03/23 at 17:54:15, Xianzhu wrote:
> Nit: indent the continuation.

Done.

https://codereview.chromium.org/2768453003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:3347:
"marker-mid='url(#markerMiddle)' fill='none' stroke-width='8px' "
On 2017/03/23 at 17:54:15, Xianzhu wrote:
> Nit: Ditto

Done.

[chrishtr, 2017-03-23 23:09:21]

Updated to LayoutSVGHiddenContainer and added a corresponding unittest.

[pdr., 2017-03-23 23:10:39]

On 2017/03/23 at 23:09:21, chrishtr wrote:
> Updated to LayoutSVGHiddenContainer and added a corresponding unittest.

LGTM

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

The CQ bit was checked by chrishtr@chromium.org

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

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

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

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-24 22:41:13]

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

[commit-bot: I haz the power, 2017-03-24 22:41:14]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chrishtr, 2017-03-26 21:47:06]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-03-26 21:47:07]

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

[commit-bot: I haz the power, 2017-03-26 21:47:15]

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-26 22:41:23]

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

[commit-bot: I haz the power, 2017-03-26 22:41:24]

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

[chrishtr, 2017-03-26 22:54:15]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2017-03-26 22:54:23]

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-27 01:34:07]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1490568855039950,
"parent_rev": "a12e52d81346dd23d8284763d44c4ee657f11cea", "commit_rev":
"d364139fee76154a1d9fa8875ad0cbb5ccb523c3"}

[commit-bot: I haz the power, 2017-03-27 01:35:16]

Description was changed from

==========
Put SVGResourceContainer subtypes into their own property trees.

These objects are always painted into other places in SVG, which should ignore
any property tree state above the container.

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

to

==========
Put SVGResourceContainer subtypes into their own property trees.

These objects are always painted into other places in SVG, which should ignore
any property tree state above the container.

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

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

[commit-bot: I haz the power, 2017-03-27 01:35:17]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/d364139fee76154a1d9fa8875ad0...