Bug 60177 – Media Stream API: add local stream requests

• Component: DOM
• Status: RESOLVED FIXED

[Leandro Graciá Gil]

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.

[Leandro Graciá Gil]

Created attachment 92267 [details]
Patch

[Tony Gentilcore]

Comment on attachment 92267 [details]
Patch

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?

[Leandro Graciá Gil]

Created attachment 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.

[Leandro Graciá Gil]

Comment on attachment 92267 [details]
Patch

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.

[Tony Gentilcore]

> >> 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.

[Tony Gentilcore]

Comment on attachment 92632 [details]
Patch

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?

[Leandro Graciá Gil]

Created attachment 92795 [details]
Patch

Patch. Bots should be able to test this now.

[Leandro Graciá Gil]

Comment on attachment 92632 [details]
Patch

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.

[Leandro Graciá Gil]

Created attachment 93649 [details]
Patch

Rebasing to avoid problems with test_expectations.txt.

[WebKit Commit Bot]

Comment on attachment 93649 [details]
Patch

Clearing flags on attachment: 93649

Committed r86583: <http://trac.webkit.org/changeset/86583>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.