Bug 60174 - Fix getUserMedia callback bindings for JSC
Summary: Fix getUserMedia callback bindings for JSC
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Tommy Widenflycht
URL:
Keywords:
Depends on:
Blocks: 60394
  Show dependency treegraph
 
Reported: 2011-05-04 09:04 PDT by Adam Bergkvist
Modified: 2011-05-31 06:46 PDT (History)
13 users (show)

See Also:


Attachments
Proposed patch (43.84 KB, patch)
2011-05-04 09:19 PDT, Adam Bergkvist
no flags Details | Formatted Diff | Diff
Updated patch (2.95 KB, patch)
2011-05-06 12:09 PDT, Adam Bergkvist
no flags Details | Formatted Diff | Diff
Updated patch (1.92 KB, patch)
2011-05-11 05:51 PDT, Adam Bergkvist
tonyg: review-
tonyg: commit-queue-
Details | Formatted Diff | Diff
Patch (1.39 KB, patch)
2011-05-31 06:04 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Bergkvist 2011-05-04 09:04:26 PDT
The existing JSC bindings does not work.
Comment 1 Adam Bergkvist 2011-05-04 09:19:15 PDT
Created attachment 92255 [details]
Proposed patch
Comment 2 Early Warning System Bot 2011-05-04 09:29:05 PDT
Attachment 92255 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8557559
Comment 3 Build Bot 2011-05-04 09:46:53 PDT
Attachment 92255 [details] did not build on win:
Build output: http://queues.webkit.org/results/8554559
Comment 4 Alexey Proskuryakov 2011-05-04 12:31:27 PDT
> The existing JSC bindings does not work.
+        No new functionality to be tested.

These sentences seem to be contradictory.
Comment 5 Collabora GTK+ EWS bot 2011-05-04 13:58:34 PDT
Attachment 92255 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8557645
Comment 6 WebKit Review Bot 2011-05-04 21:07:45 PDT
Attachment 92255 [details] did not pass chromium-ews:
Output: http://queues.webkit.org/results/8571290
Comment 7 Adam Bergkvist 2011-05-06 12:07:21 PDT
(In reply to comment #4)
> > The existing JSC bindings does not work.
> +        No new functionality to be tested.
> 
> These sentences seem to be contradictory.

You're right. What I meant to say was that the functionality is already covered by existing tests. Although the tests pass, I had to tweak one of them since the expected results differed between JSC and V8 (see http://webkit.org/b/60391).
Comment 8 Adam Bergkvist 2011-05-06 12:09:24 PDT
Created attachment 92619 [details]
Updated patch

The problem can be solved while still using generated bindings. However, the currently generated bindings fail due to a callback argument not being translated correctly resulting in the handleEvent() function not being overridden as it should (argument mismatch between int and int*). To avoid updating the JSC bindings generator, the fix was to change the failing "dummy" argument type to DOMString since primitive number types are not supported as callback arguments. Also, in the webkitGetUserMedia() custom binding the argument count is checked before trying to convert the first argument (fixes a failure in the argument-types test).
Comment 9 Darin Adler 2011-05-10 09:30:26 PDT
Comment on attachment 92619 [details]
Updated patch

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

> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:43
> +    if (exec->argumentCount() < 2) {
> +        setDOMException(exec, TYPE_MISMATCH_ERR);
> +        return jsUndefined();
> +    }

The desired behavior here, then, is that undefined turns into "undefined" for options, but that a missing argument yields TYPE_MISMATCH_ERR?

Similarly, I’m surprised that undefined for successCallback has different behavior than a missing argument, given that the missing argument behavior is TYPE_MISMATCH_ERR.

I’m a little surprised by these. Are they really covered by existing tests? I don’t see tests that are passing undefined.
Comment 10 Adam Bergkvist 2011-05-11 05:51:34 PDT
Created attachment 93112 [details]
Updated patch

I agree. I made the mistake of trying to get an existing test to pass instead of checking the specification for the correct behavior. According to the specification there should never be any type mismatch errors and getUserMedia should fail silently if successCallback is null. If options end up being "undefined" a NOT_SUPPORTED_ERR exception will be thrown at a later stage. I have created a separate bug to fix this (see http://webkit.org/b/60622).
Comment 11 Darin Adler 2011-05-11 09:21:43 PDT
Comment on attachment 93112 [details]
Updated patch

This change seems OK. I don’t fully understand the context, but clearly it will do no harm.
Comment 12 Leandro Graciá Gil 2011-05-26 09:13:02 PDT
I think this has been caused by my arbitrary decision of using int as the dummy type. My apologies if it caused problems in JSC. I'll keep it in mind in case I see myself in a similar situation in the future.

In any case, this code you're trying to patch doesn't have dummies anymore since the bug 56666 introducing the GeneratedStream class has been finally landed. This makes the patch unnecessary at this point.

Layout tests for getUserMedia should be working now with the expected results in both JSC and V8. In case they don't, please don't hesitate to contact me or tommyw@google.com.

Thanks for contributing to this!
Comment 13 Tony Gentilcore 2011-05-26 09:18:00 PDT
r-/cq- as this won't merge and isn't necessary after http://trac.webkit.org/changeset/87150.
Comment 14 Adam Bergkvist 2011-05-30 07:58:33 PDT
(In reply to comment #12)
> I think this has been caused by my arbitrary decision of using int as the dummy type. My apologies if it caused problems in JSC. I'll keep it in mind in case I see myself in a similar situation in the future.
>

The JSC bindings generator didn't work too well with the int. I think it should be avoided since it's not a primitive type in WebIDL, although I've seen it used in other places in WebKit as well.

> In any case, this code you're trying to patch doesn't have dummies anymore since the bug 56666 introducing the GeneratedStream class has been finally landed. This makes the patch unnecessary at this point.

It was obviously temporary code but the fix turned out to be so tiny so I thought it would be nice to be able to run the tests (on the GTK port) asap. 

> Layout tests for getUserMedia should be working now with the expected results in both JSC and V8. In case they don't, please don't hesitate to contact me or tommyw@google.com.

I still get the same mismatching diff against the expected results (see http://webkit.org/b/60391).

I've noticed another difference between the V8 and JSC bindings regarding how the exception code is handled. It is not initialized (to 0) at creation in the JSC bindings, but I've seen that you've added an ec = 0; further down in the controller code to handle the consequences.
Comment 15 Tommy Widenflycht 2011-05-31 05:37:56 PDT
> I've noticed another difference between the V8 and JSC bindings regarding how the exception code is handled. It is not initialized (to 0) at creation in the JSC bindings, but I've seen that you've added an ec = 0; further down in the controller code to handle the consequences.

I'll take care of the initialization problem, thanks for pointing it out!
Please advise if you want me to create a new bug for it instead of reusing this one.
Comment 16 Tommy Widenflycht 2011-05-31 06:04:35 PDT
Created attachment 95423 [details]
Patch
Comment 17 WebKit Commit Bot 2011-05-31 06:44:54 PDT
The commit-queue encountered the following flaky tests while processing attachment 95423 [details]:

http/tests/websocket/tests/error-detect.html bug 54012 (author: abarth@webkit.org)
http/tests/websocket/tests/handshake-fail-by-no-upgrade-header.html bug 61784 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 18 WebKit Commit Bot 2011-05-31 06:46:26 PDT
Comment on attachment 95423 [details]
Patch

Clearing flags on attachment: 95423

Committed r87725: <http://trac.webkit.org/changeset/87725>
Comment 19 WebKit Commit Bot 2011-05-31 06:46:33 PDT
All reviewed patches have been landed.  Closing bug.