[webkit-reviews] review denied: [Bug 60260] [Chromium] Implement the embedders for the HTML5 Track List objects : [Attachment 102912] 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 60260: [Chromium] Implement the embedders for the HTML5 Track List objects
https://bugs.webkit.org/show_bug.cgi?id=60260

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=102912&action=review


> Source/WebKit/chromium/public/WebMediaStreamTrack.h:32
> +#include "WebString.h"

WebString can just be forward declared.

> Source/WebKit/chromium/public/WebMediaStreamTrack.h:50
> +    WebMediaStreamTrack(WTF::PassRefPtr<WebCore::MediaStreamTrack>);

nit: parameter should be 'const Foo&'

> Source/WebKit/chromium/public/WebMediaStreamTrack.h:58
> +typedef WebVector<WebMediaStreamTrack> WebTrackVector;

nit: the name of this type should be WebMediaStreamTrackVector.  also, we
normally create a unique header file for each top-level type, so this should go
into a WebMediaStreamTrackVector.h all by its lonesome self.

but i have to wonder... what is the difference between a
WebMediaStreamTrackVector and a WebMediaStreamTrackList?  why do we need two
container types for WebMediaStreamTrack objects?

> Source/WebKit/chromium/public/WebMediaStreamTrackList.h:49
> +    WebMediaStreamTrackList(WTF::PassRefPtr<WebCore::MediaStreamTrackList>);


nit: parameter should be 'const Foo&'

> Source/WebKit/chromium/src/WebMediaStreamTrackList.cpp:42
> +    m_private = WebCore::MediaStreamTrackList::create(tracks);

nit: please add a 'using namespace WebCore' at the top of this file and remove
the WebCore:: prefixes.