Bug 121940 – [MediaStream API] update MediaStreamTrack object to match spec

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Eric Carlson]

MISSING:
    readonly    attribute boolean               muted;
    readonly    attribute boolean               _readonly;
    readonly    attribute boolean               remote;
              attribute EventHandler      onstarted;
    Dictionary?                                          constraints ();			// MediaTrackConstraints
    Dictionary                                            states ();				// MediaSourceStates
    Dictionary                                            capabilities ();			// AllVideoCapabilities or AllAudioCapabilities
    		void                                                     applyConstraints (MediaTrackConstraints constraints);
              attribute EventHandler      onoverconstrained;
    MediaStreamTrack                             clone ();
    void                                                     stop ();


<rdar://problem/15022423>

[Eric Carlson]

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

[Thiago de Barros Lacerda]

(In reply to comment #1)
> Created an attachment (id=213545) [details]
> Patch for the hungry, hungry bots.

My points about the patch:

* What do you think about having the audio and video capabilities in their own classes, instead of having it all in MediaStreamCapabilities and making AllAudioCapabilities and AllVideoCapabilities inheriting from it? The spec is very specific about what is video and audio related.

* Why the MediaStreamCenter notifications methods were removed?

Thanks

[Eric Carlson]

Created attachment 213558 [details]
Rebased patch

[WebKit Commit Bot]

Attachment 213558 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/mediastream/MediaStreamTrack.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Modules/mediastream/AllAudioCapabilities.h', u'Source/WebCore/Modules/mediastream/AllAudioCapabilities.idl', u'Source/WebCore/Modules/mediastream/AllVideoCapabilities.h', u'Source/WebCore/Modules/mediastream/AllVideoCapabilities.idl', u'Source/WebCore/Modules/mediastream/AudioStreamTrack.cpp', u'Source/WebCore/Modules/mediastream/AudioStreamTrack.h', u'Source/WebCore/Modules/mediastream/CapabilityRange.cpp', u'Source/WebCore/Modules/mediastream/CapabilityRange.h', u'Source/WebCore/Modules/mediastream/CapabilityRange.idl', u'Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h', 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/MediaStream.h', u'Source/WebCore/Modules/mediastream/MediaStreamCapabilities.cpp', u'Source/WebCore/Modules/mediastream/MediaStreamCapabilities.h', u'Source/WebCore/Modules/mediastream/MediaStreamCapabilities.idl', u'Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp', u'Source/WebCore/Modules/mediastream/MediaStreamTrack.h', u'Source/WebCore/Modules/mediastream/MediaStreamTrack.idl', u'Source/WebCore/Modules/mediastream/MediaTrackConstraint.cpp', u'Source/WebCore/Modules/mediastream/MediaTrackConstraint.h', u'Source/WebCore/Modules/mediastream/MediaTrackConstraint.idl', u'Source/WebCore/Modules/mediastream/MediaTrackConstraintSet.cpp', u'Source/WebCore/Modules/mediastream/MediaTrackConstraintSet.h', u'Source/WebCore/Modules/mediastream/MediaTrackConstraintSet.idl', u'Source/WebCore/Modules/mediastream/MediaTrackConstraints.cpp', u'Source/WebCore/Modules/mediastream/MediaTrackConstraints.h', u'Source/WebCore/Modules/mediastream/MediaTrackConstraints.idl', u'Source/WebCore/Modules/mediastream/NavigatorMediaStream.cpp', u'Source/WebCore/Modules/mediastream/UserMediaRequest.cpp', u'Source/WebCore/Modules/mediastream/VideoStreamTrack.cpp', u'Source/WebCore/Modules/mediastream/VideoStreamTrack.h', u'Source/WebCore/Modules/webaudio/AudioContext.cpp', u'Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.cpp', u'Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.h', u'Source/WebCore/Modules/webaudio/MediaStreamAudioSource.cpp', u'Source/WebCore/Modules/webaudio/MediaStreamAudioSource.h', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/JSMediaSourceStatesCustom.cpp', u'Source/WebCore/bindings/js/JSMediaStreamCapabilitiesCustom.cpp', u'Source/WebCore/dom/EventNames.h', u'Source/WebCore/platform/mediastream/MediaStreamCenter.cpp', u'Source/WebCore/platform/mediastream/MediaStreamCenter.h', u'Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp', u'Source/WebCore/platform/mediastream/MediaStreamDescriptor.h', 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/blackberry/MediaStreamCenterBlackBerry.cpp', u'Source/WebCore/platform/mediastream/blackberry/MediaStreamCenterBlackBerry.h', u'Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterGStreamer.cpp', u'Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterGStreamer.h', u'Source/WebCore/platform/mediastream/mac/MediaStreamCenterMac.cpp', u'Source/WebCore/platform/mediastream/mac/MediaStreamCenterMac.h', u'Source/WebCore/platform/mock/MockMediaStreamCenter.cpp', u'Source/WebCore/platform/mock/MockMediaStreamCenter.h']" exit_code: 1
Source/WebCore/Modules/mediastream/NavigatorMediaStream.cpp:39:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/Modules/mediastream/NavigatorMediaStream.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/Modules/mediastream/NavigatorMediaStream.cpp:43:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/Modules/mediastream/NavigatorMediaStream.cpp:47:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/Modules/mediastream/NavigatorMediaStream.cpp:58:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/mediastream/MediaStreamSource.h:86:  The parameter name "constraints" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/js/JSMediaStreamCapabilitiesCustom.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/CMakeLists.txt:797:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/CMakeLists.txt:799:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/CMakeLists.txt:801:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/CMakeLists.txt:807:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/CMakeLists.txt:808:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/CMakeLists.txt:809:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/CMakeLists.txt:190:  Alphabetical sorting problem. "Modules/mediastream/AllAudioCapabilities.idl" should be before "Modules/mediastream/AllVideoCapabilities.idl".  [list/order] [5]
Source/WebCore/CMakeLists.txt:202:  Alphabetical sorting problem. "Modules/mediastream/MediaTrackConstraintSet.idl" should be before "Modules/mediastream/MediaTrackConstraints.idl".  [list/order] [5]
Total errors found: 15 in 58 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 213558 [details]
Rebased patch

