[Webkit-unassigned] [Bug 131973] [MediaStream] Implement MediaStream active attribute

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=131973





--- Comment #4 from Praveen Jadhav <praveen.j at samsung.com>  2014-04-23 22:23:33 PST ---
(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.

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