Created attachment 113150 [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 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 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.

Created attachment 113200 [details] Patch

Created attachment 113202 [details] Patch

Comment on attachment 113202 [details] Patch Clearing flags on attachment: 113202 Committed r99004: <http://trac.webkit.org/changeset/99004>

All reviewed patches have been landed. Closing bug.