Bug 56586

Created attachment 86088 [details] Patch

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

Created attachment 86181 [details] Patch

Adding the webkit prefix to the new introduced methods (getUserMedia). Fixing the build files of other WebKit ports.

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

Created attachment 86198 [details] Patch

Fixing windows build.

Comment on attachment 86198 [details] Patch What spec covers this? Please update your ChangeLog to explain. Also, please use webkit-patch or at least svn-create-patch to create your diffs so the ChangeLogs end up at the top. :)

The WHATWG HTML version does. This was recently changed by Hixie. http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#video-conferencing-and-peer-to-peer-communication

Comment on attachment 86198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86198&action=review > Source/WebCore/page/NavigatorUserMediaError.idl:33 > + // FIXME: delete this interface and implement a custom binding since spec defines it as a NoInterfaceObject. Any reason not to do this now? > Source/WebCore/page/NavigatorUserMediaErrorCallback.cpp:46 > +// NavigatorUserMediaSuccessCallback and BlobCallback. Again, why not do this now? > Source/WebCore/page/Page.cpp:858 > + m_streamController.set(new StreamController(m_streamClient, mainFrame()->document())); Do you plan to add a runtime flag? Other controller/client features only instantiate the controller when the runtime flag is set, as the controller is otherwsise not used. Make sure that the client is still signalled to shut down, if required, in this case though. Also, why doe we use the main frame's document? How does this work in the case of iframes? > Source/WebCore/page/Page.h:344 > + OwnPtr<StreamController> m_streamController; I understand that the spec refers to this as just 'Stream', but the name seems a little vague. Could you use a more descriptive name, or at least add a comment about what kind of stream it is? > Source/WebCore/page/StreamListener.h:41 > +// be implemented by both the StreamController and the StreamClientMock. I don't understand this. Can you explain why the mock client would implement this interface?

Created attachment 86487 [details] Patch

Comment on attachment 86198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86198&action=review >> Source/WebCore/page/NavigatorUserMediaError.idl:33 >> + // FIXME: delete this interface and implement a custom binding since spec defines it as a NoInterfaceObject. > > Any reason not to do this now? Fixed for all callback objects. Since the spec is still changing and being discussed we're delaying the removal of the idl file for NavigatorUserMediaError. >> Source/WebCore/page/NavigatorUserMediaErrorCallback.cpp:46 >> +// NavigatorUserMediaSuccessCallback and BlobCallback. > > Again, why not do this now? Done. Check CallbackTask.h file in the new patch. >> Source/WebCore/page/Page.cpp:858 >> + m_streamController.set(new StreamController(m_streamClient, mainFrame()->document())); > > Do you plan to add a runtime flag? Other controller/client features only instantiate the controller when the runtime flag is set, as the controller is otherwsise not used. Make sure that the client is still signalled to shut down, if required, in this case though. > > Also, why doe we use the main frame's document? How does this work in the case of iframes? Yes, we plan to add a runtime flag in chromium. The new patch also introduces the EnabledAtRuntime flag to the corresponding idl files. Fixed the problems with the frame, the security origins and replying using the same execution context that made the call. >> Source/WebCore/page/Page.h:344 >> + OwnPtr<StreamController> m_streamController; > > I understand that the spec refers to this as just 'Stream', but the name seems a little vague. Could you use a more descriptive name, or at least add a comment about what kind of stream it is? This is not a Stream but the class controlling them and acting as an interface to the client. Streams are introduced in bug 56666. In any case, I've added a comment in the StreamController.h and Stream.h (next patch) files to make clear that this file is about the Media Stream API. >> Source/WebCore/page/StreamListener.h:41 >> +// be implemented by both the StreamController and the StreamClientMock. > > I don't understand this. Can you explain why the mock client would implement this interface? Fixed. StreamListener removed, as the StreamController is now used directly.

