This patch will add the CustomFilterOperation and add the code in CSSStyleSelector for creating it. The only parsed arguments will be the urls for the shaders. Also it should add code for the Computed style, so that we can test the functionality.
Created attachment 115163 [details] Patch V1
Comment on attachment 115163 [details] Patch V1 Attachment 115163 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10484359
Created attachment 115190 [details] Patch V2 Fixed the Chromium gypi file
Comment on attachment 115163 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=115163&action=review r- up front for the chromium build issue, otherwise looks good. I wonder if we should retitle the bug because it now does a lot more than just computed style. > LayoutTests/css3/filters/script-tests/custom-filter-property-computed-style.js:1 > +description("Test the parsing of the custom() function of the -webkit-filter property."); This needs updating. > LayoutTests/css3/filters/script-tests/custom-filter-property-computed-style.js:82 > +testFilterRule("Custom with vertex shader", > + "custom(url(vertex.shader))", "custom(url(vertex.shader))"); > + > +testFilterRule("Custom with fragment shader", > + "custom(none url(fragment.shader))", "custom(none url(fragment.shader))"); > + > +testFilterRule("Custom with both shaders", > + "custom(url(vertex.shader) url(fragment.shader))", "custom(url(vertex.shader) url(fragment.shader))"); > + > +testFilterRule("Multiple with fragment shader", > + "grayscale() custom(none url(fragment.shader)) sepia()", "grayscale(1) custom(none url(fragment.shader)) sepia(1)", > + ["WebKitCSSFilterValue.CSS_FILTER_GRAYSCALE", > + "WebKitCSSFilterValue.CSS_FILTER_CUSTOM", > + "WebKitCSSFilterValue.CSS_FILTER_SEPIA"], > + ["grayscale(1)", > + "custom(none url(fragment.shader))", > + "sepia(1)"]); Nit: some indentation differences up there. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:66 > +#include <algorithm> Is there a reason for this? > Source/WebCore/css/CSSStyleSelector.h:211 > + StyleShader* styleShader(CSSValue*); > + StyleShader* cachedOrPendingFromValue(WebKitCSSShaderValue*); I wonder if this should be named cachedOrPendingStyleShaderFromValue ? > Source/WebCore/rendering/style/StyleShader.h:57 > + bool m_isCachedShader:1; > + bool m_isPendingShader:1; I think we typically separate the : and num with spaces.
oops! patch was updated as I was reviewing :)
Comment on attachment 115190 [details] Patch V2 r+ with the changes from the previous review. I do think the bug needs a better title.
I will post a new patch in a few moments. > > LayoutTests/css3/filters/script-tests/custom-filter-property-computed-style.js:1 > > +description("Test the parsing of the custom() function of the -webkit-filter property."); > > This needs updating. Ok. > > > LayoutTests/css3/filters/script-tests/custom-filter-property-computed-style.js:82 > > +testFilterRule("Custom with vertex shader", > > + "custom(url(vertex.shader))", "custom(url(vertex.shader))"); > > + > > +testFilterRule("Custom with fragment shader", > > + "custom(none url(fragment.shader))", "custom(none url(fragment.shader))"); > > + > > +testFilterRule("Custom with both shaders", > > + "custom(url(vertex.shader) url(fragment.shader))", "custom(url(vertex.shader) url(fragment.shader))"); > > + > > +testFilterRule("Multiple with fragment shader", > > + "grayscale() custom(none url(fragment.shader)) sepia()", "grayscale(1) custom(none url(fragment.shader)) sepia(1)", > > + ["WebKitCSSFilterValue.CSS_FILTER_GRAYSCALE", > > + "WebKitCSSFilterValue.CSS_FILTER_CUSTOM", > > + "WebKitCSSFilterValue.CSS_FILTER_SEPIA"], > > + ["grayscale(1)", > > + "custom(none url(fragment.shader))", > > + "sepia(1)"]); > Nit: some indentation differences up there. Ok. > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:66 > > +#include <algorithm> > > Is there a reason for this? Good catch. I will remove it. I've actually took the big patch posted on the #71446 and deleted some code. It was used for std::sort, to sort the parameters by name. Sorting the parameters is not really required, but it makes testing easier. > > Source/WebCore/css/CSSStyleSelector.h:211 > > + StyleShader* styleShader(CSSValue*); > > + StyleShader* cachedOrPendingFromValue(WebKitCSSShaderValue*); > > I wonder if this should be named cachedOrPendingStyleShaderFromValue ? Ok. cachedOrPendingStyleShaderFromValue makes sense. > > Source/WebCore/rendering/style/StyleShader.h:57 > > + bool m_isCachedShader:1; > > + bool m_isPendingShader:1; > > I think we typically separate the : and num with spaces. Ok. That was copied from StyleImage.h, so I thought it was correct. It seems like both styles are used in WebKit's code (ie. CSSValue.h uses space), so I assume that the new style is to use spaces.
Created attachment 115202 [details] Patch V3
(In reply to comment #7) > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:66 > > > +#include <algorithm> > > > > Is there a reason for this? > Good catch. I will remove it. I've actually took the big patch posted on the #71446 and deleted some code. It was used for std::sort, to sort the parameters by name. Sorting the parameters is not really required, but it makes testing easier. Cool. > > > Source/WebCore/rendering/style/StyleShader.h:57 > > > + bool m_isCachedShader:1; > > > + bool m_isPendingShader:1; > > > > I think we typically separate the : and num with spaces. > > Ok. That was copied from StyleImage.h, so I thought it was correct. It seems like both styles are used in WebKit's code (ie. CSSValue.h uses space), so I assume that the new style is to use spaces. Yeah, I'm not sure myself.
Is everything ok with the last patch? Can you r+ and cq+ please?
(In reply to comment #10) > Is everything ok with the last patch? Can you r+ and cq+ please? Yep, doing so now.
Comment on attachment 115202 [details] Patch V3 View in context: https://bugs.webkit.org/attachment.cgi?id=115202&action=review > Source/WebCore/css/CSSStyleSelector.cpp:5519 > + operations.operations().append(operation); Reading this line makes my head hurt :) I wonder if we should rename the operations() accessor on FilterOperations. What do you think? (or we could put the append method on FilterOperations)
(In reply to comment #12) > (From update of attachment 115202 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115202&action=review > > > Source/WebCore/css/CSSStyleSelector.cpp:5519 > > + operations.operations().append(operation); > > Reading this line makes my head hurt :) I wonder if we should rename the operations() accessor on FilterOperations. What do you think? (or we could put the append method on FilterOperations) It looks like there's a lot of operations.operations().append in CSSStyleSelector.cpp. CSS Transforms have the same issue. Yes adding an append() method on the FilterOperations sound great. Also, operationAt() might be useful.
(In reply to comment #13) > (In reply to comment #12) > > (From update of attachment 115202 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=115202&action=review > > > > > Source/WebCore/css/CSSStyleSelector.cpp:5519 > > > + operations.operations().append(operation); > > > > Reading this line makes my head hurt :) I wonder if we should rename the operations() accessor on FilterOperations. What do you think? (or we could put the append method on FilterOperations) > > It looks like there's a lot of operations.operations().append in CSSStyleSelector.cpp. CSS Transforms have the same issue. > > Yes adding an append() method on the FilterOperations sound great. Also, operationAt() might be useful. OK. I filed https://bugs.webkit.org/show_bug.cgi?id=72406
<rdar://problem/10450040>
Comment on attachment 115202 [details] Patch V3 Rejecting attachment 115202 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: n/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Updating chromium port dependencies using gclient... ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/10310664
Looks like the patch didn't apply. Not sure why the bot produced a junky error message.
Created attachment 115323 [details] Patch for landing
(In reply to comment #18) > Created an attachment (id=115323) [details] > Patch for landing I've rebased the patch.
Comment on attachment 115323 [details] Patch for landing Clearing flags on attachment: 115323 Committed r100416: <http://trac.webkit.org/changeset/100416>
All reviewed patches have been landed. Closing bug.