Summary: | [chromium] MediaStream API: Add WebMediaStreamRegistry | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tommy Widenflycht <tommyw> | ||||||||
Component: | DOM | Assignee: | 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
Tommy Widenflycht
2011-11-03 04:23:05 PDT
Created attachment 113467 [details]
Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API. 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. Created attachment 113640 [details]
Patch
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 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. Created attachment 114017 [details]
Patch
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 on attachment 114017 [details] Patch Clearing flags on attachment: 114017 Committed r99796: <http://trac.webkit.org/changeset/99796> All reviewed patches have been landed. Closing bug. |