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

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


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

nit: this include is not necessary.

> Source/WebKit/chromium/public/WebMediaStreamTrack.h:47
> +    WEBKIT_EXPORT void set(const WebString& id, const WebString& kind, const
WebString& label);

since this performs and allocation, i'd probably name the method
'initialize()'.  that'd be more consistent with other WebKit APIs too.

perhaps you should have an isNull() function too?  see for example
WebHistoryItem, which similarly wraps a reference counted WebCore class.

> Source/WebKit/chromium/public/WebMediaStreamTrackList.h:31
> +#include "WebMediaStreamTrack.h"

WebMediaStreamTrack can just be forward declared, right?

> Source/WebKit/chromium/public/WebMediaStreamTrackList.h:33
> +

this one needs the WebVector include, or at least a forward declaration. 
forward declaration is more common in WebKit APIs.

template <typename T> class WebVector;

> Source/WebKit/chromium/public/WebMediaStreamTrackList.h:45
> +    WEBKIT_EXPORT void set(const WebVector<WebMediaStreamTrack>& webTracks);


nit: same suggestion.  this method should probably be named initialize() and
there should be an isNull() method.