Ensure SVG PaintOffset is reset at the HTML/SVG boundary [spv2]

Ensure SVG PaintOffset is reset at the HTML/SVG boundary [spv2]

This patch fixes a crash on https://codepen.io where the paint offset
in PaintPropertyTreeBuilder::updateSvgLocalToBorderBoxTransform would
not be reset if transformToPixelSnappedBorderBox was identity.

The new testcase, SvgPixelSnappingShouldResetPaintOffset, both ensures
the DCHECK in PaintPropertyTreeBuilder::updateTransform is not hit, and
verifies the transform property tree is correct in this case.

BUG=600618
Committed: https://crrev.com/306affbd20fa8e16b8dd7677061fabde355ead8d
Cr-Commit-Position: refs/heads/master@{#404483}

[pdr., 2016-07-08 01:49:24]

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

[pdr., 2016-07-08 01:49:29]

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

[commit-bot: I haz the power, 2016-07-08 01:49:53]

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, 2016-07-08 03:23:53]

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

[commit-bot: I haz the power, 2016-07-08 03:23:53]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2016-07-08 15:07:03]

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

https://codereview.chromium.org/2131803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:266: if
(transformToBorderBox.isIdentity()) {
What does paintOffset being zero have to do with transformToBorderBox being
identity? Is identity an indirect way to check
for the boundary between SVG and HTML?

Also, why did it crash before this CL?

[pdr., 2016-07-08 17:19:29]

On 2016/07/08 at 15:07:03, chrishtr wrote:
>
https://codereview.chromium.org/2131803002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/2131803002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:266: if
(transformToBorderBox.isIdentity()) {
> What does paintOffset being zero have to do with transformToBorderBox being
identity? Is identity an indirect way to check
> for the boundary between SVG and HTML?

transformToBorderBox includes pixel snapping caused by the paint offset. If
there is a small paint offset like (0.1, 0), this will be pixel snapped to (0,
0). When this happens, the transformToBorderBox will actually be identity,
meaning the paint offset is just ignored. updateSvgLocalToBorderBoxTransform
should always include the paint offset and reset it to (0, 0) but we were
missing that for the early-out of an identity transform.

> 
> Also, why did it crash before this CL?

This crashed because of the DCHECK in PaintPropertyTreeBuilder::updateTransform:
// SVG does not use paint offset internally.
DCHECK(context.paintOffset == LayoutPoint());

[chrishtr, 2016-07-08 17:40:21]

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

https://codereview.chromium.org/2131803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:269:
context.paintOffset = LayoutPoint();
How about just moving this line up above 266 then, deleting line 276, and adding
a comment that paint offset is not used
within SVG, and it's instead stored in |transformToBorderBox|. paintOffset is
not used after line 265.

[pdr., 2016-07-08 18:04:40]

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

https://codereview.chromium.org/2131803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:269:
context.paintOffset = LayoutPoint();
On 2016/07/08 at 17:40:21, chrishtr wrote:
> How about just moving this line up above 266 then, deleting line 276, and
adding a comment that paint offset is not used
> within SVG, and it's instead stored in |transformToBorderBox|. paintOffset is
not used after line 265.

Sounds reasonable, done.

PTAL

[chrishtr, 2016-07-08 18:05:27]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-07-08 18:05:27]

lgtm

[commit-bot: I haz the power, 2016-07-08 18:06:11]

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-07-08 20:46:10]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-08 20:47:53]

Description was changed from

==========
Ensure SVG PaintOffset is reset at the HTML/SVG boundary [spv2]

This patch fixes a crash on https://codepen.io where the paint offset
in PaintPropertyTreeBuilder::updateSvgLocalToBorderBoxTransform would
not be reset if transformToPixelSnappedBorderBox was identity.

The new testcase, SvgPixelSnappingShouldResetPaintOffset, both ensures
the DCHECK in PaintPropertyTreeBuilder::updateTransform is not hit, and
verifies the transform property tree is correct in this case.

BUG=600618
==========

to

==========
Ensure SVG PaintOffset is reset at the HTML/SVG boundary [spv2]

This patch fixes a crash on https://codepen.io where the paint offset
in PaintPropertyTreeBuilder::updateSvgLocalToBorderBoxTransform would
not be reset if transformToPixelSnappedBorderBox was identity.

The new testcase, SvgPixelSnappingShouldResetPaintOffset, both ensures
the DCHECK in PaintPropertyTreeBuilder::updateTransform is not hit, and
verifies the transform property tree is correct in this case.

BUG=600618

Committed: https://crrev.com/306affbd20fa8e16b8dd7677061fabde355ead8d
Cr-Commit-Position: refs/heads/master@{#404483}
==========

[commit-bot: I haz the power, 2016-07-08 20:47:58]

Patchset 2 (id:??) landed as
https://crrev.com/306affbd20fa8e16b8dd7677061fabde355ead8d
Cr-Commit-Position: refs/heads/master@{#404483}