(In reply to comment #8) > (From update of attachment 86198 [details]) > What spec covers this? Please update your ChangeLog to explain. > > Also, please use webkit-patch or at least svn-create-patch to create your diffs so the ChangeLogs end up at the top. :) Fixed.

(In reply to comment #9) > The WHATWG HTML version does. This was recently changed by Hixie. > > http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#video-conferencing-and-peer-to-peer-communication There seem to be no changes regarding the part of the spec implemented here (the getUserMedia method).

Comment on attachment 86487 [details] Patch This is a huge patch and the bindings stuff will need some careful review. I think it would speed things up to split it into smaller patches ... - Add the runtime flag - Add CallbackTask.h and update Blob use it - Add the JavaScript bindings - Add the outline of the controller/client Smaller patches are almost always better!

(In reply to comment #15) > (From update of attachment 86487 [details]) > This is a huge patch and the bindings stuff will need some careful review. I think it would speed things up to split it into smaller patches ... > - Add the runtime flag > - Add CallbackTask.h and update Blob use it > - Add the JavaScript bindings > - Add the outline of the controller/client > > Smaller patches are almost always better! Ok, I'm splitting this in 3 patches: - Add the runtime flag: https://bugs.webkit.org/show_bug.cgi?id=56921 - Add getUserMedia to the navigator and the required JS bindings: (this bug, updating soon) - Add the skeleton of MediaStreamController and MediaStreamClient: https://bugs.webkit.org/show_bug.cgi?id=56922 I have also renamed the StreamController and StreamClient classes to MediaStream ones to make their purposes clearer.

Created attachment 86614 [details] Patch

Split the bug in to three patches (see comment above).

Created attachment 86616 [details] Patch

Rebasing to test the bots. No code changes from last patch.

Attachment 86616 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8228780

Comment on attachment 86616 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86616&action=review first couple of comments ... > Source/WebCore/page/CallbackTask.h:43 > +// Auxiliar template to schedule calls to callbacks using their own script execution context. 'Auxiliary' or just 'Helper' > Source/WebCore/page/CallbackTask.h:73 > + , m_data(adoptRef(data)) Why are you calling adoptRef() here? adoptRef() is intended for use when an object is first created with 'new'. See http://www.webkit.org/coding/RefPtr.html

Created attachment 86663 [details] Patch

Rebasing, fixing a problem in WebCore.pro and changing raw pointers into PassRefPtr in CallbackTask.h as part of an update to this and 56922.

Comment on attachment 86616 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86616&action=review >> Source/WebCore/page/CallbackTask.h:43 >> +// Auxiliar template to schedule calls to callbacks using their own script execution context. > > 'Auxiliary' or just 'Helper' Fixed. >> Source/WebCore/page/CallbackTask.h:73 >> + , m_data(adoptRef(data)) > > Why are you calling adoptRef() here? adoptRef() is intended for use when an object is first created with 'new'. See http://www.webkit.org/coding/RefPtr.html Fixed. Was part of a temporary fix to avoid cycles while including.

Created attachment 86676 [details] Patch

(In reply to comment #26) > Created an attachment (id=86676) [details] > Patch Added missing virtual destructors to JavaScriptCore custom binding headers.

Comment on attachment 86676 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86676&action=review I think we can avoid the need for any custom callbacks. See Bug 40065. Also, you should add some tests for the bindings. See LayoutTests/fast/dom/Geolocation/enabled.html and LayoutTests/fast/dom/Geolocation/argument-types.html for example. > Source/WebCore/bindings/js/JSNavigatorCustom.cpp:50 > + // FIXME: This check disallows callable objects created via JSC API. It's not clear what exactly the specification intends to allow. A FIXME is OK here for now, but add a link to https://bugs.webkit.org/show_bug.cgi?id=40012 > Source/WebCore/bindings/js/JSNavigatorCustom.cpp:62 > + // Argument is optional (hence undefined is allowed), and null is allowed. Are you sure that null is allowed? I don't see this in the spec. > Source/WebCore/page/NavigatorUserMediaError.idl:33 > + // FIXME: delete this interface and implement a custom binding since spec defines it as a NoInterfaceObject. I don't think that it being marked NoInterfaceObject means that it needs to have custom bindings. It just means that the constructor object shouldn't appear on the global object. You should add a test for this.

Created attachment 87905 [details] Patch

Comment on attachment 86676 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86676&action=review >> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:50 >> + // FIXME: This check disallows callable objects created via JSC API. It's not clear what exactly the specification intends to allow. > > A FIXME is OK here for now, but add a link to https://bugs.webkit.org/show_bug.cgi?id=40012 Fixed. >> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:62 >> + // Argument is optional (hence undefined is allowed), and null is allowed. > > Are you sure that null is allowed? I don't see this in the spec. Fixed. >> Source/WebCore/page/NavigatorUserMediaError.idl:33 >> + // FIXME: delete this interface and implement a custom binding since spec defines it as a NoInterfaceObject. > > I don't think that it being marked NoInterfaceObject means that it needs to have custom bindings. It just means that the constructor object shouldn't appear on the global object. You should add a test for this. Fixed.

Comment on attachment 87905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87905&action=review Need to update the Android makefiles and fix the EFL build. I don't see a test for that fact that NavigatorUserMediaError is marked NoInterfaceObject? Also, can you add a test that navigator.webkitGetUserMedia() is present and enumerable, like fast/dom/Geolocation/enabled.html. > LayoutTests/fast/dom/MediaStream/script-tests/argument-types.js:16 > + shouldThrow(expression, '(function() { return "' + expectedException + '"; })();'); Do you use this functionality? > LayoutTests/fast/dom/MediaStream/script-tests/argument-types.js:76 > +test('navigator.webkitGetUserMedia("video", emptyFunction, -Infinity)', true); I don't think that any of these should throw, other than the one using objectThrowingException. I think the argument should just get converted to a string using the normal JS conversions. > Source/WebCore/DerivedSources.make:780 > +endif Usually we add new files to the makefiles for all ports, but leave it up to that port to add the machinery for flags etc. > Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:4 > + * Redistribution and use in source and binary forms, with or without Wrong license - http://trac.webkit.org/browser/trunk/Source/WebKit/LICENSE - throughout > Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:48 > +static void throwTypeMismatchException() This function and constant are copied from Geolocation, right? Is it used elsewhere too? It would be good to move it to a common place and share it. Maybe do this as a separate patch to keep this one manageable. > Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:53 > +static PassRefPtr<NavigatorUserMediaSuccessCallback> createNavigatorUserMediaSuccessCallback(v8::Local<v8::Value> value, bool& succeeded) This method and the one below are very similar to each other and to corresponding methods in Geolocation. How about we factor them out by adding allowNull and allowUndefined arguments? Are there other places in existing code that could use such helpers? > Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:97 > + if (!succeeded || !successCallback) Is the check for successCallback required? Can V8NavigatorUserMediaSuccessCallback::create return 0? > Source/WebCore/page/CallbackTask.h:84 > +class CallbackTask2 : public ScriptExecutionContext::Task { It looks like CallbackTask2 isn't used in this patch? Also, it's not clear from this patch why the error callback needs to implement CallbackTask1::Scheduler, as the functionality isn't used yet. I think it's better to take CallbackTask out of this patch. It can then be added separately before we add the UserMedia functionality that actually requires it. > Source/WebCore/page/NavigatorUserMediaError.h:49 > + unsigned short code() const { return m_code; } The constructor param and this getter should use the enum type.

Comment on attachment 87905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87905&action=review >> LayoutTests/fast/dom/MediaStream/script-tests/argument-types.js:76 > > I don't think that any of these should throw, other than the one using objectThrowingException. I think the argument should just get converted to a string using the normal JS conversions. The IDL type of the third argument is "in optional NavigatorUserMediaErrorCallback", which is a function only interface. It's my understanding that string type should not be compatible with this, unless I am mistaken?

Comment on attachment 87905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87905&action=review > LayoutTests/fast/dom/MediaStream/script-tests/argument-types.js:35 > +// 1 Argument Maybe add a comment that the reason all these throw is not due to the type conversion, but simply because the API requires 2 arguments. Test with emptyFunction too for completeness. > LayoutTests/fast/dom/MediaStream/script-tests/argument-types.js:40 > +test('navigator.webkitGetUserMedia("video")', true); Could use "video" for the other arguments below, too. > LayoutTests/fast/dom/MediaStream/script-tests/argument-types.js:52 > +test('navigator.webkitGetUserMedia(objectThrowingException, emptyFunction)', false); Shouldn't this one throw 'valueOf threw exception'? Or does toString() not use valueOf()? Maybe try an object with a custom toString() that throws? >> LayoutTests/fast/dom/MediaStream/script-tests/argument-types.js:76 >> +test('navigator.webkitGetUserMedia("video", emptyFunction, -Infinity)', true); > > I don't think that any of these should throw, other than the one using objectThrowingException. I think the argument should just get converted to a string using the normal JS conversions. Sorry, just realised that this comment is wrong - ignore it. I was getting confused with the Geolocation API.

Created attachment 88637 [details] Patch

Comment on attachment 87905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87905&action=review >> LayoutTests/fast/dom/MediaStream/script-tests/argument-types.js:16 >> + shouldThrow(expression, '(function() { return "' + expectedException + '"; })();'); > > Do you use this functionality? We use it now. Fixed. >> LayoutTests/fast/dom/MediaStream/script-tests/argument-types.js:35 >> +// 1 Argument > > Maybe add a comment that the reason all these throw is not due to the type conversion, but simply because the API requires 2 arguments. Test with emptyFunction too for completeness. Fixed. >> LayoutTests/fast/dom/MediaStream/script-tests/argument-types.js:40 >> +test('navigator.webkitGetUserMedia("video")', true); > > Could use "video" for the other arguments below, too. Fixed. >> LayoutTests/fast/dom/MediaStream/script-tests/argument-types.js:52 >> +test('navigator.webkitGetUserMedia(objectThrowingException, emptyFunction)', false); > > Shouldn't this one throw 'valueOf threw exception'? Or does toString() not use valueOf()? Maybe try an object with a custom toString() that throws? Fixed. >> Source/WebCore/DerivedSources.make:780 >> +endif > > Usually we add new files to the makefiles for all ports, but leave it up to that port to add the machinery for flags etc. Fixed. >> Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:4 >> + * Redistribution and use in source and binary forms, with or without > > Wrong license - http://trac.webkit.org/browser/trunk/Source/WebKit/LICENSE - throughout Fixed. >> Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:48 >> +static void throwTypeMismatchException() > > This function and constant are copied from Geolocation, right? Is it used elsewhere too? It would be good to move it to a common place and share it. Maybe do this as a separate patch to keep this one manageable. Fixed. Integrating into V8Utilities in https://bugs.webkit.org/show_bug.cgi?id=57760. >> Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:53 >> +static PassRefPtr<NavigatorUserMediaSuccessCallback> createNavigatorUserMediaSuccessCallback(v8::Local<v8::Value> value, bool& succeeded) > > This method and the one below are very similar to each other and to corresponding methods in Geolocation. How about we factor them out by adding allowNull and allowUndefined arguments? Are there other places in existing code that could use such helpers? Fixed. Integrating into V8Utilities in https://bugs.webkit.org/show_bug.cgi?id=57760. >> Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:97 >> + if (!succeeded || !successCallback) > > Is the check for successCallback required? Can V8NavigatorUserMediaSuccessCallback::create return 0? Fixed. >> Source/WebCore/page/CallbackTask.h:84 >> +class CallbackTask2 : public ScriptExecutionContext::Task { > > It looks like CallbackTask2 isn't used in this patch? Also, it's not clear from this patch why the error callback needs to implement CallbackTask1::Scheduler, as the functionality isn't used yet. > > I think it's better to take CallbackTask out of this patch. It can then be added separately before we add the UserMedia functionality that actually requires it. Fixed. >> Source/WebCore/page/NavigatorUserMediaError.h:49 >> + unsigned short code() const { return m_code; } > > The constructor param and this getter should use the enum type. Fixed.

(In reply to comment #31) > (From update of attachment 87905 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87905&action=review > > Need to update the Android makefiles and fix the EFL build. > > I don't see a test for that fact that NavigatorUserMediaError is marked NoInterfaceObject? Also, can you add a test that navigator.webkitGetUserMedia() is present and enumerable, like fast/dom/Geolocation/enabled.html. > Done.

Comment on attachment 88637 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88637&action=review > LayoutTests/fast/dom/MediaStream/script-tests/enabled.js:3 > +function hasGeolocationProperty() Geolocation ?! > Source/WebCore/bindings/js/JSNavigatorCustom.cpp:54 > + if (exec->hadException() || !successCallback) Do you need to check !successCallback? checkFunctionOnlyCallback is called with no flags so will only return null when it sets an exception. Can JSNavigatorUserMediaSuccessCallback::create() return null? > Source/WebCore/page/NavigatorUserMediaError.h:37 > + PERMISSION_DENIED = 1 Maybe add a comment that these values need to kept in sync with those in the IDL? > Source/WebCore/page/NavigatorUserMediaSuccessCallback.idl:2 > + * Copyright (C) 2011 Apple Inc. All rights reserved. Google Inc - throughout

Created attachment 88795 [details] Patch

Comment on attachment 88637 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88637&action=review >> LayoutTests/fast/dom/MediaStream/script-tests/enabled.js:3 >> +function hasGeolocationProperty() > > Geolocation ?! Sorry, I missed that one. Fixed. >> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:54 >> + if (exec->hadException() || !successCallback) > > Do you need to check !successCallback? checkFunctionOnlyCallback is called with no flags so will only return null when it sets an exception. Can JSNavigatorUserMediaSuccessCallback::create() return null? Fixed. >> Source/WebCore/page/NavigatorUserMediaError.h:37 >> + PERMISSION_DENIED = 1 > > Maybe add a comment that these values need to kept in sync with those in the IDL? Fixed. >> Source/WebCore/page/NavigatorUserMediaSuccessCallback.idl:2 >> + * Copyright (C) 2011 Apple Inc. All rights reserved. > > Google Inc - throughout Fixed.

Comment on attachment 88795 [details] Patch r=me

Comment on attachment 88795 [details] Patch Rejecting attachment 88795 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-..." exit_code: 2 Last 500 characters of output: /NavigatorUserMediaError.h patching file Source/WebCore/page/NavigatorUserMediaError.idl patching file Source/WebCore/page/NavigatorUserMediaErrorCallback.h patching file Source/WebCore/page/NavigatorUserMediaErrorCallback.idl patching file Source/WebCore/page/NavigatorUserMediaSuccessCallback.h patching file Source/WebCore/page/NavigatorUserMediaSuccessCallback.idl Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Steve Block', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/8373497

Created attachment 88798 [details] Patch

Comment on attachment 88798 [details] Patch Rebased.

Comment on attachment 88798 [details] Patch r=me

Comment on attachment 88798 [details] Patch Rejecting attachment 88798 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'apply-..." exit_code: 2 Last 500 characters of output: /NavigatorUserMediaError.h patching file Source/WebCore/page/NavigatorUserMediaError.idl patching file Source/WebCore/page/NavigatorUserMediaErrorCallback.h patching file Source/WebCore/page/NavigatorUserMediaErrorCallback.idl patching file Source/WebCore/page/NavigatorUserMediaSuccessCallback.h patching file Source/WebCore/page/NavigatorUserMediaSuccessCallback.idl Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Steve Block', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/8346989

Committed r83287: <http://trac.webkit.org/changeset/83287>

http://trac.webkit.org/changeset/83287 might have broken Qt Linux Release The following tests are not passing: fast/dom/MediaStream/argument-types.html fast/dom/MediaStream/enabled.html fast/dom/MediaStream/no-interface-object.html

(In reply to comment #47) > http://trac.webkit.org/changeset/83287 might have broken Qt Linux Release > The following tests are not passing: > fast/dom/MediaStream/argument-types.html > fast/dom/MediaStream/enabled.html > fast/dom/MediaStream/no-interface-object.html Same for GTK+ bots

Leandro is preparing a patch to add these new tests to all the relevant skip lists

Created attachment 88814 [details] Patch Adding the new tests to the skip lists of all non-chromium platforms as the Media Stream API is not enabled in them.

Comment on attachment 88814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88814&action=review > LayoutTests/platform/gtk/Skipped:1384 > +fast/dom/MediaStream/no-interface-object.html You cna just list fast/dom/MediaStream. Also, be sure to add to the appropriate section of each list (usually something like 'unimplemented features') > LayoutTests/platform/mac-leopard/Skipped:227 > +fast/dom/MediaStream/no-interface-object.html No need for this. The skipped lists inherit from each other, so platform-foo inherits everything from platform.

Created attachment 88820 [details] Patch

Comment on attachment 88814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88814&action=review >> LayoutTests/platform/gtk/Skipped:1384 >> +fast/dom/MediaStream/no-interface-object.html > > You cna just list fast/dom/MediaStream. Also, be sure to add to the appropriate section of each list (usually something like 'unimplemented features') Fixed. >> LayoutTests/platform/mac-leopard/Skipped:227 >> +fast/dom/MediaStream/no-interface-object.html > > No need for this. The skipped lists inherit from each other, so platform-foo inherits everything from platform. Fixed.

Committed r83292: <http://trac.webkit.org/changeset/83292>

Comment on attachment 88820 [details] Patch Added new tests to skipped lists

http://trac.webkit.org/changeset/83292 might have broken GTK Linux 32-bit Debug