WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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 attachment
proposed patch, testcase, etc.
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
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
Comment 7
2013-10-06 21:15:18 PDT
Created
attachment 213562
[details]
YARP
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
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 11
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
Build Bot
Comment 12
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
Build Bot
Comment 13
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
Build Bot
Comment 14
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
kov's GTK+ EWS bot
Comment 15
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
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
Committed
r157068
:
http://trac.webkit.org/changeset/157068
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug