Pixel snap SVG's root border box to local transform [spv2]

Pixel snap SVG's root border box to local transform [spv2]

This patch extracts out SVGRootPainter's pixel snapping logic into a
common function used by both SVGRootPainter and the property tree
builder. With this patch a DCHECK in SVGTransformContext can be removed
and 77 new tests pass with --enable-slimming-paint-v2.

BUG=600618
Committed: https://crrev.com/58a321c5c07599b51b9a80d00c497049ed9bd171
Cr-Commit-Position: refs/heads/master@{#403970}

[pdr., 2016-07-06 05:16:23]

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

[commit-bot: I haz the power, 2016-07-06 05:16:33]

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-06 06:58:11]

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

[commit-bot: I haz the power, 2016-07-06 06:58:12]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-07-06 17:22:45]

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

[trchen, 2016-07-06 17:32:31]

Looks great! lgtm.

[chrishtr, 2016-07-06 17:50:58]

lgtm

https://codereview.chromium.org/2128463002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/SVGRootPainter.h (right):

https://codereview.chromium.org/2128463002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/SVGRootPainter.h:25: // Convert from
container offsets (html/LayoutUnits) to our snapped local SVG transform space.
pixelSnapped?

[pdr., 2016-07-06 18:33:44]

https://codereview.chromium.org/2128463002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/SVGRootPainter.h (right):

https://codereview.chromium.org/2128463002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/SVGRootPainter.h:25: // Convert from
container offsets (html/LayoutUnits) to our snapped local SVG transform space.
On 2016/07/06 at 17:50:58, chrishtr wrote:
> pixelSnapped?

I wonder if we can start moving away from "pixel snapped" because we're really
not snapping to pixels in blink. For consistency with pixelSnappedIntRect, I can
see an argument for switching this to pixelSnapedPaintOffsetToBorderBox though.
WDUT?

[trchen, 2016-07-06 19:00:21]

https://codereview.chromium.org/2128463002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/SVGRootPainter.h (right):

https://codereview.chromium.org/2128463002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/SVGRootPainter.h:25: // Convert from
container offsets (html/LayoutUnits) to our snapped local SVG transform space.
On 2016/07/06 18:33:44, pdr. wrote:
> On 2016/07/06 at 17:50:58, chrishtr wrote:
> > pixelSnapped?
> I wonder if we can start moving away from "pixel snapped" because we're really
> not snapping to pixels in blink. For consistency with pixelSnappedIntRect, I
can
> see an argument for switching this to pixelSnapedPaintOffsetToBorderBox
though.
> WDUT?

Now I think of it, it is not really "snapped paint offset", the return value is
not even an offset. Perhaps stretchToSnappedBorderBoxFromUnsnapped or something
alike.

To be more precise, we are not snapping to physical pixels but CSS/DOM pixels. I
don't think "pixel snapped" is particularly wrong, but we do need a strong
signal to distinguish "layout snapping" and "raster snapping".

The comments here could be more verbose though. This transform here is specific
to SVG root, and deserves more explanation. Perhaps something like:

The embedded SVG document use unsnapped viewport box to layout, while SVG root's
border box ultimately gets snapped during paint. A translate/scale transform is
applied to compensate the difference.

[chrishtr, 2016-07-06 20:17:43]

On 2016/07/06 at 19:00:21, trchen wrote:
>
https://codereview.chromium.org/2128463002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/paint/SVGRootPainter.h (right):
> 
>
https://codereview.chromium.org/2128463002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/SVGRootPainter.h:25: // Convert from
container offsets (html/LayoutUnits) to our snapped local SVG transform space.
> On 2016/07/06 18:33:44, pdr. wrote:
> > On 2016/07/06 at 17:50:58, chrishtr wrote:
> > > pixelSnapped?
> > I wonder if we can start moving away from "pixel snapped" because we're
really
> > not snapping to pixels in blink. For consistency with pixelSnappedIntRect, I
can
> > see an argument for switching this to pixelSnapedPaintOffsetToBorderBox
though.
> > WDUT?
> 
> Now I think of it, it is not really "snapped paint offset", the return value
is not even an offset. Perhaps stretchToSnappedBorderBoxFromUnsnapped or
something alike.
> 
> To be more precise, we are not snapping to physical pixels but CSS/DOM pixels.
I don't think "pixel snapped" is particularly wrong, but we do need a strong
signal to distinguish "layout snapping" and "raster snapping".
> 
> The comments here could be more verbose though. This transform here is
specific to SVG root, and deserves more explanation. Perhaps something like:
> 
> The embedded SVG document use unsnapped viewport box to layout, while SVG
root's border box ultimately gets snapped during paint. A translate/scale
transform is applied to compensate the difference.

It's pixel-snapped in the same sense that pixelSnappedInt rect is, AFAICS, so
keeping that consistent is best for this patch.

[pdr., 2016-07-06 20:23:27]

https://codereview.chromium.org/2128463002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/SVGRootPainter.h (right):

https://codereview.chromium.org/2128463002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/SVGRootPainter.h:25: // Convert from
container offsets (html/LayoutUnits) to our snapped local SVG transform space.
On 2016/07/06 at 19:00:21, trchen wrote:
> On 2016/07/06 18:33:44, pdr. wrote:
> > On 2016/07/06 at 17:50:58, chrishtr wrote:
> > > pixelSnapped?
> > I wonder if we can start moving away from "pixel snapped" because we're
really
> > not snapping to pixels in blink. For consistency with pixelSnappedIntRect, I
can
> > see an argument for switching this to pixelSnapedPaintOffsetToBorderBox
though.
> > WDUT?
> 
> Now I think of it, it is not really "snapped paint offset", the return value
is not even an offset. Perhaps stretchToSnappedBorderBoxFromUnsnapped or
something alike.
> 
> To be more precise, we are not snapping to physical pixels but CSS/DOM pixels.
I don't think "pixel snapped" is particularly wrong, but we do need a strong
signal to distinguish "layout snapping" and "raster snapping".
> 
> The comments here could be more verbose though. This transform here is
specific to SVG root, and deserves more explanation. Perhaps something like:
> 
> The embedded SVG document use unsnapped viewport box to layout, while SVG
root's border box ultimately gets snapped during paint. A translate/scale
transform is applied to compensate the difference.

I updated the name to be "transformToPixelSnappedBorderBox" and wrote a slightly
tweaked version of your comment.

PTAL

[trchen, 2016-07-06 20:38:39]

lgtm again. (y)

[chrishtr, 2016-07-06 20:42:00]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-07-06 20:42:00]

lgtm

[commit-bot: I haz the power, 2016-07-06 20:42:51]

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-06 23:22:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-06 23:22:18]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-06 23:25:09]

Description was changed from

==========
Pixel snap SVG's root border box to local transform [spv2]

This patch extracts out SVGRootPainter's pixel snapping logic into a
common function used by both SVGRootPainter and the property tree
builder. With this patch a DCHECK in SVGTransformContext can be removed
and 77 new tests pass with --enable-slimming-paint-v2.

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

to

==========
Pixel snap SVG's root border box to local transform [spv2]

This patch extracts out SVGRootPainter's pixel snapping logic into a
common function used by both SVGRootPainter and the property tree
builder. With this patch a DCHECK in SVGTransformContext can be removed
and 77 new tests pass with --enable-slimming-paint-v2.

BUG=600618

Committed: https://crrev.com/58a321c5c07599b51b9a80d00c497049ed9bd171
Cr-Commit-Position: refs/heads/master@{#403970}
==========

[commit-bot: I haz the power, 2016-07-06 23:25:10]

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