[webkit-reviews] review denied: [Bug 71008] [chromium] Media Stream API: Adding supporting classes to WebPeerConnectionHandler : [Attachment 113150] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Tommy Widenflycht
<tommyw at google.com>'s request for review:
Bug 71008: [chromium] Media Stream API: Adding supporting classes to
WebPeerConnectionHandler
https://bugs.webkit.org/show_bug.cgi?id=71008

Attachment 113150: Patch
https://bugs.webkit.org/attachment.cgi?id=113150&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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.