Delete deprecated PeerConnection methods, and corresponding using declarations.

Delete deprecated PeerConnection methods, and corresponding using declarations.

BUG=None

Review-Url: https://codereview.webrtc.org/2632203003
Cr-Commit-Position: refs/heads/master@{#17120}
Committed: https://chromium.googlesource.com/external/webrtc/+/7f067663acc38e6c2ca8502947c0a1353601f387

[nisse-webrtc, 2017-01-16 16:04:29]

nisse@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, sakal@webrtc.org

[nisse-webrtc, 2017-01-16 16:04:29]

Can these be deleted now?

Is there any old bug number to associated with this cl?

[Taylor Brandstetter, 2017-01-16 23:44:57]

I'd just use the chromium trybot and see what (if anything) breaks, but I did
notice one thing.

https://codereview.webrtc.org/2632203003/diff/20001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface.h (left):

https://codereview.webrtc.org/2632203003/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:455: std::vector<MediaStreamInterface*>
streams) {
It doesn't look like chromium's mock PeerConnection overrides this yet:
https://cs.chromium.org/chromium/src/content/renderer/media/mock_peer_connect...

[Taylor Brandstetter, 2017-01-16 23:45:26]

Oh, and no, I don't know of any bug number.

[nisse-webrtc, 2017-01-17 07:22:05]

On 2017/01/16 23:44:57, Taylor Brandstetter wrote:
> I'd just use the chromium trybot and see what (if anything) breaks, but I did
> notice one thing.
> 
>
https://codereview.webrtc.org/2632203003/diff/20001/webrtc/api/peerconnection...
> File webrtc/api/peerconnectioninterface.h (left):
> 
>
https://codereview.webrtc.org/2632203003/diff/20001/webrtc/api/peerconnection...
> webrtc/api/peerconnectioninterface.h:455: std::vector<MediaStreamInterface*>
> streams) {
> It doesn't look like chromium's mock PeerConnection overrides this yet:
>
https://cs.chromium.org/chromium/src/content/renderer/media/mock_peer_connect...

Thanks, I'll use the trybots and see what needs to be done.

[nisse-webrtc, 2017-01-18 13:37:55]

On 2017/01/17 07:22:05, nisse-webrtc wrote:
> Thanks, I'll use the trybots and see what needs to be done.

It seems both Chrome and some other downstream apps break, with mock objects not
implementing the new methods. And not obvious to me how they should be
implemented.

Hmm, and it looks like I was a bit confused when preparing this cl. My original
intention was to drop compatibility methods of
PeerConnectionObserver::OnAddStream and friends. But it seems that's already
done, there are just a couple of using declarations and TODO items left around,
which should be cleaned up.

[Taylor Brandstetter, 2017-01-25 23:15:43]

lgtm for this CL itself, as long as downstream things don't break. Thanks for
cleaning up my TODOs!

[sakal, 2017-01-26 11:57:47]

lgtm if you get the bots to pass

[nisse-webrtc, 2017-01-30 11:23:50]

On 2017/01/26 11:57:47, sakal wrote:
> lgtm if you get the bots to pass

I'm confused by the failure of the Chrome bot. I would have expected only
compile-time problems, but they compile fine and then fail a large number of
tests.

[nisse-webrtc, 2017-01-30 15:54:47]

https://codereview.webrtc.org/2632203003/diff/40001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface.h (left):

https://codereview.webrtc.org/2632203003/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:661: virtual void
OnAddStream(MediaStreamInterface* stream) {}
This change breaks tests. Unclear why, I'll try adding proper override
declarations in subclasses, to see if that triggers any errors at compile time
rather than run time.

[nisse-webrtc, 2017-01-31 12:47:05]

On 2017/01/30 15:54:47, nisse-webrtc wrote:
>
https://codereview.webrtc.org/2632203003/diff/40001/webrtc/api/peerconnection...
> File webrtc/api/peerconnectioninterface.h (left):
> 
>
https://codereview.webrtc.org/2632203003/diff/40001/webrtc/api/peerconnection...
> webrtc/api/peerconnectioninterface.h:661: virtual void
> OnAddStream(MediaStreamInterface* stream) {}
> This change breaks tests. Unclear why, I'll try adding proper override
> declarations in subclasses, to see if that triggers any errors at compile time
> rather than run time.

I think I've found the problem. Both the old and new callbacks were called. And
the compiler didn't complain when I deleted one of them from the interface, most
likely due to implicit conversion between raw pointer and scoped_refptr. We'll
see if deleting the calls makes the bots happier.

[nisse-webrtc, 2017-01-31 14:26:42]

And it seems it does break some downstream projects (even after I undid one of
the changes). Taylor, would you like to help fixing these? internal_bot logs
point out the problems.

[Taylor Brandstetter, 2017-01-31 17:49:09]

On 2017/01/31 14:26:42, nisse-webrtc wrote:
> And it seems it does break some downstream projects (even after I undid one of
> the changes). Taylor, would you like to help fixing these? internal_bot logs
> point out the problems.

Sure, I'll take a look.

[nisse-webrtc, 2017-02-24 16:12:56]

On 2017/01/31 17:49:09, Taylor Brandstetter wrote:
> On 2017/01/31 14:26:42, nisse-webrtc wrote:
> > And it seems it does break some downstream projects (even after I undid one
of
> > the changes). Taylor, would you like to help fixing these? internal_bot logs
> > point out the problems.
> 
> Sure, I'll take a look.

Internal bot now passing. I'm on vacation next week, feel free to land in my
absence.

[nisse-webrtc, 2017-03-08 08:11:19]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-03-08 08:11:20]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org,
sakal@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2632203003/#ps140001 (title:
"Rebased, and also reverted the revert in ps#7.")

