Fix under-invalidation on SVG shape/image resize in subpixels

Fix under-invalidation on SVG shape/image resize in subpixels

Previously we detected SVG shape/image geometry changes for paint
invalidation by checking change of paint invalidation bounds. This
is accurate because the bounds may be expanded to hide subpixel
changes.

Now mark the object for full paint invalidation during layout if
geometry changes.

BUG=660195
Committed: https://crrev.com/4091f74e621de0fc0d59bfb30311c7a04a51cd8b
Cr-Commit-Position: refs/heads/master@{#428956}

[Xianzhu, 2016-10-28 23:32:08]

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

[commit-bot: I haz the power, 2016-10-28 23:32:24]

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-10-29 00:31:13]

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

[commit-bot: I haz the power, 2016-10-29 00:31:14]

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_...)

[Xianzhu, 2016-10-31 16:05:51]

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

[commit-bot: I haz the power, 2016-10-31 16:06:04]

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

[Xianzhu, 2016-10-31 16:28:16]

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

[commit-bot: I haz the power, 2016-10-31 16:28:31]

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

[Xianzhu, 2016-10-31 16:28:42]

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

[commit-bot: I haz the power, 2016-10-31 18:20:05]

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

[commit-bot: I haz the power, 2016-10-31 18:20:06]

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_...)

[pdr., 2016-10-31 20:49:05]

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

https://codereview.chromium.org/2458233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:111: if
(oldBoundaries != m_objectBoundingBox)
Can we use m_needsBoundariesUpdate here?

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

https://codereview.chromium.org/2458233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:185: // the
shape-update and the bounds-update flag, since .
Not your fault (https://crrev.com/fe8421b9573ca696e596248811edf0a5d23fc191) but
can you fix this broken comment:
// the shape-update and the bounds-update flag.

(remove ", since ")

https://codereview.chromium.org/2458233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:187: FloatRect
oldStrokeBoundingBox = strokeBoundingBox();
Are you using the stroke bounding box because it includes the object bounding
box too?

If so, could you add that as a comment?
// We use the stroke bounding box because it includes both the stroke and the
object bounding box.

[Xianzhu, 2016-10-31 22:10:31]

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

https://codereview.chromium.org/2458233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:111: if
(oldBoundaries != m_objectBoundingBox)
On 2016/10/31 20:49:05, pdr. wrote:
> Can we use m_needsBoundariesUpdate here?

It may be true before this check.

Combined the two checks into one 'if'.

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

https://codereview.chromium.org/2458233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:185: // the
shape-update and the bounds-update flag, since .
On 2016/10/31 20:49:05, pdr. wrote:
> Not your fault (https://crrev.com/fe8421b9573ca696e596248811edf0a5d23fc191)
but
> can you fix this broken comment:
> // the shape-update and the bounds-update flag.
> 
> (remove ", since ")

The fix is in https://codereview.chromium.org/2465983002/ :)

https://codereview.chromium.org/2458233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:187: FloatRect
oldStrokeBoundingBox = strokeBoundingBox();
On 2016/10/31 20:49:05, pdr. wrote:
> Are you using the stroke bounding box because it includes the object bounding
> box too?
> 
> If so, could you add that as a comment?
> // We use the stroke bounding box because it includes both the stroke and the
> object bounding box.

Used strokeBoundingBox just because it is used for paint invalidation below.
This should be correct[1] but using objectBoundingBox is more intuitive. Changed
to objectBoundingBox.

[1] It's correct because a) it catches object bounding box change in most cases,
b) if object bounding box changes but other changes (e.g. stroke width change)
happen to make stroke bounding box not change, other changes will cause paint
invalidation.

[pdr., 2016-11-01 01:35:24]

LGTM

[Xianzhu, 2016-11-01 05:34:07]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-11-01 05:34:14]

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-11-01 07:24:53]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-01 07:26:49]

Description was changed from

==========
Fix under-invalidation on SVG shape/image resize in subpixels

Previously we detected SVG shape/image geometry changes for paint
invalidation by checking change of paint invalidation bounds. This
is accurate because the bounds may be expanded to hide subpixel
changes.

Now mark the object for full paint invalidation during layout if
geometry changes.

BUG=660195
==========

to

==========
Fix under-invalidation on SVG shape/image resize in subpixels

Previously we detected SVG shape/image geometry changes for paint
invalidation by checking change of paint invalidation bounds. This
is accurate because the bounds may be expanded to hide subpixel
changes.

Now mark the object for full paint invalidation during layout if
geometry changes.

BUG=660195

Committed: https://crrev.com/4091f74e621de0fc0d59bfb30311c7a04a51cd8b
Cr-Commit-Position: refs/heads/master@{#428956}
==========

[commit-bot: I haz the power, 2016-11-01 07:26:50]

Patchset 4 (id:??) landed as
https://crrev.com/4091f74e621de0fc0d59bfb30311c7a04a51cd8b
Cr-Commit-Position: refs/heads/master@{#428956}