Bug 123025 - MediaStreamTrack now tracks its own state
Summary: MediaStreamTrack now tracks its own state
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 124288
  Show dependency treegraph
 
Reported: 2013-10-18 09:27 PDT by Thiago de Barros Lacerda
Modified: 2013-11-13 10:09 PST (History)
6 users (show)

See Also:


Attachments
Patch (6.63 KB, patch)
2013-10-18 09:30 PDT, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff
Patch (5.52 KB, patch)
2013-10-21 13:33 PDT, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago de Barros Lacerda 2013-10-18 09:27:49 PDT
The spec says that a MediaStreamSource can be shared by different tracks, so a track must have its own state tracking.
In the old implementation if a user invoked the stop method, its readyState method was still returning the MediaStreamSource state, which was wrong.
This also adds a setEnabled method, which can be used to set the state of a track when a remote peer ends it, for instance.
Comment 1 Thiago de Barros Lacerda 2013-10-18 09:30:51 PDT
Created attachment 214581 [details]
Patch
Comment 2 Jer Noble 2013-10-21 12:44:00 PDT
Comment on attachment 214581 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:80
> +    m_state = m_source ? (TrackState) m_source->readyState() : MediaStreamTrack::New;

The space between the cast and variable is unnecessary, but I'm more worried about the cast itself.  There should only be one instance of this enum, either in MediaStreamTrack, or MediaStreamSource.

So, if you reverted the m_readyState -> m_state change, removed the MediaStreamTrack::TrackState enum, and kept the new setState() method (but renamed it setReadyState()), this patch would be a lot simpler.

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:307
> +    setState((TrackState) m_source->readyState());

Ditto.
Comment 3 Thiago de Barros Lacerda 2013-10-21 12:56:31 PDT
(In reply to comment #2)
> (From update of attachment 214581 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214581&action=review
> 
> > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:80
> > +    m_state = m_source ? (TrackState) m_source->readyState() : MediaStreamTrack::New;
> 
> The space between the cast and variable is unnecessary, but I'm more worried about the cast itself.  There should only be one instance of this enum, either in MediaStreamTrack, or MediaStreamSource.
> 
> So, if you reverted the m_readyState -> m_state change, removed the MediaStreamTrack::TrackState enum, and kept the new setState() method (but renamed it setReadyState()), this patch would be a lot simpler.
> 
> > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:307
> > +    setState((TrackState) m_source->readyState());
> 
> Ditto.

Sounds good.
Comment 4 Thiago de Barros Lacerda 2013-10-21 13:33:48 PDT
Created attachment 214772 [details]
Patch
Comment 5 Jer Noble 2013-10-21 13:37:04 PDT
Comment on attachment 214772 [details]
Patch

Looks good. r=me.
Comment 6 WebKit Commit Bot 2013-10-21 14:12:58 PDT
Comment on attachment 214772 [details]
Patch

Clearing flags on attachment: 214772

Committed r157733: <http://trac.webkit.org/changeset/157733>
Comment 7 WebKit Commit Bot 2013-10-21 14:13:01 PDT
All reviewed patches have been landed.  Closing bug.