Attachment 213558 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/3356107

[EFL EWS Bot]

Comment on attachment 213558 [details]
Rebased patch

Attachment 213558 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3514090

[Eric Carlson]

Created attachment 213562 [details]
YARP

[Eric Carlson]

(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=213545) [details] [details]
> > Patch for the hungry, hungry bots.
> 
> My points about the patch:
> 
> * What do you think about having the audio and video capabilities in their own classes, instead of having it all in MediaStreamCapabilities and making AllAudioCapabilities and AllVideoCapabilities inheriting from it? The spec is very specific about what is video and audio related.
> 
AllAudioCapabilities and AllVideoCapabilities do inherit from MediaStreamCapabilities, and we have the behavior the spec requires because the .idl files for these three classes list the correct attributes. Just to be certain, MediaStreamCapabilities.cpp ASSERTs on all of the video-specific attributes if the capability does not have video.

> * Why the MediaStreamCenter notifications methods were removed?
> 
I know that some of the methods are unnecessary so I removed all of the ones that are not implemented in *any* of the port-specific MediaStreamCenter implementations. As I say in the ChangeLog "It isn't clear that they are necessary, and we can add them back if/when we need them."

[WebKit Commit Bot]

Attachment 213562 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/mediastream/MediaStreamTrack.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Modules/mediastream/AllAudioCapabilities.h', u'Source/WebCore/Modules/mediastream/AllAudioCapabilities.idl', u'Source/WebCore/Modules/mediastream/AllVideoCapabilities.h', u'Source/WebCore/Modules/mediastream/AllVideoCapabilities.idl', u'Source/WebCore/Modules/mediastream/AudioStreamTrack.cpp', u'Source/WebCore/Modules/mediastream/AudioStreamTrack.h', u'Source/WebCore/Modules/mediastream/CapabilityRange.cpp', u'Source/WebCore/Modules/mediastream/CapabilityRange.h', u'Source/WebCore/Modules/mediastream/CapabilityRange.idl', u'Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h', 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/MediaStream.h', u'Source/WebCore/Modules/mediastream/MediaStreamCapabilities.cpp', u'Source/WebCore/Modules/mediastream/MediaStreamCapabilities.h', u'Source/WebCore/Modules/mediastream/MediaStreamCapabilities.idl', u'Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp', u'Source/WebCore/Modules/mediastream/MediaStreamTrack.h', u'Source/WebCore/Modules/mediastream/MediaStreamTrack.idl', u'Source/WebCore/Modules/mediastream/MediaTrackConstraint.cpp', u'Source/WebCore/Modules/mediastream/MediaTrackConstraint.h', u'Source/WebCore/Modules/mediastream/MediaTrackConstraint.idl', u'Source/WebCore/Modules/mediastream/MediaTrackConstraintSet.cpp', u'Source/WebCore/Modules/mediastream/MediaTrackConstraintSet.h', u'Source/WebCore/Modules/mediastream/MediaTrackConstraintSet.idl', u'Source/WebCore/Modules/mediastream/MediaTrackConstraints.cpp', u'Source/WebCore/Modules/mediastream/MediaTrackConstraints.h', u'Source/WebCore/Modules/mediastream/MediaTrackConstraints.idl', u'Source/WebCore/Modules/mediastream/UserMediaRequest.cpp', u'Source/WebCore/Modules/mediastream/VideoStreamTrack.cpp', u'Source/WebCore/Modules/mediastream/VideoStreamTrack.h', u'Source/WebCore/Modules/webaudio/AudioContext.cpp', u'Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.cpp', u'Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.h', u'Source/WebCore/Modules/webaudio/MediaStreamAudioSource.cpp', u'Source/WebCore/Modules/webaudio/MediaStreamAudioSource.h', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/JSMediaSourceStatesCustom.cpp', u'Source/WebCore/bindings/js/JSMediaStreamCapabilitiesCustom.cpp', u'Source/WebCore/dom/EventNames.h', u'Source/WebCore/platform/mediastream/MediaStreamCenter.cpp', u'Source/WebCore/platform/mediastream/MediaStreamCenter.h', u'Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp', u'Source/WebCore/platform/mediastream/MediaStreamDescriptor.h', 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/blackberry/MediaStreamCenterBlackBerry.cpp', u'Source/WebCore/platform/mediastream/blackberry/MediaStreamCenterBlackBerry.h', u'Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterGStreamer.cpp', u'Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterGStreamer.h', u'Source/WebCore/platform/mediastream/mac/MediaStreamCenterMac.cpp', u'Source/WebCore/platform/mediastream/mac/MediaStreamCenterMac.h', u'Source/WebCore/platform/mock/MockMediaStreamCenter.cpp', u'Source/WebCore/platform/mock/MockMediaStreamCenter.h']" exit_code: 1
Source/WebCore/bindings/js/JSMediaStreamCapabilitiesCustom.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 213562 [details]
YARP

