Issue 2951713002: RTCPeerConnection.addTrack and removeTrack added (behind flag)

hbos_chromium

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

3 years, 5 months ago (2017-06-27 09:59:54 UTC) #1

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2951713002/100001

3 years, 5 months ago (2017-06-27 10:00:11 UTC) #2

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 5 months ago (2017-06-27 11:07:38 UTC) #3

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/458474)

3 years, 5 months ago (2017-06-27 11:07:39 UTC) #4

hbos_chromium

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

3 years, 5 months ago (2017-07-03 18:58:30 UTC) #6

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2951713002/160001

3 years, 5 months ago (2017-07-03 18:58:43 UTC) #8

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 5 months ago (2017-07-03 19:01:13 UTC) #15

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-clang/builds/130264) ios-simulator-xcode-clang on ...

3 years, 5 months ago (2017-07-03 19:01:19 UTC) #16

hbos_chromium

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

3 years, 5 months ago (2017-07-03 19:12:09 UTC) #17

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2951713002/180001

3 years, 5 months ago (2017-07-03 19:12:15 UTC) #18

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 5 months ago (2017-07-03 20:13:39 UTC) #19

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/492882)

3 years, 5 months ago (2017-07-03 20:13:40 UTC) #20

hbos_chromium

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

3 years, 5 months ago (2017-07-04 08:36:13 UTC) #23

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2951713002/260001

3 years, 5 months ago (2017-07-04 08:36:24 UTC) #24

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 5 months ago (2017-07-04 10:36:57 UTC) #25

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 5 months ago (2017-07-04 10:36:58 UTC) #26

hbos_chromium

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

3 years, 5 months ago (2017-07-04 12:21:28 UTC) #28

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2951713002/300001

3 years, 5 months ago (2017-07-04 12:21:42 UTC) #30

hbos_chromium

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

3 years, 5 months ago (2017-07-04 12:23:56 UTC) #36

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2951713002/320001

3 years, 5 months ago (2017-07-04 12:24:00 UTC) #37

hbos_chromium

Description was changed from ========== RTCPeerConnection.addTrack WIP BUG=700916, 705901 ========== to ========== RTCPeerConnection.addTrack and removeTrack ...

3 years, 5 months ago (2017-07-04 12:29:47 UTC) #38

hbos_chromium

hbos@chromium.org changed reviewers: + guidou@chromium.org

3 years, 5 months ago (2017-07-04 12:29:47 UTC) #39

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 5 months ago (2017-07-04 14:24:02 UTC) #41

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 5 months ago (2017-07-04 14:24:03 UTC) #42

Guido Urdaneta

Looking good. Havent't looked at the tests yet, but here are a few nits and ...

3 years, 5 months ago (2017-07-05 15:37:06 UTC) #43

Guido Urdaneta

https://codereview.chromium.org/2951713002/diff/320001/chrome/test/data/webrtc/peerconnection_rtp.js File chrome/test/data/webrtc/peerconnection_rtp.js (right): https://codereview.chromium.org/2951713002/diff/320001/chrome/test/data/webrtc/peerconnection_rtp.js#newcode133 chrome/test/data/webrtc/peerconnection_rtp.js:133: let audioSender = (!audioStream) why "(!audioStream)" and not just ...

3 years, 5 months ago (2017-07-06 09:49:57 UTC) #44

hbos_chromium

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

3 years, 5 months ago (2017-07-06 12:22:12 UTC) #45

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2951713002/330001

3 years, 5 months ago (2017-07-06 12:22:17 UTC) #46

hbos_chromium

hbos@chromium.org changed reviewers: + deadbeef@chromium.org

3 years, 5 months ago (2017-07-06 12:31:36 UTC) #47

hbos_chromium

Please take a look, deadbeef. And PTAL guidou. deadbeef for test coverage. There are two ...

3 years, 5 months ago (2017-07-06 12:31:38 UTC) #48

Please take a look, deadbeef. And PTAL guidou.

deadbeef for test coverage. There are two types of tests, browser
tests and layout tests. The browser tests run the whole stack, setting
up a call between two tabs. The layout tests uses a mock peer
connection handler, meaning it can test that exceptions are or are not
thrown but is limited in that a mock is used for the lower layer
functionality.

