Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2570223002:
[SPv2] Associate effect property nodes for SVG paint chunks (Closed)
|
Created:
4 years ago by trchen Modified:
3 years, 12 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, krit, eae+blinkwatch, f(malita), fs, gyuyoung2, jchaffraix+rendering, kouhei+svg_chromium.org, leviw+renderwatch, pdr+svgwatchlist_chromium.org, pdr+renderingwatchlist_chromium.org, rwlbuis, Stephen Chennney, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[SPv2] Associate effect property nodes for SVG paint chunks
Previously we only created the effect nodes for SVG effects but without
actually using them. This CL adds SPv2 support to SVGPaintContext.
As a side change, guard LayoutObject::hasNonIsolatedBlendingDescendants()
with NOTREACHED() because it is only implemented for SVG.
BUG=673500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/edf04d1d0d3ecd74b9ea1e11d65dfa9113797511
Cr-Commit-Position: refs/heads/master@{#440678}
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix isolation & apply effects to SVG graphics elements #
Total comments: 3
Patch Set 3 : fix NONREACHED() because some SVG didn't override hasNonIsolatedDescendant, fix condition to determ… #Patch Set 4 : rebased #
Total comments: 2
Patch Set 5 : probably the last missed hasIsolatedDescendant override (I hope) #Patch Set 6 : keep CompositingRecorder, update expectation #
Total comments: 4
Patch Set 7 : revert hasNonIso... change #
Messages
Total messages: 49 (31 generated)
Description was changed from ========== [SPv2] Associate effect property nodes for SVG paint chunks Previously we only created the effect nodes for SVG effects but without actually using them. This CL adds SPv2 support to SVGPaintContext. As a side change, guard LayoutObject::hasNonIsolatedBlendingDescendants() with NOTREACHED() because it is only implemented for SVG. BUG=673500 ========== to ========== [SPv2] Associate effect property nodes for SVG paint chunks Previously we only created the effect nodes for SVG effects but without actually using them. This CL adds SPv2 support to SVGPaintContext. As a side change, guard LayoutObject::hasNonIsolatedBlendingDescendants() with NOTREACHED() because it is only implemented for SVG. BUG=673500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
trchen@chromium.org changed reviewers: + chrishtr@chromium.org, pdr@chromium.org, wangxianzhu@chromium.org
Here's the said SVG change.
https://codereview.chromium.org/2570223002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2570223002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.h:651: virtual bool hasNonIsolatedBlendingDescendants() const { Why not just remove the thing and make it part of LayoutSVGModelObject interface you might ask? It is because LayoutSVGRoot is LayoutBoxModelObject, not LayoutSVGModelObject. There is a FIXME on line 603. IMO our layout object type system is not designed well to handle things crossing flow type boundary. LayoutSVGRoot is an inline-flow to its parent (inline replaced), but contains SVG-flow. Likewise, LayoutSVGBlock is a SVG-flow to its parent, but contains block-flow. Ideally this really should be solved with properly designed diamond inheritance...
Looks good. https://codereview.chromium.org/2570223002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2 (right): https://codereview.chromium.org/2570223002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:2176: #Bug(none) css3/blending/svg-blend-not-allowed.html [ Failure ] Are these fixed yet?
(FWIW, I also agree this looks good. Just waiting for dependent patchset to land)
Fixed the three. Revealed one more which should be filed for later. https://codereview.chromium.org/2570223002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2 (right): https://codereview.chromium.org/2570223002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:2174: Bug(none) css3/blending/svg-isolation-add-masking.html [ Failure ] This new failure is interesting. It revealed some bad code which I should file bug & fix. SVGLayoutSupport::willIsolateBlendingDescendantsForStyle() checks for svgStyle.hasMasker(), but in fact that method is not implemented (should remove it). Should check the SVG resource cache instead.
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
https://codereview.chromium.org/2570223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2570223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:376: const bool isSVGGraphicsElement = Why skip SVG graphics elements?
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
https://codereview.chromium.org/2570223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2570223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:376: const bool isSVGGraphicsElement = On 2016/12/15 19:21:23, chrishtr wrote: > Why skip SVG graphics elements? It is to include SVG graphics elements. SVGLayoutSupport::willIsolateBlendingDescendantsForObject() exclude them because they can't have descendants. But anyhow, I found this condition is still not quite right. The new patchset fixed existing tests. (Hope it now matches the existing behavior... It does not 100% adhere to the spec and I have to reverse engineer it.)
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2570223002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2 (right): https://codereview.chromium.org/2570223002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:2175: Bug(none) css3/blending/svg-isolation-add-masking.html [ Failure ] Why does this fail now but not before? File a bug also?
https://codereview.chromium.org/2570223002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2 (right): https://codereview.chromium.org/2570223002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:2175: Bug(none) css3/blending/svg-isolation-add-masking.html [ Failure ] On 2016/12/16 00:23:24, chrishtr wrote: > Why does this fail now but not before? File a bug also? It didn't fail due to a double fail. The parent didn't isolate, and the child didn't blend. After this CL the child do blend, but the parent still don't isolate. The parent has a (all white, i.e. no-op) mask and should isolate, but the mask check in SVGLayoutSupport is bugged. I will file bug and fix it separately.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
Patchset #6 (id:100001) has been deleted
Okay, patchset 6 should be more correct. Most layout tests involved only opacity and blend now either pass or fail with rounding errors. Note that patchset 6 included non-trivial change in SVGPaintContext. Prior to patchset 6 the CompositingRecorder were skipped because I thought the display items were ignored anyway. However SVG filters fold its whole subtree (including meta display items) into a single drawing display item. Until we have a plan for composited SVG filter, we can't get away without meta display items. :( PTAL again. Thanks!
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2570223002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2570223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.h:654: NOTREACHED(); What about DCHECK(isSVG()); and remove the other overrides?
https://codereview.chromium.org/2570223002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2570223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.h:652: virtual bool hasNonIsolatedBlendingDescendants() const { Why do you need to make the changes to hasNonIsolatedBlendingDescendants? Your patch no longer calls that method.
https://codereview.chromium.org/2570223002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2570223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.h:652: virtual bool hasNonIsolatedBlendingDescendants() const { On 2016/12/21 21:39:24, chrishtr wrote: > Why do you need to make the changes to hasNonIsolatedBlendingDescendants? Your > patch no longer calls > that method. Yea... This really belongs to a split out. Done. https://codereview.chromium.org/2570223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.h:654: NOTREACHED(); On 2016/12/21 19:12:52, pdr (OOO Dec 23 to Jan 2) wrote: > What about DCHECK(isSVG()); and remove the other overrides? That's genius! I'll do that instead (in a split out of course).
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by trchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1482621603435640,
"parent_rev": "87995f7647b753d1ed16f52cf5f2ad66a6a2da38", "commit_rev":
"314385965b7181ff0438169ca27202ea48df64cb"}
Message was sent while issue was closed.
Description was changed from ========== [SPv2] Associate effect property nodes for SVG paint chunks Previously we only created the effect nodes for SVG effects but without actually using them. This CL adds SPv2 support to SVGPaintContext. As a side change, guard LayoutObject::hasNonIsolatedBlendingDescendants() with NOTREACHED() because it is only implemented for SVG. BUG=673500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [SPv2] Associate effect property nodes for SVG paint chunks Previously we only created the effect nodes for SVG effects but without actually using them. This CL adds SPv2 support to SVGPaintContext. As a side change, guard LayoutObject::hasNonIsolatedBlendingDescendants() with NOTREACHED() because it is only implemented for SVG. BUG=673500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2570223002 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [SPv2] Associate effect property nodes for SVG paint chunks Previously we only created the effect nodes for SVG effects but without actually using them. This CL adds SPv2 support to SVGPaintContext. As a side change, guard LayoutObject::hasNonIsolatedBlendingDescendants() with NOTREACHED() because it is only implemented for SVG. BUG=673500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2570223002 ========== to ========== [SPv2] Associate effect property nodes for SVG paint chunks Previously we only created the effect nodes for SVG effects but without actually using them. This CL adds SPv2 support to SVGPaintContext. As a side change, guard LayoutObject::hasNonIsolatedBlendingDescendants() with NOTREACHED() because it is only implemented for SVG. BUG=673500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/edf04d1d0d3ecd74b9ea1e11d65dfa9113797511 Cr-Commit-Position: refs/heads/master@{#440678} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/edf04d1d0d3ecd74b9ea1e11d65dfa9113797511 Cr-Commit-Position: refs/heads/master@{#440678} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
