Issue 1987943002: [wip] unprefix filter

Created:
4 years, 7 months ago by Noel Gordon

Modified:
4 years ago

Reviewers:
Timothy Loh, chrishtr, pdr., alancutter (OOO until 2018), fs

CC:
ajuma+watch-canvas_chromium.org, darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-animation_chromium.org, blink-reviews-css, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, blink-reviews-style_chromium.org, Rik, chromium-reviews, dcheng, dglazkov+blink, dshwang, krit, eae+blinkwatch, Eric Willigers, f(malita), gyuyoung2, haraken, jchaffraix+rendering, Justin Novosad, kouhei+svg_chromium.org, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, pdr+svgwatchlist_chromium.org, rjwright, rwlbuis, Stephen Chennney, shans, slimming-paint-reviews_chromium.org, szager+layoutwatch_chromium.org, zoltan1

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

[wip] unprefix filter BUG=109224

Patch Set 1 : Compilation check, gather test failures. #

Patch Set 2 : Update 3 test results, rebaseline one progression SVGFEBlendElement-dom-in2-attr.html #

Patch Set 3 : compositing/will-change/stacking-context-creation.html, add CSSPropertyAliasWebkitFilter #

Patch Set 4 : Add svg-element-invalid-filter test, apply the SVGRoot filter on the GPU. #

Patch Set 5 : Add animation blur filter test. #

Total comments: 6

Patch Set 6 : Fix animation blur filter test, move it into LayoutTests/animations. #

Total comments: 1

Patch Set 7 : Address LayoutSVGResourceContainer::registerResource() FIXME. #

Patch Set 8 : Add two new SVG filter -webkit-box-reflect tests. #

Patch Set 9 : Decide on BUG 109224, minor test code tweaks. #

Total comments: 31

Patch Set 10 : #