browser_tests related:
  chrome/browser/media/webrtc/webrtc_browsertest_base.h
  chrome/browser/media/webrtc/webrtc_browsertest_base.cc
  chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc
  chrome/test/data/webrtc/munge_sdp.js
  chrome/test/data/webrtc/peerconnection_rtp.js

LayoutTests related:
  content/shell/test_runner/mock_webrtc_peer_connection_handler.h
  content/shell/test_runner/mock_webrtc_peer_connection_handler.cc

third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-AddRemoveTrack.html

third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getSenders.html

https://codereview.chromium.org/2951713002/diff/320001/chrome/test/data/webrt...
File chrome/test/data/webrtc/peerconnection_rtp.js (right):

https://codereview.chromium.org/2951713002/diff/320001/chrome/test/data/webrt...
chrome/test/data/webrtc/peerconnection_rtp.js:133: let audioSender =
(!audioStream)
On 2017/07/06 09:49:57, Guido Urdaneta wrote:
> why "(!audioStream)" and not just "audioStream" ?

Done.

https://codereview.chromium.org/2951713002/diff/320001/chrome/test/data/webrt...
chrome/test/data/webrtc/peerconnection_rtp.js:148: let videoSender =
(!videoStream)
On 2017/07/06 09:49:57, Guido Urdaneta wrote:
> ditto

Donno.

https://codereview.chromium.org/2951713002/diff/320001/content/renderer/media...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/2951713002/diff/320001/content/renderer/media...
content/renderer/media/rtc_peer_connection_handler.cc:1718:
rtc::scoped_refptr<webrtc::RtpSenderInterface> webrtc_sender =
On 2017/07/05 15:37:06, Guido Urdaneta wrote:
> why rtc::scoped_refptr here, but ::scoped_refptr when passed to RTCRtpSender? 

It doesn't matter which one you use, they both do the same thing. I thought I'd
use ::scoped_refptr because RTCRtpSender is in content, but considering it's
only holding on to a webrtc reference I changed it to use rtc::scoped_refptr to
allow for std::move and not have this confusing mix.

https://codereview.chromium.org/2951713002/diff/320001/content/renderer/media...
File content/renderer/media/webrtc/rtc_rtp_sender.cc (right):

https://codereview.chromium.org/2951713002/diff/320001/content/renderer/media...
content/renderer/media/webrtc/rtc_rtp_sender.cc:68: // not allowed on a stopped
sender.
On 2017/07/05 15:37:06, Guido Urdaneta wrote:
> Include a webrtc bug reference.

Done.

https://codereview.chromium.org/2951713002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-AddRemoveTrack.html
(right):

https://codereview.chromium.org/2951713002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-AddRemoveTrack.html:28:
let sender = pc.addTrack(track, stream);
On 2017/07/06 09:49:57, Guido Urdaneta wrote:
> why does this test produce the same result as the previous test.
> what difference should passing |stream| to addTrack() make?

The difference can be observed in the SDP, but this is not available due to
mocking.

There is a proposal to make getLocalStreams() return streams that are part of
senders, which would include addTrack streams (non-addStream streams) and give
an observable result here.
Added bug reference https://crbug.com/738918

https://codereview.chromium.org/2951713002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-AddRemoveTrack.html:41:
let sender = pc.addTrack(trackA, streamB);
On 2017/07/06 09:49:57, Guido Urdaneta wrote:
> Same here, should passing |streamB| have an observable effect?

Ditto.

https://codereview.chromium.org/2951713002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-AddRemoveTrack.html:60:
// The spec supports multiple streams per track but our implementation
On 2017/07/06 09:49:57, Guido Urdaneta wrote:
> Add a TODO to fix this test with reference to the corresponding bug.

Done.

https://codereview.chromium.org/2951713002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-AddRemoveTrack.html:121:
* Helper functions to tests.
On 2017/07/06 09:49:57, Guido Urdaneta wrote:
> remove " to tests"

Done.

