Bug 71008

Summary: [chromium] Media Stream API: Adding supporting classes to WebPeerConnectionHandler
Product: WebKit Reporter: Tommy Widenflycht <tommyw@google.com>
Component: WebKit APIAssignee: Tommy Widenflycht <tommyw@google.com>
Status: RESOLVED FIXED    
Severity: Normal CC: adman.com@gmail.com, donggwan.kim@samsung.com, fishd@chromium.org, grunell@chromium.org, harald@alvestrand.no, webkit.review.bot@gmail.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 58550    
Bug Blocks: 56459    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description From 2011-10-27 05:26:37 PST
Adding WebMediaStreamSource and expanding WebMediaStreamSource
------- Comment #1 From 2011-11-01 02:41:17 PST -------
Created an attachment (id=113150) [details]
Patch
------- Comment #2 From 2011-11-01 02:45:06 PST -------
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
------- Comment #3 From 2011-11-01 10:50:27 PST -------
(From update of attachment 113150 [details])
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 From 2011-11-01 11:30:00 PST -------
(From update of attachment 113150 [details])
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 From 2011-11-01 12:03:52 PST -------
Created an attachment (id=113200) [details]
Patch
------- Comment #6 From 2011-11-01 12:09:07 PST -------
Created an attachment (id=113202) [details]
Patch
------- Comment #7 From 2011-11-01 15:32:48 PST -------
(From update of attachment 113202 [details])
Clearing flags on attachment: 113202

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