Bug 125243

Created attachment 218429 [details] Patch

Comment on attachment 218429 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=218429&action=review > Source/WebCore/Modules/mediastream/MediaStream.cpp:158 > + (*observer)->didAddRemoveTrack(); I am not wild about the name "didAddRemoveTrack". I would prefer to see two methods, one for add and another for remove, or maybe name like "didAddOrRemoveTrack". > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:170 > + for (auto stream = m_localStreams.begin(); stream != m_localStreams.end(); ++stream) > + (*stream)->removeObserver(this); The vector length won't change during the loop, so a more efficient way to do this would be to cache the end in the initialization: for (auto stream = m_localStreams.begin(), end = m_localStreams.end(); stream != end; ++ stream) > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:395 > bool valid = m_peerHandler->addStream(stream->privateStream(), constraints); > if (!valid) > ec = SYNTAX_ERR; > + else { > + m_localStreams.append(stream); > + stream->addObserver(this); > + } It would be good to have a test for this change. > LayoutTests/ChangeLog:13 > + * fast/mediastream/RTCPeerConnection-onnegotiationneeded.html: Don't we also need results for this test?

(In reply to comment #2) > (From update of attachment 218429 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=218429&action=review > > > Source/WebCore/Modules/mediastream/MediaStream.cpp:158 > > + (*observer)->didAddRemoveTrack(); > > I am not wild about the name "didAddRemoveTrack". I would prefer to see two methods, one for add and another for remove, or maybe name like "didAddOrRemoveTrack". Maybe two methods will be too much, I will rename it to didAddOrRemoveTrack. Is that OK to you? > > > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:170 > > + for (auto stream = m_localStreams.begin(); stream != m_localStreams.end(); ++stream) > > + (*stream)->removeObserver(this); > > The vector length won't change during the loop, so a more efficient way to do this would be to cache the end in the initialization: Makes sense > > for (auto stream = m_localStreams.begin(), end = m_localStreams.end(); stream != end; ++ stream) > > > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:395 > > bool valid = m_peerHandler->addStream(stream->privateStream(), constraints); > > if (!valid) > > ec = SYNTAX_ERR; > > + else { > > + m_localStreams.append(stream); > > + stream->addObserver(this); > > + } > > It would be good to have a test for this change. How would I test that? The observers are not accessible in javascript... and API test does that reaches it. But, in some way, the layout test that I've updated guarantees it, since negotiationneeded will only be fired if there are observers registered > > > LayoutTests/ChangeLog:13 > > + * fast/mediastream/RTCPeerConnection-onnegotiationneeded.html: > > Don't we also need results for this test? My bad, I forgot

Created attachment 218510 [details] Patch

Comment on attachment 218429 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=218429&action=review >>> Source/WebCore/Modules/mediastream/MediaStream.cpp:158 >>> + (*observer)->didAddRemoveTrack(); >> >> I am not wild about the name "didAddRemoveTrack". I would prefer to see two methods, one for add and another for remove, or maybe name like "didAddOrRemoveTrack". > > Maybe two methods will be too much, I will rename it to didAddOrRemoveTrack. Is that OK to you? That seems fine, thanks. >>> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:395 >>> + } >> >> It would be good to have a test for this change. > > How would I test that? The observers are not accessible in javascript... and API test does that reaches it. But, in some way, the layout test that I've updated guarantees it, since negotiationneeded will only be fired if there are observers registered Can't you pass an invalid constraint when there is a 'negotiationneeded' event listener registered to ensure that the event is not fired?

Comment on attachment 218510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=218510&action=review > Source/WebCore/Modules/mediastream/MediaStream.cpp:157 > + for (auto observer = m_observers.begin(); observer != m_observers.end(); ++observer) The length of this vector is also invariant: for (auto observer = m_observers.begin(), end = m_observers.end(); observer != end; ++observer) > Source/WebCore/Modules/mediastream/MediaStream.cpp:192 > + (*observer)->didAddOrRemoveTrack(); Ditto. > Source/WebCore/Modules/mediastream/MediaStream.cpp:388 > + size_t pos = m_observers.find(observer); > + if (pos == notFound) Nit: the local variable is unnecessary. > LayoutTests/fast/mediastream/RTCPeerConnection-onnegotiationneeded.html:33 > + function onNegotiationNeeded2(event) { I see that you are just following the existing style of this test, but a function's opening brace should be on a new line. > LayoutTests/fast/mediastream/RTCPeerConnection-onnegotiationneeded.html:38 > + function onNegotiationNeeded3(event) { Ditto. > LayoutTests/fast/mediastream/RTCPeerConnection-onnegotiationneeded.html:53 > + function removeTrack() { Ditto. > LayoutTests/fast/mediastream/RTCPeerConnection-onnegotiationneeded.html:61 > + function addTrack() { Ditto.

Comment on attachment 218510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=218510&action=review > Source/WebCore/Modules/mediastream/MediaStream.cpp:191 > + for (auto observer = m_observers.begin(); observer != m_observers.end(); ++observer) "Ditto" should be here, not on the next line.

(In reply to comment #5) > (From update of attachment 218429 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=218429&action=review > > >>> Source/WebCore/Modules/mediastream/MediaStream.cpp:158 > >>> + (*observer)->didAddRemoveTrack(); > >> > >> I am not wild about the name "didAddRemoveTrack". I would prefer to see two methods, one for add and another for remove, or maybe name like "didAddOrRemoveTrack". > > > > Maybe two methods will be too much, I will rename it to didAddOrRemoveTrack. Is that OK to you? > > That seems fine, thanks. > > >>> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:395 > >>> + } > >> > >> It would be good to have a test for this change. > > > > How would I test that? The observers are not accessible in javascript... and API test does that reaches it. But, in some way, the layout test that I've updated guarantees it, since negotiationneeded will only be fired if there are observers registered > > Can't you pass an invalid constraint when there is a 'negotiationneeded' event listener registered to ensure that the event is not fired? Sorry, I thought you were talking about the add/removeObserver method, I was looking at the wrong place. Yes, this can be tested, I'm updating a test to handle that

Created attachment 218522 [details] Patch

Comment on attachment 218522 [details] Patch Clearing flags on attachment: 218522 Committed r160181: <http://trac.webkit.org/changeset/160181>

All reviewed patches have been landed. Closing bug.