Bug 100142

Created attachment 170188 [details] Patch

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 on attachment 170188 [details] Patch Attachment 170188 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14530008

Created attachment 170190 [details] Patch

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

Created attachment 170735 [details] Patch

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.

Created attachment 170739 [details] Patch

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

(In reply to comment #9) > James, could you take a look at the compositor bits? That stuff all looks great to me!

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?

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.

(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.

Created attachment 171338 [details] Patch

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.

Created attachment 171343 [details] Patch

Created attachment 171345 [details] Patch

(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.

Created attachment 172233 [details] Patch

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

Created attachment 172246 [details] Suppress failures on mac

Created attachment 172398 [details] Fix refcounting issue

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 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 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.

Created attachment 172573 [details] Patch for landing

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

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

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

(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.