Bug 123316

Created attachment 215278 [details] Patch for the hungry, hungry bots.

Attachment 215278 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/mediastream/MediaStream-add-remove-tracks-expected.txt', u'LayoutTests/fast/mediastream/MediaStream-add-remove-tracks.html', u'LayoutTests/fast/mediastream/MediaStreamConstructor-expected.txt', u'LayoutTests/fast/mediastream/MediaStreamConstructor.html', u'LayoutTests/fast/mediastream/MediaStreamTrack-expected.txt', u'LayoutTests/fast/mediastream/MediaStreamTrack-getSources.html', u'LayoutTests/fast/mediastream/MediaStreamTrack.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/mediastream/MediaSourceStates.cpp', u'Source/WebCore/Modules/mediastream/MediaSourceStates.h', u'Source/WebCore/Modules/mediastream/MediaSourceStates.idl', u'Source/WebCore/Modules/mediastream/MediaStream.cpp', u'Source/WebCore/Modules/mediastream/MediaStreamCapabilities.cpp', u'Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp', u'Source/WebCore/Modules/mediastream/MediaStreamTrack.h', u'Source/WebCore/Modules/mediastream/UserMediaRequest.cpp', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/JSMediaSourceStatesCustom.cpp', u'Source/WebCore/platform/mediastream/MediaStreamCenter.h', u'Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp', u'Source/WebCore/platform/mediastream/MediaStreamSource.cpp', u'Source/WebCore/platform/mediastream/MediaStreamSource.h', u'Source/WebCore/platform/mediastream/MediaStreamSourceCapabilities.h', u'Source/WebCore/platform/mediastream/MediaStreamSourceStates.cpp', u'Source/WebCore/platform/mediastream/MediaStreamSourceStates.h', u'Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp', u'Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h', u'Source/WebCore/platform/mediastream/mac/AVAudioCaptureSource.h', u'Source/WebCore/platform/mediastream/mac/AVAudioCaptureSource.mm', u'Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.h', u'Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm', u'Source/WebCore/platform/mediastream/mac/AVMediaCaptureSource.h', u'Source/WebCore/platform/mediastream/mac/AVMediaCaptureSource.mm', u'Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.h', u'Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm', u'Source/WebCore/platform/mediastream/mac/MediaStreamCenterMac.cpp', u'Source/WebCore/platform/mock/MockMediaStreamCenter.cpp']" exit_code: 1 Source/WebCore/platform/mediastream/mac/AVMediaCaptureSource.mm:150: Line contains tab character. [whitespace/tab] [5] Source/WebCore/platform/mediastream/mac/AVMediaCaptureSource.mm:151: Line contains tab character. [whitespace/tab] [5] Source/WebCore/platform/mediastream/mac/AVMediaCaptureSource.mm:152: Line contains tab character. [whitespace/tab] [5] Source/WebCore/platform/mediastream/MediaStreamSourceStates.cpp:35: You should add a blank line after implementation file's own header. [build/include_order] [4] Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.h:55: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.h:60: The parameter name "type" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.h:60: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.h:61: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:130: Line contains tab character. [whitespace/tab] [5] Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:145: Line contains tab character. [whitespace/tab] [5] Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:215: Line contains tab character. [whitespace/tab] [5] Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:224: Line contains tab character. [whitespace/tab] [5] Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:225: Line contains tab character. [whitespace/tab] [5] Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:234: Line contains tab character. [whitespace/tab] [5] Total errors found: 14 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 215278 [details] Patch for the hungry, hungry bots. Attachment 215278 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/15218005

Comment on attachment 215278 [details] Patch for the hungry, hungry bots. Attachment 215278 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/15218007

Created attachment 215280 [details] Updated patch

