[webkit-reviews] review requested: [Bug 68464] Update MediaStream to use WebCore platform interfaces : [Attachment 111261] Patch 3

[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 111261: Patch 3
https://bugs.webkit.org/attachment.cgi?id=111261&action=review

------- Additional Comments from Adam Bergkvist <adam.bergkvist at ericsson.com>
(In reply to comment #6)
> (From update of attachment 110871 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=110871&action=review
> 
> In one of your earlier patches, you moved all the non-platform files into a
mediastream directory.	I think that would be a valuable change to do sooner
rather than later.
>

I've created a bug to move the files. See http://webkit.org/b/70233

> Please fix the out-of-bounds read before landing.

item() is an index getter and thus the binding will prevent out-of-bounds
reads. 

> > Source/WebCore/ChangeLog:8
> > +	     Update MediaStream to use WebCore platform interfaces
> > +	     https://bugs.webkit.org/show_bug.cgi?id=68464
> > +
> > +	     Reviewed by NOBODY (OOPS!).
> > +
> > +	     Currently not testable.
> 
> Generally we like a litt more information in ChangeLogs.  For this change,
I'd explain that this is part of a series of patches that convert the DOM
interfaces over to using a new Platform API.
>

Fixed.

> Also, I'd include some information about how you plan to test this code and
at what stage of this process we'll be able to add tests.  I understand that
there are a lots of wires to connect before everything is fully functional, but
we might be able to add some basic API tests soon.
>

Fixed (now referring to an existing bug).

> > Source/WebCore/dom/MediaStream.h:84
> > +	 ScriptExecutionContext* m_scriptExecutionContext;
> 
> This doesn't need to be a RefPtr?

I made it a RefPtr to be safe.

> > Source/WebCore/dom/MediaStreamTrack.cpp:51
> > -const String& MediaStreamTrack::kind() const
> > +String MediaStreamTrack::kind() const
> >  {
> > -	 return m_kind;
> > +	 DEFINE_STATIC_LOCAL(AtomicString, audioKind, ("audio"));
> > +	 DEFINE_STATIC_LOCAL(AtomicString, videoKind, ("video"));
> 
> Should this function return an AtomicString?	If not, then should these
statics be Strings rather than AtomicStrings?  (Let me know if you're unsure of
the difference between Strings and AtomicStrings.)
>

Fixed (they don't have to be AtomicStrings).

> 
> > Source/WebCore/dom/MediaStreamTrack.cpp:70
> > +	 return m_streamDescriptor->component(m_trackIndex)->enabled();
> 
> It's kind of strange that all of these methods are basically trampolines into
m_streamDescriptor components.	I guess this object is like a DOM wrapper
around the Platform concept, which makes sense, but they we're holding on to
the root objects in the Platform object tree and walking the tree each time.  I
guess that's fine.  It's just slightly different from what I woud have
expected.

We need to hold on to the MediaStreamDescriptor object since a MediaStreamTrack
can exist without its MediaStream and the descriptor is the key to do platform
operations on a stream (including the tracks). Since the descriptor is
available we can get the information we want from it, but we could cache the
information in member variables.

> > Source/WebCore/dom/MediaStreamTrackList.cpp:54
> > +	 ASSERT(index < length());
> 
> This is an API exposed to JavaScript, right?	What ensures that JavaScript
won't call this function with a nutty index?
>

The binding makes sure that this function is called with a valid index (0 <=
index < length()).

> > Source/WebCore/page/MediaStreamController.h:44
> >  class MediaStreamController {
> 
> All of MediaStreamController is going away eventually, right?
>
> > Source/WebCore/page/MediaStreamFrameController.h:52
> >	 MediaStreamFrameController(Frame*);
> 
> This one is going away too, I presume.

We have a single controller and a page client that only deals with user consent
for getUserMedia(). I hope that they could be accepted as replacements for the
current controllers and client. I'll file a bug to discuss this further.