Bug 60622

(In reply to comment #0) > From the spec: > > 4. If successCallback is null, abort these steps. I think you're right on this. Currently, createFunctionOnlyCallback has optional flags as its last parameter for both JSC and V8 bindings. To accept null you just need to add the appropriate flag there. However, I disagree about undefined as mentioned in the bug title. Please correct me if I'm wrong, but I think that allowing undefined would mean that we consider valid to call the function with only one argument, which seems not to be the case.

(In reply to comment #1) > (In reply to comment #0) > > From the spec: > > > > 4. If successCallback is null, abort these steps. > > I think you're right on this. > > Currently, createFunctionOnlyCallback has optional flags as its last parameter for both JSC and V8 bindings. To accept null you just need to add the appropriate flag there. > > However, I disagree about undefined as mentioned in the bug title. Please correct me if I'm wrong, but I think that allowing undefined would mean that we consider valid to call the function with only one argument, which seems not to be the case. Since variables passed as arguments can be null or undefined (as opposed to having a "real" value) they are usually handled the same. If one would like to prevent the function from being called with only one argument one would have to check the number of arguments actually passed. I think the current specification should be read as: the successCallback argument is mandatory for the function to do what it should do, and the errorCallback is optional for handling errors.

To be precise: 1) For functions built in to language, JavaScript makes no distinction between undefined arguments and missing arguments. Any arguments omitted are simply set to undefined. 2) For DOM functions, WebKit originally did the same, never checking numbers of arguments, and checking for the undefined value to handle missing arguments. But Mozilla has always checked numbers of arguments, and this different choice in the WebKit DOM has been controversial and is a possible source of incompatibilities between the browser engines. The JavaScript runtime does allow functions to tell the difference between actual undefined arguments and arguments that are missing. 3) The value null is not involved in this. It is a separate value, not the same as undefined, and unrelated to missing arguments. The only connection to null is that it's common to handle both null and undefined in the same way in a function implementation. Hope this explanation helps.

