RTCRtpSender with track behind RuntimeEnabled flag

RTCRtpSender with track behind RuntimeEnabled flag ("RTCRtpSender")

Implements getSenders() and RTCRtpSender with a track. The other
members and methods will be added in future CLs.

Layers:
- blink::RTCRtpSender (javascript)
- WebRTCRtpSender (interface)
- content::RTCRtpSender (implementation)
- (webrtc::RtpSenderInterface)

Lookup of sender's track is done by searching local streams for the
track with the correct ID.

Spec:
- https://w3c.github.io/webrtc-pc/#rtcrtpsender-interface
- https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-getsenders

Intent to Implement & Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/GnlJt54O_EY

BUG=700916
Review-Url: https://codereview.chromium.org/2749703005
Cr-Commit-Position: refs/heads/master@{#469645}
Committed: https://chromium.googlesource.com/chromium/src/+/3c633b6376623a8d0de8d567a822c6b96d2c8263

[hbos_chromium, 2017-04-27 13:17:50]

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

[commit-bot: I haz the power, 2017-04-27 13:18:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hbos_chromium, 2017-04-27 13:20:22]

Patchset #2 (id:20001) has been deleted

[hbos_chromium, 2017-04-27 13:20:27]

Patchset #1 (id:1) has been deleted

[hbos_chromium, 2017-04-27 13:20:35]

Patchset #1 (id:40001) has been deleted

[hbos_chromium, 2017-04-27 13:20:42]

Patchset #1 (id:60001) has been deleted

[hbos_chromium, 2017-04-27 13:20:48]

Patchset #1 (id:80001) has been deleted

[hbos_chromium, 2017-04-27 13:20:54]

Patchset #1 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-04-27 13:37:17]

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

[commit-bot: I haz the power, 2017-04-27 13:37:18]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hbos_chromium, 2017-04-27 14:10:41]

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

[commit-bot: I haz the power, 2017-04-27 14:11:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 14:30:20]

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

[commit-bot: I haz the power, 2017-04-27 14:30:21]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[hbos_chromium, 2017-04-27 14:34:49]

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

[hbos_chromium, 2017-04-27 14:34:52]

Patchset #2 (id:140001) has been deleted

[hbos_chromium, 2017-04-27 14:34:59]

Patchset #1 (id:120001) has been deleted

[commit-bot: I haz the power, 2017-04-27 14:35:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 15:50:40]

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

[commit-bot: I haz the power, 2017-04-27 15:50:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hbos_chromium, 2017-04-28 10:33:24]

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

[commit-bot: I haz the power, 2017-04-28 10:33:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hbos_chromium, 2017-04-28 10:34:12]

Patchset #1 (id:160001) has been deleted

[hbos_chromium, 2017-04-28 10:36:53]

Description was changed from

==========
RTCRtpSender WIP

BUG=700916
NOCOMMIT=true
==========

to

==========
RTCRtpSender with track behind RuntimeEnabled flag ("RTCRtpSender")

Implements getSenders() and RTCRtpSender with a track. The other
members and methods will be added in future CLs.

Spec:
- https://w3c.github.io/webrtc-pc/#rtcrtpsender-interface
- https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-getsenders

BUG=700916
==========

[hbos_chromium, 2017-04-28 10:36:53]

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

[hbos_chromium, 2017-04-28 10:38:21]

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

[hbos_chromium, 2017-04-28 10:38:47]

Please take a look, guidou and jochen.

[hbos_chromium, 2017-04-28 10:42:35]

Description was changed from

==========
RTCRtpSender with track behind RuntimeEnabled flag ("RTCRtpSender")

Implements getSenders() and RTCRtpSender with a track. The other
members and methods will be added in future CLs.

Spec:
- https://w3c.github.io/webrtc-pc/#rtcrtpsender-interface
- https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-getsenders

BUG=700916
==========

to

==========
RTCRtpSender with track behind RuntimeEnabled flag ("RTCRtpSender")

Implements getSenders() and RTCRtpSender with a track. The other
members and methods will be added in future CLs.

Layers:
- blink::RTCRtpSender (javascript)
- WebRTCRtpSender (interface)
- content::RTCRtpSender (implementation)
- (webrtc::RtpSenderInterface)

Lookup of sender's track is done by searching local streams for the
track with the correct ID.

Spec:
- https://w3c.github.io/webrtc-pc/#rtcrtpsender-interface
- https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-getsenders

BUG=700916
==========

[commit-bot: I haz the power, 2017-04-28 11:58:53]

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

[commit-bot: I haz the power, 2017-04-28 11:58:54]

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

[jochen (gone - plz use gerrit), 2017-05-02 09:16:51]

can you link to the intent to implement from the CL description please

[hbos_chromium, 2017-05-02 10:15:56]

Description was changed from

==========
RTCRtpSender with track behind RuntimeEnabled flag ("RTCRtpSender")

Implements getSenders() and RTCRtpSender with a track. The other
members and methods will be added in future CLs.

