Drop [UsePointersEvenForNonNullableObjectArguments] from MediaStream interfaces
Created attachment 277047 [details] Patch
Comment on attachment 277047 [details] Patch Attachment 277047 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1201762 New failing tests: http/tests/media/media-source/mediasource-append-buffer.html
Created attachment 277052 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 277047 [details] Patch Attachment 277047 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1201769 New failing tests: http/tests/media/media-source/mediasource-append-buffer.html
Created attachment 277053 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 277055 [details] Removing not related test
Comment on attachment 277055 [details] Removing not related test View in context: https://bugs.webkit.org/attachment.cgi?id=277055&action=review > Source/WebCore/Modules/mediastream/MediaStream.cpp:129 > +void MediaStream::addTrack(MediaStreamTrack& track) Should probably be a Ref<MediaStreamTrack>&& since we pass ownership. > Source/WebCore/Modules/mediastream/MediaStream.cpp:209 > +bool MediaStream::internalAddTrack(MediaStreamTrack& track, StreamModifier streamModifier) Should probably be a Ref<MediaStreamTrack>&& > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:104 > +RefPtr<RTCRtpSender> RTCPeerConnection::addTrack(MediaStreamTrack& track, Vector<MediaStream*> streams, ExceptionCode& ec) Should probably be a Ref<>&& > Source/WebCore/Modules/mediastream/RTCRtpReceiver.h:42 > + static Ref<RTCRtpReceiver> create(MediaStreamTrack& track) Should probably be a Ref<>&& > Source/WebCore/Modules/mediastream/RTCRtpReceiver.h:48 > + explicit RTCRtpReceiver(MediaStreamTrack&); Ditto > Source/WebCore/Modules/mediastream/RTCRtpSender.h:51 > + static Ref<RTCRtpSender> create(MediaStreamTrack& track, Vector<String>&& mediaStreamIds, RTCRtpSenderClient& client) Should probably be a Ref<>&& > Source/WebCore/Modules/mediastream/RTCRtpSender.h:64 > + RTCRtpSender(MediaStreamTrack&, Vector<String>&& mediaStreamIds, RTCRtpSenderClient&); ditto. > Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.h:49 > + MediaStreamTrack& track() { return m_track.get(); } No need for the .get() > Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.h:-52 > - RTCRtpSenderReceiverBase(RefPtr<MediaStreamTrack>&& track) Ref<>&& > LayoutTests/fast/mediastream/MediaStream-add-remove-tracks.html:105 > + tryThrowing(function() { stream1.addTrack(null); }); Can we use the pre-existing shouldThrow() rather than writing our own? > LayoutTests/fast/mediastream/RTCPeerConnection-add-removeTrack.html:8 > + function tryThrowing(callback) { ditto. > LayoutTests/fast/mediastream/RTCPeerConnection-datachannel.html:10 > + function tryThrowing(callback) { ditto.
Created attachment 277242 [details] Patch
Thanks for the review, I updated the patch accordingly.
Created attachment 277243 [details] Patch
Comment on attachment 277243 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277243&action=review > Source/WebCore/Modules/mediastream/MediaStream.cpp:199 > + internalAddTrack(MediaStreamTrack::create(*context, trackPrivate).get(), StreamModifier::Platform); I don't believe you need the .get() here. > Source/WebCore/Modules/mediastream/MediaStream.h:135 > + // FIXME: m_trackSet should take Ref<MediaStreamTrack>. We don't support this for HashMaps.. > Source/WebCore/Modules/mediastream/RTCRtpReceiver.cpp:38 > +RTCRtpReceiver::RTCRtpReceiver(MediaStreamTrack& track) Should be a Ref<>&& > Source/WebCore/Modules/mediastream/RTCRtpReceiver.h:42 > + static Ref<RTCRtpReceiver> create(MediaStreamTrack& track) Should be a Ref<>&& > Source/WebCore/Modules/mediastream/RTCRtpReceiver.h:48 > + explicit RTCRtpReceiver(MediaStreamTrack&); should be a Ref<>&& > LayoutTests/fast/mediastream/MediaStream-add-remove-tracks-expected.txt:60 > +FAIL stream2.active should be true. Was false. New FAIL? > LayoutTests/fast/mediastream/MediaStream-add-remove-tracks-expected.txt:63 > +FAIL stream2.active should be true. Was false. New FAIL?
Created attachment 277361 [details] Patch
Created attachment 277362 [details] Patch
Thanks for the review, I updated accordingly. I also made addTrack take a ref and not a Ref<>, following on bug 156901 discussion. > > Source/WebCore/Modules/mediastream/MediaStream.cpp:199 > > + internalAddTrack(MediaStreamTrack::create(*context, trackPrivate).get(), StreamModifier::Platform); > > I don't believe you need the .get() here. Fixed. > > Source/WebCore/Modules/mediastream/MediaStream.h:135 > > + // FIXME: m_trackSet should take Ref<MediaStreamTrack>. > > We don't support this for HashMaps.. Do you know the reason? > > Source/WebCore/Modules/mediastream/RTCRtpReceiver.cpp:38 > > +RTCRtpReceiver::RTCRtpReceiver(MediaStreamTrack& track) > > Should be a Ref<>&& Fixed. > > Source/WebCore/Modules/mediastream/RTCRtpReceiver.h:42 > > + static Ref<RTCRtpReceiver> create(MediaStreamTrack& track) > > Should be a Ref<>&& Fixed. > > Source/WebCore/Modules/mediastream/RTCRtpReceiver.h:48 > > + explicit RTCRtpReceiver(MediaStreamTrack&); > > should be a Ref<>&& Fixed. > > LayoutTests/fast/mediastream/MediaStream-add-remove-tracks-expected.txt:60 > > +FAIL stream2.active should be true. Was false. > > New FAIL? This is unrelated to this patch changes. This test is skipped so it does not get rebased. In the new patch, the testing of null/undefined gets its own test file. > > LayoutTests/fast/mediastream/MediaStream-add-remove-tracks-expected.txt:63 > > +FAIL stream2.active should be true. Was false. > > New FAIL? Ditto.
Comment on attachment 277362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277362&action=review Change is generally quite good, but there is a possible lifetime bug introduced in MediaStream::internalRemoveTrack that we should not land. > Source/WebCore/Modules/mediastream/MediaStream.cpp:206 > - RefPtr<MediaStreamTrack> track = getTrackById(trackPrivate.id()); > + MediaStreamTrack* track = getTrackById(trackPrivate.id()); > ASSERT(track); > - internalRemoveTrack(WTFMove(track), StreamModifier::Platform); > + internalRemoveTrack(*track, StreamModifier::Platform); This is an inefficient algorithm; looks up the track by ID once here, then calls internalRemoveTrack which does the same thing again. Given how it works, the internalRemoveTrack function should take a track ID as its argument rather than taking a track. > Source/WebCore/Modules/mediastream/MediaStream.cpp:216 > + MediaStreamTrack& track = *result.iterator->value; I would have used auto& here. > Source/WebCore/Modules/mediastream/MediaStream.cpp:232 > + track.removeObserver(this); I believe that if the track is not in a Ref or RefPtr, it could have been deleted by the call to remove above; that might have been the last reference to the track. So this change may introduce a bug. I think we do need a Ref inside this function even though the argument doesn’t need to be a Ref. The cleanest way to do this would be to use HashMap::take rather than HashMap::remove in the implementation of this function. That would be a natural thing to do if we changed it to take a track ID as its argument rather than taking a track. > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:224 > +void RTCDataChannel::send(ArrayBufferView* data, ExceptionCode& ec) Why does this function take a pointer instead of a reference? > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:229 > RefPtr<ArrayBuffer> arrayBuffer(data->buffer()); > - send(arrayBuffer.release(), ec); > + ASSERT(arrayBuffer); > + send(*arrayBuffer, ec); There is no reason for this local variable. Here’s what I would write: ASSERT(data.buffer()); send(*data.buffer(), ec); But also, not sure why that assertion is safe. If that function can ever return nullptr, then it’s an incorrect assertion. If it can’t return nullptr, then it should return a & or Ref, not RefPtr. > Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.h:49 > + MediaStreamTrack& track() { return m_track; } Should remove the extra space here after the "{".
Comment on attachment 277362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277362&action=review > Source/WebCore/Modules/mediastream/MediaStream.cpp:129 > +void MediaStream::addTrack(MediaStreamTrack& track) I still don't understand why we wouldn't use Ref<>&& here. The caller is clearly passing ownership and internalAddTrack() already takes a Ref<>&&. We anyway move the Ref<> around so there is no ref-counting churn as far as I can tell.
Comment on attachment 277362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277362&action=review >> Source/WebCore/Modules/mediastream/MediaStream.cpp:129 >> +void MediaStream::addTrack(MediaStreamTrack& track) > > I still don't understand why we wouldn't use Ref<>&& here. The caller is clearly passing ownership and internalAddTrack() already takes a Ref<>&&. We anyway move the Ref<> around so there is no ref-counting churn as far as I can tell. Lets discuss this in person at some point. Really either way is OK with me but I see two issues: 1) There’s not a good reason to put the reference counting at the call site of addTrack inside the bindings rather than inside the function. And that’s what using Ref&& does. 2) I think we generally should prefer standard references to rvalue references unless the move semantic is an important optimization. I don’t think we should always do it any time we pass a reference counted object even if the semantic is passing ownership. The way you’d see this in the past is massive over-use of PassRefPtr. But I suppose that point (2) is debatable.
Thanks for the review. Please see inline for more comments. > > Source/WebCore/Modules/mediastream/MediaStream.cpp:206 > > - RefPtr<MediaStreamTrack> track = getTrackById(trackPrivate.id()); > > + MediaStreamTrack* track = getTrackById(trackPrivate.id()); > > ASSERT(track); > > - internalRemoveTrack(WTFMove(track), StreamModifier::Platform); > > + internalRemoveTrack(*track, StreamModifier::Platform); > > This is an inefficient algorithm; looks up the track by ID once here, then > calls internalRemoveTrack which does the same thing again. Given how it > works, the internalRemoveTrack function should take a track ID as its > argument rather than taking a track. OK. > > Source/WebCore/Modules/mediastream/MediaStream.cpp:216 > > + MediaStreamTrack& track = *result.iterator->value; > > I would have used auto& here. OK. > > Source/WebCore/Modules/mediastream/MediaStream.cpp:232 > > + track.removeObserver(this); > > I believe that if the track is not in a Ref or RefPtr, it could have been > deleted by the call to remove above; that might have been the last reference > to the track. So this change may introduce a bug. I think we do need a Ref > inside this function even though the argument doesn’t need to be a Ref. The > cleanest way to do this would be to use HashMap::take rather than > HashMap::remove in the implementation of this function. That would be a > natural thing to do if we changed it to take a track ID as its argument > rather than taking a track. Right, take sounds good. > > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:224 > > +void RTCDataChannel::send(ArrayBufferView* data, ExceptionCode& ec) > > Why does this function take a pointer instead of a reference? Binding generator limitation. > > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:229 > > RefPtr<ArrayBuffer> arrayBuffer(data->buffer()); > > - send(arrayBuffer.release(), ec); > > + ASSERT(arrayBuffer); > > + send(*arrayBuffer, ec); > > There is no reason for this local variable. Here’s what I would write: > > ASSERT(data.buffer()); > send(*data.buffer(), ec); ArrayBufferView::buffer() is not const. It may for instance x that is why I did not want to call it twice. > But also, not sure why that assertion is safe. If that function can ever > return nullptr, then it’s an incorrect assertion. If it can’t return > nullptr, then it should return a & or Ref, not RefPtr. There is a theoretical case where it can returns null (ArrayBufferView > > > Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.h:49 > > + MediaStreamTrack& track() { return m_track; } > > Should remove the extra space here after the "{". OK
> > ASSERT(data.buffer()); > > send(*data.buffer(), ec); > > ArrayBufferView::buffer() is not const. > It may for instance x that is why I did not want to call it twice. It may call for instance slowDownAndWasteMemory. It probably does not have any side-effect calling it twice in debug but I thought it would be better to call it just once. > > But also, not sure why that assertion is safe. If that function can ever > > return nullptr, then it’s an incorrect assertion. If it can’t return > > nullptr, then it should return a & or Ref, not RefPtr. > > There is a theoretical case where it can returns null (ArrayBufferView ... in case ArrayBufferView is a JSDataView. That said, I am not familiar with these classes.
Created attachment 277465 [details] Patch
(In reply to comment #20) > Created attachment 277465 [details] > Patch I kept ASSERT() the same as the previous patch.
Comment on attachment 277465 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277465&action=review > Source/WebCore/Modules/mediastream/MediaStream.cpp:98 > + Ref<MediaStreamTrack> track = MediaStreamTrack::create(context, *trackPrivate); I would have used auto here rather than repeat the class name twice. > Source/WebCore/Modules/mediastream/MediaStream.cpp:227 > + RefPtr<MediaStreamTrack> track = m_trackSet.take(trackId); I would have used auto here rather than naming the type explicitly. > Source/WebCore/Modules/mediastream/MediaStream.cpp:236 > - dispatchEvent(MediaStreamTrackEvent::create(eventNames().removetrackEvent, false, false, WTFMove(track))); > + dispatchEvent(MediaStreamTrackEvent::create(eventNames().removetrackEvent, false, false, track)); Why would we make this change? Still seems like an optimization to use WTFMove here.
Created attachment 277674 [details] Patch for landing
(In reply to comment #22) > Comment on attachment 277465 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277465&action=review > > > Source/WebCore/Modules/mediastream/MediaStream.cpp:98 > > + Ref<MediaStreamTrack> track = MediaStreamTrack::create(context, *trackPrivate); > > I would have used auto here rather than repeat the class name twice. I changed it. Fact is we loose that this is a Ref<> and not a RefPtr<>. > > > Source/WebCore/Modules/mediastream/MediaStream.cpp:227 > > + RefPtr<MediaStreamTrack> track = m_trackSet.take(trackId); > > I would have used auto here rather than naming the type explicitly. OK > > Source/WebCore/Modules/mediastream/MediaStream.cpp:236 > > - dispatchEvent(MediaStreamTrackEvent::create(eventNames().removetrackEvent, false, false, WTFMove(track))); > > + dispatchEvent(MediaStreamTrackEvent::create(eventNames().removetrackEvent, false, false, track)); > > Why would we make this change? Still seems like an optimization to use > WTFMove here. Right, fixed in landing patch.
Comment on attachment 277674 [details] Patch for landing Clearing flags on attachment: 277674 Committed r200231: <http://trac.webkit.org/changeset/200231>
All reviewed patches have been landed. Closing bug.