Bug 100142 - [chromium] Implement reference (url) filters on composited layers.
Summary: [chromium] Implement reference (url) filters on composited layers.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Stephen White
URL:
Keywords:
Depends on: 101925
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-23 10:23 PDT by Stephen White
Modified: 2012-12-20 17:45 PST (History)
10 users (show)

See Also:


Attachments
Patch (34.79 KB, patch)
2012-10-23 10:34 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (34.68 KB, patch)
2012-10-23 10:47 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (35.44 KB, patch)
2012-10-25 14:57 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (35.44 KB, patch)
2012-10-25 15:04 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (42.14 KB, patch)
2012-10-29 16:52 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (42.14 KB, patch)
2012-10-29 17:14 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (42.13 KB, patch)
2012-10-29 17:21 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (49.33 KB, patch)
2012-11-03 16:53 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Suppress failures on mac (50.07 KB, patch)
2012-11-04 08:11 PST, Stephen White
no flags Details | Formatted Diff | Diff
Fix refcounting issue (50.15 KB, patch)
2012-11-05 13:58 PST, Stephen White
no flags Details | Formatted Diff | Diff
Patch for landing (49.66 KB, patch)
2012-11-06 06:42 PST, Stephen White
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen White 2012-10-23 10:23:44 PDT
[skia] Implement reference (url) filters on composited layers.
Comment 1 Stephen White 2012-10-23 10:34:42 PDT
Created attachment 170188 [details]
Patch
Comment 2 WebKit Review Bot 2012-10-23 10:38:06 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Early Warning System Bot 2012-10-23 10:41:42 PDT
Comment on attachment 170188 [details]
Patch

Attachment 170188 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14530008
Comment 4 Stephen White 2012-10-23 10:47:14 PDT
Created attachment 170190 [details]
Patch
Comment 5 Build Bot 2012-10-23 12:21:30 PDT
Comment on attachment 170190 [details]
Patch

Attachment 170190 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14529054
Comment 6 Stephen White 2012-10-25 14:57:20 PDT
Created attachment 170735 [details]
Patch
Comment 7 WebKit Review Bot 2012-10-25 15:00:00 PDT
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.
Comment 8 Stephen White 2012-10-25 15:04:51 PDT
Created attachment 170739 [details]
Patch
Comment 9 Stephen White 2012-10-26 08:33:43 PDT
James, could you take a look at the compositor bits?
Comment 10 James Robinson 2012-10-29 13:46:16 PDT
(In reply to comment #9)
> James, could you take a look at the compositor bits?

That stuff all looks great to me!
Comment 11 James Robinson 2012-10-29 13:47:27 PDT
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?
Comment 12 Adam Barth 2012-10-29 13:49:06 PDT
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.
Comment 13 Stephen White 2012-10-29 14:20:51 PDT
(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.
Comment 14 Stephen White 2012-10-29 16:52:40 PDT
Created attachment 171338 [details]
Patch
Comment 15 WebKit Review Bot 2012-10-29 16:58:49 PDT
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.
Comment 16 Stephen White 2012-10-29 17:14:33 PDT
Created attachment 171343 [details]
Patch
Comment 17 Stephen White 2012-10-29 17:21:03 PDT
Created attachment 171345 [details]
Patch
Comment 18 Stephen White 2012-10-30 12:49:23 PDT
(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.
Comment 19 Stephen White 2012-11-03 16:53:15 PDT
Created attachment 172233 [details]
Patch
Comment 20 Build Bot 2012-11-04 07:53:02 PST
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
Comment 21 Stephen White 2012-11-04 08:11:07 PST
Created attachment 172246 [details]
Suppress failures on mac
Comment 22 Stephen White 2012-11-05 13:58:20 PST
Created attachment 172398 [details]
Fix refcounting issue
Comment 23 WebKit Review Bot 2012-11-05 14:07:08 PST
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.
Comment 24 James Robinson 2012-11-05 14:30:15 PST
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?
Comment 25 Stephen White 2012-11-06 06:40:51 PST
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.
Comment 26 Stephen White 2012-11-06 06:42:34 PST
Created attachment 172573 [details]
Patch for landing
Comment 27 Stephen White 2012-11-06 07:34:41 PST
Committed r133608: <http://trac.webkit.org/changeset/133608>
Comment 28 Ryosuke Niwa 2012-12-20 17:19:46 PST
Why is this patch missing the change log entry in LayoutTests?
Comment 29 Ryosuke Niwa 2012-12-20 17:21:42 PST
Also LayoutTests/css3/filters/effect-reference-ordering-hw.html was added without any expected results. How does that work?
Comment 30 Stephen White 2012-12-20 17:45:27 PST
(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.