Bug 71008

Summary:

[chromium] Media Stream API: Adding supporting classes to WebPeerConnectionHandler

Product:

WebKit

Reporter:

Tommy Widenflycht <tommyw>

Component:

WebKit API

Assignee:

Tommy Widenflycht <tommyw>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

adman.com, donggwan.kim, fishd, grunell, harald, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

58550

Bug Blocks:

56459

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Tommy Widenflycht

Reported 2011-10-27 05:26:37 PDT

Adding WebMediaStreamSource and expanding WebMediaStreamSource

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Tommy Widenflycht

Comment 1 2011-11-01 02:41:17 PDT

Created attachment 113150 [details] Patch

WebKit Review Bot

Comment 2 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.

Darin Fisher (:fishd, Google)

Comment 3 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.

Tommy Widenflycht

Comment 4 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.

Tommy Widenflycht

Comment 5 2011-11-01 12:03:52 PDT

Created attachment 113200 [details] Patch

Tommy Widenflycht

Comment 6 2011-11-01 12:09:07 PDT

Created attachment 113202 [details] Patch

WebKit Review Bot

Comment 7 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>

WebKit Review Bot

Comment 8 2011-11-01 15:32:53 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.