Bug 71008 - [chromium] Media Stream API: Adding supporting classes to WebPeerConnectionHandler
Summary: [chromium] Media Stream API: Adding supporting classes to WebPeerConnectionHa...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tommy Widenflycht
URL:
Keywords:
Depends on: 58550
Blocks: 56459
  Show dependency treegraph
 
Reported: 2011-10-27 05:26 PDT by Tommy Widenflycht
Modified: 2011-11-01 15:32 PDT (History)
6 users (show)

See Also:


Attachments
Patch (11.94 KB, patch)
2011-11-01 02:41 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (12.50 KB, patch)
2011-11-01 12:03 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (13.01 KB, patch)
2011-11-01 12:09 PDT, 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-10-27 05:26:37 PDT
Adding WebMediaStreamSource and expanding WebMediaStreamSource
Comment 1 Tommy Widenflycht 2011-11-01 02:41:17 PDT
Created attachment 113150 [details]
Patch
Comment 2 WebKit Review Bot 2011-11-01 02:45:06 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-01 10:50:27 PDT
Comment on attachment 113150 [details]
Patch

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

> Source/WebKit/chromium/public/WebMediaStreamSource.h:42
> +    enum WebType {

nit: we drop the "Web" prefix for enums defined within the scope of a class.  so, just Type and Type{Audio,Video} would do.

> Source/WebKit/chromium/src/WebMediaStreamDescriptor.cpp:67
> +    WebCore::MediaStreamSourceVector s;

nit: perhaps a 'using namespace WebCore' would be nice to have at the top of this .cpp file?

> Source/WebKit/chromium/src/WebMediaStreamDescriptor.cpp:69
> +        WTF::PassRefPtr<WebCore::MediaStreamSource> curr = sources[i];

nit: no need for the WTF:: prefix

> Source/WebKit/chromium/src/WebMediaStreamSource.cpp:37
> +WebMediaStreamSource::WebMediaStreamSource(const WTF::PassRefPtr<WebCore::MediaStreamSource>& mediaStreamSource)

ditto.  no need for WTF:: prefix in .cpp files

> Source/WebKit/chromium/src/WebMediaStreamSource.cpp:47
> +WebMediaStreamSource::operator WTF::PassRefPtr<WebCore::MediaStreamSource>() const

nit: usually we put a 'using namespace WebCore' at the top of the file so we can avoid
writing WebCore:: everywhere.

> Source/WebKit/chromium/src/WebMediaStreamSource.cpp:54
> +    m_private = WebCore::MediaStreamSource::create(id, (WebCore::MediaStreamSource::Type)type, name);

nit: use static_cast instead of C-style cast

> Source/WebKit/chromium/src/WebMediaStreamSource.cpp:60
> +    return WebString(m_private.get()->id());

nit: explicit construction of WebString should be unnecessary.  it should just happen implicitly.

> Source/WebKit/chromium/src/WebMediaStreamSource.cpp:66
> +    return (WebType)(m_private.get()->type());

nit: use static_cast

> Source/WebKit/chromium/src/WebMediaStreamSource.cpp:72
> +    return WebString(m_private.get()->name());

nit: rely on implicit construction of a WebString.
Comment 4 Tommy Widenflycht 2011-11-01 11:30:00 PDT
Comment on attachment 113150 [details]
Patch

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

>> Source/WebKit/chromium/public/WebMediaStreamSource.h:42
>> +    enum WebType {
> 
> nit: we drop the "Web" prefix for enums defined within the scope of a class.  so, just Type and Type{Audio,Video} would do.

Fixed.

>> Source/WebKit/chromium/src/WebMediaStreamDescriptor.cpp:67
>> +    WebCore::MediaStreamSourceVector s;
> 
> nit: perhaps a 'using namespace WebCore' would be nice to have at the top of this .cpp file?

Fixed.

>> Source/WebKit/chromium/src/WebMediaStreamDescriptor.cpp:69

> 
> nit: no need for the WTF:: prefix

Fixed.

>> Source/WebKit/chromium/src/WebMediaStreamSource.cpp:37
>> +WebMediaStreamSource::WebMediaStreamSource(const WTF::PassRefPtr<WebCore::MediaStreamSource>& mediaStreamSource)
> 
> ditto.  no need for WTF:: prefix in .cpp files

Fixed.

>> Source/WebKit/chromium/src/WebMediaStreamSource.cpp:47
>> +WebMediaStreamSource::operator WTF::PassRefPtr<WebCore::MediaStreamSource>() const
> 
> nit: usually we put a 'using namespace WebCore' at the top of the file so we can avoid
> writing WebCore:: everywhere.

Fixed.

>> Source/WebKit/chromium/src/WebMediaStreamSource.cpp:54
>> +    m_private = WebCore::MediaStreamSource::create(id, (WebCore::MediaStreamSource::Type)type, name);
> 
> nit: use static_cast instead of C-style cast

Fixed.

>> Source/WebKit/chromium/src/WebMediaStreamSource.cpp:60
>> +    return WebString(m_private.get()->id());
> 
> nit: explicit construction of WebString should be unnecessary.  it should just happen implicitly.

Fixed.

>> Source/WebKit/chromium/src/WebMediaStreamSource.cpp:66
>> +    return (WebType)(m_private.get()->type());
> 
> nit: use static_cast

Fixed.

>> Source/WebKit/chromium/src/WebMediaStreamSource.cpp:72
>> +    return WebString(m_private.get()->name());
> 
> nit: rely on implicit construction of a WebString.

Fixed.
Comment 5 Tommy Widenflycht 2011-11-01 12:03:52 PDT
Created attachment 113200 [details]
Patch
Comment 6 Tommy Widenflycht 2011-11-01 12:09:07 PDT
Created attachment 113202 [details]
Patch
Comment 7 WebKit Review Bot 2011-11-01 15:32:48 PDT
Comment on attachment 113202 [details]
Patch

Clearing flags on attachment: 113202

Committed r99004: <http://trac.webkit.org/changeset/99004>
Comment 8 WebKit Review Bot 2011-11-01 15:32:53 PDT
All reviewed patches have been landed.  Closing bug.