Summary: | [MediaStream API] update MediaStreamTrack object to match spec | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||||||||||||||
Component: | WebCore Misc. | Assignee: | Eric Carlson <eric.carlson> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | buildbot, cdumez, commit-queue, dino, eflews.bot, esprehn+autocc, glenn, gtk-ews, gyuyoung.kim, hta, hugo.lima, jer.noble, kangil.han, kondapallykalyan, philn, rakuco, rniwa, thiago.lacerda, tommyw, xan.lopez | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||
Bug Blocks: | 124288 | ||||||||||||||||||||
Attachments: |
|
Description
Eric Carlson
2013-09-25 22:00:45 PDT
Created attachment 213545 [details]
Patch for the hungry, hungry bots.
(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 Created attachment 213558 [details]
Rebased patch
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 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 on attachment 213558 [details] Rebased patch Attachment 213558 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/3514090 Created attachment 213562 [details]
YARP
(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." 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 on attachment 213562 [details] YARP Attachment 213562 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/3489082 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 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 on attachment 213562 [details] YARP Attachment 213562 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/3490097 Comment on attachment 213562 [details] YARP Attachment 213562 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/3562014 Comment on attachment 213562 [details] YARP Attachment 213562 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/3514103 Created attachment 213589 [details]
Add the missing compile flag
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 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 Created attachment 213599 [details]
New patch to appease my style-bot master
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 Created attachment 213600 [details]
Updated patch
Fix path in CMakeLists.txt
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 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! Created attachment 213612 [details]
Updated patch
Correct a typo in MockMediaStreamCenter.cpp, add updated test results.
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 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. Committed r157068: http://trac.webkit.org/changeset/157068 |