Created: 4 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1186 lines, -411 lines)			Patch
A	third_party/WebKit/LayoutTests/animations/animated-filter-svg-element.html	View	1 2 3 4 5	1 chunk	+23 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/animations/animated-filter-svg-element-expected.html	View	1 2 3 4 5	1 chunk	+10 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/compositing/will-change/stacking-context-creation.html	View	1 2	2 chunks	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/compositing/will-change/stacking-context-creation-expected.html	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-all-on-background-expected.html	View		1 chunk	+11 lines, -11 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-blur.html	View		1 chunk	+4 lines, -4 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-blur-hw.html	View		1 chunk	+4 lines, -4 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-brightness.html	View		1 chunk	+7 lines, -7 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-brightness-clamping.html	View		1 chunk	+12 lines, -12 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-brightness-clamping-hw.html	View		1 chunk	+12 lines, -12 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-brightness-hw.html	View		1 chunk	+7 lines, -7 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-combined.html	View		1 chunk	+6 lines, -6 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-combined-hw.html	View		1 chunk	+6 lines, -6 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-contrast.html	View		1 chunk	+6 lines, -6 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-contrast-hw.html	View		1 chunk	+6 lines, -6 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-drop-shadow.html	View		1 chunk	+5 lines, -5 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-drop-shadow-hw.html	View		1 chunk	+5 lines, -5 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-grayscale.html	View		1 chunk	+6 lines, -6 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-grayscale-hw.html	View		1 chunk	+6 lines, -6 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-hue-rotate.html	View		1 chunk	+6 lines, -6 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-hue-rotate-hw.html	View		1 chunk	+6 lines, -6 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-invert.html	View		1 chunk	+6 lines, -6 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-invert-hw.html	View		1 chunk	+6 lines, -6 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-opacity.html	View		1 chunk	+6 lines, -6 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-opacity-hw.html	View		1 chunk	+6 lines, -6 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-reference.html	View		1 chunk	+14 lines, -14 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-reference-composite.html	View		1 chunk	+12 lines, -12 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-reference-composite-hw.html	View		1 chunk	+12 lines, -12 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-reference-hw.html	View		1 chunk	+14 lines, -14 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-reference-ordering.html	View		1 chunk	+4 lines, -4 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-reference-ordering-hw.html	View		1 chunk	+4 lines, -4 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-saturate.html	View		1 chunk	+6 lines, -6 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-saturate-hw.html	View		1 chunk	+6 lines, -6 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-sepia.html	View		1 chunk	+6 lines, -6 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/effect-sepia-hw.html	View		1 chunk	+6 lines, -6 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/fecomposite-non-zero-inoffset.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/feoffset-region-zoom-expected.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/filter-property-computed-style-expected.txt	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/filter-property-parsing-expected.txt	View		2 chunks	+78 lines, -78 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/filter-region-transformed-child-expected.html	View		1 chunk	+0 lines, -1 line	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/filter-repaint-blur.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/filter-repaint-shadow.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/script-tests/filter-property-computed-style.js	View		2 chunks	+3 lines, -3 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/filters/script-tests/filter-property-parsing.js	View		2 chunks	+3 lines, -3 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/css3/filters/script-tests/unprefixed.js	View		1 chunk	+293 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/css3/filters/unprefixed.html	View		1 chunk	+11 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/css3/filters/unprefixed-expected.txt	View		1 chunk	+201 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css/getComputedStyle/computed-style-listing-expected.txt	View	1 2 3 4 5 6 7 8 9	1 chunk	+0 lines, -1 line	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css/getComputedStyle/computed-style-without-renderer-listing-expected.txt	View	1 2 3 4 5 6 7 8 9	1 chunk	+0 lines, -1 line	0 comments	Download
M	third_party/WebKit/LayoutTests/svg/css/getComputedStyle-listing-expected.txt	View	1 2 3 4 5 6 7 8 9	1 chunk	+0 lines, -1 line	0 comments	Download
M	third_party/WebKit/LayoutTests/svg/filters/filter-refresh-expected.txt	View	1	1 chunk	+1 line, -1 line	0 comments	Download
A	third_party/WebKit/LayoutTests/svg/filters/svg-element-invalid-filter.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+97 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/svg/filters/svg-element-invalid-filter-expected.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+43 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/svg/filters/svg-filter-child-box-reflect.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+22 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/svg/filters/svg-filter-child-box-reflect-expected.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+26 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/svg/filters/svg-filter-root-box-reflect.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+22 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/svg/filters/svg-filter-root-box-reflect-expected.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+26 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/virtual/stable/webexposed/css-properties-as-js-properties-expected.txt	View	1 2 3 4 6 7 8 9	1 chunk	+0 lines, -1 line	0 comments	Download
M	third_party/WebKit/LayoutTests/webexposed/css-properties-as-js-properties-expected.txt	View	1	1 chunk	+0 lines, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/animation/CompositorAnimations.cpp	View	1 2 3 4 5 6 7 8 9	7 chunks	+7 lines, -7 lines	0 comments	Download
M	third_party/WebKit/Source/core/animation/DeferredLegacyStyleInterpolationTest.cpp	View		1 chunk	+13 lines, -13 lines	0 comments	Download
M	third_party/WebKit/Source/core/animation/ElementAnimations.cpp	View	1 2 3 4 5 6 7 8 9	2 chunks	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/core/animation/FilterListPropertyFunctions.h	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/core/animation/PropertyInterpolationTypesMapping.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/animation/css/CSSAnimatableValueFactory.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp	View		1 chunk	+0 lines, -5 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.cpp	View	1 2 3 4 5 6 7 8 9	1 chunk	+0 lines, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/css/CSSProperties.in	View		3 chunks	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/CSSPropertyEquality.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp	View	1 2 3 4 5 6 7 8 9	2 chunks	+1 line, -5 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp	View	1 2 3 4 5 6 7 8 9	2 chunks	+1 line, -2 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/resolver/AnimatedStyleBuilder.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp	View	1 2 3 4 5 6 7 8 9	1 chunk	+2 lines, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/frame/UseCounter.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.cpp	View	1 2 3 4 5 6 7 8 9	1 chunk	+6 lines, -7 lines	0 comments	Download
M	third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp	View	1 2 3 4 5 6 7 8 9	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/layout/svg/SVGLayoutTreeAsText.cpp	View	1 2 3 4 5 6 7 8 9	3 chunks	+29 lines, -8 lines	0 comments	Download
M	third_party/WebKit/Source/core/layout/svg/SVGResources.h	View		3 chunks	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/core/layout/svg/SVGResources.cpp	View	1 2 3 4 5 6 7 8 9	4 chunks	+15 lines, -6 lines	0 comments	Download
M	third_party/WebKit/Source/core/layout/svg/SVGResourcesCache.cpp	View		1 chunk	+1 line, -3 lines	0 comments	Download
M	third_party/WebKit/Source/core/paint/FilterPainter.cpp	View	1 2 3 4 5 6 7 8 9	1 chunk	+3 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/paint/PaintLayer.cpp	View	1 2 3 4 5 6 7 8 9	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/paint/SVGPaintContext.cpp	View	4 5 6 7 8 9	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/style/SVGComputedStyle.h	View		4 chunks	+0 lines, -9 lines	0 comments	Download
M	third_party/WebKit/Source/core/style/SVGComputedStyleDefs.h	View		1 chunk	+0 lines, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/style/SVGComputedStyleDefs.cpp	View		3 chunks	+0 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp	View	1 2 3 4 5 6 7 8 9	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 38 (12 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1987943002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1987943002/100001

4 years, 7 months ago (2016-05-20 08:05:55 UTC) #3

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 7 months ago (2016-05-20 09:25:07 UTC) #5

Stephen White

Wow! Great that you've taken this on, Noel. I'll a deeper look once you're ready, ...

4 years, 7 months ago (2016-05-20 16:55:02 UTC) #6

alancutter (OOO until 2018)

core/animation lgtm with test nits. https://codereview.chromium.org/1987943002/diff/100001/third_party/WebKit/LayoutTests/css3/filters/animated-filter-svg-element.html File third_party/WebKit/LayoutTests/css3/filters/animated-filter-svg-element.html (right): https://codereview.chromium.org/1987943002/diff/100001/third_party/WebKit/LayoutTests/css3/filters/animated-filter-svg-element.html#newcode1 third_party/WebKit/LayoutTests/css3/filters/animated-filter-svg-element.html:1: <!doctype html> This test ...

4 years, 7 months ago (2016-05-23 00:55:10 UTC) #8

Noel Gordon

https://codereview.chromium.org/1987943002/diff/100001/third_party/WebKit/LayoutTests/css3/filters/animated-filter-svg-element.html File third_party/WebKit/LayoutTests/css3/filters/animated-filter-svg-element.html (right): https://codereview.chromium.org/1987943002/diff/100001/third_party/WebKit/LayoutTests/css3/filters/animated-filter-svg-element.html#newcode1 third_party/WebKit/LayoutTests/css3/filters/animated-filter-svg-element.html:1: <!doctype html> On 2016/05/23 00:55:10, alancutter wrote: > This ...

4 years, 7 months ago (2016-05-23 02:12:07 UTC) #9

Noel Gordon

https://codereview.chromium.org/1987943002/diff/120001/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.cpp File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.cpp (right): https://codereview.chromium.org/1987943002/diff/120001/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.cpp#newcode255 third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.cpp:255: // who added it and why. Deal with this ...

4 years, 7 months ago (2016-05-23 03:28:24 UTC) #10

Noel Gordon

Noticed that -webkit-box-reflect uses filterOperations to perform box reflection (behind a runtime flag). We should ...

4 years, 7 months ago (2016-05-23 04:52:12 UTC) #11

Noel Gordon

Pick-a-bug (there are so many) BUG=109224. Minor test style tweakage. New patch uploaded: patch set ...

4 years, 7 months ago (2016-05-23 05:19:50 UTC) #13

On 2016/05/23 at 05:19:50, noel wrote: > Pick-a-bug (there are so many) BUG=109224. Minor test ...

4 years, 7 months ago (2016-05-23 12:29:53 UTC) #15

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/LayoutTests/TestExpectations File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/LayoutTests/TestExpectations#newcode91 third_party/WebKit/LayoutTests/TestExpectations:91: crbug.com/109224 svg/dynamic-updates/SVGFEBlendElement-dom-in2-attr.html [ NeedsRebaseline ] On 2016/05/23 at 12:29:52, ...

4 years, 7 months ago (2016-05-23 14:20:00 UTC) #16

Noel Gordon

On 2016/05/23 14:20:00, fs wrote: > https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/LayoutTests/TestExpectations > File third_party/WebKit/LayoutTests/TestExpectations (right): > > https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/LayoutTests/TestExpectations#newcode91 > ...

4 years, 7 months ago (2016-05-24 02:03:18 UTC) #17

On 2016/05/23 14:20:00, fs wrote:
>
https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/TestExpectations:91: crbug.com/109224
> svg/dynamic-updates/SVGFEBlendElement-dom-in2-attr.html [ NeedsRebaseline ]
> On 2016/05/23 at 12:29:52, fs wrote:
> > It looks like this test has a typo (missing '#' for darkenFilter), so we
> should probably just fix that up instead (first).
> > 
> > (I don't see the connection between crbug.com/608829 and this test.)
> 
> CL to fix the typo: https://codereview.chromium.org/2004023002

Seems fine, reviewed with minor nits.

> The blue->nothing transition indicates a bug/regression in this CL though.
("If
> the filter references a non-existent object or the referenced object is not a
> filter element, then the whole filter chain is ignored. No filter is applied
to
> the object." https://drafts.fxtf.org/filters/#typedef-filter-url)

That's according to that spec, which this patch is not implementing.

> Surprised there are no specific tests for that... quite possible there are

We do have a test for it, LayoutTests/svg/W3C-SVG-1.1-SE/filters-felem-01-b.svg,
which says that the element with the bogus filter should not be drawn.  I found
no reference to that rule in SVG1.1, but there is specific code for it in
applyFilterIfNecessary

 bool SVGPaintContext::applyFilterIfNecessary(SVGResources* resources)
 {
     if (!resources) {
         if (m_object.style()->svgStyle().hasFilter())
             return false;
     }

Not documented in the applyFilterIfNecessary API, but return false here prevents
a draw, and that allows LayoutTests/svg/W3C-SVG-1.1-SE/filters-felem-01-b.svg to
pass.  I added test svg/filters/svg-element-invalid-filter.html to check our
error handling to see if was consistent.  I get the same result for this test in
Safari 9.1 so there is at least some compat.

Once we get to https://drafts.fxtf.org/filters, and again that is a patch for
another day, then we can decide on removing that rule or something.

Noel Gordon

On 2016/05/23 12:29:53, fs wrote: > On 2016/05/23 at 05:19:50, noel wrote: > > Pick-a-bug ...

4 years, 7 months ago (2016-05-24 02:38:37 UTC) #18

pdr.

Noel, is this out for review or just a work in progress? If it's out ...

4 years, 7 months ago (2016-05-24 02:59:12 UTC) #20

On 2016/05/24 at 02:03:18, noel wrote: > On 2016/05/23 14:20:00, fs wrote: > > https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/LayoutTests/TestExpectations ...

4 years, 7 months ago (2016-05-24 11:42:08 UTC) #22

On 2016/05/24 at 02:03:18, noel wrote:
> On 2016/05/23 14:20:00, fs wrote:
> >
https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Lay...
> > File third_party/WebKit/LayoutTests/TestExpectations (right):
> > 
> >
https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Lay...
> > third_party/WebKit/LayoutTests/TestExpectations:91: crbug.com/109224
> > svg/dynamic-updates/SVGFEBlendElement-dom-in2-attr.html [ NeedsRebaseline ]
> > On 2016/05/23 at 12:29:52, fs wrote:
> > > It looks like this test has a typo (missing '#' for darkenFilter), so we
> > should probably just fix that up instead (first).
> > > 
> > > (I don't see the connection between crbug.com/608829 and this test.)
> > 
> > CL to fix the typo: https://codereview.chromium.org/2004023002
> 
> Seems fine, reviewed with minor nits.
> 
> > The blue->nothing transition indicates a bug/regression in this CL though.
("If
> > the filter references a non-existent object or the referenced object is not
a
> > filter element, then the whole filter chain is ignored. No filter is applied
to
> > the object." https://drafts.fxtf.org/filters/#typedef-filter-url)
> 
> That's according to that spec, which this patch is not implementing.

Frankly speaking, that's what this patch should be implementing. That spec is in
every way a more refined version of what was in SVG 1.1. Also, since
-webkit-filter implements that spec, it'll be really odd to have one property
follow the semantics of two spec (where in one case said semantics may not even
be defined.)

> 
> > Surprised there are no specific tests for that... quite possible there are
> 
> We do have a test for it,
LayoutTests/svg/W3C-SVG-1.1-SE/filters-felem-01-b.svg, which says that the
element with the bogus filter should not be drawn.  I found no reference to that
rule in SVG1.1, but there is specific code for it in applyFilterIfNecessary
> 
>  bool SVGPaintContext::applyFilterIfNecessary(SVGResources* resources)
>  {
>      if (!resources) {
>          if (m_object.style()->svgStyle().hasFilter())
>              return false;
>      }
> 
> Not documented in the applyFilterIfNecessary API, but return false here
prevents a draw, and that allows
LayoutTests/svg/W3C-SVG-1.1-SE/filters-felem-01-b.svg to pass.  I added test
svg/filters/svg-element-invalid-filter.html to check our error handling to see
if was consistent.  I get the same result for this test in Safari 9.1 so there
is at least some compat.

If there's compat then keeping that behavior (for now) seems reasonable. Worth
noting though that with the the current style of URL resolution there seems to
be a bug with certain forms of URL - which means "url(foo)" fails during style
resolving and SVGComputedStyle::hasFilter ends up returning false. We should
check for the reason this was changed (defined) in Filter Effects though (i.e it
may well have been due to using the "wrong" test.)

> Once we get to https://drafts.fxtf.org/filters, and again that is a patch for
another day, then we can decide on removing that rule or something.

Luckily it is another day already! =D

If we think we know better than the spec though (and others currently agree with
us), there's no reason to follow said spec regardless.

On 2016/05/24 at 02:38:37, noel wrote: > On 2016/05/23 12:29:53, fs wrote: > > On ...

4 years, 7 months ago (2016-05-24 11:55:43 UTC) #23

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1987943002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1987943002/200001

4 years, 7 months ago (2016-05-26 08:59:15 UTC) #25

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 7 months ago (2016-05-26 10:50:51 UTC) #27

Noel Gordon

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/LayoutTests/css3/filters/script-tests/unprefixed.js File third_party/WebKit/LayoutTests/css3/filters/script-tests/unprefixed.js (right): https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/LayoutTests/css3/filters/script-tests/unprefixed.js#newcode1 third_party/WebKit/LayoutTests/css3/filters/script-tests/unprefixed.js:1: description("Test that -webkit-filter matches filter."); On 2016/05/23 12:29:52, fs ...

4 years, 6 months ago (2016-05-29 04:15:29 UTC) #29

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/css3/filters/script-tests/unprefixed.js
(right):

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/css3/filters/script-tests/unprefixed.js:1:
description("Test that -webkit-filter matches filter.");
On 2016/05/23 12:29:52, fs wrote:
> This looks like it could quite trivially be converted to a testharness.js test
> (and folded into the .html file.)

Maybe, but I won't have the time for it, so it can be trivially done one some
other bug if you feel it is important. The form of the test is not so important
to me at this time, that the test passes is.

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/svg/filters/svg-element-invalid-filter.html
(right):

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/svg/filters/svg-element-invalid-filter.html:2:
<html>
On 2016/05/23 12:29:52, fs wrote:
> Drop.

Done.

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/svg/filters/svg-element-invalid-filter.html:24:
<body>
On 2016/05/23 12:29:52, fs wrote:
> Drop.

Done.

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/svg/filters/svg-element-invalid-filter.html:27:
<!-- SVG root element: the first filter is valid, the rest are not. -->
On 2016/05/23 12:29:52, fs wrote:
> I'd expected the second and third to pass as well.

They don't per SVG1.1, only a url() filter is valid on SVG elements therein. 
They don't pass in Safari either.  And as mentioned on
https://crbug.com/109224#c31 the idea here is compat and SVG1.1 conformance.

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/svg/filters/svg-filter-child-box-reflect.html
(right):

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/svg/filters/svg-filter-child-box-reflect.html:2:
<html>
On 2016/05/23 12:29:52, fs wrote:
> Same as above.

Done.

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/svg/filters/svg-filter-root-box-reflect.html
(right):

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/svg/filters/svg-filter-root-box-reflect.html:2:
<html>
On 2016/05/23 12:29:52, fs wrote:
> Drop.

Done.

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.cpp
(right):

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.cpp:250: //
If this is the SVG root, we could have 'filter' applied and
On 2016/05/23 12:29:53, fs wrote:
> This comment does make a lot of sense anymore. If !style.hasFilter() this
> resource does not apply at all, so 'continue' should be enough in that case. I
> guess for LayoutSVGRoot we might need to call
> markForLayoutAndParentResourceInvalidation (like clientStyleChanged does now)
to
> notify any dependents. filterNeedsPaintInvalidation() will mark for paint
> invalidation and don't think marking for layout should be needed anymore (used
> to be needed because some bounds needed to be recomputed I think.)
> 
> So maybe something like:
> 
> if (!style.hasFilter())
>     continue;
> toLayoutBoxModelObject(layoutObject)->layer()->filterNeedsPaintInvalidation();
> if (layoutObject->isSVGRoot())
>     markForLayoutAndParentResourceInvalidation(layoutObject, false);
> continue;

Thanks for the code.  I tried it and the two tests in question failed

  svg/custom/cross-referenced-resources.html [ Failure ]
  svg/filters/filter-on-svg-root-w-layer.html [ Failure ]

sorry to say.  If I let the svgRoot fall into the full invalidation code that
the original comment mentioned, then these tests pass.  So I did that for now
(and adjusted the comment).

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/svg/SVGResources.cpp (right):

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/SVGResources.cpp:233: if
(computedStyle.hasFilter()) {
On 2016/05/23 12:29:53, fs wrote:
> I think LayoutSVGRoot should be left out of this - i.e not store a filter in
> SVGResources. If it does it'll be sitting on two chairs - and inconsistently
so
> because this path is limited to only "url(#foo)". 

I stored the filter in SVGResources, and prevented FilterPainter from trying to
create / apply the filter.

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/SVGResources.cpp:239: AtomicString id
=
SVGURIReference::fragmentIdentifierFromIRIString(referenceFilterOperation.url(),
element->document());
On 2016/05/23 12:29:53, fs wrote:
> uNit: could use |treeScope| here (instead of element->document()) so that
we're
> consistently using one or the other (should matter here ATM I think.)

Yes, done.

> That being said though, ReferenceFilterOperation::url() is already resolved
> against the base URL here, and the fragment is extracted (to
> ReferenceFilterOperation::fragment()), so it seems the only thing needed here
is
> to use those (check if the URL is local and use the stored fragment if it is.)

Didn't quite follow what you are asking for here.  The filter url(reference)
could be to another document, eg., url(other.svg#fragment) best I can tell.

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FilterPainter.cpp (right):

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FilterPainter.cpp:62: 
On 2016/05/23 12:29:53, fs wrote:
> Drop?

Done.

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGPaintContext.cpp (right):

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGPaintContext.cpp:152: return true;
On 2016/05/23 12:29:53, fs wrote:
> return !m_object.style()->hasFilter();
> 
> 
> ?

I restored the old code, so done.

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGPaintContext.cpp:156: // The root
element's filter is applied by the layer filter painter.
On 2016/05/23 12:29:53, fs wrote:
> Shouldn't need to call this at all if m_object.isSVGRoot() - similarly to what
> applyCompositingIfNecessary() does. Now that there's two checks like that it
> could make sense to hoist that to applyClipMaskAndFilterIfNecessary? (Also,
the
> hasLayer call here seems redundant.)
> 
> There's also a check: !hasFilterResource in SVGRootPainter that seems
redundant
> now when the filter is (always) handled by the PaintLayer.

ditto, restored the old code.

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGPaintContext.cpp:175: 
On 2016/05/23 12:29:53, fs wrote:
> Drop.

ditto.

Noel Gordon

On 2016/05/24 02:59:12, pdr. wrote: > Noel, is this out for review or just a ...

4 years, 6 months ago (2016-05-29 04:31:19 UTC) #30

Noel Gordon

On 2016/05/24 11:42:08, fs wrote: > On 2016/05/24 at 02:03:18, noel wrote: > > On ...

4 years, 6 months ago (2016-05-29 04:52:47 UTC) #31

On 2016/05/24 11:42:08, fs wrote:
> On 2016/05/24 at 02:03:18, noel wrote:
> > On 2016/05/23 14:20:00, fs wrote:
> > >
>
https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Lay...
> > > File third_party/WebKit/LayoutTests/TestExpectations (right):
> > > 
> > >
>
https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Lay...
> > > third_party/WebKit/LayoutTests/TestExpectations:91: crbug.com/109224
> > > svg/dynamic-updates/SVGFEBlendElement-dom-in2-attr.html [ NeedsRebaseline
]
> > > On 2016/05/23 at 12:29:52, fs wrote:
> > > > It looks like this test has a typo (missing '#' for darkenFilter), so we
> > > should probably just fix that up instead (first).
> > > > 
> > > > (I don't see the connection between crbug.com/608829 and this test.)
> > > 
> > > CL to fix the typo: https://codereview.chromium.org/2004023002
> > 
> > Seems fine, reviewed with minor nits.
> > 
> > > The blue->nothing transition indicates a bug/regression in this CL though.
> ("If
> > > the filter references a non-existent object or the referenced object is
not
> a
> > > filter element, then the whole filter chain is ignored. No filter is
applied
> to
> > > the object." https://drafts.fxtf.org/filters/#typedef-filter-url)
> > 
> > That's according to that spec, which this patch is not implementing.
> 
> Frankly speaking, that's what this patch should be implementing. That spec is
in
> every way a more refined version of what was in SVG 1.1. Also, since
> -webkit-filter implements that spec, it'll be really odd to have one property
> follow the semantics of two spec (where in one case said semantics may not
even
> be defined.)

The new spec needs a test suite, or one we build ourselves or something, but for
now I went for SVG1.1 conformance to gain compat with Safari as I'm about to go
OOO.  Perhaps the oddness is due to that filter was unprefixed for SVG but not
for HTML elements, but at least browsers would be oddly compatible at SVG1.1
conformance.

> > > Surprised there are no specific tests for that... quite possible there are
> > 
> > We do have a test for it,
> LayoutTests/svg/W3C-SVG-1.1-SE/filters-felem-01-b.svg, which says that the
> element with the bogus filter should not be drawn.  I found no reference to
that
> rule in SVG1.1, but there is specific code for it in applyFilterIfNecessary
> > 
> >  bool SVGPaintContext::applyFilterIfNecessary(SVGResources* resources)
> >  {
> >      if (!resources) {
> >          if (m_object.style()->svgStyle().hasFilter())
> >              return false;
> >      }
> > 
> > Not documented in the applyFilterIfNecessary API, but return false here
> prevents a draw, and that allows
> LayoutTests/svg/W3C-SVG-1.1-SE/filters-felem-01-b.svg to pass.  I added test
> svg/filters/svg-element-invalid-filter.html to check our error handling to see
> if was consistent.  I get the same result for this test in Safari 9.1 so there
> is at least some compat.
> 
> If there's compat then keeping that behavior (for now) seems reasonable.

It's required for SVG1.1 compat.  Note for draft Filters 1 spec though, the
required behavior was changed to say the filter should not be applied, which is
very different behavior.

> Worth noting though that with the the current style of URL resolution there
seems to
> be a bug with certain forms of URL - which means "url(foo)" fails during style
> resolving and SVGComputedStyle::hasFilter ends up returning false. We should
> check for the reason this was changed (defined) in Filter Effects though (i.e
it
> may well have been due to using the "wrong" test.)
>
> > Once we get to https://drafts.fxtf.org/filters, and again that is a patch
for
> another day, then we can decide on removing that rule or something.
> 
> Luckily it is another day already! =D

In a day or so I will be OOO for many months.  Moving to
https://drafts.fxtf.org/filters will require more work and a good test suite. 
Not something I can do in  day.  We could 1) ship SVG1.1 conformance now, and
make https://drafts.fxtf.org/filters conformance a work in progress.  Developers
of HTML content will be very glad they do not have to prefix filter anymore. 
For SVG elements, we gain SVG1.1 compat at least, but sure, there is more work
to gain https://drafts.fxtf.org/filters conformance for SVG elements. 

> If we think we know better than the spec though (and others currently agree
with
> us), there's no reason to follow said spec regardless.

Mozilla does not accept url(foo) when foo is a reference to some unknown
document, and nor does my patch, if that is what you meant.

Noel Gordon

On 2016/05/24 11:55:43, fs wrote: > On 2016/05/24 at 02:38:37, noel wrote: > > On ...

4 years, 6 months ago (2016-05-29 05:04:37 UTC) #32

Noel Gordon

On 2016/05/20 16:55:02, Stephen White wrote: > Wow! Great that you've taken this on, Noel. ...

4 years, 6 months ago (2016-05-29 05:58:55 UTC) #33

On 2016/05/20 16:55:02, Stephen White wrote:
> Wow! Great that you've taken this on, Noel. I'll a deeper look once you're
> ready, but a few early questions/comments.

Since I'm going OOO, I handed off all my work these last months, but someone
asked if I could take a quick look at the filter unprefixing problem before I go
OOO :)  For HTML elements, unprefixing seems easy, and developers would rejoice.
 But given the comments on the bug, and the state of the draft Filters 1 spec,
SVG1.1 conformance for unprefixed filter on SVG elements would gain browser
compat now, and seemed do-able in the allotted time.  More work would be needed
to support the draft Filters 1 spec of course, needs a lot more tests focusing
on SVG elements with filter style for a start, but not something I'm gonna get
to anytime soon.

> Does this mean that the SVG filters path still applies the CSS "filter"
property
> using the SVG codepath? (i.e., only applies a single url() reference)? This is
> probably fine for now, since the SVG code actually has some optimizations
> (caching, in particular) that the HTML path does not.

Good question.  For child SVG elements, the filtering is still done by the SVG
code path, so you still get the optimizations you mentioned for SVG child
elements, however ...

svgStyle() for filters is removed in this patch; the filter style moves onto
element.style().  This means that for SVG root elements, they gain a layer, and
the layer painter can see the filter style() on the SVG root element, so it uses
FilterPainter to apply the filter.  But the SVG code path would also apply the
filter :) and some layout tests failed because the filter was being applied
twice!

I had to decide on a winner: FilterPainter in the initial patches, done by
blocking the SVG code path from applying the filter, said optimizations gone for
SVG root elements.

In the latest patch #10, I've done the reverse: I made the SVG code path win by
blocking FilterPainter from applying the filter.  I think with that, SVG root
elements now get the optimizations you mentioned.

> And you'll probably need to file an "intent to implement" (or "intent to
> unprefix", if there is such a thing) on blink-dev.

Agree.  Guess we have to decide if SVG1.1 compat is good enough for SVG
elements, and ship that now, or else have someone continue this patch if we
decide we want the perfection of draft Filters 1, add svg filter tests for same,
and ship once that's all done.

On 2016/05/29 at 04:52:47, noel wrote: > On 2016/05/24 11:42:08, fs wrote: > > On ...

4 years, 6 months ago (2016-05-30 11:58:52 UTC) #34

On 2016/05/29 at 04:52:47, noel wrote:
> On 2016/05/24 11:42:08, fs wrote:
> > On 2016/05/24 at 02:03:18, noel wrote:
> > > On 2016/05/23 14:20:00, fs wrote:
...
> > Worth noting though that with the the current style of URL resolution there
seems to
> > be a bug with certain forms of URL - which means "url(foo)" fails during
style
> > resolving and SVGComputedStyle::hasFilter ends up returning false. We should
> > check for the reason this was changed (defined) in Filter Effects though
(i.e it
> > may well have been due to using the "wrong" test.)
> >
> > > Once we get to https://drafts.fxtf.org/filters, and again that is a patch
for
> > another day, then we can decide on removing that rule or something.
> > 
> > Luckily it is another day already! =D
> 
> In a day or so I will be OOO for many months.  Moving to
https://drafts.fxtf.org/filters will require more work and a good test suite. 
Not something I can do in  day.  We could 1) ship SVG1.1 conformance now, and
make https://drafts.fxtf.org/filters conformance a work in progress.  Developers
of HTML content will be very glad they do not have to prefix filter anymore. 
For SVG elements, we gain SVG1.1 compat at least, but sure, there is more work
to gain https://drafts.fxtf.org/filters conformance for SVG elements. 
> 
> > If we think we know better than the spec though (and others currently agree
with
> > us), there's no reason to follow said spec regardless.
> 
> Mozilla does not accept url(foo) when foo is a reference to some unknown
document, and nor does my patch, if that is what you meant.

What I was getting at here was that, prior to this CL (i.e on current trunk),
"url(dontexist)" and "url(#dontexist)" failed in two different ways. (Probably
because convertFragmentIdentifier produces "" and (null) for the latter, and
SVGComputedStyle::hasFilter() is using isEmpty() - which is true for both.)

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/css3/filters/script-tests/unprefixed.js
(right):

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/css3/filters/script-tests/unprefixed.js:1:
description("Test that -webkit-filter matches filter.");
On 2016/05/29 at 04:15:28, noel gordon wrote:
> On 2016/05/23 12:29:52, fs wrote:
> > This looks like it could quite trivially be converted to a testharness.js
test
> > (and folded into the .html file.)
> 
> Maybe, but I won't have the time for it, so it can be trivially done one some
other bug if you feel it is important. The form of the test is not so important
to me at this time, that the test passes is.

Fair enough, I can fix it up later.

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.cpp
(right):

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.cpp:250: //
If this is the SVG root, we could have 'filter' applied and
On 2016/05/29 at 04:15:28, noel gordon wrote:
> On 2016/05/23 12:29:53, fs wrote:
> > This comment does make a lot of sense anymore. If !style.hasFilter() this
> > resource does not apply at all, so 'continue' should be enough in that case.
I
> > guess for LayoutSVGRoot we might need to call
> > markForLayoutAndParentResourceInvalidation (like clientStyleChanged does
now) to
> > notify any dependents. filterNeedsPaintInvalidation() will mark for paint
> > invalidation and don't think marking for layout should be needed anymore
(used
> > to be needed because some bounds needed to be recomputed I think.)
> > 
> > So maybe something like:
> > 
> > if (!style.hasFilter())
> >     continue;
> >
toLayoutBoxModelObject(layoutObject)->layer()->filterNeedsPaintInvalidation();
> > if (layoutObject->isSVGRoot())
> >     markForLayoutAndParentResourceInvalidation(layoutObject, false);
> > continue;
> 
> Thanks for the code.  I tried it and the two tests in question failed
> 
>   svg/custom/cross-referenced-resources.html [ Failure ]
>   svg/filters/filter-on-svg-root-w-layer.html [ Failure ]
> 
> sorry to say.  If I let the svgRoot fall into the full invalidation code that
the original comment mentioned, then these tests pass.  So I did that for now
(and adjusted the comment).

Yes, this was under the assumption that FilterPainter was applying the filter,
and no state was kept in SVGResources. If the PaintLayer isn't painting the
filter, then the filterNeedsPaintInvalidation() ought to be redundant instead.

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/svg/SVGResources.cpp (right):

https://codereview.chromium.org/1987943002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/SVGResources.cpp:239: AtomicString id
=
SVGURIReference::fragmentIdentifierFromIRIString(referenceFilterOperation.url(),
element->document());
On 2016/05/29 at 04:15:28, noel gordon wrote:
> On 2016/05/23 12:29:53, fs wrote:
> > uNit: could use |treeScope| here (instead of element->document()) so that
we're
> > consistently using one or the other (should matter here ATM I think.)
> 
> Yes, done.
> 
> > That being said though, ReferenceFilterOperation::url() is already resolved
> > against the base URL here, and the fragment is extracted (to
> > ReferenceFilterOperation::fragment()), so it seems the only thing needed
here is
> > to use those (check if the URL is local and use the stored fragment if it
is.)
> 
> Didn't quite follow what you are asking for here.  The filter url(reference)
could be to another document, eg., url(other.svg#fragment) best I can tell.

Yes, you still need to check that the reference is local ("check if the URL is
local"), so something like:

KURL url(ParsedURLString, referenceFilterOperation.url());
if (equalIgnoringFragmentIdentifier(url, document->url())
    id = AtomicString(referenceFilterOperation.fragment());

is what I had in mind. NVM though, I can fix that up later.

Timothy Loh

I think Noel is now OOO for many weeks. fs@, is there any chance you ...

4 years, 6 months ago (2016-06-01 04:26:17 UTC) #36

4 years, 6 months ago (2016-06-01 08:25:38 UTC) #37

On 2016/06/01 at 04:26:17, timloh wrote:
> I think Noel is now OOO for many weeks. fs@, is there any chance you have
bandwidth to take this on? The changes in core/animation and core/css all look
fine to me so the remaining work seems to all be in SVG. I don't remember an
Intent going by, so that needs to be done too. There's no runtime flag currently
for this, which I think is fine, but if this will need a fair amount of feature
work and a flag I can look into making one for this.

If we can get rough agreement on the scope, then I could absolutely do that.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages