Introduce Layout*::adjustVisualRectForRasterEffects and use it for SVG hairlines. (issue 2444593002 by chrishtr@chromium.org)

0 views
Skip to first unread message

chri...@chromium.org

unread,
Oct 21, 2016, 8:01:11 PM10/21/16
to p...@chromium.org, wangx...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, dsch...@chromium.org, eae+bli...@chromium.org, fma...@chromium.org, f...@opera.com, gyuyou...@chromium.org, jchaffraix...@chromium.org, kouhe...@chromium.org, leviw+re...@chromium.org, pdr+svgw...@chromium.org, pdr+renderi...@chromium.org, rob....@samsung.com, sche...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
Reviewers: pdr., Xianzhu
CL: https://codereview.chromium.org/2444593002/


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

https://codereview.chromium.org/2444593002/diff/20001/third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp#newcode52
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:52:
visualRect.expand(IntRectOutsets(2, 2, 2, 2));
Because this code doesn't understand anything except the bounding box of
the shape, it is forced
to unconditionally expand the rect.

Description:
Introduce Layout*::adjustVisualRectForRasterEffects and use it for SVG
hairlines.

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

Affected files (+434, -425 lines):
M third_party/WebKit/LayoutTests/paint/invalidation/svg/absolute-sized-document-no-scrollbars-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/deep-dynamic-updates-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/hairline-stroke-squarecap-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/js-late-marker-and-object-creation-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/js-late-marker-creation-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/js-repaint-rect-on-path-with-stroke-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/js-update-bounce-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/js-update-container-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/js-update-polygon-changes-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/js-update-polygon-removal-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/js-update-style-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/js-update-transform-addition-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/js-update-transform-changes-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/marker-child-changes-css-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/marker-child-changes-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/marker-strokeWidth-changes-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/marker-viewBox-changes-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/paintorder-filtered-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/path-pathlength-change-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/relative-sized-document-scrollbars-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/repaint-paintorder-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/repaint-stroke-width-changes-expected.txt
M third_party/WebKit/LayoutTests/paint/invalidation/svg/stroke-opacity-update-expected.txt
M third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/svg/animated-path-inside-transformed-html-expected.txt
M third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/svg/hit-test-with-br-expected.png
M third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/svg/js-late-clipPath-creation-expected.png
M third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/svg/relative-sized-content-with-resources-expected.txt
M third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/svg/tabgroup-expected.txt
M third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/svg/use-detach-expected.txt
M third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/svg/use-setAttribute-crash-expected.txt
M third_party/WebKit/LayoutTests/platform/win/paint/invalidation/svg/deep-nested-embedded-svg-size-changes-no-layout-triggers-1-expected.png
M third_party/WebKit/LayoutTests/platform/win/paint/invalidation/svg/deep-nested-embedded-svg-size-changes-no-layout-triggers-2-expected.png
M third_party/WebKit/LayoutTests/platform/win/paint/invalidation/svg/relative-sized-document-scrollbars-expected.png
M third_party/WebKit/LayoutTests/platform/win/paint/invalidation/svg/relative-sized-document-scrollbars-text-zoom-expected.png
M third_party/WebKit/LayoutTests/platform/win/paint/invalidation/svg/window-expected.txt
M third_party/WebKit/Source/core/layout/LayoutObject.h
M third_party/WebKit/Source/core/layout/LayoutObject.cpp
M third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.h
M third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp
M third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp
M third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.h


p...@chromium.org

unread,
Oct 21, 2016, 8:16:52 PM10/21/16
to chri...@chromium.org, wangx...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, dsch...@chromium.org, eae+bli...@chromium.org, fma...@chromium.org, f...@opera.com, gyuyou...@chromium.org, jchaffraix...@chromium.org, kouhe...@chromium.org, leviw+re...@chromium.org, pdr+svgw...@chromium.org, pdr+renderi...@chromium.org, rob....@samsung.com, sche...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
This was much simpler than I expected. Looking forward to the try bot results
for svg/

https://codereview.chromium.org/2444593002/

wangx...@chromium.org

unread,
Oct 21, 2016, 8:35:48 PM10/21/16
to chri...@chromium.org, p...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, dsch...@chromium.org, eae+bli...@chromium.org, fma...@chromium.org, f...@opera.com, gyuyou...@chromium.org, jchaffraix...@chromium.org, kouhe...@chromium.org, leviw+re...@chromium.org, pdr+svgw...@chromium.org, pdr+renderi...@chromium.org, rob....@samsung.com, sche...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

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

