[Webkit-unassigned] [Bug 60177] Media Stream API: add local stream requests

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=60177





--- Comment #4 from Leandro Graciá Gil <leandrogracia at chromium.org>  2011-05-06 13:17:17 PST ---
(From update of attachment 92267)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.