[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 Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #110871|review?                     |review+, commit-queue-
               Flag|                            |




--- Comment #6 from Adam Barth <abarth at webkit.org>  2011-10-13 12:06:43 PST ---
(From update of attachment 110871)
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.

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

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

> Source/WebCore/dom/MediaStream.h:84
> +    ScriptExecutionContext* m_scriptExecutionContext;

This doesn't need to be a RefPtr?

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

> Source/WebCore/dom/MediaStreamTrack.cpp:61
> +    ASSERT_NOT_REACHED();
>  }

I'm surprised you don't need to return String() here.

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

> Source/WebCore/dom/MediaStreamTrack.h:54
> +    const size_t m_trackIndex;

We don't usually have const member variables, but it works nicely here.

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

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

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