[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 #2 from Tony Gentilcore <tonyg at chromium.org>  2011-05-05 03:35:49 PST ---
(From update of attachment 92267)
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?

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