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

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Adam Bergkvist
<adam.bergkvist at ericsson.com>'s request 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 Barth <abarth at webkit.org>
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.