Summary: | [MediaStream] Implement MediaStream active attribute | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Praveen Jadhav <praveen.j> | ||||||||
Component: | WebKit Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, dev_sachin, eric.carlson, esprehn+autocc, glenn, hta, jer.noble, kangil.han, kondapallykalyan, philipj, sergio, tommyw | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Praveen Jadhav
2014-04-21 23:17:40 PDT
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.
Created attachment 229952 [details]
Patch
Updated patch attached.
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. (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. Created attachment 230050 [details]
Patch
All comments included.
Comment on attachment 230050 [details] Patch Clearing flags on attachment: 230050 Committed r167750: <http://trac.webkit.org/changeset/167750> All reviewed patches have been landed. Closing bug. 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. |