Attachment 213562 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3489082

[EFL EWS Bot]

Comment on attachment 213562 [details]
YARP

Attachment 213562 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/3493177

[Build Bot]

Comment on attachment 213562 [details]
YARP

Attachment 213562 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3490096

[Build Bot]

Comment on attachment 213562 [details]
YARP

Attachment 213562 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3490097

[Build Bot]

Comment on attachment 213562 [details]
YARP

Attachment 213562 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3562014

[kov's GTK+ EWS bot]

Comment on attachment 213562 [details]
YARP

Attachment 213562 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/3514103

[Eric Carlson]

Created attachment 213589 [details]
Add the missing compile flag

[WebKit Commit Bot]

Attachment 213589 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/mediastream/MediaStreamTrack.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Modules/mediastream/AllAudioCapabilities.h', u'Source/WebCore/Modules/mediastream/AllAudioCapabilities.idl', u'Source/WebCore/Modules/mediastream/AllVideoCapabilities.h', u'Source/WebCore/Modules/mediastream/AllVideoCapabilities.idl', u'Source/WebCore/Modules/mediastream/AudioStreamTrack.cpp', u'Source/WebCore/Modules/mediastream/AudioStreamTrack.h', u'Source/WebCore/Modules/mediastream/CapabilityRange.cpp', u'Source/WebCore/Modules/mediastream/CapabilityRange.h', u'Source/WebCore/Modules/mediastream/CapabilityRange.idl', u'Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h', 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/MediaStream.h', u'Source/WebCore/Modules/mediastream/MediaStreamCapabilities.cpp', u'Source/WebCore/Modules/mediastream/MediaStreamCapabilities.h', u'Source/WebCore/Modules/mediastream/MediaStreamCapabilities.idl', u'Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp', u'Source/WebCore/Modules/mediastream/MediaStreamTrack.h', u'Source/WebCore/Modules/mediastream/MediaStreamTrack.idl', u'Source/WebCore/Modules/mediastream/MediaTrackConstraint.cpp', u'Source/WebCore/Modules/mediastream/MediaTrackConstraint.h', u'Source/WebCore/Modules/mediastream/MediaTrackConstraint.idl', u'Source/WebCore/Modules/mediastream/MediaTrackConstraintSet.cpp', u'Source/WebCore/Modules/mediastream/MediaTrackConstraintSet.h', u'Source/WebCore/Modules/mediastream/MediaTrackConstraintSet.idl', u'Source/WebCore/Modules/mediastream/MediaTrackConstraints.cpp', u'Source/WebCore/Modules/mediastream/MediaTrackConstraints.h', u'Source/WebCore/Modules/mediastream/MediaTrackConstraints.idl', u'Source/WebCore/Modules/mediastream/UserMediaRequest.cpp', u'Source/WebCore/Modules/mediastream/VideoStreamTrack.cpp', u'Source/WebCore/Modules/mediastream/VideoStreamTrack.h', u'Source/WebCore/Modules/webaudio/AudioContext.cpp', u'Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.cpp', u'Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.h', u'Source/WebCore/Modules/webaudio/MediaStreamAudioSource.cpp', u'Source/WebCore/Modules/webaudio/MediaStreamAudioSource.h', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/JSMediaSourceStatesCustom.cpp', u'Source/WebCore/bindings/js/JSMediaStreamCapabilitiesCustom.cpp', u'Source/WebCore/dom/EventNames.h', u'Source/WebCore/platform/mediastream/MediaStreamCenter.cpp', u'Source/WebCore/platform/mediastream/MediaStreamCenter.h', u'Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp', u'Source/WebCore/platform/mediastream/MediaStreamDescriptor.h', 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/blackberry/MediaStreamCenterBlackBerry.cpp', u'Source/WebCore/platform/mediastream/blackberry/MediaStreamCenterBlackBerry.h', u'Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterGStreamer.cpp', u'Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterGStreamer.h', u'Source/WebCore/platform/mediastream/mac/MediaStreamCenterMac.cpp', u'Source/WebCore/platform/mediastream/mac/MediaStreamCenterMac.h', u'Source/WebCore/platform/mock/MockMediaStreamCenter.cpp', u'Source/WebCore/platform/mock/MockMediaStreamCenter.h']" exit_code: 1
Source/WebCore/bindings/js/JSMediaStreamCapabilitiesCustom.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 213589 [details]
Add the missing compile flag

