Bug 71458 - [chromium] MediaStream API: Add WebMediaStreamRegistry
Summary: [chromium] MediaStream API: Add WebMediaStreamRegistry
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tommy Widenflycht
URL:
Keywords:
Depends on:
Blocks: 56459
  Show dependency treegraph
 
Reported: 2011-11-03 04:23 PDT by Tommy Widenflycht
Modified: 2011-11-09 18:25 PST (History)
4 users (show)

See Also:


Attachments
Patch (6.14 KB, patch)
2011-11-03 05:29 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (6.09 KB, patch)
2011-11-04 02:59 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (6.03 KB, patch)
2011-11-08 02:22 PST, Tommy Widenflycht
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.