[webkit-reviews] review granted: [Bug 131973] [MediaStream] Implement MediaStream active attribute : [Attachment 229952] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Praveen Jadhav
<praveen.j at samsung.com>'s request for review:
Bug 131973: [MediaStream] Implement MediaStream active attribute
https://bugs.webkit.org/show_bug.cgi?id=131973

Attachment 229952: Patch
https://bugs.webkit.org/attachment.cgi?id=229952&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=229952&action=review


r=me with some minor caveats.

> Source/WebCore/ChangeLog:29
> +	   * Modules/mediastream/MediaStream.cpp:
> +	   (WebCore::MediaStream::active):
> +	   (WebCore::MediaStream::setActive):
> +	   (WebCore::MediaStream::addTrack):
> +	   (WebCore::MediaStream::removeTrack):
> +	   (WebCore::MediaStream::trackDidEnd):
> +	   (WebCore::MediaStream::streamDidEnd):
> +	   (WebCore::MediaStream::streamIsActive):
> +	   * Modules/mediastream/MediaStream.h:
> +	   * Modules/mediastream/MediaStream.idl:
> +	   * dom/EventNames.h:
> +	   * platform/mediastream/MediaStreamPrivate.cpp:
> +	   (WebCore::MediaStreamPrivate::MediaStreamPrivate):
> +	   (WebCore::MediaStreamPrivate::setEnded):
> +	   (WebCore::MediaStreamPrivate::setActive):
> +	   * platform/mediastream/MediaStreamPrivate.h:
> +	   (WebCore::MediaStreamPrivate::active):

I think it is helpful to have per-method comments in the ChangeLog to make it
easier to see at a glance what changed.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:231
>	   setEnded();

Nit: A "FIXME: to be removed in bug http://xxxx" comment would be good.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:282
>      setEnded();

Ditto.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:296
> +void MediaStream::streamIsActive(bool streamActive)

This name suggests the method checks to see if the stream is active. Please use
a name that makes it clear that it changes the stream active state, eg.
something like "setStreamIsActive".

> Source/WebCore/Modules/mediastream/MediaStream.cpp:299
> +    if (active() == streamActive)
> +	   return;

active() returns m_private->active(), so this will only work if the private
stream sets its state after making the client callback, which I think is a bad
idea (see below). It seems like you need to keep track the state the last time
an event was fired.

> Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp:131
>      , m_ended(false)

Nit: not that this is deprecated.

> Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp:146
>      if (providedSourcesSize > 0 && !tracksSize)
>	   m_ended = true;

Ditto.

> Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp:155
>      , m_ended(false)

Ditto.

> Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp:170
>      if (providedTracksSize > 0 && !tracksSize)
>	   m_ended = true;

Ditto.

> Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp:190
> +void MediaStreamPrivate::setActive(bool active)
> +{
> +    if (m_client)
> +	   m_client->streamIsActive(active);
> +
> +    m_isActive = active;
> +}

Setting m_isActive after notifying the client is a bad idea because you can't
know what the client will do in the callback. It also seems unnecessary to call
the client if (m_isActive == active).

> Source/WebCore/platform/mediastream/MediaStreamPrivate.h:91
>      bool ended() const { return m_ended; }
>      void setEnded();

Nit: comment that these are deprecated

> Source/WebCore/platform/mediastream/MediaStreamPrivate.h:113
>      bool m_ended;

Ditto.