Bug 56586 - Media Stream API: add the getUserMedia method and the Javascript bindings
: Media Stream API: add the getUserMedia method and the Javascript bindings
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Leandro Graciá Gil
:
Depends on: 56458 56921 57760 57770 57963
Blocks: 56459 56922 58147 58459 60387
  Show dependency treegraph
 
Reported: 2011-03-17 13:43 PDT by Leandro Graciá Gil
Modified: 2012-09-19 07:20 PDT (History)
13 users (show)

See Also:


Attachments
Patch (63.84 KB, patch)
2011-03-17 13:53 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (63.02 KB, patch)
2011-03-18 11:17 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (62.34 KB, patch)
2011-03-18 12:46 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (89.97 KB, patch)
2011-03-22 11:49 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (74.21 KB, patch)
2011-03-23 07:24 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (74.21 KB, patch)
2011-03-23 07:32 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (73.40 KB, patch)
2011-03-23 12:19 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (73.45 KB, patch)
2011-03-23 13:05 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (63.78 KB, patch)
2011-04-01 13:13 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (61.25 KB, patch)
2011-04-07 07:49 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (61.27 KB, patch)
2011-04-08 04:04 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (61.25 KB, patch)
2011-04-08 04:40 PDT, Leandro Graciá Gil
steveblock: review+
commit-queue: commit‑queue-
Details | Formatted Diff | Diff
Patch (8.46 KB, patch)
2011-04-08 06:54 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (2.23 KB, patch)
2011-04-08 07:15 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leandro Graciá Gil 2011-03-17 13:43:30 PDT
This patch adds the getUserMedia method to the navigator with all its intermediate Javascript types and the basic skeleton of the stream controller and the client and listener interfaces.
Comment 1 Leandro Graciá Gil 2011-03-17 13:53:53 PDT
Created attachment 86088 [details]
Patch
Comment 2 Build Bot 2011-03-17 14:19:26 PDT
Attachment 86088 [details] did not build on win:
Build output: http://queues.webkit.org/results/8191773
Comment 3 Leandro Graciá Gil 2011-03-18 11:17:36 PDT
Created attachment 86181 [details]
Patch
Comment 4 Leandro Graciá Gil 2011-03-18 11:18:23 PDT
Adding the webkit prefix to the new introduced methods (getUserMedia). Fixing the build files of other WebKit ports.
Comment 5 Build Bot 2011-03-18 12:06:50 PDT
Attachment 86181 [details] did not build on win:
Build output: http://queues.webkit.org/results/8205242
Comment 6 Leandro Graciá Gil 2011-03-18 12:46:07 PDT
Created attachment 86198 [details]
Patch
Comment 7 Leandro Graciá Gil 2011-03-18 12:46:31 PDT
Fixing windows build.
Comment 8 Eric Seidel 2011-03-18 14:25:01 PDT
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. :)
Comment 9 Peter Beverloo 2011-03-18 14:30:17 PDT
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 10 Steve Block 2011-03-21 03:18:25 PDT
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?
Comment 11 Leandro Graciá Gil 2011-03-22 11:49:24 PDT
Created attachment 86487 [details]
Patch
Comment 12 Leandro Graciá Gil 2011-03-22 11:55:04 PDT
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.
Comment 13 Leandro Graciá Gil 2011-03-22 11:55:37 PDT
(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.
Comment 14 Leandro Graciá Gil 2011-03-22 11:56:25 PDT
(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 15 Steve Block 2011-03-23 05:28:45 PDT
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!
Comment 16 Leandro Graciá Gil 2011-03-23 06:53:05 PDT
(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.
Comment 17 Leandro Graciá Gil 2011-03-23 07:24:32 PDT
Created attachment 86614 [details]
Patch
Comment 18 Leandro Graciá Gil 2011-03-23 07:25:47 PDT
Split the bug in to three patches (see comment above).
Comment 19 Leandro Graciá Gil 2011-03-23 07:32:24 PDT
Created attachment 86616 [details]
Patch
Comment 20 Leandro Graciá Gil 2011-03-23 07:32:49 PDT
Rebasing to test the bots. No code changes from last patch.
Comment 21 WebKit Review Bot 2011-03-23 07:38:50 PDT
Attachment 86616 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8228780
Comment 22 Steve Block 2011-03-23 11:50:34 PDT
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
Comment 23 Leandro Graciá Gil 2011-03-23 12:19:20 PDT
Created attachment 86663 [details]
Patch
Comment 24 Leandro Graciá Gil 2011-03-23 12:20:37 PDT
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 25 Leandro Graciá Gil 2011-03-23 12:22:26 PDT
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.
Comment 26 Leandro Graciá Gil 2011-03-23 13:05:03 PDT
Created attachment 86676 [details]
Patch
Comment 27 Leandro Graciá Gil 2011-03-23 13:05:45 PDT
(In reply to comment #26)
> Created an attachment (id=86676) [details]
> Patch

Added missing virtual destructors to JavaScriptCore custom binding headers.
Comment 28 Steve Block 2011-03-24 04:40:27 PDT
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.
Comment 29 Leandro Graciá Gil 2011-04-01 13:13:12 PDT
Created attachment 87905 [details]
Patch
Comment 30 Leandro Graciá Gil 2011-04-01 13:14:25 PDT
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 31 Steve Block 2011-04-04 06:37:22 PDT
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 32 John Knottenbelt 2011-04-04 07:19:04 PDT
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 33 Steve Block 2011-04-04 07:24:33 PDT
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.
Comment 34 Leandro Graciá Gil 2011-04-07 07:49:45 PDT
Created attachment 88637 [details]
Patch
Comment 35 Leandro Graciá Gil 2011-04-07 07:51:20 PDT
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.
Comment 36 Leandro Graciá Gil 2011-04-07 07:51:53 PDT
(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 37 Steve Block 2011-04-08 02:36:37 PDT
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
Comment 38 Leandro Graciá Gil 2011-04-08 04:04:30 PDT
Created attachment 88795 [details]
Patch
Comment 39 Leandro Graciá Gil 2011-04-08 04:04:58 PDT
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 40 Steve Block 2011-04-08 04:07:11 PDT
Comment on attachment 88795 [details]
Patch

r=me
Comment 41 WebKit Commit Bot 2011-04-08 04:23:52 PDT
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
Comment 42 Leandro Graciá Gil 2011-04-08 04:40:59 PDT
Created attachment 88798 [details]
Patch
Comment 43 Leandro Graciá Gil 2011-04-08 04:41:39 PDT
Comment on attachment 88798 [details]
Patch

Rebased.
Comment 44 Steve Block 2011-04-08 04:42:20 PDT
Comment on attachment 88798 [details]
Patch

r=me
Comment 45 WebKit Commit Bot 2011-04-08 04:45:05 PDT
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
Comment 46 Steve Block 2011-04-08 05:10:20 PDT
Committed r83287: <http://trac.webkit.org/changeset/83287>
Comment 47 WebKit Review Bot 2011-04-08 05:49:41 PDT
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
Comment 48 Sergio Villar Senin 2011-04-08 06:23:49 PDT
(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
Comment 49 Steve Block 2011-04-08 06:28:57 PDT
Leandro is preparing a patch to add these new tests to all the relevant skip lists
Comment 50 Leandro Graciá Gil 2011-04-08 06:54:20 PDT
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 51 Steve Block 2011-04-08 07:00:42 PDT
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.
Comment 52 Leandro Graciá Gil 2011-04-08 07:15:52 PDT
Created attachment 88820 [details]
Patch
Comment 53 Leandro Graciá Gil 2011-04-08 07:17:18 PDT
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.
Comment 54 Steve Block 2011-04-08 07:23:18 PDT
Committed r83292: <http://trac.webkit.org/changeset/83292>
Comment 55 Steve Block 2011-04-08 07:24:07 PDT
Comment on attachment 88820 [details]
Patch

Added new tests to skipped lists
Comment 56 WebKit Review Bot 2011-04-08 09:03:07 PDT
http://trac.webkit.org/changeset/83292 might have broken GTK Linux 32-bit Debug