Attachment 213589 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3648002

[Eric Carlson]

Created attachment 213599 [details]
New patch to appease my style-bot master

[EFL EWS Bot]

Comment on attachment 213599 [details]
New patch to appease my style-bot master

Attachment 213599 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3611020

[Eric Carlson]

Created attachment 213600 [details]
Updated patch

Fix path in CMakeLists.txt

[Philippe Normand]

Comment on attachment 213600 [details]
Updated patch

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

Here are the few issues I found when trying to build this patch on GTK (without --media-stream)

> Source/WebCore/GNUmakefile.list.am:473
> +	DerivedSources/WebCore/JSMediaSourceStatesCustom.cpp \

That path is incorrect and the file should be listed in webcore_sources

> Source/WebCore/GNUmakefile.list.am:482
> +	DerivedSources/WebCore/JSMediaStreamCapabilitiesCustom.cpp \

Ditto

> Source/WebCore/GNUmakefile.list.am:5372
> +	Source/WebCore/platform/mediastream/MediaStreamAudioSource.cpp \
> +	Source/WebCore/platform/mediastream/MediaStreamAudioSource.h \

The files are in Modules/webaudio

> Source/WebCore/bindings/js/JSMediaStreamCapabilitiesCustom.cpp:51
> +#endif // ENABLE(MEDIA_STREAM)

