Bug 71458

Summary: [chromium] MediaStream API: Add WebMediaStreamRegistry
Product: WebKit Reporter: Tommy Widenflycht <tommyw>
Component: DOMAssignee: Tommy Widenflycht <tommyw>
Status: RESOLVED FIXED    
Severity: Normal CC: donggwan.kim, fishd, grunell, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 56459    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Tommy Widenflycht 2011-11-03 04:23:05 PDT
Add WebMediaStreamRegistry to the MediaStream api.
Comment 1 Tommy Widenflycht 2011-11-03 05:29:28 PDT
Created attachment 113467 [details]
Patch
Comment 2 WebKit Review Bot 2011-11-03 05:32:41 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Darin Fisher (:fishd, Google) 2011-11-03 09:13:40 PDT
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.
Comment 4 Tommy Widenflycht 2011-11-04 02:59:58 PDT
Created attachment 113640 [details]
Patch
Comment 5 Tommy Widenflycht 2011-11-04 03:02:28 PDT
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 :)
Comment 6 Darin Fisher (:fishd, Google) 2011-11-07 09:32:08 PST
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.
Comment 7 Tommy Widenflycht 2011-11-08 02:22:43 PST
Created attachment 114017 [details]
Patch
Comment 8 Tommy Widenflycht 2011-11-08 02:23:12 PST
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.
Comment 9 WebKit Review Bot 2011-11-09 18:25:25 PST
Comment on attachment 114017 [details]
Patch

Clearing flags on attachment: 114017

Committed r99796: <http://trac.webkit.org/changeset/99796>
Comment 10 WebKit Review Bot 2011-11-09 18:25:31 PST
All reviewed patches have been landed.  Closing bug.