https://codereview.chromium.org/2951713002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2951713002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:1279:
RTCRtpSender* rtp_sender = new RTCRtpSender(std::move(web_rtp_sender), track);
On 2017/07/05 15:37:06, Guido Urdaneta wrote:
> nit: I think the preferred style in Blink is to have the constructor private
and
> add a static create method to ensure that the object is always created in the
> Oilpan heap.
> Check code from other modules to be sure if that's actually the norm.

Yes, done. I didn't make it Create because unlike the other Create this is not a
WebIDL constructor, I didn't think about forcing it on the Oilpan heap and other
code does in fact use Create for all constructors, even ones not defined in
WebIDL.

https://codereview.chromium.org/2951713002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:1287:
DCHECK(sender);
On 2017/07/05 15:37:06, Guido Urdaneta wrote:
> Is this DCHECK right? What prevents the application from sending null here?

Yes, it's non-optional in the IDL so null should already have thrown an
exception for us.

https://codereview.chromium.org/2951713002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:1300: //
Note: until re-using senders is supported these are also removed by
On 2017/07/05 15:37:06, Guido Urdaneta wrote:
> Is reusing senders not supported by the WebRTC library? If so, indicate it
here.
> From the comment it's not clear what layer has the limitation.

Yes I was referring to the WebRTC library limitations, comment updated.

https://codereview.chromium.org/2951713002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
(right):

https://codereview.chromium.org/2951713002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl:116: 
On 2017/07/05 15:37:06, Guido Urdaneta wrote:
> nit: remove this blank line

Done.

https://codereview.chromium.org/2951713002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/peerconnection/RTCRtpSender.cpp (right):

https://codereview.chromium.org/2951713002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCRtpSender.cpp:16: return
web_track && web_track->Id() == static_cast<WebString>(track->id());
On 2017/07/05 15:37:06, Guido Urdaneta wrote:
> How does this static_cast actually work?
> 
> Shouldn't you just use WebString(track->id())?

Done.
I had to do some sort of casting because == is defined for both String and
WebString. Explicitly calling the constructor is clearer what happens, instead
of relying on static_cast to choose the operator to use, which may include both
implicit single argument constructors and operator T functions.

Guido Urdaneta

lgtm % nit, but wait for deadbeef@'s review https://codereview.chromium.org/2951713002/diff/320001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2951713002/diff/320001/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp#newcode1300 third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:1300: // ...

3 years, 5 months ago (2017-07-06 12:43:42 UTC) #49

hbos_chromium

hbos@chromium.org changed reviewers: + jochen@chromium.org

3 years, 5 months ago (2017-07-06 12:50:46 UTC) #50

hbos_chromium

Please take a look jochen for content/shell/test_runner/mock_webrtc_peer_connection_handler.cc content/shell/test_runner/mock_webrtc_peer_connection_handler.h third_party/WebKit/Source/platform/testing/TestingPlatformSupportWithWebRTC.cpp third_party/WebKit/Source/platform/testing/TestingPlatformSupportWithWebRTC.h third_party/WebKit/public/platform/WebRTCPeerConnectionHandler.h

3 years, 5 months ago (2017-07-06 12:50:47 UTC) #51

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 5 months ago (2017-07-06 16:40:44 UTC) #54

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 5 months ago (2017-07-06 16:40:45 UTC) #55

Taylor_Brandstetter

Reviewed tests https://codereview.chromium.org/2951713002/diff/330001/chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc File chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc (right): https://codereview.chromium.org/2951713002/diff/330001/chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc#newcode82 chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc:82: EXPECT_NE("null", audio_stream_id); I think we should fix ...

3 years, 5 months ago (2017-07-06 22:45:15 UTC) #56

hbos_chromium

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

3 years, 5 months ago (2017-07-07 12:05:37 UTC) #57

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2951713002/350001

3 years, 5 months ago (2017-07-07 12:05:47 UTC) #58

hbos_chromium

PTAL deadbeef https://codereview.chromium.org/2951713002/diff/330001/chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc File chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc (right): https://codereview.chromium.org/2951713002/diff/330001/chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc#newcode82 chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc:82: EXPECT_NE("null", audio_stream_id); On 2017/07/06 22:45:14, Taylor_Brandstetter wrote: ...