Misplaced #endif

[Eric Carlson]

Created attachment 213603 [details]
Updated patch

(In reply to comment #22)
> (From update of attachment 213600 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213600&action=review
> 
> Here are the few issues I found when trying to build this patch on GTK (without --media-stream)
> 
> > Source/WebCore/GNUmakefile.list.am:473
> > +	DerivedSources/WebCore/JSMediaSourceStatesCustom.cpp \
> 
Fixed.

> That path is incorrect and the file should be listed in webcore_sources
> 
> > Source/WebCore/GNUmakefile.list.am:482
> > +	DerivedSources/WebCore/JSMediaStreamCapabilitiesCustom.cpp \
> 
> Ditto
> 
Fixed.

> > Source/WebCore/GNUmakefile.list.am:5372
> > +	Source/WebCore/platform/mediastream/MediaStreamAudioSource.cpp \
> > +	Source/WebCore/platform/mediastream/MediaStreamAudioSource.h \
> 
> The files are in Modules/webaudio
> 
Fixed.

> > Source/WebCore/bindings/js/JSMediaStreamCapabilitiesCustom.cpp:51
> > +#endif // ENABLE(MEDIA_STREAM)
> 
> Misplaced #endif

Fixed.

Thanks!

[Eric Carlson]

Created attachment 213612 [details]
Updated patch

Correct a typo in MockMediaStreamCenter.cpp, add updated test results.

[Jer Noble]

Comment on attachment 213612 [details]
Updated patch

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

r=me, with nits.

> Source/WebCore/Modules/mediastream/AllAudioCapabilities.h:42
> +    static PassRefPtr<AllAudioCapabilities> create(PassRefPtr<MediaStreamSourceCapabilities> capabilities)

Nit: PassRefPtrs are no longer needed, ever since Anders added move-semantics to the RefPtr class.  You can return RefPtr here.

> Source/WebCore/Modules/mediastream/AllVideoCapabilities.h:40
> +    static PassRefPtr<AllVideoCapabilities> create(PassRefPtr<MediaStreamSourceCapabilities> capabilities)

Ditto.

> Source/WebCore/Modules/mediastream/AudioStreamTrack.cpp:50
>  PassRefPtr<AudioStreamTrack> AudioStreamTrack::create(ScriptExecutionContext* context, const Dictionary& audioConstraints)
>  {
> -    RefPtr<AudioStreamTrack> track = adoptRef(new AudioStreamTrack(context, 0, &audioConstraints));
> -    return track.release();
> +    return adoptRef(new AudioStreamTrack(context, 0, &audioConstraints));
>  }
>  
>  PassRefPtr<AudioStreamTrack> AudioStreamTrack::create(ScriptExecutionContext* context, MediaStreamSource* source)
>  {
> -    RefPtr<AudioStreamTrack> track = adoptRef(new AudioStreamTrack(context, source, 0));
> -    return track.release();
> +    return adoptRef(new AudioStreamTrack(context, source, 0));
> +}
> +
> +PassRefPtr<AudioStreamTrack> AudioStreamTrack::create(MediaStreamTrack* track)
> +{
> +    return adoptRef(new AudioStreamTrack(track));

And super-ditto on these three.

> Source/WebCore/Modules/mediastream/CapabilityRange.cpp:43
> +PassRefPtr<CapabilityRange> CapabilityRange::create(const MediaStreamSourceCapabilityRange& rangeInfo)

Yet another ditto.

> Source/WebCore/Modules/mediastream/MediaSourceStates.cpp:36
> +PassRefPtr<MediaSourceStates> MediaSourceStates::create(const MediaStreamSourceStates& states)

Ditto.

> Source/WebCore/Modules/mediastream/MediaStreamCapabilities.cpp:40
> +PassRefPtr<MediaStreamCapabilities> MediaStreamCapabilities::create(PassRefPtr<MediaStreamSourceCapabilities> capabilities)

Ditto.

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:120
>  void MediaStreamTrack::setSource(MediaStreamSource* source)
>  {
>      ASSERT(source == m_source || !m_source);

(Existing code)

Wha wha?  What exactly is this ASSERT protecting us from?

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:254
> +PassRefPtr<MediaTrackConstraints> MediaStreamTrack::constraints() const
> +{
> +    // FIXME: https://bugs.webkit.org/show_bug.cgi?id=122428
> +    notImplemented();
> +    return 0;
> +}
> +
> +PassRefPtr<MediaSourceStates> MediaStreamTrack::states() const
> +{
> +    if (!m_source)
> +        return 0;
> +    
> +    return MediaSourceStates::create(m_source->states());
> +}
> +
> +PassRefPtr<MediaStreamCapabilities> MediaStreamTrack::capabilities() const
> +{
> +    if (!m_source)
> +        return 0;
> +
> +    return MediaStreamCapabilities::create(m_source->capabilities());
> +}

Another mega PassRefPtr -> RefPtr Ditto.

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:338
> +    // 4.3.1
> +    // ... media from the source only flows when a MediaStreamTrack object is both unmuted and enabled
> +}

This looks suspicious.  Was there supposed to be a FIXME here?

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:386
> +    Vector<RefPtr<Event>> events;
> +    {
> +        MutexLocker locker(m_mutex);
> +
> +        m_eventDispatchScheduled = false;
> +        if (!scriptExecutionContext()) {
> +            m_scheduledEvents.clear();
> +            return;
> +        }
> +
> +        events.swap(m_scheduledEvents);
> +    }

I feel like this could be cleaned up slightly to minimize the time inside the mutex:

Vector<RefPtr<Event>> events;
{
    MutexLocker locker(m_mutex);
    m_eventDispatchScheduled = false;
    events.swap(m_scheduledEvents);
}
if (!scriptExecutionContext())
    return;

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:390
> +    Vector<RefPtr<Event> >::iterator it = events.begin();
> +    for (; it != events.end(); ++it)
> +        dispatchEvent((*it).release());

The newly supported "auto" keyword would be awesome here:

for (auto it = events.begin(); it != events.end(); ++it)
    dispatchEvent((*it).release());

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:392
> +    events.clear();

This seems unnecessary.

> Source/WebCore/Modules/mediastream/MediaTrackConstraint.cpp:37
> +PassRefPtr<MediaTrackConstraint> MediaTrackConstraint::create(const Dictionary& constraint)

PassRefPtr -> RefPtr ditto.

> Source/WebCore/Modules/mediastream/MediaTrackConstraintSet.cpp:37
> +PassRefPtr<MediaTrackConstraintSet> MediaTrackConstraintSet::create(const Dictionary& constraints)

Man, I'm only 1/2 way through?  Phew.

Oh, ditto.

> Source/WebCore/Modules/mediastream/MediaTrackConstraints.cpp:41
> +PassRefPtr<MediaTrackConstraints> MediaTrackConstraints::create(PassRefPtr<MediaConstraintsImpl> constraints)

Ditto.

> Source/WebCore/Modules/mediastream/VideoStreamTrack.cpp:50
> +PassRefPtr<VideoStreamTrack> VideoStreamTrack::create(MediaStreamTrack* track)

Ditto.

> Source/WebCore/Modules/webaudio/MediaStreamAudioSource.cpp:38
> +PassRefPtr<MediaStreamAudioSource> MediaStreamAudioSource::create()

Ditto.

[Darin Adler]

Comment on attachment 213612 [details]
Updated patch

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

>> Source/WebCore/Modules/mediastream/AllAudioCapabilities.h:42
>> +    static PassRefPtr<AllAudioCapabilities> create(PassRefPtr<MediaStreamSourceCapabilities> capabilities)
> 
> Nit: PassRefPtrs are no longer needed, ever since Anders added move-semantics to the RefPtr class.  You can return RefPtr here.

Jer means that PassRefPtrs are no longer needed *for return values*. We haven’t quite gotten to the point where we don’t need them for function arguments yet.

[Eric Carlson]

Committed r157068: http://trac.webkit.org/changeset/157068