Comment on attachment 215280 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=215280&action=review > Source/WebCore/platform/mediastream/MediaStreamSource.cpp:91 > + if (m_readyState == Ended) > + // There are no more consumers of this source's data, shut it down as appropriate. > + // http://www.w3.org/TR/mediacapture-streams/#widl-MediaStreamTrack-stop-void > + // If the data is being generated from a live source (e.g., a microphone or camera), then the user > + // agent should remove any active "on-air" indicator for that source. If the data is being > + // generated from a prerecorded source (e.g. a video file), any remaining content in the file is ignored. > + stopProducingData(); It goes agains the idea of the platform layer to embed any HTML level concepts (like this comment) here. > Source/WebCore/platform/mediastream/MediaStreamSource.cpp:127 > + for (Vector<Observer*>::iterator i = m_observers.begin(); i != m_observers.end(); ++i) { This should use auto. > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:32 > +#include "Dictionary.h" #including this is a layering violation. > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:33 > +#include "MediaSourceStates.h" #including this is a layering violation.

Created attachment 215320 [details] Updated patch

(In reply to comment #6) > (From update of attachment 215280 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=215280&action=review > > > Source/WebCore/platform/mediastream/MediaStreamSource.cpp:91 > > + if (m_readyState == Ended) > > + // There are no more consumers of this source's data, shut it down as appropriate. > > + // http://www.w3.org/TR/mediacapture-streams/#widl-MediaStreamTrack-stop-void > > + // If the data is being generated from a live source (e.g., a microphone or camera), then the user > > + // agent should remove any active "on-air" indicator for that source. If the data is being > > + // generated from a prerecorded source (e.g. a video file), any remaining content in the file is ignored. > > + stopProducingData(); > > It goes agains the idea of the platform layer to embed any HTML level concepts (like this comment) here. > Removed. > > Source/WebCore/platform/mediastream/MediaStreamSource.cpp:127 > > + for (Vector<Observer*>::iterator i = m_observers.begin(); i != m_observers.end(); ++i) { > > This should use auto. > Done. > > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:32 > > +#include "Dictionary.h" > > #including this is a layering violation. > And completely unnecessary, removed. > > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:33 > > +#include "MediaSourceStates.h" > > #including this is a layering violation. Removed.

Could not post comments in the code due to permission, so I'm pasting the output here: I think that would be better to split this patch i 2: one for WebCore stuff and other to deal with AVFoundation implementation > Source/WebCore/Modules/mediastream/MediaSourceStates.cpp:48 > + return MediaStreamSourceStates::sourceType(m_sourceStates.sourceType()); Why don't just return m_sourceStates.sourceType()? Did understand why put it static. > Source/WebCore/Modules/mediastream/MediaSourceStates.cpp:53 > + return MediaStreamSourceStates::facingMode(m_sourceStates.facingMode()); Ditto. > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:192 > void MediaStreamTrack::stopProducingData() I haven't seen anybody calling this method. Is that still being used? > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:199 > + m_stoppingTrack = true; Is this really needed? > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:200 > + m_privateTrack->stop(MediaStreamTrackPrivate::StopTrackAndStopSource); I think we can't stop the source here because it can be shared with another track that has not been stopped yet. > Source/WebCore/platform/mediastream/MediaStreamSource.cpp:153 > + // This is called from the track.stop() method, which should "Permanently stop the generation of data This source can be shared, so a track calling stop does not mean that the source should stop as well. I think that only the source can tell if it has to stop or not (by checking if there are any tracks using it, or the backend has told that it has stopped) > Source/WebCore/platform/mediastream/MediaStreamSource.h:74 > + enum Type { None, Audio, Video }; Does make sense to create a source of no type? > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:49 > + setSource(other->source()); Calling this here can result in muted and readystate events to be fired on the new track. Is that what we want? > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:175 > + if (stopSource == StopTrackAndStopSource && m_source) We can't stop the source, since it can be shared > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:234 > +RefPtr<MediaStreamCapabilities> MediaStreamTrackPrivate::capabilities() const Layering violation?

(In reply to comment #9) > > > Source/WebCore/Modules/mediastream/MediaSourceStates.cpp:48 > > + return MediaStreamSourceStates::sourceType(m_sourceStates.sourceType()); > > Why don't just return m_sourceStates.sourceType()? Did understand why put it static. > m_sourceStates.sourceType() returns an Enum, which is a string in JS so MediaSourceStates::sourceType() needs to return the string that represents the enum. > > Source/WebCore/Modules/mediastream/MediaSourceStates.cpp:53 > > + return MediaStreamSourceStates::facingMode(m_sourceStates.facingMode()); > > Ditto. > Ditto. > > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:192 > > void MediaStreamTrack::stopProducingData() > > I haven't seen anybody calling this method. Is that still being used? > Yes, this is called when track.stop() is called from JavaScript (because we use "[ImplementedAs= stopProducingData]" in the IDL file). > > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:199 > > + m_stoppingTrack = true; > > Is this really needed? > Yes. The spec says that the 'ended' event should not be fired when track.stop() is called. > > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:200 > > + m_privateTrack->stop(MediaStreamTrackPrivate::StopTrackAndStopSource); > > I think we can't stop the source here because it can be shared with another track that has not been stopped yet. > We have to, the spec says the stop() method must Permanently stop the generation of data for track's source. If the data is being generated from a live source (e.g., a microphone or camera), then the user agent should remove any active "on-air" indicator for that source. If the data is being generated from a prerecorded source (e.g. a video file), any remaining content in the file is ignored. (http://www.w3.org/TR/mediacapture-streams/#dom-mediastreamtrack-stop) > > Source/WebCore/platform/mediastream/MediaStreamSource.cpp:153 > > + // This is called from the track.stop() method, which should "Permanently stop the generation of data > > This source can be shared, so a track calling stop does not mean that the source should stop as well. I think that only the source can tell if it has to stop or not (by checking if there are any tracks using it, or the backend has told that it has stopped) > That isn't my interpretation of the spec, but I have sent email to Adam Bergkvist asking for clarification. In the meantime, I have modified MediaStreamTrack.html to verify that WebKit behaves I think it should. > > Source/WebCore/platform/mediastream/MediaStreamSource.h:74 > > + enum Type { None, Audio, Video }; > > Does make sense to create a source of no type? > MediaStreamTrackPrivate::type has to return something when m_source is NULL. > > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:49 > > + setSource(other->source()); > > Calling this here can result in muted and readystate events to be fired on the new track. Is that what we want? > Probably not, good catch. > > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:175 > > + if (stopSource == StopTrackAndStopSource && m_source) > > We can't stop the source, since it can be shared > But we have to stop the source, even if it is shared :-) > > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:234 > > +RefPtr<MediaStreamCapabilities> MediaStreamTrackPrivate::capabilities() const > > Layering violation? > Oops!

(In reply to comment #10) > (In reply to comment #9) > > > > > Source/WebCore/Modules/mediastream/MediaSourceStates.cpp:48 > > > + return MediaStreamSourceStates::sourceType(m_sourceStates.sourceType()); > > > > Why don't just return m_sourceStates.sourceType()? Did understand why put it static. > > > > m_sourceStates.sourceType() returns an Enum, which is a string in JS so MediaSourceStates::sourceType() needs to return the string that represents the enum. Huum, I see. Why don't just make this conversion inside MediaSourceState with a method like: toSourceTypeString (or any better name)? > > > > Source/WebCore/Modules/mediastream/MediaSourceStates.cpp:53 > > > + return MediaStreamSourceStates::facingMode(m_sourceStates.facingMode()); > > > > Ditto. > > > > Ditto. > > > > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:192 > > > void MediaStreamTrack::stopProducingData() > > > > I haven't seen anybody calling this method. Is that still being used? > > > > Yes, this is called when track.stop() is called from JavaScript (because we use "[ImplementedAs= stopProducingData]" in the IDL file). OK > > > > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:199 > > > + m_stoppingTrack = true; > > > > Is this really needed? > > > > Yes. The spec says that the 'ended' event should not be fired when track.stop() is called. But, check this -> http://dev.w3.org/2011/webrtc/editor/getusermedia.html#event-mediastreamtrack-ended > > > > > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:200 > > > + m_privateTrack->stop(MediaStreamTrackPrivate::StopTrackAndStopSource); > > > > I think we can't stop the source here because it can be shared with another track that has not been stopped yet. > > > > We have to, the spec says the stop() method must > > Permanently stop the generation of data for track's source. If > the data is being generated from a live source (e.g., a > microphone or camera), then the user agent should remove any > active "on-air" indicator for that source. If the data is being > generated from a prerecorded source (e.g. a video file), any > remaining content in the file is ignored. > > (http://www.w3.org/TR/mediacapture-streams/#dom-mediastreamtrack-stop) > You are right, didn't notice this on the spec. > > > > Source/WebCore/platform/mediastream/MediaStreamSource.cpp:153 > > > + // This is called from the track.stop() method, which should "Permanently stop the generation of data > > > > This source can be shared, so a track calling stop does not mean that the source should stop as well. I think that only the source can tell if it has to stop or not (by checking if there are any tracks using it, or the backend has told that it has stopped) > > > > That isn't my interpretation of the spec, but I have sent email to Adam Bergkvist asking for clarification. In the meantime, I have modified MediaStreamTrack.html to verify that WebKit behaves I think it should. As you stated in the previous comment, this is the right behaviour. > > > > Source/WebCore/platform/mediastream/MediaStreamSource.h:74 > > > + enum Type { None, Audio, Video }; > > > > Does make sense to create a source of no type? > > > MediaStreamTrackPrivate::type has to return something when m_source is NULL. fair enough. > > > > > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:49 > > > + setSource(other->source()); > > > > Calling this here can result in muted and readystate events to be fired on the new track. Is that what we want? > > > > Probably not, good catch. > > > > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:175 > > > + if (stopSource == StopTrackAndStopSource && m_source) > > > > We can't stop the source, since it can be shared > > > > But we have to stop the source, even if it is shared :-) Yes, same as other previous comments :) > > > > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:234 > > > +RefPtr<MediaStreamCapabilities> MediaStreamTrackPrivate::capabilities() const > > > > Layering violation? > > > > Oops!

(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > > > > > Source/WebCore/Modules/mediastream/MediaSourceStates.cpp:48 > > > > + return MediaStreamSourceStates::sourceType(m_sourceStates.sourceType()); > > > > > > Why don't just return m_sourceStates.sourceType()? Did understand why put it static. > > > > > > > m_sourceStates.sourceType() returns an Enum, which is a string in JS so MediaSourceStates::sourceType() needs to return the string that represents the enum. > > Huum, I see. Why don't just make this conversion inside MediaSourceState with a method like: toSourceTypeString (or any better name)? > I wanted the strings to be in /platform so they can be used by platform code when validating/evaluating constraints. See AVCaptureDeviceManager::validFacingModes for example. > > > > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:199 > > > > + m_stoppingTrack = true; > > > > > > Is this really needed? > > > > > > > Yes. The spec says that the 'ended' event should not be fired when track.stop() is called. > > But, check this -> http://dev.w3.org/2011/webrtc/editor/getusermedia.html#event-mediastreamtrack-ended > I asked a spec editor about this over the weekend and he said that section of the spec is incorrect.

Created attachment 215329 [details] Updated patch

(In reply to comment #13) > Created an attachment (id=215329) [details] > Updated patch We can get rid of m_stoppingTrack by only assigning the Ended state to m_readyState in MediaStreamTrackPrivate::stop, instead of calling setReadyState

Comment on attachment 215329 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=215329&action=review r=me, with a few nits. > Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:323 > +Vector<RefPtr<TrackSourceInfo>> AVCaptureDeviceManager::getSourcesInfo(const String& /*requestOrigin*/) Why is this commented out? Can we either uncomment and add a UNUSED_PARAM() or remove it? > LayoutTests/fast/mediastream/MediaStreamTrack.html:28 > +counter = 0; > +var t = []; Weird indentation. > LayoutTests/fast/mediastream/MediaStreamTrack.html:31 > +t[counter++] = event.target; Ditto. > LayoutTests/fast/mediastream/MediaStreamTrack.html:84 > + debug(" " + i + " : " + list[i]); Ditto.

Comment on attachment 215329 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=215329&action=review > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:207 > + m_stoppingTrack = true; If you set m_stopped to true in MediaStreamTrackPrivate::stop prior to calling setReadyState you can get rid of this protection

Comment on attachment 215329 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=215329&action=review > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:48 > + m_ignoreMutations = true; You can also just set the properties (m_enabled, m_muted and m_readyState) by assigning it from the source, instead of calling setSource, which will call trackReadyStateChanged and trackMutedChanged. By doing this you can also discard the m_ignoreMutations protection

(In reply to comment #16) > (From update of attachment 215329 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=215329&action=review > > > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:207 > > + m_stoppingTrack = true; > > If you set m_stopped to true in MediaStreamTrackPrivate::stop prior to calling setReadyState you can get rid of this protection We could, but the reason we need to do this is to avoid firing an 'ended' event when track.stop() is called, and that is spec logic that we must not be aware of in platform (see comment #6). (In reply to comment #17) > (From update of attachment 215329 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=215329&action=review > > > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:48 > > + m_ignoreMutations = true; > > You can also just set the properties (m_enabled, m_muted and m_readyState) by assigning it from the source, instead of calling setSource, which will call trackReadyStateChanged and trackMutedChanged. By doing this you can also discard the m_ignoreMutations protection We would have to set m_enabled, m_muted, m_readyState, m_source, and call m_source->addObserver() . This would mean that we would have the same code in both versions of the constructor and in setSource(). I think having a flag is preferable.

(In reply to comment #18) > (In reply to comment #16) > > (From update of attachment 215329 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=215329&action=review > > > > > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:207 > > > + m_stoppingTrack = true; > > > > If you set m_stopped to true in MediaStreamTrackPrivate::stop prior to calling setReadyState you can get rid of this protection > > We could, but the reason we need to do this is to avoid firing an 'ended' event when track.stop() is called, and that is spec logic that we must not be aware of in platform (see comment #6). Doing what I said would prevent that as well. And also the decision of firing the event or not are still being decided in MediaStreamTrack (we are just not telling it that it has ended). Additionally, I think adding this flag can lead to future problems (like if one forget to set it when it was supposed to set). Just my two cents on that. > > (In reply to comment #17) > > (From update of attachment 215329 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=215329&action=review > > > > > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:48 > > > + m_ignoreMutations = true; > > > > You can also just set the properties (m_enabled, m_muted and m_readyState) by assigning it from the source, instead of calling setSource, which will call trackReadyStateChanged and trackMutedChanged. By doing this you can also discard the m_ignoreMutations protection > > We would have to set m_enabled, m_muted, m_readyState, m_source, and call m_source->addObserver() . This would mean that we would have the same code in both versions of the constructor and in setSource(). I think having a flag is preferable. Maybe you could group those stuff in a method (initilizeFromSource?)

(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #16) > > > (From update of attachment 215329 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=215329&action=review > > > > > > > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:207 > > > > + m_stoppingTrack = true; > > > > > > If you set m_stopped to true in MediaStreamTrackPrivate::stop prior to calling setReadyState you can get rid of this protection > > > > We could, but the reason we need to do this is to avoid firing an 'ended' event when track.stop() is called, and that is spec logic that we must not be aware of in platform (see comment #6). > > Doing what I said would prevent that as well. And also the decision of firing the event or not are still being decided in MediaStreamTrack (we are just not telling it that it has ended). Additionally, I think adding this flag can lead to future problems (like if one forget to set it when it was supposed to set). > Just my two cents on that. > MediaStreamTrack.cpp doesn't have an m_stopped instance variable. I could add one, but then we would be switching one flag for another :-) > > > > (In reply to comment #17) > > > (From update of attachment 215329 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=215329&action=review > > > > > > > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:48 > > > > + m_ignoreMutations = true; > > > > > > You can also just set the properties (m_enabled, m_muted and m_readyState) by assigning it from the source, instead of calling setSource, which will call trackReadyStateChanged and trackMutedChanged. By doing this you can also discard the m_ignoreMutations protection > > > > We would have to set m_enabled, m_muted, m_readyState, m_source, and call m_source->addObserver() . This would mean that we would have the same code in both versions of the constructor and in setSource(). I think having a flag is preferable. > > Maybe you could group those stuff in a method (initilizeFromSource?) And have the same code in initilizeFromSource and setSource?

(In reply to comment #20) > (In reply to comment #19) > > (In reply to comment #18) > > > (In reply to comment #16) > > > > (From update of attachment 215329 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=215329&action=review > > > > > > > > > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:207 > > > > > + m_stoppingTrack = true; > > > > > > > > If you set m_stopped to true in MediaStreamTrackPrivate::stop prior to calling setReadyState you can get rid of this protection > > > > > > We could, but the reason we need to do this is to avoid firing an 'ended' event when track.stop() is called, and that is spec logic that we must not be aware of in platform (see comment #6). > > > > Doing what I said would prevent that as well. And also the decision of firing the event or not are still being decided in MediaStreamTrack (we are just not telling it that it has ended). Additionally, I think adding this flag can lead to future problems (like if one forget to set it when it was supposed to set). > > Just my two cents on that. > > > > MediaStreamTrack.cpp doesn't have an m_stopped instance variable. I could add one, but then we would be switching one flag for another :-) No no. The workflow would be as follows: MediaStreamTrack::stop -> MediaStreamTrackPrivate::stop, then in MediaStreamTrackPrivate::stop the m_stopped property could be set before calling MediaStreamTrackPrivate::setReadyState and inside MediaStreamTrackPrivate::setReadyState you can add a check (maybe in the same if (!m_client || !m_ignoreMutations)) to early return if track is stopped. Or, you could just do m_readyState = MediaStreamSource::Ended in MediaStreamTrackPrivate::stop, instead of calling MediaStreamTrackPrivate::setReadyState. :) > > > > > > > (In reply to comment #17) > > > > (From update of attachment 215329 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=215329&action=review > > > > > > > > > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:48 > > > > > + m_ignoreMutations = true; > > > > > > > > You can also just set the properties (m_enabled, m_muted and m_readyState) by assigning it from the source, instead of calling setSource, which will call trackReadyStateChanged and trackMutedChanged. By doing this you can also discard the m_ignoreMutations protection > > > > > > We would have to set m_enabled, m_muted, m_readyState, m_source, and call m_source->addObserver() . This would mean that we would have the same code in both versions of the constructor and in setSource(). I think having a flag is preferable. > > > > Maybe you could group those stuff in a method (initilizeFromSource?) > > And have the same code in initilizeFromSource and setSource? No, setSource would still calling setMuted(m_source->muted()) and setReadyState(m_source->readyState()), because it wants to notify WebCore that something has chanaged, this initializeFromSource is something only to be used by a new created MediaStreamTrackPrivate

(In reply to comment #15) > (From update of attachment 215329 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=215329&action=review > > r=me, with a few nits. > > > Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:323 > > +Vector<RefPtr<TrackSourceInfo>> AVCaptureDeviceManager::getSourcesInfo(const String& /*requestOrigin*/) > > Why is this commented out? Can we either uncomment and add a UNUSED_PARAM() or remove it? > Fixed. > > LayoutTests/fast/mediastream/MediaStreamTrack.html:28 > > +counter = 0; > > +var t = []; > > Weird indentation. > > > LayoutTests/fast/mediastream/MediaStreamTrack.html:31 > > +t[counter++] = event.target; > > Ditto. > Oops, debug code :-O. Removed. > > LayoutTests/fast/mediastream/MediaStreamTrack.html:84 > > + debug(" " + i + " : " + list[i]); > > Ditto. > Actually this fixes the indentation, that line is in the for(...) loop.

Committed r158220 : https://trac.webkit.org/r158220