Bug 72378

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.