Bug 100142 – [chromium] Implement reference (url) filters on composited layers.

• Component: New Bugs
• Status: RESOLVED FIXED

[Stephen White]

[skia] Implement reference (url) filters on composited layers.

[Stephen White]

Created attachment 170188 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Early Warning System Bot]

Comment on attachment 170188 [details]
Patch

Attachment 170188 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14530008

[Stephen White]

Created attachment 170190 [details]
Patch

[Build Bot]

Comment on attachment 170190 [details]
Patch

Attachment 170190 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14529054

[Stephen White]

Created attachment 170735 [details]
Patch

[WebKit Review Bot]

Attachment 170735 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebCore/ChangeLog:13:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephen White]

Created attachment 170739 [details]
Patch

[Stephen White]

James, could you take a look at the compositor bits?

[James Robinson]

(In reply to comment #9)
> James, could you take a look at the compositor bits?

That stuff all looks great to me!

[James Robinson]

Comment on attachment 170739 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170739&action=review

> Source/Platform/chromium/public/WebLayer.h:129
> +    virtual void setFilter(SkImageFilter*) = 0;

Could you document what's going on with ownership here?  I believe the answer is that the WebLayer is supposed to take a SkRef on the passed in SkImageFilter and it's expected that the filter chain won't be modified by WebKit after this call, is that accurate?

[Adam Barth]

I'm not sure if it's related, but we're disabling external SVG references pending a security review:

https://bugs.webkit.org/show_bug.cgi?id=100635

References internal to a document should continue to work.

[Stephen White]

(In reply to comment #11)
> (From update of attachment 170739 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170739&action=review
> 
> > Source/Platform/chromium/public/WebLayer.h:129
> > +    virtual void setFilter(SkImageFilter*) = 0;
> 
> Could you document what's going on with ownership here?  I believe the answer is that the WebLayer is supposed to take a SkRef on the passed in SkImageFilter and it's expected that the filter chain won't be modified by WebKit after this call, is that accurate?

Exactly.  SkImageFilters are essentially immutable after construction anyway.  I'll add a comment to that effect.

[Stephen White]

Created attachment 171338 [details]
Patch

[WebKit Review Bot]

Attachment 171338 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebCore/ChangeLog:22:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:44:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephen White]

Created attachment 171343 [details]
Patch

[Stephen White]

Created attachment 171345 [details]
Patch

[Stephen White]

(In reply to comment #12)
> I'm not sure if it's related, but we're disabling external SVG references pending a security review:

I guessed as much, from the recent discussion on webkit-dev.  This seems prudent.

> https://bugs.webkit.org/show_bug.cgi?id=100635
> 
> References internal to a document should continue to work.

Thanks for the heads-up.

[Stephen White]

Created attachment 172233 [details]
Patch

[Build Bot]

Comment on attachment 172233 [details]
Patch

Attachment 172233 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14713608

New failing tests:
css3/filters/effect-reference-hw.html
css3/filters/effect-reference-ordering-hw.html
css3/filters/effect-reference.html

[Stephen White]

Created attachment 172246 [details]
Suppress failures on mac

[Stephen White]

Created attachment 172398 [details]
Fix refcounting issue

[WebKit Review Bot]

Attachment 172398 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/css3/filters/effect-reference-..." exit_code: 1
LayoutTests/platform/mac/TestExpectations:804:  Path does not exist.  [test/expectations] [5]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Comment on attachment 172398 [details]
Fix refcounting issue

View in context: https://bugs.webkit.org/attachment.cgi?id=172398&action=review

R=me

> Source/WebCore/platform/graphics/filters/skia/SkiaImageFilterBuilder.cpp:45
> +    //    Old implementation, a la the draft spec, a straight-up scale,
> +    //    representing <feFunc[R|G|B] type="linear" slope="[amount]">
> +    //    (See http://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html#brightnessEquivalent)
> +    // matrix[0] = matrix[6] = matrix[12] = amount;
> +    // matrix[18] = 1;

is there any reason to keep this around?

[Stephen White]

Comment on attachment 172398 [details]
Fix refcounting issue

View in context: https://bugs.webkit.org/attachment.cgi?id=172398&action=review

>> Source/WebCore/platform/graphics/filters/skia/SkiaImageFilterBuilder.cpp:45
>> +    // matrix[18] = 1;
> 
> is there any reason to keep this around?

Nope.  Removed.

[Stephen White]

Created attachment 172573 [details]
Patch for landing

[Stephen White]

Committed r133608: <http://trac.webkit.org/changeset/133608>

[Ryosuke Niwa]

Why is this patch missing the change log entry in LayoutTests?

[Ryosuke Niwa]

Also LayoutTests/css3/filters/effect-reference-ordering-hw.html was added without any expected results. How does that work?

[Stephen White]

(In reply to comment #28)
> Why is this patch missing the change log entry in LayoutTests?

That's odd.  I'm not sure how that happened.

(In reply to comment #29)
> Also LayoutTests/css3/filters/effect-reference-ordering-hw.html was added without any expected results. How does that work?

Also strange.  At any rate, I rebaselined it in r135539.