RESOLVED FIXED 71458
[chromium] MediaStream API: Add WebMediaStreamRegistry 
https://bugs.webkit.org/show_bug.cgi?id=71458
Summary [chromium] MediaStream API: Add WebMediaStreamRegistry
Tommy Widenflycht
Reported 2011-11-03 04:23:05 PDT
Add WebMediaStreamRegistry to the MediaStream api.
Attachments
Patch (6.14 KB, patch)
2011-11-03 05:29 PDT, Tommy Widenflycht 		no flags
DetailsFormatted DiffDiff
Patch (6.09 KB, patch)
2011-11-04 02:59 PDT, Tommy Widenflycht 		no flags
DetailsFormatted DiffDiff
Patch (6.03 KB, patch)
2011-11-08 02:22 PST, Tommy Widenflycht 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Tommy Widenflycht
Comment 1 2011-11-03 05:29:28 PDT
Created attachment 113467 [details] Patch
WebKit Review Bot
Comment 2 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.
Darin Fisher (:fishd, Google)
Comment 3 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.
Tommy Widenflycht
Comment 4 2011-11-04 02:59:58 PDT
Created attachment 113640 [details] Patch
Tommy Widenflycht
Comment 5 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 :)
Darin Fisher (:fishd, Google)
Comment 6 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.
Tommy Widenflycht
Comment 7 2011-11-08 02:22:43 PST
Created attachment 114017 [details] Patch
Tommy Widenflycht
Comment 8 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.
WebKit Review Bot
Comment 9 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>
WebKit Review Bot
Comment 10 2011-11-09 18:25:31 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.