[commit-bot: I haz the power, 2017-03-08 08:11:22]

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

[commit-bot: I haz the power, 2017-03-08 08:31:39]

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

[commit-bot: I haz the power, 2017-03-08 08:31:40]

Failed to apply patch for webrtc/api/peerconnectioninterface.h:
While running git apply --index -p1;
  error: patch failed: webrtc/api/peerconnectioninterface.h:577
  error: webrtc/api/peerconnectioninterface.h: patch does not apply

Patch:       webrtc/api/peerconnectioninterface.h
Index: webrtc/api/peerconnectioninterface.h
diff --git a/webrtc/api/peerconnectioninterface.h
b/webrtc/api/peerconnectioninterface.h
index
ea94f504423e15c4841ad7feadcab770bbb6e3d9..ce1b42edc9769466874f35ee68bcdabdce5cedf2
100644
--- a/webrtc/api/peerconnectioninterface.h
+++ b/webrtc/api/peerconnectioninterface.h
@@ -567,9 +567,6 @@ class PeerConnectionInterface : public
rtc::RefCountInterface {
   // remote peer is notified.
   virtual void RemoveStream(MediaStreamInterface* stream) = 0;
 
-  // TODO(deadbeef): Make the following two methods pure virtual once
-  // implemented by all subclasses of PeerConnectionInterface.
-
   // Add a new MediaStreamTrack to be sent on this PeerConnection, and return
   // the newly created RtpSender.
   //
@@ -577,15 +574,11 @@ class PeerConnectionInterface : public
rtc::RefCountInterface {
   // with.
   virtual rtc::scoped_refptr<RtpSenderInterface> AddTrack(
       MediaStreamTrackInterface* track,
-      std::vector<MediaStreamInterface*> streams) {
-    return nullptr;
-  }
+      std::vector<MediaStreamInterface*> streams) = 0;
 
   // Remove an RtpSender from this PeerConnection.
   // Returns true on success.
-  virtual bool RemoveTrack(RtpSenderInterface* sender) {
-    return false;
-  }
+  virtual bool RemoveTrack(RtpSenderInterface* sender) = 0;
 
   // Returns pointer to a DtmfSender on success. Otherwise returns NULL.
   //
@@ -829,21 +822,15 @@ class PeerConnectionObserver {
   // pointer version.
 
   // Triggered when media is received on a new stream from remote peer.
-  virtual void OnAddStream(rtc::scoped_refptr<MediaStreamInterface> stream) {}
-  // Deprecated; please use the version that uses a scoped_refptr.
-  virtual void OnAddStream(MediaStreamInterface* stream) {}
+  virtual void OnAddStream(rtc::scoped_refptr<MediaStreamInterface> stream) =
0;
 
   // Triggered when a remote peer close a stream.
-  virtual void OnRemoveStream(rtc::scoped_refptr<MediaStreamInterface> stream)
{
-  }
-  // Deprecated; please use the version that uses a scoped_refptr.
-  virtual void OnRemoveStream(MediaStreamInterface* stream) {}
+  virtual void OnRemoveStream(
+      rtc::scoped_refptr<MediaStreamInterface> stream) = 0;
 
   // Triggered when a remote peer opens a data channel.
   virtual void OnDataChannel(
-      rtc::scoped_refptr<DataChannelInterface> data_channel) {}
-  // Deprecated; please use the version that uses a scoped_refptr.
-  virtual void OnDataChannel(DataChannelInterface* data_channel) {}
+      rtc::scoped_refptr<DataChannelInterface> data_channel) = 0;
 
   // Triggered when renegotiation is needed. For example, an ICE restart
   // has begun.

[nisse-webrtc, 2017-03-08 09:33:07]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 09:33:11]

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

[commit-bot: I haz the power, 2017-03-08 10:17:56]

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

[commit-bot: I haz the power, 2017-03-08 10:17:57]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-03-08 14:57:39]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-03-08 14:57:39]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org,
sakal@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2632203003/#ps160001 (title:
"Rebased.")

[commit-bot: I haz the power, 2017-03-08 14:57:48]

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

[commit-bot: I haz the power, 2017-03-08 14:59:46]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1488985059018190,
"parent_rev": "cbbd8c76e8a8a1cf3f1193ea50980bc9d858458d", "commit_rev":
"7f067663acc38e6c2ca8502947c0a1353601f387"}

[commit-bot: I haz the power, 2017-03-08 14:59:49]

Description was changed from

==========
Delete deprecated PeerConnection methods, and corresponding using declarations.

BUG=None
==========

to

==========
Delete deprecated PeerConnection methods, and corresponding using declarations.

BUG=None

Review-Url: https://codereview.webrtc.org/2632203003
Cr-Commit-Position: refs/heads/master@{#17120}
Committed:
https://chromium.googlesource.com/external/webrtc/+/7f067663acc38e6c2ca850294...
==========

[commit-bot: I haz the power, 2017-03-08 14:59:51]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/external/webrtc/+/7f067663acc38e6c2ca850294...