Bug 93011 - [CSS Shaders] Add IDL file and bindings for mix function
Summary: [CSS Shaders] Add IDL file and bindings for mix function
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P3 Normal
Assignee: Michelangelo De Simone
URL:
Keywords:
Depends on: 90101
Blocks: 71392
  Show dependency treegraph
 
Reported: 2012-08-02 11:39 PDT by Max Vujovic
Modified: 2012-11-26 11:58 PST (History)
17 users (show)

See Also:


Attachments
Not for review (24.78 KB, patch)
2012-11-14 12:59 PST, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Patch (21.48 KB, patch)
2012-11-15 10:10 PST, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Patch (Rebased) (21.56 KB, patch)
2012-11-15 13:57 PST, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Patch (22.69 KB, patch)
2012-11-19 14:14 PST, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Patch (29.68 KB, patch)
2012-11-19 15:37 PST, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Patch for landing (36.91 KB, patch)
2012-11-26 10:56 PST, Michelangelo De Simone
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Max Vujovic 2012-08-02 11:39:05 PDT
In this example:

-webkit-filter: custom(none mix(shader.frag normal src-over));

It would be nice if "mix(...)" shows up in CSSOM as WebKitCSSMixFunctionValue instead of CSSValueList.
Comment 1 Michelangelo De Simone 2012-11-14 12:59:32 PST
Created attachment 174234 [details]
Not for review
Comment 2 Peter Beverloo (cr-android ews) 2012-11-14 20:06:20 PST
Comment on attachment 174234 [details]
Not for review

Attachment 174234 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14857032
Comment 3 Build Bot 2012-11-15 02:49:20 PST
Comment on attachment 174234 [details]
Not for review

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

New failing tests:
inspector-protocol/nmi-webaudio.html
Comment 4 Michelangelo De Simone 2012-11-15 10:10:12 PST
Created attachment 174478 [details]
Patch
Comment 5 Michelangelo De Simone 2012-11-15 13:57:56 PST
Created attachment 174507 [details]
Patch (Rebased)
Comment 6 Alexandru Chiculita 2012-11-15 14:18:50 PST
Comment on attachment 174507 [details]
Patch (Rebased)

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

Looks good!

> Source/WebCore/css/WebKitCSSMixFunctionValue.idl:32
> +] interface WebKitCSSMixFunctionValue : CSSValueList {

I think you also need to add the "WebKitCSSMixFunctionValueConstructor" to the DOMWindow.idl . Also, I think there's a test you need to update after that.

attribute CSSValueConstructor CSSValue;
attribute CSSPrimitiveValueConstructor CSSPrimitiveValue;
attribute CSSValueListConstructor CSSValueList;
attribute WebKitCSSTransformValueConstructor WebKitCSSTransformValue;

> LayoutTests/ChangeLog:15
> +        * platform/chromium/css3/filters/custom/custom-filter-mix-bindings-expected.txt: Added.

I think you need to explain why Chromium fails two of the tests.

> LayoutTests/css3/filters/script-tests/custom-filter-mix-bindings.js:39
> +var mixRule = filterRule[0][0][1]

I think mixRule is actually the mixFunction, right?

Should you also test the variables inside the rule?
Comment 7 Dean Jackson 2012-11-15 16:34:40 PST
Comment on attachment 174507 [details]
Patch (Rebased)

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

>> Source/WebCore/css/WebKitCSSMixFunctionValue.idl:32
>> +] interface WebKitCSSMixFunctionValue : CSSValueList {
> 
> I think you also need to add the "WebKitCSSMixFunctionValueConstructor" to the DOMWindow.idl . Also, I think there's a test you need to update after that.
> 
> attribute CSSValueConstructor CSSValue;
> attribute CSSPrimitiveValueConstructor CSSPrimitiveValue;
> attribute CSSValueListConstructor CSSValueList;
> attribute WebKitCSSTransformValueConstructor WebKitCSSTransformValue;

Alex is right. You'll need to update two tests. One is window, the other is an SVG version of the same.
Comment 8 Alexandru Chiculita 2012-11-16 09:15:04 PST
(In reply to comment #7)
> (From update of attachment 174507 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174507&action=review
> 
> >> Source/WebCore/css/WebKitCSSMixFunctionValue.idl:32
> >> +] interface WebKitCSSMixFunctionValue : CSSValueList {
> > 
> > I think you also need to add the "WebKitCSSMixFunctionValueConstructor" to the DOMWindow.idl . Also, I think there's a test you need to update after that.
> > 
> > attribute CSSValueConstructor CSSValue;
> > attribute CSSPrimitiveValueConstructor CSSPrimitiveValue;
> > attribute CSSValueListConstructor CSSValueList;
> > attribute WebKitCSSTransformValueConstructor WebKitCSSTransformValue;
> 
> Alex is right. You'll need to update two tests. One is window, the other is an SVG version of the same.

Michelangelo, can you check if the other CSS Filters related IDL files are added to the window object correctly? If not, could you add bugs for them?
Comment 9 Michelangelo De Simone 2012-11-16 10:38:46 PST
(In reply to comment #8)

> Michelangelo, can you check if the other CSS Filters related IDL files are added to the window object correctly? If not, could you add bugs for them?

Aye aye, sir!
Comment 10 Michelangelo De Simone 2012-11-16 14:17:20 PST
Looks like that the supposed infamous test (which I encountered in the past) is not there anymore. I'll rebase the patch with minor modifications and upload it for r? again.

If somebody has more information about this...
Comment 11 Michelangelo De Simone 2012-11-19 14:14:26 PST
Created attachment 175039 [details]
Patch
Comment 12 Michelangelo De Simone 2012-11-19 15:24:43 PST
The test we were all talking about is fast/js/global-constructors.html which is actually in the process of being removed (see #90833).
As of now, it's already skipped on Mac and MacWin.

I'll rebaseline anyway and upload a new patch momentarily.
Comment 13 Michelangelo De Simone 2012-11-19 15:37:50 PST
Created attachment 175058 [details]
Patch
Comment 14 Dean Jackson 2012-11-26 10:02:55 PST
r+ing this. For years now we've been told to stop using CSSValueList for things, but until someone takes on the task of redesigning the CSS OM we're stuck.
Comment 15 WebKit Review Bot 2012-11-26 10:49:37 PST
Comment on attachment 175058 [details]
Patch

Rejecting attachment 175058 [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:
ed.txt
patching file LayoutTests/platform/qt-5.0/fast/js/global-constructors-expected.txt
Hunk #1 succeeded at 322 (offset 2 lines).
patching file LayoutTests/platform/qt/fast/js/global-constructors-expected.txt
Hunk #1 succeeded at 305 (offset 2 lines).
patching file LayoutTests/platform/win/fast/js/global-constructors-expected.txt

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Dean Jacks..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/14985785
Comment 16 Michelangelo De Simone 2012-11-26 10:56:22 PST
Created attachment 176035 [details]
Patch for landing
Comment 17 WebKit Review Bot 2012-11-26 11:58:29 PST
Comment on attachment 176035 [details]
Patch for landing

Clearing flags on attachment: 176035

Committed r135749: <http://trac.webkit.org/changeset/135749>
Comment 18 WebKit Review Bot 2012-11-26 11:58:35 PST
All reviewed patches have been landed.  Closing bug.