3 years, 5 months ago (2017-07-07 12:07:48 UTC) #59

PTAL deadbeef

https://codereview.chromium.org/2951713002/diff/330001/chrome/browser/media/w...
File chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc (right):

https://codereview.chromium.org/2951713002/diff/330001/chrome/browser/media/w...
chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc:82: EXPECT_NE("null",
audio_stream_id);
On 2017/07/06 22:45:14, Taylor_Brandstetter wrote:
> I think we should fix this before we ship the feature (meaning, if you add a
> track without a stream, no stream should get generated).

SGTM. Will fix before unflagging. Added a TODO.

The EXPECT_NE changed to EXPECT_EQ because now that I don't parse the SDP I
don't know the resulting stream. Test updated to test both sender/receiver and
local/remote streams.

https://codereview.chromium.org/2951713002/diff/330001/chrome/test/data/webrt...
File chrome/test/data/webrtc/munge_sdp.js (right):

https://codereview.chromium.org/2951713002/diff/330001/chrome/test/data/webrt...
chrome/test/data/webrtc/munge_sdp.js:15: function getSdpStreamsAndTracks(sdp) {
On 2017/07/06 22:45:14, Taylor_Brandstetter wrote:
> Relying on SDP parsing in this test is somewhat fragile (it will break when we
> switch to "a=msid"); would it be possible to get the track/stream
relationships
> simply by seeing what remote streams are created on the "right tab" after
> setting the remote description?

Good point. Done. Removed SDP usage from test, and added a test to check if
there is a remote stream containing a track with specified ids.

For the addTrack without a stream case the stream id is unknown without sdp
parsing, but we can still verify that some stream contain the track (before
addTrack with no stream is fixed not to create streams), and in both cases we
can make sure there is a receiver for the track.

https://codereview.chromium.org/2951713002/diff/330001/chrome/test/data/webrt...
File chrome/test/data/webrtc/peerconnection_rtp.js (right):

https://codereview.chromium.org/2951713002/diff/330001/chrome/test/data/webrt...
chrome/test/data/webrtc/peerconnection_rtp.js:137: throw
failTest('audioSender.track !== audioTrack');
On 2017/07/06 22:45:14, Taylor_Brandstetter wrote:
> I don't really like mixing test assertions with helper methods, unless it's
> obvious what assertions the helper method will be making.

The bridge between C++ tests and JavaScript is not very nice, I can only return
strings from here. I could return sender.track.id and parse it in C++ to do
EXPECT_EQ(sender_track_id, track_id) but with the hassle of returning things
from javascript it seems a bit silly to return two track IDs that can never be
different unless there is an unlikely bug.

As for the other assertions, I could verify that there is a sender for the track
in a separate function call: EXPECT_TRUE(hasSenderWithTrackId(left_tab_,
track_id)). Just like we now on the remote side do
EXPECT_TRUE(hasReceiverWithTrack(right_tab_, track_id)).

I've reduced but not eliminated the number of assertions. Is that ok, otherwise
what do you propose?

https://codereview.chromium.org/2951713002/diff/330001/chrome/test/data/webrt...
chrome/test/data/webrtc/peerconnection_rtp.js:139:
peerConnection_().onnegotiationneeded = function() {
On 2017/07/06 22:45:14, Taylor_Brandstetter wrote:
> Why does this have to be done in onnegotiationneeded?

It doesn't, I just wanted to verify that it's called. Added a counter separately
and verify that the count is what we expect in a follow-up call in C++ to JS.

https://codereview.chromium.org/2951713002/diff/330001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-AddRemoveTrack.html
(right):

https://codereview.chromium.org/2951713002/diff/330001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-AddRemoveTrack.html:107:
}, 'addTrack() after addStream() throws InvalidAccessError.');
On 2017/07/06 22:45:14, Taylor_Brandstetter wrote:
> May be helpful to call out the fact that the interactions between addTrack and
> addStream aren't standardized, since addStream no longer exists in the
standard.

Done.

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 5 months ago (2017-07-07 14:17:34 UTC) #60

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 5 months ago (2017-07-07 14:17:35 UTC) #61

