Bug 149499 – Move webkitGetUserMedia to JS Builtin

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

This may remove code to move to JS builtin.

[youenn fablet]

Created attachment 262781 [details]
WIP

[youenn fablet]

Created attachment 262889 [details]
Patch

[youenn fablet]

(In reply to comment #2)
> Created attachment 262889 [details]
> Patch

Patch is migrating webkitGetUserMedia to js builtins.
It also fixes the code generator to properly handle builtins in partial interfaces.
I can split that as a different bug if that eases the review.

adambe, if this patch is going to the trunk, next step might be to remove
DOMPromiseWithCallback which would not be used in WebKit trunk code any longer.
Let me know your thoughts about it.

[youenn fablet]

Created attachment 262896 [details]
Fixing Win build

[Adam Bergkvist]

With the move to JSBulitins for RTCPeerConnection I'm not using DOMPromiseWithCallback anymore. So please go ahead.

[Adam Bergkvist]

Comment on attachment 262896 [details]
Fixing Win build

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

> Source/WebCore/Modules/mediastream/NavigatorUserMedia.js:30
> +    if (arguments.length != 3)

Perhaps we should use 'arguments.length < 3' here to not explicitly disallow calling with more than 3 arguments.

[Adam Bergkvist]

Comment on attachment 262896 [details]
Fixing Win build

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

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:102
> +        GlobalPropertyInfo(clientData.builtinNames().readableStreamClosedPrivateName(), jsNumber(1), DontDelete | ReadOnly),

This one ended up outside the ENABLE(STREAMS_API) guard.

[youenn fablet]

Created attachment 262986 [details]
Addressing Adam comments

[youenn fablet]

(In reply to comment #8)
> Created attachment 262986 [details]
> Addressing Adam comments

Thanks Adam.
Patch fixed accordingly.

I also added a fixme in the js file, as we should probably raise a DOM unsupported exception (like done in the custom binding) in lieu of a TypeError.

We might actually want to add a @DOMException() accessible from JS builtins to create DOM exceptions.

[youenn fablet]

Comment on attachment 262986 [details]
Addressing Adam comments

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

> Source/WebCore/ChangeLog:20
> +        This is done afterwards when calling navigator.MediaDevices.@getUserMedia.

This comment is no longer true. I will remove it when landing or in a next patch.

> Source/WebCore/Modules/mediastream/NavigatorUserMedia.js:32
> +

Implementing consistently in JS builtins the checks done by what would be the WebIDL generated code may not always be easy.
This may cause small behavior differences when migrating an existing function to JS builtins, as can be seen from this case.
I wonder whether this is good enough or whether a better solution should be found out.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:101
> +        GlobalPropertyInfo(clientData.builtinNames().getUserMediaPrivateName(), getUserMediaFunction, DontDelete | ReadOnly),

Adding getUserMedia to a private slot in JSDOMWindowBase is a temporary solution.
JSMediaDevicesPrototype should have a private symbol slot for it instead.
This is quite straightforward for the binding generator to handle this, using a [Private] extended keyword for instance.

[youenn fablet]

> Adding getUserMedia to a private slot in JSDOMWindowBase is a temporary
> solution.
> JSMediaDevicesPrototype should have a private symbol slot for it instead.
> This is quite straightforward for the binding generator to handle this,
> using a [Private] extended keyword for instance.

I gave a try at the [Private] approach.
It can be found in bug 150167.

I also splitted the changes to the binding generator in bug 150163 to ease the review.

[Adam Bergkvist]

(In reply to comment #10)
> 
> Implementing consistently in JS builtins the checks done by what would be
> the WebIDL generated code may not always be easy.
> This may cause small behavior differences when migrating an existing
> function to JS builtins, as can be seen from this case.
> I wonder whether this is good enough or whether a better solution should be
> found out.
> 

Implementing complete type checking routines in JS is quite a large task, but we could perhaps start with a smaller set of reusable helper functions.

For example:

throwIfNotEnoughArguments(requiredArgCount);

[youenn fablet]

Created attachment 263479 [details]
Rebasing

[youenn fablet]

(In reply to comment #12)
> (In reply to comment #10)
> > 
> > Implementing consistently in JS builtins the checks done by what would be
> > the WebIDL generated code may not always be easy.
> > This may cause small behavior differences when migrating an existing
> > function to JS builtins, as can be seen from this case.
> > I wonder whether this is good enough or whether a better solution should be
> > found out.
> > 
> 
> Implementing complete type checking routines in JS is quite a large task,
> but we could perhaps start with a smaller set of reusable helper functions.
> 
> For example:
> 
> throwIfNotEnoughArguments(requiredArgCount);

That might be handy indeed.
Uploaded patch is keeping the previous version argument checking code.
Let's continue discussing the level of fidelity we actually want.

[youenn fablet]

Created attachment 264575 [details]
Rebbasing

[youenn fablet]

Created attachment 264576 [details]
Rebasing

[youenn fablet]

Created attachment 264580 [details]
Fixing build

[youenn fablet]

Created attachment 264586 [details]
Adding missing CMake file

[Darin Adler]

Comment on attachment 264586 [details]
Adding missing CMake file

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

This looks fine. I can’t help thinking we are writing too much checking code and should find a simpler way to deal with type checking of both "this" and of arguments.

> Source/WebCore/Modules/mediastream/NavigatorUserMedia.js:30
> +    // FIXME: We should raise a DOM unsupported exception if there is no navigator and properly detect whether method is not called on a Navigator object.

Best way to document this would probably be test cases. Not even sure we need the FIXME if we had those test cases and documented them well.

> Source/WebCore/Modules/mediastream/NavigatorUserMedia.js:43
> +    if (arguments.length < 3)
> +        throw new @TypeError("Not enough arguments");
> +
> +    if (options !== Object(options))
> +        throw new @TypeError("Argument 1 (options) to Navigator.webkitGetUserMedia must be an object");
> +
> +    if (typeof successCallback !== "function")
> +        throw new @TypeError("Argument 2 ('successCallback') to Navigator.webkitGetUserMedia must be a function");
> +    if (typeof errorCallback !== "function")
> +        throw new @TypeError("Argument 3 ('errorCallback') to Navigator.webkitGetUserMedia must be a function");

I don’t have a real sense of how important it is to do all these checks. Seems a bit unnatural to check things that the next level down is perfectly able to check well enough.

[youenn fablet]

Thanks for the review.

(In reply to comment #19)
> Comment on attachment 264586 [details]
> Adding missing CMake file
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264586&action=review
> 
> This looks fine. I can’t help thinking we are writing too much checking code
> and should find a simpler way to deal with type checking of both "this" and
> of arguments.

We can do type check in different ways.
We can  mimic C++ checks (as done in current code).
If we want to continue into that direction, code could probably be generated based on the WebIDL.
That would ensure consistency across all built-ins APIs (and also with native APIs).

Another option is to do only the checks that are required for the code.
Taking NavigatorUserMedia.webkitGetUserMedia as an example, we really need only to check that successCallback and errorCallback are functions.
- 'options' check is done by MediaDevices.@getUserMediaFromJS (check will be done in a different order than its C++ version).
- 'this' check will be done when accessing this.mediaDevices.@getUserMediaFromJS ("object is null" exception raising).
- 'disconnected frame error case' will also be done when accessing this.mediaDevices ("object is null" exception raising).
In all cases, we got an exception, but error message or type would be different compared to its C++ counterpart. 

JS code would be down to:
function webkitGetUserMedia(options, successCallback, errorCallback)
{
    "use strict";

    if (typeof successCallback !== "function")
        throw new @TypeError("Argument 2 ('successCallback') to Navigator.webkitGetUserMedia must be a function");
    if (typeof errorCallback !== "function")
        throw new @TypeError("Argument 3 ('errorCallback') to Navigator.webkitGetUserMedia must be a function");

    this.mediaDevices.@getUserMediaFromJS(options).then(successCallback, errorCallback);
}

> > Source/WebCore/Modules/mediastream/NavigatorUserMedia.js:30
> > +    // FIXME: We should raise a DOM unsupported exception if there is no navigator and properly detect whether method is not called on a Navigator object.
> 
> Best way to document this would probably be test cases. Not even sure we
> need the FIXME if we had those test cases and documented them well.

This is somehow documented by rebased LayoutTests/http/tests/media/media-stream/disconnected-frame-already-expected.txt

[WebKit Commit Bot]

Comment on attachment 264586 [details]
Adding missing CMake file

Clearing flags on attachment: 264586

Committed r191949: <http://trac.webkit.org/changeset/191949>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.