Bug 60177 - Media Stream API: add local stream requests
: Media Stream API: add local stream requests
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
: 56922 60387
: 56459 56666 58550 60205
  Show dependency treegraph
 
Reported: 2011-05-04 09:21 PST by
Modified: 2012-09-19 07:21 PST (History)


Attachments
Patch (32.04 KB, patch)
2011-05-04 10:02 PST, Leandro Graciá Gil
no flags Review Patch | Details | Formatted Diff | Diff
Patch (36.80 KB, patch)
2011-05-06 13:15 PST, Leandro Graciá Gil
no flags Review Patch | Details | Formatted Diff | Diff
Patch (37.04 KB, patch)
2011-05-09 08:47 PST, Leandro Graciá Gil
no flags Review Patch | Details | Formatted Diff | Diff
Patch (37.05 KB, patch)
2011-05-16 08:58 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-05-04 09:21:24 PST
Add the code and messages for requesting the generation of local streams and getting the reply back. This affects the page and frame controllers as well as the embedder client interface.
------- Comment #1 From 2011-05-04 10:02:42 PST -------
Created an attachment (id=92267) [details]
Patch
------- Comment #2 From 2011-05-05 03:35:49 PST -------
(From update of attachment 92267 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=92267&action=review

Mostly just questions.

> Source/WebCore/page/CallbackTask.h:25
> +#ifndef CallbackTask_h

The name CallbackTask under page/ implies a general purpose utility. However, this is only enabled for media stream.

Do you have additional plans for this class? If not, perhaps it should just be implemented in NavigatorUserMediaErrorCallback.h?

> Source/WebCore/page/CallbackTask.h:40
> +class CallbackTask1 : public ScriptExecutionContext::Task {

Why the "1" in the name? Are you planning other versions?

> Source/WebCore/page/CallbackTask.h:52
> +    class Scheduler {

Why a subclass instead of a static method on the parent? If it should be a class, it needs a virtual dtor, right?

> Source/WebCore/page/MediaStreamClient.h:42
> +    virtual void generateStream(int requestId, bool audio, bool video, PassRefPtr<SecurityOrigin>) = 0;

Instead of two bools, do you think an enum or bitfield would help ensure the args are passed properly? Could also prevent argument proliferation if there ever happen to be more types of streams in the future (e.g. front facing vs rear facing video or speaker vs handset audio).

> Source/WebCore/page/MediaStreamFrameController.cpp:196
> +bool MediaStreamFrameController::generateStream(const String& options,

Probably worth adding a comment mentioning that this method is meant to implement this:
http://www.whatwg.org/specs/web-apps/current-work/#dom-navigator-getusermedia

> Source/WebCore/page/MediaStreamFrameController.cpp:206
> +    options.split(",", splitOptions);

Can you split on the char ',' instead of string?

> Source/WebCore/page/MediaStreamFrameController.cpp:207
> +    for (Vector<String>::const_iterator it = splitOptions.begin(); it != splitOptions.end() && (!audio || !video); ++it)

Won't the (!audio || !video) condition prevent this loop from setting both audio and video?

> Source/WebCore/page/MediaStreamFrameController.cpp:210
> +        else if (*it == "video")

Probably worth adding a FIXME about parsing out suboptions.

That will require splitting on spaces which effectively strips whitespace so that "audio, video" matches as well as "audio,video". So in the meantime, you should probably call stripWhiteSpace() on each *it.

> Source/WebCore/page/MediaStreamFrameController.cpp:214
> +        return false;

This should throw a NOT_SUPPORTED_ERROR as well.

> Source/WebCore/page/MediaStreamFrameController.h:141
> +    class IdGenerator {

For the new classes, can you use WTF_MAKE_NONCOPYABLE where possible?
------- Comment #3 From 2011-05-06 13:15:46 PST -------
Created an attachment (id=92632) [details]
Patch

Updating to the lastest navigator.getUserMedia spec. Please ignore the ChangeLog diff problem as it will be automatically solved when rebasing after landing 60387.
------- Comment #4 From 2011-05-06 13:17:17 PST -------
(From update of attachment 92267 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=92267&action=review

>> Source/WebCore/page/CallbackTask.h:25
>> +#ifndef CallbackTask_h
> 
> The name CallbackTask under page/ implies a general purpose utility. However, this is only enabled for media stream.
> 
> Do you have additional plans for this class? If not, perhaps it should just be implemented in NavigatorUserMediaErrorCallback.h?

It will be also implemented by the future BlobCallback when stream recording is implemented. In the same way, it will be used by the PeerConnection API (see next comment for details).

>> Source/WebCore/page/CallbackTask.h:40
>> +class CallbackTask1 : public ScriptExecutionContext::Task {
> 
> Why the "1" in the name? Are you planning other versions?

Yes, there should be a 2-argument version used by the PeerConnection API. Concretely, to implement the SignalingCallback: http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#signalingcallback

>> Source/WebCore/page/CallbackTask.h:52
>> +    class Scheduler {
> 
> Why a subclass instead of a static method on the parent? If it should be a class, it needs a virtual dtor, right?

It's a subclass because the callbacks inherit from the scheduler to simplify the usage and increase the reuse of the code. That way they also avoid passing the this pointer everytime.

It could be changed into a static method removing the inheritance as you propose, but then scheduling would change from something like this:
m_errorCallback->scheduleCallback(scriptExecutionContext(), error);

To something like this:
CallbackTask1<NavigatorUserMediaErrorCallback, NavigatorUserMediaError>::schedule(scriptExecutionContext(), m_errorCallback, error);

Which I think is a lot dirtier. In any case, the existing option would require a virtual dtor as you say. What do you think?

>> Source/WebCore/page/MediaStreamClient.h:42
>> +    virtual void generateStream(int requestId, bool audio, bool video, PassRefPtr<SecurityOrigin>) = 0;
> 
> Instead of two bools, do you think an enum or bitfield would help ensure the args are passed properly? Could also prevent argument proliferation if there ever happen to be more types of streams in the future (e.g. front facing vs rear facing video or speaker vs handset audio).

After noticing the lastest changes in the getUserMedia spec I'm following your proposal. Fixed.

>> Source/WebCore/page/MediaStreamFrameController.cpp:196
>> +bool MediaStreamFrameController::generateStream(const String& options,
> 
> Probably worth adding a comment mentioning that this method is meant to implement this:
> http://www.whatwg.org/specs/web-apps/current-work/#dom-navigator-getusermedia

Fixed.

>> Source/WebCore/page/MediaStreamFrameController.cpp:206
>> +    options.split(",", splitOptions);
> 
> Can you split on the char ',' instead of string?

Fixed.

>> Source/WebCore/page/MediaStreamFrameController.cpp:207
>> +    for (Vector<String>::const_iterator it = splitOptions.begin(); it != splitOptions.end() && (!audio || !video); ++it)
> 
> Won't the (!audio || !video) condition prevent this loop from setting both audio and video?

Maybe I'm wrong, but I think it won't. It should stop going through the loop after both have been set, not before. Anyway I'm removing it as I'm reimplementing this.

>> Source/WebCore/page/MediaStreamFrameController.cpp:210
>> +        else if (*it == "video")
> 
> Probably worth adding a FIXME about parsing out suboptions.
> 
> That will require splitting on spaces which effectively strips whitespace so that "audio, video" matches as well as "audio,video". So in the meantime, you should probably call stripWhiteSpace() on each *it.

Fixed.

>> Source/WebCore/page/MediaStreamFrameController.cpp:214
>> +        return false;
> 
> This should throw a NOT_SUPPORTED_ERROR as well.

Fixed, although it will require landing the bug 60387 first.

>> Source/WebCore/page/MediaStreamFrameController.h:141
>> +    class IdGenerator {
> 
> For the new classes, can you use WTF_MAKE_NONCOPYABLE where possible?

Fixed.
------- Comment #5 From 2011-05-09 02:43:01 PST -------
> >> Source/WebCore/page/MediaStreamFrameController.cpp:214
> >> +        return false;
> > 
> > This should throw a NOT_SUPPORTED_ERROR as well.
> 
> Fixed, although it will require landing the bug 60387 first.

You could just add a FIXME here if it is easier to land this first. I'll take a look at 60387.
------- Comment #6 From 2011-05-09 02:43:24 PST -------
(From update of attachment 92632 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=92632&action=review

Everything else looks good. Could you sync up your client so the EWS bots can run?

> Source/WebCore/page/MediaStreamFrameController.cpp:110
> +    while (!this->isEmpty())

Since there is a comment, this loop should have {} braces.

> Source/WebCore/page/MediaStreamFrameController.cpp:111
> +        // unregister should remove the element from the map.

Is there some way to ASSERT this? If it doesn't, this could just spin forever.

> Source/WebCore/page/MediaStreamFrameController.cpp:250
> +    ec = 0;

Should this be the first line in the method so that if (!successCallback) is reached, then ec is cleared?
------- Comment #7 From 2011-05-09 08:47:01 PST -------
Created an attachment (id=92795) [details]
Patch

Patch. Bots should be able to test this now.
------- Comment #8 From 2011-05-09 08:47:56 PST -------
(From update of attachment 92632 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=92632&action=review

>> Source/WebCore/page/MediaStreamFrameController.cpp:110
>> +    while (!this->isEmpty())
> 
> Since there is a comment, this loop should have {} braces.

Fixed.

>> Source/WebCore/page/MediaStreamFrameController.cpp:111
>> +        // unregister should remove the element from the map.
> 
> Is there some way to ASSERT this? If it doesn't, this could just spin forever.

Fixed.

>> Source/WebCore/page/MediaStreamFrameController.cpp:250
>> +    ec = 0;
> 
> Should this be the first line in the method so that if (!successCallback) is reached, then ec is cleared?

Fixed.
------- Comment #9 From 2011-05-16 08:58:14 PST -------
Created an attachment (id=93649) [details]
Patch

Rebasing to avoid problems with test_expectations.txt.
------- Comment #10 From 2011-05-16 10:16:42 PST -------
(From update of attachment 93649 [details])
Clearing flags on attachment: 93649

Committed r86583: <http://trac.webkit.org/changeset/86583>
------- Comment #11 From 2011-05-16 10:16:48 PST -------
All reviewed patches have been landed.  Closing bug.