[webkit-reviews] review requested: [Bug 68464] Update MediaStream to use WebCore platform interfaces : [Attachment 110871] Updated patch

[bugzilla-daemon at webkit.org]

Adam Bergkvist <adam.bergkvist at ericsson.com> has asked	for review:
Bug 68464: Update MediaStream to use WebCore platform interfaces
https://bugs.webkit.org/show_bug.cgi?id=68464

Attachment 110871: Updated patch
https://bugs.webkit.org/attachment.cgi?id=110871&action=review

------- Additional Comments from Adam Bergkvist <adam.bergkvist at ericsson.com>
(In reply to comment #2)
> (From update of attachment 110604 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=110604&action=review
> 
> This looks pretty good.  There's still the question of whether |context|
should be first in constructors.

I've updated the patch to follow the new context first convention.

> Also, almost all the places in this patch where we're using unsigned, we
probably should be using size_t.

Fixed "internal" usage (left unsigned in the DOM API signatures).

> > Source/WebCore/dom/MediaStream.cpp:48
> > +	 MediaStreamTrackVector trackVector;
> > +	 unsigned numberOfTracks = m_descriptor->numberOfComponents();
> 
> You can call reserveCapacity on the vector if you know ahead of time how many
items you're going to append.

Good point. Fixed.

> > Source/WebCore/dom/MediaStreamTrack.cpp:50
> > +	 DEFINE_STATIC_LOCAL(AtomicString, kind,
(m_streamDescriptor->component(m_trackIndex)->source()->type() ==
MediaStreamSource::TypeAudio ? "audio" : "video"));
> 
> It's strange to use a static local that's based on member variables...  This
doesn't seem right.

True. That was a bad last minute optimization. :) Fixed.