Bug 131973 – [MediaStream] Implement MediaStream active attribute

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Praveen Jadhav]

MediaStream.active, MediaStream.onactive and MediaStream.oninactive are not implemented.

[Praveen Jadhav]

Created attachment 229949 [details]
Patch

MediaStream IDL specifications are revised to use 'active' attribute instead of 'ended'. This patch implements the newly introduced attribute as well as the event listeners 'onactive' and 'oninactive' associated with it. Code related to 'ended' attribute shall be deprecated. Work on the same is in progress.

[Praveen Jadhav]

Created attachment 229952 [details]
Patch

Updated patch attached.

[Eric Carlson]

Comment on attachment 229952 [details]
Patch

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.

[Praveen Jadhav]

(In reply to comment #3)
> (From update of attachment 229952 [details])
> 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.

Right. Brief comments added here. 

> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:231
> >          setEnded();
> 
> Nit: A "FIXME: to be removed in bug http://xxxx" comment would be good.

New bug https://bugs.webkit.org/show_bug.cgi?id=132104 is raised. Updated in patch as well.

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

Updated.

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

True. Corrections done.

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

Updated.

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

Logic updated.

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

[Praveen Jadhav]

Created attachment 230050 [details]
Patch

All comments included.

[WebKit Commit Bot]

Comment on attachment 230050 [details]
Patch

Clearing flags on attachment: 230050

Committed r167750: <http://trac.webkit.org/changeset/167750>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Carlson]

Comment on attachment 230050 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230050&action=review

> Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp:188
> +    if (m_isActive != active) {

Nit: WebKit style is to use an early return in a situation like this, eg. "if (m_isActive == active) return ...". Please try to remember to change this when you fix bug #132104.