https://codereview.chromium.org/2444593002/diff/20001/third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp#newcode52
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:52:
visualRect.expand(IntRectOutsets(2, 2, 2, 2));
On 2016/10/22 00:01:11, chrishtr - OOO wrote:
> Because this code doesn't understand anything except the bounding box
of the
> shape, it is forced
> to unconditionally expand the rect.

It seems too much to expand by 2 pixels each side.

This could be more precise with:
if (styleRef().svgStyle().hasVisibleStroke()) {
LayoutUnit pad = 0.5;
if (styleRef().svgStyle().capStyle() != ButtCap)
pad += 0.5;
visualRect.inflate(pad);
}

https://codereview.chromium.org/2444593002/

chri...@chromium.org

unread,
Oct 22, 2016, 2:21:04 AM10/22/16
to p...@chromium.org, wangx...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, dsch...@chromium.org, eae+bli...@chromium.org, fma...@chromium.org, f...@opera.com, gyuyou...@chromium.org, jchaffraix...@chromium.org, kouhe...@chromium.org, leviw+re...@chromium.org, pdr+svgw...@chromium.org, pdr+renderi...@chromium.org, rob....@samsung.com, sche...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

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

https://codereview.chromium.org/2444593002/diff/20001/third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp#newcode52
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:52:
visualRect.expand(IntRectOutsets(2, 2, 2, 2));
On 2016/10/22 at 00:35:46, Xianzhu wrote:
> On 2016/10/22 00:01:11, chrishtr - OOO wrote:
> > Because this code doesn't understand anything except the bounding
box of the
> > shape, it is forced
> > to unconditionally expand the rect.
>
> It seems too much to expand by 2 pixels each side.
>
> This could be more precise with:
> if (styleRef().svgStyle().hasVisibleStroke()) {
> LayoutUnit pad = 0.5;
> if (styleRef().svgStyle().capStyle() != ButtCap)
> pad += 0.5;
> visualRect.inflate(pad);
> }

chri...@chromium.org

unread,
Oct 22, 2016, 3:32:08 PM10/22/16
to p...@chromium.org, wangx...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, dsch...@chromium.org, eae+bli...@chromium.org, fma...@chromium.org, f...@opera.com, gyuyou...@chromium.org, jchaffraix...@chromium.org, kouhe...@chromium.org, leviw+re...@chromium.org, pdr+svgw...@chromium.org, pdr+renderi...@chromium.org, rob....@samsung.com, sche...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
There are some failures in svg/, which are all changes to the layout tree as
text size of
LayoutSVGShape. This is because the method I removed code from in
SVGLayoutSupport is used
in mapToVisualRectInAncestorSpace, which in this case is used in (1) paint
invalidation and
(2) a couple of random cases for accessibility, and (3) SVGLayoutTreeAsText.

Therefore I think it's fine that these are smaller by 1px or so

https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_rel_ng/321922/layout-test-results/results.html
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel_ng/320552/layout-test-results/results.html
https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel_ng/317376/layout-test-results/results.html

https://codereview.chromium.org/2444593002/

chri...@chromium.org

unread,
Oct 22, 2016, 7:24:50 PM10/22/16
to p...@chromium.org, wangx...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, dsch...@chromium.org, eae+bli...@chromium.org, fma...@chromium.org, f...@opera.com, gyuyou...@chromium.org, jchaffraix...@chromium.org, kouhe...@chromium.org, leviw+re...@chromium.org, pdr+svgw...@chromium.org, pdr+renderi...@chromium.org, rob....@samsung.com, sche...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
The latest patchset passes the dry-run CQ.

Please double-check the changes to invalidation output as indicated on earlier
patchsets and
files changed in this patch.

I'm now off on vacation, so will likely not be able to take any action on this
patch for a week.
Hopefully it can just go in cleanly.

https://codereview.chromium.org/2444593002/

wangx...@chromium.org

unread,
Oct 24, 2016, 12:17:30 PM10/24/16
to chri...@chromium.org, p...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, dsch...@chromium.org, eae+bli...@chromium.org, fma...@chromium.org, f...@opera.com, gyuyou...@chromium.org, jchaffraix...@chromium.org, kouhe...@chromium.org, leviw+re...@chromium.org, pdr+svgw...@chromium.org, pdr+renderi...@chromium.org, rob....@samsung.com, sche...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
lgtm.

The layout test results look good.

As chrishtr is on vacation I'd like to land this first and address the comments
with another patch.

pdr@ wdyt?


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

