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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 17 07:37:02 PDT 2011


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.


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

More information about the webkit-unassigned mailing list