SVG root viewport clip in paint property tree

SVG root viewport clip in paint property tree

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

Committed: https://crrev.com/00b1ddc91d630dfc1d836fbef415841dc01ae7da
Cr-Commit-Position: refs/heads/master@{#418747}

[Xianzhu, 2016-09-14 19:08:47]

Description was changed from

==========
SVG root viewport clip in paint property tree

BUG=637316
==========

to

==========
SVG root viewport clip in paint property tree

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

[Xianzhu, 2016-09-14 19:09:11]

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

[commit-bot: I haz the power, 2016-09-14 19:10:13]

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

[Xianzhu, 2016-09-14 19:11:11]

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

[pdr., 2016-09-14 19:27:12]

Cool! Can you add a new unit test and/or remove newly passing tests from
FlagExpectations/enable-slimming-paint-v2

[commit-bot: I haz the power, 2016-09-14 19:45:53]

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

[commit-bot: I haz the power, 2016-09-14 19:45:55]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[Xianzhu, 2016-09-14 21:29:56]

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

[commit-bot: I haz the power, 2016-09-14 21:31:05]

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

[Xianzhu, 2016-09-14 21:32:04]

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

[commit-bot: I haz the power, 2016-09-14 21:33:09]

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

[Xianzhu, 2016-09-14 21:36:00]

Added tests and updated enable-slimming-paint-v2.

There are new pixel failures about invisible diffs on borders, texts, etc.,
perhaps because of changed clipping method. No idea how.

https://codereview.chromium.org/2343673003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/svg/custom/viewport-update2-expected.txt
(right):

https://codereview.chromium.org/2343673003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/custom/viewport-update2-expected.txt:5:
LayoutSVGRect {rect} at (5,5) size 100x100 [fill={[type=SOLID] [color=#FF0000]}]
[x=-100.00] [y=-100.00] [width=300.00] [height=300.00]
This is because of the change in SVGLayoutSupport.cpp. Previously the viewport
clip is applied using the border box in SVGLayoutSupport.cpp, but content box in
other places (in painter etc.). Now use the same box for all clipping.

https://codereview.chromium.org/2343673003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGRootTest.cpp (right):

https://codereview.chromium.org/2343673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGRootTest.cpp:51:
EXPECT_EQ(LayoutRect(90, 90, 100, 20), rect);
This is also because of the change in SVGLayoutSupport.cpp.

[pdr., 2016-09-14 22:42:40]

Looks great overall.

For the new pixel differences--I think it's okay to figure them out in a
followup.

https://codereview.chromium.org/2343673003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):

https://codereview.chromium.org/2343673003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:3: # We
are focused on fast/, compositing/, and crbug.com/637316 svg/ with all other
directories skipped (for now).
Nit: I think you added this by mistake.

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

https://codereview.chromium.org/2343673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:27:
SVGLayoutSupport::mapToVisualRectInAncestorSpace(object, &ancestor, rect,
result);
If we override this in all SVG subclasses, will this special-case be needed? I'd
prefer if we could rely on this working through the
object.mapToVisualRectInAncestorSpace call below, but I'm not sure if it is
possible. We already override LayoutSVGBlock::mapToVisualRectInAncestorSpace, do
we just need to override it in LayoutSVGModelObject, LayoutSVGBlock, and
LayoutSVGInline too?

[commit-bot: I haz the power, 2016-09-14 23:00:31]

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

[commit-bot: I haz the power, 2016-09-14 23:00:32]

Dry run: 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_...)

[Xianzhu, 2016-09-14 23:35:31]

https://codereview.chromium.org/2343673003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):

https://codereview.chromium.org/2343673003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:3: # We
are focused on fast/, compositing/, and crbug.com/637316 svg/ with all other
directories skipped (for now).
On 2016/09/14 22:42:39, pdr. wrote:
> Nit: I think you added this by mistake.

Done.

It was caused by a bad global text replace.

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

https://codereview.chromium.org/2343673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:27:
SVGLayoutSupport::mapToVisualRectInAncestorSpace(object, &ancestor, rect,
result);
On 2016/09/14 22:42:39, pdr. wrote:
> If we override this in all SVG subclasses, will this special-case be needed?
I'd
> prefer if we could rely on this working through the
> object.mapToVisualRectInAncestorSpace call below, but I'm not sure if it is
> possible. We already override LayoutSVGBlock::mapToVisualRectInAncestorSpace,
do
> we just need to override it in LayoutSVGModelObject, LayoutSVGBlock, and
> LayoutSVGInline too?

We didn't override mapToVisualRectInAncestorSpace in other SVG objects because
they were never called. SVGLayoutSupport::mapToVisualRectInAncestorSpace() was
used by only LayoutSVGBlock::mapToVisualRectInAncestorSpace() (which may be
called from HTML descendants) and
SVGLayoutSupport::clippedOverflowRectForPaintInvalidation(). Its input rect is a
FloatRect so if we match it to LayoutObject::mapToVisualRectInAncestorSpace() we
may lose accuracy.

I would keep this special case for now because this code is temporary for
filters before paint property tree supports filters.

[pdr., 2016-09-14 23:51:50]

On 2016/09/14 at 23:35:31, wangxianzhu wrote:
>
https://codereview.chromium.org/2343673003/diff/40001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):
> 
>
https://codereview.chromium.org/2343673003/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:3: #
We are focused on fast/, compositing/, and crbug.com/637316 svg/ with all other
directories skipped (for now).
> On 2016/09/14 22:42:39, pdr. wrote:
> > Nit: I think you added this by mistake.
> 
> Done.
> 
> It was caused by a bad global text replace.
> 
>
https://codereview.chromium.org/2343673003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):
> 
>
https://codereview.chromium.org/2343673003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:27:
SVGLayoutSupport::mapToVisualRectInAncestorSpace(object, &ancestor, rect,
result);
> On 2016/09/14 22:42:39, pdr. wrote:
> > If we override this in all SVG subclasses, will this special-case be needed?
I'd
> > prefer if we could rely on this working through the
> > object.mapToVisualRectInAncestorSpace call below, but I'm not sure if it is
> > possible. We already override
LayoutSVGBlock::mapToVisualRectInAncestorSpace, do
> > we just need to override it in LayoutSVGModelObject, LayoutSVGBlock, and
> > LayoutSVGInline too?
> 
> We didn't override mapToVisualRectInAncestorSpace in other SVG objects because
they were never called. SVGLayoutSupport::mapToVisualRectInAncestorSpace() was
used by only LayoutSVGBlock::mapToVisualRectInAncestorSpace() (which may be
called from HTML descendants) and
SVGLayoutSupport::clippedOverflowRectForPaintInvalidation(). Its input rect is a
FloatRect so if we match it to LayoutObject::mapToVisualRectInAncestorSpace() we
may lose accuracy.
> 
> I would keep this special case for now because this code is temporary for
filters before paint property tree supports filters.

OK SG LGTM

[Xianzhu, 2016-09-15 00:06:45]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-09-15 00:06:46]

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/2343673003/#ps80001
(title: "-")

[commit-bot: I haz the power, 2016-09-15 00:07:57]

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, 2016-09-15 01:38:32]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-15 01:43:11]

Description was changed from

==========
SVG root viewport clip in paint property tree

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

to

==========
SVG root viewport clip in paint property tree

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

Committed: https://crrev.com/00b1ddc91d630dfc1d836fbef415841dc01ae7da
Cr-Commit-Position: refs/heads/master@{#418747}
==========

[commit-bot: I haz the power, 2016-09-15 01:43:12]

Patchset 5 (id:??) landed as
https://crrev.com/00b1ddc91d630dfc1d836fbef415841dc01ae7da
Cr-Commit-Position: refs/heads/master@{#418747}