Bug 60174

Created attachment 92255 [details] Proposed patch

Attachment 92255 [details] did not build on qt: Build output: http://queues.webkit.org/results/8557559

Attachment 92255 [details] did not build on win: Build output: http://queues.webkit.org/results/8554559

> The existing JSC bindings does not work. + No new functionality to be tested. These sentences seem to be contradictory.

Attachment 92255 [details] did not build on gtk: Build output: http://queues.webkit.org/results/8557645

Attachment 92255 [details] did not pass chromium-ews: Output: http://queues.webkit.org/results/8571290

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

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

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 on attachment 93112 [details] Updated patch This change seems OK. I don’t fully understand the context, but clearly it will do no harm.

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!

r-/cq- as this won't merge and isn't necessary after http://trac.webkit.org/changeset/87150.

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

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

Created attachment 95423 [details] Patch

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 on attachment 95423 [details] Patch Clearing flags on attachment: 95423 Committed r87725: <http://trac.webkit.org/changeset/87725>

All reviewed patches have been landed. Closing bug.