Taylor_Brandstetter

lgtm with some minor comments about negotiationneeded https://codereview.chromium.org/2951713002/diff/330001/chrome/test/data/webrtc/peerconnection_rtp.js File chrome/test/data/webrtc/peerconnection_rtp.js (right): https://codereview.chromium.org/2951713002/diff/330001/chrome/test/data/webrtc/peerconnection_rtp.js#newcode137 chrome/test/data/webrtc/peerconnection_rtp.js:137: throw failTest('audioSender.track ...

3 years, 5 months ago (2017-07-07 19:44:14 UTC) #62

lgtm with some minor comments about negotiationneeded

https://codereview.chromium.org/2951713002/diff/330001/chrome/test/data/webrt...
File chrome/test/data/webrtc/peerconnection_rtp.js (right):

https://codereview.chromium.org/2951713002/diff/330001/chrome/test/data/webrt...
chrome/test/data/webrtc/peerconnection_rtp.js:137: throw
failTest('audioSender.track !== audioTrack');
On 2017/07/07 12:07:48, hbos_chromium wrote:
> On 2017/07/06 22:45:14, Taylor_Brandstetter wrote:
> > I don't really like mixing test assertions with helper methods, unless it's
> > obvious what assertions the helper method will be making.
> 
> The bridge between C++ tests and JavaScript is not very nice, I can only
return
> strings from here. I could return sender.track.id and parse it in C++ to do
> EXPECT_EQ(sender_track_id, track_id) but with the hassle of returning things
> from javascript it seems a bit silly to return two track IDs that can never be
> different unless there is an unlikely bug.
> 
> As for the other assertions, I could verify that there is a sender for the
track
> in a separate function call: EXPECT_TRUE(hasSenderWithTrackId(left_tab_,
> track_id)). Just like we now on the remote side do
> EXPECT_TRUE(hasReceiverWithTrack(right_tab_, track_id)).
> 
> I've reduced but not eliminated the number of assertions. Is that ok,
otherwise
> what do you propose?

This seems ok to me.

https://codereview.chromium.org/2951713002/diff/350001/chrome/browser/media/w...
File chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc (right):

https://codereview.chromium.org/2951713002/diff/350001/chrome/browser/media/w...
chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc:84:
WaitUntilOnNegotiationCountIs(left_tab_, 2);
Actually, if we followed the standard, negotiationneeded would only fire once.
It's only supposed to fire if the flag flips from "false" to "true", or if it's
still "true" after an offer/answer finishes. Should leave a TODO here.

https://codereview.chromium.org/2951713002/diff/350001/chrome/browser/media/w...
chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc:289: }
I like this change; it's much easier to see what the tests are testing by
reading this code by itself.

It still may be a good idea to move the "negotiationneeded" testing and
"removeTrack" testing into standalone tests, though. I'll let you decide if you
want to do that.

https://codereview.chromium.org/2951713002/diff/350001/chrome/test/data/webrt...
File chrome/test/data/webrtc/peerconnection_rtp.js (right):