https://codereview.chromium.org/2444593002/diff/3/third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp#newcode51
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:51: if
(styleRef().svgStyle().hasVisibleStroke()) {
We should add !visualRect.isEmpty() to avoid inflating an empty rect to
non-empty.

https://codereview.chromium.org/2444593002/diff/3/third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp#newcode52
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:52: float
pad = 0.5f;
Nit: s/float/LayoutUnit/

https://codereview.chromium.org/2444593002/

p...@chromium.org

unread,
Oct 24, 2016, 12:50:16 PM10/24/16
to chri...@chromium.org, wangx...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, dsch...@chromium.org, eae+bli...@chromium.org, fma...@chromium.org, f...@opera.com, gyuyou...@chromium.org, jchaffraix...@chromium.org, kouhe...@chromium.org, leviw+re...@chromium.org, pdr+svgw...@chromium.org, pdr+renderi...@chromium.org, rob....@samsung.com, sche...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
LGTM to land and fixup in a followup. Thank you!

https://codereview.chromium.org/2444593002/

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,
Oct 24, 2016, 12:59:03 PM10/24/16
to chri...@chromium.org, p...@chromium.org, wangx...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, dsch...@chromium.org, eae+bli...@chromium.org, fma...@chromium.org, f...@opera.com, gyuyou...@chromium.org, jchaffraix...@chromium.org, kouhe...@chromium.org, leviw+re...@chromium.org, pdr+svgw...@chromium.org, pdr+renderi...@chromium.org, rob....@samsung.com, sche...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,
Oct 24, 2016, 3:28:54 PM10/24/16
to chri...@chromium.org, p...@chromium.org, wangx...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, dsch...@chromium.org, eae+bli...@chromium.org, fma...@chromium.org, f...@opera.com, gyuyou...@chromium.org, jchaffraix...@chromium.org, kouhe...@chromium.org, leviw+re...@chromium.org, pdr+svgw...@chromium.org, pdr+renderi...@chromium.org, rob....@samsung.com, sche...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,
Oct 24, 2016, 3:48:32 PM10/24/16
to chri...@chromium.org, p...@chromium.org, wangx...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, dsch...@chromium.org, eae+bli...@chromium.org, fma...@chromium.org, f...@opera.com, gyuyou...@chromium.org, jchaffraix...@chromium.org, kouhe...@chromium.org, leviw+re...@chromium.org, pdr+svgw...@chromium.org, pdr+renderi...@chromium.org, rob....@samsung.com, sche...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
Committed patchset #4 (id:3)

https://codereview.chromium.org/2444593002/

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,
Oct 24, 2016, 3:56:03 PM10/24/16
to chri...@chromium.org, p...@chromium.org, wangx...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, dsch...@chromium.org, eae+bli...@chromium.org, fma...@chromium.org, f...@opera.com, gyuyou...@chromium.org, jchaffraix...@chromium.org, kouhe...@chromium.org, leviw+re...@chromium.org, pdr+svgw...@chromium.org, pdr+renderi...@chromium.org, rob....@samsung.com, sche...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
Patchset 4 (id:??) landed as
https://crrev.com/e0679d9453c26e2c2c1de1d3bfefadc7f83a364a
Cr-Commit-Position: refs/heads/master@{#427125}

https://codereview.chromium.org/2444593002/

chri...@chromium.org

unread,
Oct 31, 2016, 4:52:01 PM10/31/16
to p...@chromium.org, wangx...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, dsch...@chromium.org, eae+bli...@chromium.org, fma...@chromium.org, f...@opera.com, gyuyou...@chromium.org, jchaffraix...@chromium.org, kouhe...@chromium.org, leviw+re...@chromium.org, pdr+svgw...@chromium.org, pdr+renderi...@chromium.org, rob....@samsung.com, sche...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

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

https://codereview.chromium.org/2444593002/diff/3/third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp#newcode51
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:51: if
(styleRef().svgStyle().hasVisibleStroke()) {
On 2016/10/24 at 16:17:30, Xianzhu wrote:
> We should add !visualRect.isEmpty() to avoid inflating an empty rect
to non-empty.

Verified done by wangxianzhu.


https://codereview.chromium.org/2444593002/diff/3/third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp#newcode52
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:52: float
pad = 0.5f;
On 2016/10/24 at 16:17:30, Xianzhu wrote:
> Nit: s/float/LayoutUnit/


Verified done by wangxianzhu.

https://codereview.chromium.org/2444593002/
Reply all
Reply to author
Forward
0 new messages