A bunch of interfaces: Return scoped_ptr<SSLCertificate>

A bunch of interfaces: Return scoped_ptr<SSLCertificate>

Instead of using a raw pointer output parameter. This affects

  SSLStreamAdapter::GetPeerCertificate
  Transport::GetRemoteSSLCertificate
  TransportChannel::GetRemoteSSLCertificate
  TransportController::GetRemoteSSLCertificate
  WebRtcSession::GetRemoteSSLCertificate

This is a good idea in general, but will also be very convenient when
scoped_ptr is gone, since unique_ptr doesn't have an .accept() method.

BUG=webrtc:5520
Committed: https://crrev.com/b4d01c4ded7ed1f7ff5ff75751dc52b7851a3918
Cr-Commit-Position: refs/heads/master@{#12262}

[kwiberg-webrtc, 2016-03-21 13:21:52]

Description was changed from

==========
A bunch of interfaces: Return scoped_ptr<SSLCertificate>

Instead of using a raw pointer output parameter. This is a good idea
in general, but will also be very convenient when scoped_ptr is gone,
since unique_ptr doesn't have an .accept() method.

BUG=webrtc:5520
==========

to

==========
A bunch of interfaces: Return scoped_ptr<SSLCertificate>

Instead of using a raw pointer output parameter. This affects

  SSLStreamAdapter::GetPeerCertificate
  Transport::GetRemoteSSLCertificate
  TransportChannel::GetRemoteSSLCertificate
  TransportController::GetRemoteSSLCertificate
  WebRtcSession::GetRemoteSSLCertificate

This is a good idea in general, but will also be very convenient when
scoped_ptr is gone, since unique_ptr doesn't have an .accept() method.

BUG=webrtc:5520
==========

[kwiberg-webrtc, 2016-03-21 14:40:15]

kwiberg@webrtc.org changed reviewers:
+ tommi@webrtc.org, torbjorng@webrtc.org

[kwiberg-webrtc, 2016-03-21 14:40:15]

Before landing, this, the following patch needs to be landed in Chromium:

diff --git a/remoting/protocol/channel_socket_adapter_unittest.cc
b/remoting/protocol/channel_socket_adapter_unittest.cc
index 3ecab7e..bb67eafa 100644
--- a/remoting/protocol/channel_socket_adapter_unittest.cc
+++ b/remoting/protocol/channel_socket_adapter_unittest.cc
@@ -55,6 +55,14 @@ class MockTransportChannel : public cricket::TransportChannel
{
                      rtc::scoped_refptr<rtc::RTCCertificate>());
   MOCK_CONST_METHOD1(GetRemoteSSLCertificate,
                      bool(rtc::SSLCertificate** cert));
+
+  // This can't be a real mock method because gmock doesn't support move-only
+  // return values.
+  virtual rtc::scoped_ptr<rtc::SSLCertificate> GetRemoteSSLCertificate() const
{
+    EXPECT_TRUE(false);  // Never called.
+    return nullptr;
+  }
+
   MOCK_METHOD6(ExportKeyingMaterial,
                bool(const std::string& label,
                     const uint8_t* context,

And after this CL has landed, we can land the following in Chromium:

diff --git a/remoting/protocol/channel_socket_adapter_unittest.cc
b/remoting/protocol/channel_socket_adapter_unittest.cc
index bb67eafa..8105eec 100644
--- a/remoting/protocol/channel_socket_adapter_unittest.cc
+++ b/remoting/protocol/channel_socket_adapter_unittest.cc
@@ -53,12 +53,11 @@ class MockTransportChannel : public
cricket::TransportChannel {
   MOCK_METHOD1(GetSslCipher, bool(std::string* cipher));
   MOCK_CONST_METHOD0(GetLocalCertificate,
                      rtc::scoped_refptr<rtc::RTCCertificate>());
-  MOCK_CONST_METHOD1(GetRemoteSSLCertificate,
-                     bool(rtc::SSLCertificate** cert));
 
   // This can't be a real mock method because gmock doesn't support move-only
   // return values.
-  virtual rtc::scoped_ptr<rtc::SSLCertificate> GetRemoteSSLCertificate() const
{
+  rtc::scoped_ptr<rtc::SSLCertificate> GetRemoteSSLCertificate()
+      const override {
     EXPECT_TRUE(false);  // Never called.
     return nullptr;
   }

Looking at the list of API directories in PRESUBMIT.py, the only thing that
might need backwards compatibility support is the change to
SSLStreamAdapter::GetPeerCertificate in webrtc/base/sslstreamadapter.h. I can't
find anything outside the WebRTC tree that calls or overrides that function; do
we expect such a thing to exist, or can I just try to make the change?

[kwiberg-webrtc, 2016-03-21 14:41:12]

Patchset #1 (id:1) has been deleted

[torbjorng (webrtc), 2016-03-23 14:18:23]

lgtm, but please consider readability wrt auto usage.

https://codereview.webrtc.org/1802013002/diff/20001/webrtc/api/statscollector.cc
File webrtc/api/statscollector.cc (right):

https://codereview.webrtc.org/1802013002/diff/20001/webrtc/api/statscollector...
webrtc/api/statscollector.cc:706: auto cert =
pc_->session()->GetRemoteSSLCertificate(
I think this use of auto makes the code less readable; one needs to look for the
type instead of seeing it right away.

[torbjorng (webrtc), 2016-03-23 14:18:26]

lgtm, but please consider readability wrt auto usage.

[kwiberg-webrtc, 2016-03-23 15:05:10]

Patchset #2 (id:40001) has been deleted

[kwiberg-webrtc, 2016-03-23 15:05:27]

New patch set with less auto.

https://codereview.webrtc.org/1802013002/diff/20001/webrtc/api/statscollector.cc
File webrtc/api/statscollector.cc (right):

https://codereview.webrtc.org/1802013002/diff/20001/webrtc/api/statscollector...
webrtc/api/statscollector.cc:706: auto cert =
pc_->session()->GetRemoteSSLCertificate(
On 2016/03/23 14:18:23, torbjorng (webrtc) wrote:
> I think this use of auto makes the code less readable; one needs to look for
the
> type instead of seeing it right away.

Done. Also in a few other places; the only remaining autos that I introduce are
now in places where there wasn't a named variable at all in the old code.

[kwiberg-webrtc, 2016-03-31 12:56:58]

kwiberg@webrtc.org changed reviewers:
+ perkj@webrtc.org
- tommi@webrtc.org

[kwiberg-webrtc, 2016-03-31 12:56:59]

Per, would you be able to review this?

[perkj_webrtc, 2016-04-01 14:19:09]

lgtm

https://codereview.webrtc.org/1802013002/diff/80001/webrtc/base/sslstreamadap...
File webrtc/base/sslstreamadapter_unittest.cc (right):

https://codereview.webrtc.org/1802013002/diff/80001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter_unittest.cc:1046: auto client_peer_cert =
GetPeerCertificate(true);
please use rtc::scoped_ptr<rtc::SSLCertificate> for readability here and below.

https://codereview.webrtc.org/1802013002/diff/80001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/1802013002/diff/80001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:878: auto remote_cert1 =
client1_.transport()->GetRemoteSSLCertificate();
please do not use auto for readability..

[kwiberg-webrtc, 2016-04-02 01:20:03]

Uploaded new patch set with more autos removed.

https://codereview.webrtc.org/1802013002/diff/80001/webrtc/base/sslstreamadap...
File webrtc/base/sslstreamadapter_unittest.cc (right):

https://codereview.webrtc.org/1802013002/diff/80001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter_unittest.cc:1046: auto client_peer_cert =
GetPeerCertificate(true);
On 2016/04/01 14:19:09, perkj_webrtc wrote:
> please use rtc::scoped_ptr<rtc::SSLCertificate> for readability here and
below.

Done.

https://codereview.webrtc.org/1802013002/diff/80001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/1802013002/diff/80001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:878: auto remote_cert1 =
client1_.transport()->GetRemoteSSLCertificate();
On 2016/04/01 14:19:09, perkj_webrtc wrote:
> please do not use auto for readability..

Done.

[kwiberg-webrtc, 2016-04-06 08:57:35]

The CQ bit was checked by kwiberg@webrtc.org

[kwiberg-webrtc, 2016-04-06 08:57:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
torbjorng@webrtc.org, perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1802013002/#ps120001 (title:
"less auto")

[commit-bot: I haz the power, 2016-04-06 08:57:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1802013002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1802013002/120001

[commit-bot: I haz the power, 2016-04-06 10:58:29]

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

[commit-bot: I haz the power, 2016-04-06 10:58:30]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[kwiberg-webrtc, 2016-04-06 11:20:07]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-04-06 11:20:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1802013002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1802013002/120001

[commit-bot: I haz the power, 2016-04-06 12:15:11]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-04-06 12:15:18]

Description was changed from

==========
A bunch of interfaces: Return scoped_ptr<SSLCertificate>

Instead of using a raw pointer output parameter. This affects

  SSLStreamAdapter::GetPeerCertificate
  Transport::GetRemoteSSLCertificate
  TransportChannel::GetRemoteSSLCertificate
  TransportController::GetRemoteSSLCertificate
  WebRtcSession::GetRemoteSSLCertificate

This is a good idea in general, but will also be very convenient when
scoped_ptr is gone, since unique_ptr doesn't have an .accept() method.

BUG=webrtc:5520
==========

to

==========
A bunch of interfaces: Return scoped_ptr<SSLCertificate>

Instead of using a raw pointer output parameter. This affects

  SSLStreamAdapter::GetPeerCertificate
  Transport::GetRemoteSSLCertificate
  TransportChannel::GetRemoteSSLCertificate
  TransportController::GetRemoteSSLCertificate
  WebRtcSession::GetRemoteSSLCertificate

This is a good idea in general, but will also be very convenient when
scoped_ptr is gone, since unique_ptr doesn't have an .accept() method.

BUG=webrtc:5520

Committed: https://crrev.com/b4d01c4ded7ed1f7ff5ff75751dc52b7851a3918
Cr-Commit-Position: refs/heads/master@{#12262}
==========

[commit-bot: I haz the power, 2016-04-06 12:15:19]

Patchset 5 (id:??) landed as
https://crrev.com/b4d01c4ded7ed1f7ff5ff75751dc52b7851a3918
Cr-Commit-Position: refs/heads/master@{#12262}