https://codereview.chromium.org/2951713002/diff/350001/chrome/test/data/webrt...
chrome/test/data/webrtc/peerconnection_rtp.js:229: function
startCountingOnNegotiationNeeded() {
I don't think you really need a "start counting" method; you could just always
count, and have the test take the difference between the count at two points.

https://codereview.chromium.org/2951713002/diff/350001/chrome/test/data/webrt...
chrome/test/data/webrtc/peerconnection_rtp.js:247: window.setTimeout(checkCount,
10);
Does this really need a timeout, even if it's only 10 milliseconds? In theory,
addTrack will queue a task to fire "negotiationneeded". So as long as this task
is added to the end of the queue, it should be executed after the task that
fires the event. At least, if we follow the spec in that regard....

hbos_chromium

https://codereview.chromium.org/2951713002/diff/350001/chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc File chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc (right): https://codereview.chromium.org/2951713002/diff/350001/chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc#newcode84 chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc:84: WaitUntilOnNegotiationCountIs(left_tab_, 2); On 2017/07/07 19:44:13, Taylor_Brandstetter wrote: > Actually, ...

3 years, 5 months ago (2017-07-10 12:32:59 UTC) #63

https://codereview.chromium.org/2951713002/diff/350001/chrome/browser/media/w...
File chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc (right):

https://codereview.chromium.org/2951713002/diff/350001/chrome/browser/media/w...
chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc:84:
WaitUntilOnNegotiationCountIs(left_tab_, 2);
On 2017/07/07 19:44:13, Taylor_Brandstetter wrote:
> Actually, if we followed the standard, negotiationneeded would only fire once.
> It's only supposed to fire if the flag flips from "false" to "true", or if
it's
> still "true" after an offer/answer finishes. Should leave a TODO here.

Done, filed an issue, this is a bug that should already be affecting addStream
etc.

https://codereview.chromium.org/2951713002/diff/350001/chrome/browser/media/w...
chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc:289: }
On 2017/07/07 19:44:13, Taylor_Brandstetter wrote:
> I like this change; it's much easier to see what the tests are testing by
> reading this code by itself.
> 
> It still may be a good idea to move the "negotiationneeded" testing and
> "removeTrack" testing into standalone tests, though. I'll let you decide if
you
> want to do that.

I'd like that except the setup for the remove track tests would be identical to
all of the addtrack tests and I'd like to avoid helper functions that basically
contain an entire test or copying this many lines of code in multiple tests.

https://codereview.chromium.org/2951713002/diff/350001/chrome/test/data/webrt...
File chrome/test/data/webrtc/peerconnection_rtp.js (right):

https://codereview.chromium.org/2951713002/diff/350001/chrome/test/data/webrt...
chrome/test/data/webrtc/peerconnection_rtp.js:229: function
startCountingOnNegotiationNeeded() {
On 2017/07/07 19:44:14, Taylor_Brandstetter wrote:
> I don't think you really need a "start counting" method; you could just always
> count, and have the test take the difference between the count at two points.

I moved this to peerconnection.js where it always sets up the counting as part
of creating the pc and moved all this counter stuff to the same file.

I changed it to a count getter instead.

https://codereview.chromium.org/2951713002/diff/350001/chrome/test/data/webrt...
chrome/test/data/webrtc/peerconnection_rtp.js:247: window.setTimeout(checkCount,
10);
On 2017/07/07 19:44:13, Taylor_Brandstetter wrote:
> Does this really need a timeout, even if it's only 10 milliseconds? In theory,
> addTrack will queue a task to fire "negotiationneeded". So as long as this
task
> is added to the end of the queue, it should be executed after the task that
> fires the event. At least, if we follow the spec in that regard....

Done, changed to a getter instead of a "wait" and doing a single
window.setTimeout(..., 0) to make sure any queued events are included in the
result / avoid a race.

hbos_chromium

The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org, jochen@chromium.org, deadbeef@chromium.org Link ...

3 years, 5 months ago (2017-07-10 12:33:10 UTC) #65

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2951713002/370001

3 years, 5 months ago (2017-07-10 12:33:21 UTC) #66

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 370001, "attempt_start_ts": 1499689989423580, "parent_rev": "7f5d938ae2e6f0dda26b4fdc4564dbc8f466f5c3", "commit_rev": "93aa0e383f74ed32072fedd74baf7a486dba5702"}

3 years, 5 months ago (2017-07-10 14:32:29 UTC) #67

commit-bot: I haz the power

Description was changed from ========== RTCPeerConnection.addTrack and removeTrack added (behind flag). This allows a track ...

3 years, 5 months ago (2017-07-10 14:32:43 UTC) #68

commit-bot: I haz the power

3 years, 5 months ago (2017-07-10 14:32:45 UTC) #69

Message was sent while issue was closed.

Committed patchset #4 (id:370001) as
https://chromium.googlesource.com/chromium/src/+/93aa0e383f74ed32072fedd74baf...

Issue 2951713002: RTCPeerConnection.addTrack and removeTrack added (behind flag) (Closed)

Description

Patch Set 1 #

Patch Set 2 : Addressed guidou's comments #

Patch Set 3 : Addressed deadbeef's comments #

Patch Set 4 : Addressed deadbeef's comments #

Messages