This may remove code to move to JS builtin.
Created attachment 262781 [details] WIP
Created attachment 262889 [details] Patch
(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.
Created attachment 262896 [details] Fixing Win build
With the move to JSBulitins for RTCPeerConnection I'm not using DOMPromiseWithCallback anymore. So please go ahead.
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.
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.
Created attachment 262986 [details] Addressing Adam comments
(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.
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.
> 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.
(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);
Created attachment 263479 [details] Rebasing
(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.
Created attachment 264575 [details] Rebbasing
Created attachment 264576 [details] Rebasing
Created attachment 264580 [details] Fixing build
Created attachment 264586 [details] Adding missing CMake file
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.
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
Comment on attachment 264586 [details] Adding missing CMake file Clearing flags on attachment: 264586 Committed r191949: <http://trac.webkit.org/changeset/191949>
All reviewed patches have been landed. Closing bug.