(In reply to comment #3) > To be precise: > > 1) For functions built in to language, JavaScript makes no distinction between undefined arguments and missing arguments. Any arguments omitted are simply set to undefined. > > 2) For DOM functions, WebKit originally did the same, never checking numbers of arguments, and checking for the undefined value to handle missing arguments. But Mozilla has always checked numbers of arguments, and this different choice in the WebKit DOM has been controversial and is a possible source of incompatibilities between the browser engines. The JavaScript runtime does allow functions to tell the difference between actual undefined arguments and arguments that are missing. > > 3) The value null is not involved in this. It is a separate value, not the same as undefined, and unrelated to missing arguments. The only connection to null is that it's common to handle both null and undefined in the same way in a function implementation. > > Hope this explanation helps. Thank you very much for the explanation. In that case, I'm removing the 'undefined' from the bug title. Please don't hesitate to discuss any objections about his.

Created attachment 95430 [details] Patch

var u; var n = null; navigator.webkitGetUserMedia("audio", u); // will throw navigator.webkitGetUserMedia("audio", n); // will not throw I don't think this is the desired behavior. As Darin pointed out, it's common to handle both null and undefined in the same way.

(In reply to comment #6) > var u; > var n = null; > > navigator.webkitGetUserMedia("audio", u); // will throw > navigator.webkitGetUserMedia("audio", n); // will not throw > > I don't think this is the desired behavior. As Darin pointed out, it's common to handle both null and undefined in the same way. The problem is that if we allow undefined then this would not throw either: navigator.webkitGetUserMedia("audio"); So we would be implicitly accepting that getUserMedia can be invoked with only one argument, which seems not to be the case in the spec (the successCallback is not an optional argument). Darin, do you have any suggestions about what can we do about this? Maybe check the number of arguments?

We're removing the review flag until this is properly discussed.

(In reply to comment #7) > Darin, do you have any suggestions about what can we do about this? Maybe check the number of arguments? Sure, we can definitely check the number of arguments if we need to distinguish undefined from a missing argument. We need to figure out what behavior we want. The implementation is not tricky once we know what we want.

Created attachment 96945 [details] Patch

The latest patch makes sense to me, hope it makes sense in general.

Comment on attachment 96945 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96945&action=review > LayoutTests/fast/dom/MediaStream/argument-types-expected.txt:-18 > -PASS navigator.webkitGetUserMedia(undefined, emptyFunction) threw exception Error: NOT_SUPPORTED_ERR: DOM Exception 9. We should keep at least one test case for the DOM Exception 9. If I'm not wrong, it could be done with something like navigator.webkitGetUserMedia("invalid options", emptyFunction). It may also be a good idea to do some basic testing to the parsing of the options string using this same DOM Exception. > Source/WebCore/bindings/js/JSNavigatorCustom.cpp:41 > + // Explicitly check basic assumtions for clarity. Minor nit: assumptions. > Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:46 > + // Explicitly check basic assumtions for clarity. Same nit as before. > Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:47 > + if (isUndefinedOrNull(args[0]) || !args[0]->IsString()) This makes a lot of sense to me, but I'm still not sure if that's the intended behaviour according to the spec. This argument is obviously only useful if it's a string, but still I don't know if we can safely throw a type mismatch error when some other types could be automatically converted to Strings, even if this will render their contents to be invalid (which is likely to lead to a DOM Exception 9 instead of 17). Maybe we should ask this little detail in whatwg? We could also make sure that this first 'options' argument is not nullable, since the official spec doesn't reflect yet the changes on WebIDL about the explicit use of the '?' symbol for the nullable types. Please also make sure to keep the objectThrowingException cases if toString is used again. > Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:49 > + if (isUndefinedOrNull(args[1])) Since this is now being checked at the binding level, we should probably replace the check for successCallback in MediaStreamFrameController::generateStream with an assertion. > Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:-46 > - v8::Handle<v8::String> options = args[0]->ToString(); If removing the previous IsString, have you checked if the objectThrowingException test case makes ToString to raise an exception? Just for curiosity, as it was the reason that made me to use it instead of toWebCoreString. In any case if IsString is finally used this should be perfectly safe.

I think the first argument is handled correctly (according to the spec) already. We just want to prevent getUserMedia from throwing when the second argument is undefined or null which can be achieved by passing CallbackAllowUndefined | CallbackAllowNull to createFunctionOnlyCallback.

(In reply to comment #13) > I think the first argument is handled correctly (according to the spec) already. We just want to prevent getUserMedia from throwing when the second argument is undefined or null which can be achieved by passing CallbackAllowUndefined | CallbackAllowNull to createFunctionOnlyCallback. Sure, you're right on that and it will be solved. Thanks for pointing it out. My point is to make sure that all other details are completely ok and the spec is specific enough about this. Otherwise we might find in the future that some details are different in other non WebKit-based browsers when we're supposed to define and implement one same specification.

Comment on attachment 96945 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96945&action=review >> LayoutTests/fast/dom/MediaStream/argument-types-expected.txt:-18 >> -PASS navigator.webkitGetUserMedia(undefined, emptyFunction) threw exception Error: NOT_SUPPORTED_ERR: DOM Exception 9. > > We should keep at least one test case for the DOM Exception 9. If I'm not wrong, it could be done with something like navigator.webkitGetUserMedia("invalid options", emptyFunction). It may also be a good idea to do some basic testing to the parsing of the options string using this same DOM Exception. Done >> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:41 >> + // Explicitly check basic assumtions for clarity. > > Minor nit: assumptions. Done. >> Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:46 >> + // Explicitly check basic assumtions for clarity. > > Same nit as before. Done. >> Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:47 >> + if (isUndefinedOrNull(args[0]) || !args[0]->IsString()) > > This makes a lot of sense to me, but I'm still not sure if that's the intended behaviour according to the spec. This argument is obviously only useful if it's a string, but still I don't know if we can safely throw a type mismatch error when some other types could be automatically converted to Strings, even if this will render their contents to be invalid (which is likely to lead to a DOM Exception 9 instead of 17). > > Maybe we should ask this little detail in whatwg? We could also make sure that this first 'options' argument is not nullable, since the official spec doesn't reflect yet the changes on WebIDL about the explicit use of the '?' symbol for the nullable types. Please also make sure to keep the objectThrowingException cases if toString is used again. Sorry, it should have been NOT_SUPPORTED_ERR. >> Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:49 >> + if (isUndefinedOrNull(args[1])) > > Since this is now being checked at the binding level, we should probably replace the check for successCallback in MediaStreamFrameController::generateStream with an assertion. Done. >> Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:-46 >> - v8::Handle<v8::String> options = args[0]->ToString(); > > If removing the previous IsString, have you checked if the objectThrowingException test case makes ToString to raise an exception? Just for curiosity, as it was the reason that made me to use it instead of toWebCoreString. In any case if IsString is finally used this should be perfectly safe. Neither version triggered the ToString exception, hence my switch to the more readable version.

Created attachment 96952 [details] Patch

(In reply to comment #14) > > My point is to make sure that all other details are completely ok and the spec is specific enough about this. Otherwise we might find in the future that some details are different in other non WebKit-based browsers when we're supposed to define and implement one same specification. Yeah, +1 to that.

Comment on attachment 96952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96952&action=review Looks quite good, just a few minor details. > LayoutTests/fast/dom/MediaStream/argument-types-expected.txt:-28 > -PASS navigator.webkitGetUserMedia("video", undefined) threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17. Do you think it's a good time to add some simple tests for the option parsing? Some suggestions: navigator.webkitGetUserMedia(", this should pass, video suboptions here,, s0me rubish@$!", emptyFunction); navigator.webkitGetUserMedia("this should pass, audio, video audio, audio video, video", emptyFunction); navigator.webkitGetUserMedia("this should throw NOT_SUPPORTED_ERR", emptyFunction); navigator.webkitGetUserMedia("this should throw NOT_SUPPORTED_ERR since it's split by commas: audio", emptyFunction); Of course at this point we can only test that either video or audio are provided. Properly testing the parsing of suboptions and checking what is really required to the embedder is a task for a later test involving the successful creation of generated streams. > Source/WebCore/page/MediaStreamFrameController.cpp:280 > + ASSERT(successCallback.get()); .get() should not be required here.

Created attachment 96957 [details] Patch

Comment on attachment 96952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96952&action=review >> LayoutTests/fast/dom/MediaStream/argument-types-expected.txt:-28 >> -PASS navigator.webkitGetUserMedia("video", undefined) threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17. > > Do you think it's a good time to add some simple tests for the option parsing? Some suggestions: > > navigator.webkitGetUserMedia(", this should pass, video suboptions here,, s0me rubish@$!", emptyFunction); > navigator.webkitGetUserMedia("this should pass, audio, video audio, audio video, video", emptyFunction); > navigator.webkitGetUserMedia("this should throw NOT_SUPPORTED_ERR", emptyFunction); > navigator.webkitGetUserMedia("this should throw NOT_SUPPORTED_ERR since it's split by commas: audio", emptyFunction); > > Of course at this point we can only test that either video or audio are provided. Properly testing the parsing of suboptions and checking what is really required to the embedder is a task for a later test involving the successful creation of generated streams. done >> Source/WebCore/page/MediaStreamFrameController.cpp:280 >> + ASSERT(successCallback.get()); > > .get() should not be required here. done.

Comment on attachment 96957 [details] Patch Looks good to me. Thanks Tommy!

Comment on attachment 96957 [details] Patch Attachment 96957 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/8830975

Created attachment 97082 [details] Patch

I think it would be best to avoid doing argument checks, that are described in the specification algorithms, in the bindings. Otherwise it would be up to each binding implementation to get this right. The current patch handles the argument check for successCallback in the bindings while checking the errorCallback is left to the implementation. My suggestion is to pass CallbackAllowUndefined | CallbackAllowNull to createFunctionOnlyCallback for both the successCallback and the errorCallback and leave the successCallback null-check in MediaStreamFrameController::generateStream() as is. I still think the first argument is handled correctly already. We should just let the string conversion rules be applied, and the options parsing will handle the throwing. The new tests for options parsing make sense, but perhaps they should be introduced in a separate bug?

Comment on attachment 97082 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97082&action=review > Source/WebCore/bindings/js/JSNavigatorCustom.cpp:41 > + // Explicitly check basic assumptions for clarity. That doesn’t seem like the right tradeoff. We don’t want special case code that just does the same thing the code below does unless we need it for performance reasons. I think the tests provide sufficient clarity, or a comment could. Adding extra code at runtime just for clarity is not the right tradeoff. Or maybe this is not just for clarity, and actually has an effect. If that is so, then they comment is misleading.

Created attachment 97253 [details] Patch

Comment on attachment 97082 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97082&action=review >> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:41 >> + // Explicitly check basic assumptions for clarity. > > That doesn’t seem like the right tradeoff. We don’t want special case code that just does the same thing the code below does unless we need it for performance reasons. I think the tests provide sufficient clarity, or a comment could. Adding extra code at runtime just for clarity is not the right tradeoff. > > Or maybe this is not just for clarity, and actually has an effect. If that is so, then they comment is misleading. Removed.

I don't like that calling getUserMedia() will just return silently, but since this is the consensus I have done what you have asked (finally ;). Thanks for having patience with me while I get into the whole JavaScript/WebKit world. I'll would like leave the tests in there since more tests are always better than fewer, unless you feel strongly I should move them to a new bug. (In reply to comment #24) > I think it would be best to avoid doing argument checks, that are described in the specification algorithms, in the bindings. Otherwise it would be up to each binding implementation to get this right. The current patch handles the argument check for successCallback in the bindings while checking the errorCallback is left to the implementation. My suggestion is to pass CallbackAllowUndefined | CallbackAllowNull to createFunctionOnlyCallback for both the successCallback and the errorCallback and leave the successCallback null-check in MediaStreamFrameController::generateStream() as is. > > I still think the first argument is handled correctly already. We should just let the string conversion rules be applied, and the options parsing will handle the throwing. The new tests for options parsing make sense, but perhaps they should be introduced in a separate bug?

(In reply to comment #28) > I don't like that calling getUserMedia() will just return silently, but since this is the consensus I have done what you have asked (finally ;). Thanks :) > I'll would like leave the tests in there since more tests are always better than fewer, unless you feel strongly I should move them to a new bug. I don't have a strong opinion about the new tests, but perhaps it should be mentioned in the ChangeLog. I also noted that one test was removed. It's indeed a corner case, but if I recall correctly, that test previously showed a difference in behavior between the JSC and V8 bindings (toString() didn't throw in the V8 bindings) so it might be good to keep it.

Looks like this bug got forgotten? The change looks OK to me?

Adam says to close as wontfix.

Comment on attachment 97253 [details] Patch Cleared review? from attachment 97253 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).