Reporting of policy errors via host-offline-reason: part 2b

remoting/host/remoting_me2me_host.cc

Index: remoting/host/remoting_me2me_host.cc
diff --git a/remoting/host/remoting_me2me_host.cc b/remoting/host/remoting_me2me_host.cc
index e83a4847a97d0fd5d4bcf76b5927b98ba533a5a4..12d7ee8bb2d1c431452fb0faa6687698a86117f8 100644
--- a/remoting/host/remoting_me2me_host.cc
+++ b/remoting/host/remoting_me2me_host.cc
@@ -142,7 +142,6 @@ namespace remoting {
 
 class HostProcess
     : public ConfigWatcher::Delegate,
-      public HeartbeatSender::Listener,
       public HostChangeNotificationListener::Listener,
       public IPC::Listener,
       public base::RefCountedThreadSafe<HostProcess> {
@@ -158,9 +157,9 @@ class HostProcess
   virtual bool OnMessageReceived(const IPC::Message& message) override;
   virtual void OnChannelError() override;
 
-  // HeartbeatSender::Listener overrides.
-  virtual void OnHeartbeatSuccessful() override;
-  virtual void OnUnknownHostIdError() override;
+  // HeartbeatSender callbacks

[Lambros, 2014/11/18 00:19:30]

nit: period.
Actually, I'm not sure you need this comment? Also, do these methods need to be
public?

[Łukasz Anforowicz, 2014/11/18 01:03:22]

Yes - good point.  These methods don't need to be public after the changes.

+  void OnHeartbeatSuccessful();
+  void OnUnknownHostIdError();
 
   // HostChangeNotificationListener::Listener overrides.
   virtual void OnHostDeleted() override;
@@ -1322,8 +1321,9 @@ void HostProcess::StartHost() {
 #endif
 
   heartbeat_sender_.reset(new HeartbeatSender(
-      this, host_id_, signal_strategy_.get(), key_pair_,
-      directory_bot_jid_));
+      base::Bind(&HostProcess::OnHeartbeatSuccessful, base::Unretained(this)),

[Lambros, 2014/11/18 00:19:30]

Why do we need base::Unretained here? This is a ref-counted object.

[Łukasz Anforowicz, 2014/11/18 01:03:22]

This temporarily follows other references to |this| from this file.  I am
changing to WeakPtr in part#3.

[Lambros, 2014/11/18 02:33:52]

I see. I'm curious to know if this still compiles without the base::Unretained?
But OK to keep it for now, since it's used several times in this file already.

[Łukasz Anforowicz, 2014/11/18 17:30:45]

It would compile without base::Unretained and would even work, but would mean
that HeartbeatSender (by holding the callbacks) is extending the lifetime of
HostProcess - this different from current code and I don't want to change it
unnecessarily (although is probably fine as HostProcess explicitly destroys
HeartbeatSender in one of Shutdown methods; but then again HeartbeatSender will
outlive HostProcess after part#3, so I rather won't muck with this
unnecessarily).

+      base::Bind(&HostProcess::OnUnknownHostIdError, base::Unretained(this)),
+      host_id_, signal_strategy_.get(), key_pair_, directory_bot_jid_));
 
   host_change_notification_listener_.reset(new HostChangeNotificationListener(
       this, host_id_, signal_strategy_.get(), directory_bot_jid_));