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
: WebKit
HTML DOM
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
: 56458 56921 57760 57770 57963
: 56459 56922 58147 58459 60387
  Show dependency treegraph
 
Reported: 2011-03-17 13:43 PST by
Modified: 2012-09-19 07:20 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-03-17 13:43:30 PST
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 From 2011-03-17 13:53:53 PST -------
Created an attachment (id=86088) [details]
Patch
------- Comment #2 From 2011-03-17 14:19:26 PST -------
Attachment 86088 [details] did not build on win:
Build output: http://queues.webkit.org/results/8191773
------- Comment #3 From 2011-03-18 11:17:36 PST -------
Created an attachment (id=86181) [details]
Patch
------- Comment #4 From 2011-03-18 11:18:23 PST -------
Adding the webkit prefix to the new introduced methods (getUserMedia). Fixing the build files of other WebKit ports.
------- Comment #5 From 2011-03-18 12:06:50 PST -------
Attachment 86181 [details] did not build on win:
Build output: http://queues.webkit.org/results/8205242
------- Comment #6 From 2011-03-18 12:46:07 PST -------
Created an attachment (id=86198) [details]
Patch
------- Comment #7 From 2011-03-18 12:46:31 PST -------
Fixing windows build.
------- Comment #8 From 2011-03-18 14:25:01 PST -------
(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. :)
------- Comment #9 From 2011-03-18 14:30:17 PST -------
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 From 2011-03-21 03:18:25 PST -------
(From update of attachment 86198 [details])
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 From 2011-03-22 11:49:24 PST -------
Created an attachment (id=86487) [details]
Patch
------- Comment #12 From 2011-03-22 11:55:04 PST -------
(From update of attachment 86198 [details])
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 From 2011-03-22 11:55:37 PST -------
(In reply to comment #8)
> (From update of attachment 86198 [details] [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 From 2011-03-22 11:56:25 PST -------
(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 From 2011-03-23 05:28:45 PST -------
(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!
------- Comment #16 From 2011-03-23 06:53:05 PST -------
(In reply to comment #15)
> (From update of attachment 86487 [details] [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 From 2011-03-23 07:24:32 PST -------
Created an attachment (id=86614) [details]
Patch
------- Comment #18 From 2011-03-23 07:25:47 PST -------
Split the bug in to three patches (see comment above).
------- Comment #19 From 2011-03-23 07:32:24 PST -------
Created an attachment (id=86616) [details]
Patch
------- Comment #20 From 2011-03-23 07:32:49 PST -------
Rebasing to test the bots. No code changes from last patch.
------- Comment #21 From 2011-03-23 07:38:50 PST -------
Attachment 86616 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8228780
------- Comment #22 From 2011-03-23 11:50:34 PST -------
(From update of attachment 86616 [details])
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 From 2011-03-23 12:19:20 PST -------
Created an attachment (id=86663) [details]
Patch
------- Comment #24 From 2011-03-23 12:20:37 PST -------
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 From 2011-03-23 12:22:26 PST -------
(From update of attachment 86616 [details])
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 From 2011-03-23 13:05:03 PST -------
Created an attachment (id=86676) [details]
Patch
------- Comment #27 From 2011-03-23 13:05:45 PST -------
(In reply to comment #26)
> Created an attachment (id=86676) [details] [details]
> Patch

Added missing virtual destructors to JavaScriptCore custom binding headers.
------- Comment #28 From 2011-03-24 04:40:27 PST -------
(From update of attachment 86676 [details])
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 From 2011-04-01 13:13:12 PST -------
Created an attachment (id=87905) [details]
Patch
------- Comment #30 From 2011-04-01 13:14:25 PST -------
(From update of attachment 86676 [details])
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 From 2011-04-04 06:37:22 PST -------
(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.

> 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 From 2011-04-04 07:19:04 PST -------
(From update of attachment 87905 [details])
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 From 2011-04-04 07:24:33 PST -------
(From update of attachment 87905 [details])
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 From 2011-04-07 07:49:45 PST -------
Created an attachment (id=88637) [details]
Patch
------- Comment #35 From 2011-04-07 07:51:20 PST -------
(From update of attachment 87905 [details])
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 From 2011-04-07 07:51:53 PST -------
(In reply to comment #31)
> (From update of attachment 87905 [details] [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 From 2011-04-08 02:36:37 PST -------
(From update of attachment 88637 [details])
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 From 2011-04-08 04:04:30 PST -------
Created an attachment (id=88795) [details]
Patch
------- Comment #39 From 2011-04-08 04:04:58 PST -------
(From update of attachment 88637 [details])
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 From 2011-04-08 04:07:11 PST -------
(From update of attachment 88795 [details])
r=me
------- Comment #41 From 2011-04-08 04:23:52 PST -------
(From update of attachment 88795 [details])
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 From 2011-04-08 04:40:59 PST -------
Created an attachment (id=88798) [details]
Patch
------- Comment #43 From 2011-04-08 04:41:39 PST -------
(From update of attachment 88798 [details])
Rebased.
------- Comment #44 From 2011-04-08 04:42:20 PST -------
(From update of attachment 88798 [details])
r=me
------- Comment #45 From 2011-04-08 04:45:05 PST -------
(From update of attachment 88798 [details])
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 From 2011-04-08 05:10:20 PST -------
Committed r83287: <http://trac.webkit.org/changeset/83287>
------- Comment #47 From 2011-04-08 05:49:41 PST -------
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 From 2011-04-08 06:23:49 PST -------
(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 From 2011-04-08 06:28:57 PST -------
Leandro is preparing a patch to add these new tests to all the relevant skip lists
------- Comment #50 From 2011-04-08 06:54:20 PST -------
Created an attachment (id=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 From 2011-04-08 07:00:42 PST -------
(From update of attachment 88814 [details])
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 From 2011-04-08 07:15:52 PST -------
Created an attachment (id=88820) [details]
Patch
------- Comment #53 From 2011-04-08 07:17:18 PST -------
(From update of attachment 88814 [details])
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 From 2011-04-08 07:23:18 PST -------
Committed r83292: <http://trac.webkit.org/changeset/83292>
------- Comment #55 From 2011-04-08 07:24:07 PST -------
(From update of attachment 88820 [details])
Added new tests to skipped lists
------- Comment #56 From 2011-04-08 09:03:07 PST -------
http://trac.webkit.org/changeset/83292 might have broken GTK Linux 32-bit Debug