Layers:
- blink::RTCRtpSender (javascript)
- WebRTCRtpSender (interface)
- content::RTCRtpSender (implementation)
- (webrtc::RtpSenderInterface)

Lookup of sender's track is done by searching local streams for the
track with the correct ID.

Spec:
- https://w3c.github.io/webrtc-pc/#rtcrtpsender-interface
- https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-getsenders

BUG=700916
==========

to

==========
RTCRtpSender with track behind RuntimeEnabled flag ("RTCRtpSender")

Implements getSenders() and RTCRtpSender with a track. The other
members and methods will be added in future CLs.

Layers:
- blink::RTCRtpSender (javascript)
- WebRTCRtpSender (interface)
- content::RTCRtpSender (implementation)
- (webrtc::RtpSenderInterface)

Lookup of sender's track is done by searching local streams for the
track with the correct ID.

Spec:
- https://w3c.github.io/webrtc-pc/#rtcrtpsender-interface
- https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-getsenders

Intent to Implement & Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/GnlJt54O_EY

BUG=700916
==========

[hbos_chromium, 2017-05-02 10:16:20]

On 2017/05/02 09:16:51, jochen wrote:
> can you link to the intent to implement from the CL description please

Oh right, intent to implement & ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/GnlJt54O_EY

[Guido Urdaneta, 2017-05-02 12:31:48]

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

https://codereview.chromium.org/2749703005/diff/200001/content/renderer/media...
content/renderer/media/rtc_peer_connection_handler.cc:1692: new
RTCRtpSender(webrtc_senders[i].get(), std::move(web_track)));
nit: favor base::MakeUnique over new.

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

https://codereview.chromium.org/2749703005/diff/200001/content/renderer/media...
content/renderer/media/webrtc/rtc_rtp_sender.cc:26: return
reinterpret_cast<uintptr_t>(webrtc_rtp_sender);
Does this always work? Is there another way to get an ID?

https://codereview.chromium.org/2749703005/diff/200001/content/renderer/media...
content/renderer/media/webrtc/rtc_rtp_sender.cc:44: DCHECK(web_track_ ==
webrtc_track());
Does DCHECK_EQ work in this case?

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

https://codereview.chromium.org/2749703005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:1196: //
There does not exist a |RTCRtpSender| for this |WebRTCRtpSender|
nit/typo: a -> an

https://codereview.chromium.org/2749703005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:1198:
MediaStreamTrack* track;
nit: Initialize track to nullptr and remove the else below.
Alternatively, use  :? to initialize track to either nullptr or
GetLocalTrackById, and have a more complex DCHECK after that.

https://codereview.chromium.org/2749703005/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/peerconnection/RTCRtpSender.h (right):

https://codereview.chromium.org/2749703005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCRtpSender.h:25: // Takes
ownership of the sender.
nit: The comment is redundant, since the parameter is a unique_ptr

https://codereview.chromium.org/2749703005/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebRTCRtpSender.h (right):

https://codereview.chromium.org/2749703005/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCRtpSender.h:23: // Two |WebRtcSender|s
referencing the same WebRTC-layer sender have the same
WebRtcSender -> WebRTCRtpSender

[hbos_chromium, 2017-05-03 09:53:54]

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

[commit-bot: I haz the power, 2017-05-03 09:54:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hbos_chromium, 2017-05-03 09:54:24]

PTAL guidou and jochen.

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

https://codereview.chromium.org/2749703005/diff/200001/content/renderer/media...
content/renderer/media/rtc_peer_connection_handler.cc:1692: new
RTCRtpSender(webrtc_senders[i].get(), std::move(web_track)));
On 2017/05/02 12:31:48, Guido Urdaneta wrote:
> nit: favor base::MakeUnique over new.

Done.

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

https://codereview.chromium.org/2749703005/diff/200001/content/renderer/media...
content/renderer/media/webrtc/rtc_rtp_sender.cc:26: return
reinterpret_cast<uintptr_t>(webrtc_rtp_sender);
On 2017/05/02 12:31:48, Guido Urdaneta wrote:
> Does this always work? Is there another way to get an ID?

The only requirement of IDs is that they're a way of uniquely identifying an
existing sender, this achieves that without needing additional logic.

If a sender is deleted then theoretically another sender could be created in the
future reusing the same ID (position in memory) but this does not cause any
problems. The map is for the ID -> blink sender map, and as long as a blink
sender is kept-alive so is the webrtc sender, there are never any possible
conflicts.

I prefer this for simplicity and it's how receivers is implemented. I added a
comment in the interface clarifying that IDs may be reused.

https://codereview.chromium.org/2749703005/diff/200001/content/renderer/media...
content/renderer/media/webrtc/rtc_rtp_sender.cc:44: DCHECK(web_track_ ==
webrtc_track());
On 2017/05/02 12:31:48, Guido Urdaneta wrote:
> Does DCHECK_EQ work in this case?

No, not without defining more functions. Last I tried it got messy.

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

https://codereview.chromium.org/2749703005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:1196: //
There does not exist a |RTCRtpSender| for this |WebRTCRtpSender|
On 2017/05/02 12:31:48, Guido Urdaneta wrote:
> nit/typo: a -> an

