Reporting of policy errors via host-offline-reason: part 3

remoting/host/heartbeat_sender.h

Index: remoting/host/heartbeat_sender.h
diff --git a/remoting/host/heartbeat_sender.h b/remoting/host/heartbeat_sender.h
index 73080be9a6d3683cf1cb3c2869214f3477206fe9..cd428444506acd848906b0e48211f8c8794892a1 100644
--- a/remoting/host/heartbeat_sender.h
+++ b/remoting/host/heartbeat_sender.h
@@ -19,6 +19,7 @@
 
 namespace base {
 class MessageLoopProxy;
+class TimeDelta;
 }  // namespace base
 
 namespace buzz {
@@ -91,25 +92,32 @@ class HeartbeatSender : public SignalStrategy::Listener {
   // |signal_strategy| and |delegate| must outlive this
   // object. Heartbeats will start when the supplied SignalStrategy
   // enters the CONNECTED state.
-  HeartbeatSender(const base::Closure& on_heartbeat_successful_callback,
-                  const base::Closure& on_unknown_host_id_error,
-                  const std::string& host_id,
-                  SignalStrategy* signal_strategy,
-                  scoped_refptr<RsaKeyPair> key_pair,
-                  const std::string& directory_bot_jid);
+  HeartbeatSender(
+      // Invoked after the first successful heartbeat.
+      const base::Closure& on_heartbeat_successful_callback,
+
+      // Invoked when the host ID is permanently not recognized by the server.

[Wez, 2014/12/19 22:05:37]

These comments should be in the HeartbeatSender() comment block above, not
in-line in the declaration - the comment block above basically needs to document
how the caller should use this constructor.

[Łukasz Anforowicz, 2015/01/07 01:24:04]

Done.

+      const base::Closure& on_unknown_host_id_error,
+
+      const std::string& host_id,
+      SignalStrategy* signal_strategy,
+      const scoped_refptr<const RsaKeyPair>& host_key_pair,
+      const std::string& directory_bot_jid);
   ~HeartbeatSender() override;
 
-  // Sets host offline reason for future heartbeat stanzas,
-  // as well as intiates sending a stanza right away.
+  // Sets host offline reason for future heartbeat stanzas, and initiates
+  // sending a stanza right away.
   //
-  // See rem:host-offline-reason class-level comments for discussion
-  // of allowed values for |host_offline_reason| string.
+  // For discussion of allowed values for |host_offline_reason| argument,
+  // please see the description of rem:host-offline-reason xml attribute in
+  // the class-level comments above.
   //
-  // |ack_callback| will be called once, when the bot acks
-  // receiving the |host_offline_reason|.
+  // |ack_callback| will be called once, when the bot acks receiving the
+  // |host_offline_reason| or when |timeout| is reached.
   void SetHostOfflineReason(
       const std::string& host_offline_reason,
-      const base::Closure& ack_callback);
+      const base::TimeDelta& timeout,
+      const base::Callback<void(bool success)>& ack_callback);
 
   // SignalStrategy::Listener interface.
   void OnSignalStrategyStateChange(SignalStrategy::State state) override;
@@ -132,6 +140,10 @@ class HeartbeatSender : public SignalStrategy::Listener {
   void SetInterval(int interval);
   void SetSequenceId(int sequence_id);
 
+  void PostHostOfflineReasonAckCallback(bool success);
+  void OnHostOfflineReasonTimeout();
+  void OnHostOfflineReasonAck();

[Wez, 2014/12/19 22:05:37]

nit: A single-line comment on this block to explain what these methods are for
would be helpful, e.g. "Handlers for host-offline-reason completion and
timeout." - otherwise this class just reads like a huge list of methods and
members. :-/

[Łukasz Anforowicz, 2015/01/07 01:24:04]

Done.

+
   // Helper methods used by DoSendStanza() to generate heartbeat stanzas.
   scoped_ptr<buzz::XmlElement> CreateHeartbeatMessage();
   scoped_ptr<buzz::XmlElement> CreateSignature();
@@ -140,7 +152,7 @@ class HeartbeatSender : public SignalStrategy::Listener {
   base::Closure on_unknown_host_id_error_;
   std::string host_id_;
   SignalStrategy* signal_strategy_;
-  scoped_refptr<RsaKeyPair> key_pair_;
+  scoped_refptr<const RsaKeyPair> host_key_pair_;
   std::string directory_bot_jid_;
   scoped_ptr<IqSender> iq_sender_;
   scoped_ptr<IqRequest> request_;
@@ -153,7 +165,8 @@ class HeartbeatSender : public SignalStrategy::Listener {
   bool heartbeat_succeeded_;
   int failed_startup_heartbeat_count_;
   std::string host_offline_reason_;
-  base::Closure host_offline_reason_ack_callback_;
+  base::Callback<void(bool success)> host_offline_reason_ack_callback_;
+  base::OneShotTimer<HeartbeatSender> host_offline_reason_timeout_timer_;

[Wez, 2014/12/19 22:05:37]

Similarly here, it'd be good to separate the offline-reason related members into
a block and introduce it with an explanation that these are used to manage
communication w/ the cloud, and process lifetime, during shutdown.

[Łukasz Anforowicz, 2015/01/07 01:24:04]

Done.

 
   base::ThreadChecker thread_checker_;