Bug 121940 - [MediaStream API] update MediaStreamTrack object to match spec
Summary: [MediaStream API] update MediaStreamTrack object to match spec
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: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks: 124288
  Show dependency tree / graph
 
Reported: 2013-09-25 22:00 PDT by Eric Carlson
Modified: 2013-11-14 07:39 PST (History)
20 users (show)

See Also:

Attachments
Patch for the hungry, hungry bots. (211.15 KB, patch)
2013-10-06 17:40 PDT, Eric Carlson 		no flags Details | Formatted Diff | Diff
Rebased patch (211.00 KB, text/plain)
2013-10-06 20:38 PDT, Eric Carlson 		eflews.bot: commit-queue-
Details
YARP (209.74 KB, patch)
2013-10-06 21:15 PDT, Eric Carlson 		eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Add the missing compile flag (209.43 KB, patch)
2013-10-07 08:39 PDT, Eric Carlson 		eflews.bot: commit-queue-
Details | Formatted Diff | Diff
New patch to appease my style-bot master (230.98 KB, text/plain)
2013-10-07 09:34 PDT, Eric Carlson 		no flags Details
Updated patch (211.03 KB, patch)
2013-10-07 10:05 PDT, Eric Carlson 		pnormand: review-
Details | Formatted Diff | Diff
Updated patch (211.34 KB, patch)
2013-10-07 10:27 PDT, Eric Carlson 		no flags Details | Formatted Diff | Diff
Updated patch (215.16 KB, patch)
2013-10-07 12:33 PDT, Eric Carlson 		no flags Details | Formatted Diff | Diff
Show Obsolete (7) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2013-09-25 22:00:45 PDT 
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>
Comment 1 Eric Carlson 2013-10-06 17:40:42 PDT 
Created attachment 213545 [details]
Patch for the hungry, hungry bots.
Comment 2 Thiago de Barros Lacerda 2013-10-06 19:30:27 PDT 
(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
Comment 3 Eric Carlson 2013-10-06 20:38:56 PDT 
Created attachment 213558 [details]
Rebased patch
Comment 4 WebKit Commit Bot 2013-10-06 20:44:30 PDT 
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.
Comment 5 EFL EWS Bot 2013-10-06 20:49:34 PDT 
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
Comment 6 EFL EWS Bot 2013-10-06 20:49:52 PDT 
Comment on attachment 213558 [details]
Rebased patch

Attachment 213558 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3514090
Comment 7 Eric Carlson 2013-10-06 21:15:18 PDT 
Created attachment 213562 [details]
YARP
Comment 8 Eric Carlson 2013-10-06 21:23:10 PDT 
(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."
Comment 9 WebKit Commit Bot 2013-10-06 21:49:16 PDT 
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.
Comment 10 EFL EWS Bot 2013-10-06 21:53:49 PDT 
Comment on attachment 213562 [details]
YARP

Attachment 213562 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3489082
Comment 11 EFL EWS Bot 2013-10-06 21:55:37 PDT 
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
Comment 12 Build Bot 2013-10-06 22:33:17 PDT 
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
Comment 13 Build Bot 2013-10-06 22:49:43 PDT 
Comment on attachment 213562 [details]
YARP

Attachment 213562 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3490097
Comment 14 Build Bot 2013-10-06 23:49:18 PDT 
Comment on attachment 213562 [details]
YARP

Attachment 213562 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3562014
Comment 15 kov's GTK+ EWS bot 2013-10-07 00:11:35 PDT 
Comment on attachment 213562 [details]
YARP

Attachment 213562 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/3514103
Comment 16 Eric Carlson 2013-10-07 08:39:40 PDT 
Created attachment 213589 [details]
Add the missing compile flag
Comment 17 WebKit Commit Bot 2013-10-07 09:18:25 PDT 
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.
Comment 18 EFL EWS Bot 2013-10-07 09:33:44 PDT 
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
Comment 19 Eric Carlson 2013-10-07 09:34:11 PDT 
Created attachment 213599 [details]
New patch to appease my style-bot master
Comment 20 EFL EWS Bot 2013-10-07 09:46:24 PDT 
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
Comment 21 Eric Carlson 2013-10-07 10:05:15 PDT 
Created attachment 213600 [details]
Updated patch

Fix path in CMakeLists.txt
Comment 22 Philippe Normand 2013-10-07 10:12:26 PDT 
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
Comment 23 Eric Carlson 2013-10-07 10:27:52 PDT 
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!
Comment 24 Eric Carlson 2013-10-07 12:33:10 PDT 
Created attachment 213612 [details]
Updated patch

Correct a typo in MockMediaStreamCenter.cpp, add updated test results.
Comment 25 Jer Noble 2013-10-07 12:51:56 PDT 
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.
Comment 26 Darin Adler 2013-10-07 16:15:52 PDT 
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.
Comment 27 Eric Carlson 2013-10-07 16:50:31 PDT 
Committed r157068: http://trac.webkit.org/changeset/157068