Done.

https://codereview.chromium.org/2749703005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:1198:
MediaStreamTrack* track;
On 2017/05/02 12:31:48, Guido Urdaneta wrote:
> nit: Initialize track to nullptr and remove the else below.
> Alternatively, use  :? to initialize track to either nullptr or
> GetLocalTrackById, and have a more complex DCHECK after that.

Done.

https://codereview.chromium.org/2749703005/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/peerconnection/RTCRtpSender.h (right):

https://codereview.chromium.org/2749703005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCRtpSender.h:25: // Takes
ownership of the sender.
On 2017/05/02 12:31:48, Guido Urdaneta wrote:
> nit: The comment is redundant, since the parameter is a unique_ptr

Done.

https://codereview.chromium.org/2749703005/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebRTCRtpSender.h (right):

https://codereview.chromium.org/2749703005/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCRtpSender.h:23: // Two |WebRtcSender|s
referencing the same WebRTC-layer sender have the same
On 2017/05/02 12:31:48, Guido Urdaneta wrote:
> WebRtcSender -> WebRTCRtpSender

Done.

[commit-bot: I haz the power, 2017-05-03 10:08:34]

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

[commit-bot: I haz the power, 2017-05-03 10:08:35]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jochen (gone - plz use gerrit), 2017-05-03 10:35:34]

rubberstamp lgtm. Please wait for guidou to finish the full review

[hbos_chromium, 2017-05-03 10:53:16]

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

[commit-bot: I haz the power, 2017-05-03 10:53:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 12:36:55]

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

[commit-bot: I haz the power, 2017-05-03 12:36:56]

Dry run: This issue passed the CQ dry run.

[Guido Urdaneta, 2017-05-03 15:42:31]

lgtm

[hbos_chromium, 2017-05-04 13:37:56]

The CQ bit was checked by hbos@chromium.org

[hbos_chromium, 2017-05-04 13:37:57]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2749703005/#ps240001
(title: "Rebase with master (ScriptWrappable.h moved)")

[commit-bot: I haz the power, 2017-05-04 13:38:11]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 13:41:50]

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

[commit-bot: I haz the power, 2017-05-04 13:41:52]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[hbos_chromium, 2017-05-04 13:52:01]

The CQ bit was checked by hbos@chromium.org

[hbos_chromium, 2017-05-04 13:52:01]

The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2749703005/#ps260001
(title: "Rebase with master")

[commit-bot: I haz the power, 2017-05-04 13:52:33]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 15:11:29]

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

[commit-bot: I haz the power, 2017-05-04 15:11:31]

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_...)

[hbos_chromium, 2017-05-05 12:48:44]

The CQ bit was checked by hbos@chromium.org

[hbos_chromium, 2017-05-05 12:48:45]

The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2749703005/#ps280001
(title: "external/wpt/webrtc/RTCPeerConnection-idl-expected.txt updated passing
2 more tests")

[commit-bot: I haz the power, 2017-05-05 12:49:00]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 14:35:47]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1493988524551120,
"parent_rev": "349f4f5f76dcf3eb8961c8589f821f144aa5118d", "commit_rev":
"3c633b6376623a8d0de8d567a822c6b96d2c8263"}

[commit-bot: I haz the power, 2017-05-05 14:36:00]

Description was changed from

==========
RTCRtpSender with track behind RuntimeEnabled flag ("RTCRtpSender")

Implements getSenders() and RTCRtpSender with a track. The other
members and methods will be added in future CLs.

Layers:
- blink::RTCRtpSender (javascript)
- WebRTCRtpSender (interface)
- content::RTCRtpSender (implementation)
- (webrtc::RtpSenderInterface)

Lookup of sender's track is done by searching local streams for the
track with the correct ID.

Spec:
- https://w3c.github.io/webrtc-pc/#rtcrtpsender-interface
- https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-getsenders

Intent to Implement & Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/GnlJt54O_EY

BUG=700916
==========

to

==========
RTCRtpSender with track behind RuntimeEnabled flag ("RTCRtpSender")

Implements getSenders() and RTCRtpSender with a track. The other
members and methods will be added in future CLs.

Layers:
- blink::RTCRtpSender (javascript)
- WebRTCRtpSender (interface)
- content::RTCRtpSender (implementation)
- (webrtc::RtpSenderInterface)

Lookup of sender's track is done by searching local streams for the
track with the correct ID.

Spec:
- https://w3c.github.io/webrtc-pc/#rtcrtpsender-interface
- https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-getsenders

Intent to Implement & Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/GnlJt54O_EY

BUG=700916

Review-Url: https://codereview.chromium.org/2749703005
Cr-Commit-Position: refs/heads/master@{#469645}
Committed:
https://chromium.googlesource.com/chromium/src/+/3c633b6376623a8d0de8d567a822...
==========

[commit-bot: I haz the power, 2017-05-05 14:36:02]

Committed patchset #6 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/3c633b6376623a8d0de8d567a822...