Bug 71458 – [chromium] MediaStream API: Add WebMediaStreamRegistry

• Component: DOM
• Status: RESOLVED FIXED

[Tommy Widenflycht]

Add WebMediaStreamRegistry to the MediaStream api.

[Tommy Widenflycht]

Created attachment 113467 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 113467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113467&action=review

> Source/WebKit/chromium/public/WebMediaStreamRegistry.h:36
> +    // Returns an empty string if the url doesn't correspond to a Stream object.

empty or null?  seems like null (WebString::isNull) is what it actually returns.

> Source/WebKit/chromium/src/WebMediaStreamRegistry.cpp:44
> +    if (streamDescriptor)

it seems like you should return WebMediaStreamDescriptor here instead.  WebMediaStreamDescriptor has a label() method.
it creates an API that is a more lightweight wrapper around what WebCore actually does.

[Tommy Widenflycht]

Created attachment 113640 [details]
Patch

[Tommy Widenflycht]

Comment on attachment 113467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113467&action=review

>> Source/WebKit/chromium/public/WebMediaStreamRegistry.h:36
>> +    // Returns an empty string if the url doesn't correspond to a Stream object.
> 
> empty or null?  seems like null (WebString::isNull) is what it actually returns.

Implicitly fixed :)

>> Source/WebKit/chromium/src/WebMediaStreamRegistry.cpp:44
>> +    if (streamDescriptor)
> 
> it seems like you should return WebMediaStreamDescriptor here instead.  WebMediaStreamDescriptor has a label() method.
> it creates an API that is a more lightweight wrapper around what WebCore actually does.

Fixed, we don't need more than the label right now but this is more future proof with less code. Win/Win :)

[Darin Fisher (:fishd, Google)]

Comment on attachment 113640 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113640&action=review

> Source/WebKit/chromium/src/WebMediaStreamRegistry.cpp:44
> +    return WebMediaStreamDescriptor(MediaStreamRegistry::registry().lookupMediaStreamDescriptor(kurl.string()));

nit: it is perhaps a bit more conventional in webkit code to just use KURL(url).string() instead of creating the named temporary variable.

[Tommy Widenflycht]

Created attachment 114017 [details]
Patch

[Tommy Widenflycht]

Comment on attachment 113640 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113640&action=review

>> Source/WebKit/chromium/src/WebMediaStreamRegistry.cpp:44
>> +    return WebMediaStreamDescriptor(MediaStreamRegistry::registry().lookupMediaStreamDescriptor(kurl.string()));
> 
> nit: it is perhaps a bit more conventional in webkit code to just use KURL(url).string() instead of creating the named temporary variable.

Fixed.

[WebKit Review Bot]

Comment on attachment 114017 [details]
Patch

Clearing flags on attachment: 114017

Committed r99796: <http://trac.webkit.org/changeset/99796>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.