RESOLVED FIXED Bug 121940
[MediaStream API] update MediaStreamTrack object to match spec
https://bugs.webkit.org/show_bug.cgi?id=121940
Summary [MediaStream API] update MediaStreamTrack object to match spec
Eric Carlson
Reported 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>
Attachments
Patch for the hungry, hungry bots. (211.15 KB, patch)
2013-10-06 17:40 PDT, Eric Carlson
no flags
Rebased patch (211.00 KB, text/plain)
2013-10-06 20:38 PDT, Eric Carlson
eflews.bot: commit-queue-
YARP (209.74 KB, patch)
2013-10-06 21:15 PDT, Eric Carlson
eflews.bot: commit-queue-
Add the missing compile flag (209.43 KB, patch)
2013-10-07 08:39 PDT, Eric Carlson
eflews.bot: commit-queue-
New patch to appease my style-bot master (230.98 KB, text/plain)
2013-10-07 09:34 PDT, Eric Carlson
no flags
Updated patch (211.03 KB, patch)
2013-10-07 10:05 PDT, Eric Carlson
pnormand: review-
Updated patch (211.34 KB, patch)
2013-10-07 10:27 PDT, Eric Carlson
no flags
Updated patch (215.16 KB, patch)
2013-10-07 12:33 PDT, Eric Carlson
no flags
Eric Carlson
Comment 1 2013-10-06 17:40:42 PDT
Created attachment 213545 [details] Patch for the hungry, hungry bots.
Thiago de Barros Lacerda
Comment 2 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
Eric Carlson
Comment 3 2013-10-06 20:38:56 PDT
Created attachment 213558 [details] Rebased patch
WebKit Commit Bot
Comment 4 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.
EFL EWS Bot
Comment 5 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
EFL EWS Bot
Comment 6 2013-10-06 20:49:52 PDT
Eric Carlson
Comment 7 2013-10-06 21:15:18 PDT
Eric Carlson
Comment 8 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."
WebKit Commit Bot
Comment 9 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.
EFL EWS Bot
Comment 10 2013-10-06 21:53:49 PDT
EFL EWS Bot
Comment 11 2013-10-06 21:55:37 PDT
Build Bot
Comment 12 2013-10-06 22:33:17 PDT
Build Bot
Comment 13 2013-10-06 22:49:43 PDT
Build Bot
Comment 14 2013-10-06 23:49:18 PDT
kov's GTK+ EWS bot
Comment 15 2013-10-07 00:11:35 PDT
Eric Carlson
Comment 16 2013-10-07 08:39:40 PDT
Created attachment 213589 [details] Add the missing compile flag
WebKit Commit Bot
Comment 17 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.
EFL EWS Bot
Comment 18 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
Eric Carlson
Comment 19 2013-10-07 09:34:11 PDT
Created attachment 213599 [details] New patch to appease my style-bot master
EFL EWS Bot
Comment 20 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
Eric Carlson
Comment 21 2013-10-07 10:05:15 PDT
Created attachment 213600 [details] Updated patch Fix path in CMakeLists.txt
Philippe Normand
Comment 22 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
Eric Carlson
Comment 23 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!
Eric Carlson
Comment 24 2013-10-07 12:33:10 PDT
Created attachment 213612 [details] Updated patch Correct a typo in MockMediaStreamCenter.cpp, add updated test results.
Jer Noble
Comment 25 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.
Darin Adler
Comment 26 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.
Eric Carlson
Comment 27 2013-10-07 16:50:31 PDT
Note You need to log in before you can comment on or make changes to this bug.