[Webkit-unassigned] [Bug 68464] Update MediaStream to use WebCore platform interfaces

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=68464


Adam Bergkvist <adam.bergkvist at ericsson.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #110871|0                           |1
        is obsolete|                            |
 Attachment #111261|                            |review?
               Flag|                            |




--- Comment #7 from Adam Bergkvist <adam.bergkvist at ericsson.com>  2011-10-17 07:37:01 PST ---
Created an attachment (id=111261)
 --> (https://bugs.webkit.org/attachment.cgi?id=111261&action=review)
Patch 3

(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.