Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(119)

Issue 2512493003: Avoid special handling of LayoutSVGForeignObject about its paint offset (Closed)

Created:
4 years, 1 month ago by Xianzhu
Modified:
4 years, 1 month ago
Reviewers:
pdr.
CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, rwlbuis, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, fs, krit, f(malita), jchaffraix+rendering, blink-reviews-paint_chromium.org, blink-reviews, gyuyoung2, Stephen Chennney, kouhei+svg_chromium.org, dshwang, pdr+svgwatchlist_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Avoid special handling of LayoutSVGForeignObject about its paint offset Previously we painted a LayoutSVGForeignObject in the following way: 1 Issue transform and clip based on localSVGTransform; 2 Call BlockFlowPainter::paint with zero LayoutPoint() 3 BlockFlowPainter::paint uses the object's location() as the actual paint offset. The above painting method also required us to generate the same paint properties in PaintPropertyTreeBuilder. This CL change painting of LayoutSVGForiegnObject in a new way: 1 Issue transform and clip based on localToSVGParentTransform which includes the viewport offset; 2 Call BlockFlowPainter::paint with -location() as the paint offset; 3 BlockFlowPainter::paint then add the object's location() to the paint offset which results a zero paint offset. With the new method, LayoutSVGForeignObject behaves more like a normal SVG object so we can remove some special handling of it. BUG=666416 x CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/74454dfeaa27a9f696824e0a2a1548175d2e26ab Cr-Commit-Position: refs/heads/master@{#433080}

Patch Set 1 #

Patch Set 2 : - #

Unified diffs Side-by-side diffs Delta from patch set Stats (+50 lines, -53 lines) Patch
M third_party/WebKit/Source/core/layout/svg/LayoutSVGForeignObject.h View 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/svg/LayoutSVGForeignObject.cpp View 2 chunks +5 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/core/layout/svg/LayoutSVGViewportContainer.h View 1 chunk +2 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/paint/ObjectPainter.cpp View 1 2 chunks +11 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp View 2 chunks +4 lines, -13 lines 0 comments Download
M third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp View 2 chunks +13 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/core/paint/SVGForeignObjectPainter.cpp View 2 chunks +13 lines, -9 lines 0 comments Download

Messages

Total messages: 20 (12 generated)
Xianzhu
4 years, 1 month ago (2016-11-17 19:48:17 UTC) #7
pdr.
On 2016/11/17 at 19:48:17, wangxianzhu wrote: > Nice cleanup! LGTM
4 years, 1 month ago (2016-11-17 21:56:50 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2512493003/20001
4 years, 1 month ago (2016-11-17 22:03:41 UTC) #12
commit-bot: I haz the power
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_ng/builds/334314)
4 years, 1 month ago (2016-11-18 00:06:43 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2512493003/20001
4 years, 1 month ago (2016-11-18 00:18:04 UTC) #16
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 1 month ago (2016-11-18 03:54:21 UTC) #17
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/74454dfeaa27a9f696824e0a2a1548175d2e26ab Cr-Commit-Position: refs/heads/master@{#433080}
4 years, 1 month ago (2016-11-18 03:56:58 UTC) #19
Xianzhu
4 years, 1 month ago (2016-11-20 02:13:23 UTC) #20
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2513293002/ by wangxianzhu@chromium.org.

The reason for reverting is: It is incorrect to treat the HTML viewport defined
by a LayoutSVGForeignObject
as the "local SVG coordinate space", because it's not the space that the local
svg
transform, clip path and filters apply. This CL is wrong because it treats the
wrong LayoutSVGForeignObject::localToParentSVGTransform() as correct and used a
wrong way
to remedy that. This CL breaks clip path on foreignObject because the clip path
is
applied in wrong space.
.

Powered by Google App Engine
This is Rietveld 408576698