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

remoting/host/heartbeat_sender.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef REMOTING_HOST_HEARTBEAT_SENDER_H_
 #define REMOTING_HOST_HEARTBEAT_SENDER_H_
 
 #include <string>
 
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/threading/thread_checker.h"
 #include "base/timer/timer.h"
 #include "remoting/base/rsa_key_pair.h"
 #include "remoting/signaling/signal_strategy.h"
 
 namespace base {
 class MessageLoopProxy;
+class TimeDelta;
 }  // namespace base
 
 namespace buzz {
 class XmlElement;
 }  // namespace buzz
 
 namespace remoting {
 
 class RsaKeyPair;
 class IqRequest;
@@ skipping 56 matching lines @@
 //  </iq>
 class HeartbeatSender : public SignalStrategy::Listener {
  public:
   // |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 scoped_refptr<const RsaKeyPair>& 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, as well as
+  // intiates sending a stanza right away.

[Lambros, 2014/12/16 02:14:05]

typo: initiates (maybe "initiating"? Or better: "..stanzas, and initiates..")

[Łukasz Anforowicz, 2014/12/17 18:02:04]

Done.

   //
-  // See rem:host-offline-reason class-level comments for discussion
-  // of allowed values for |host_offline_reason| string.
+  // See rem:host-offline-reason class-level comments for discussion of

[Lambros, 2014/12/16 02:14:05]

nit: What is "rem:host-offline-reason"? If it's an actual class, use the real
name (with or without the remoting:: prefix).

[Łukasz Anforowicz, 2014/12/17 18:02:04]

This is an xml attribute referred to in the class-level comments at the top. 
I'll qualify the comment here.

+  // allowed values for |host_offline_reason| string.
   //
-  // |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.
+  enum AckOrTimeout { Ack, Timeout };

[Wez, 2014/12/18 00:46:20]

Elsewhere we just return a |bool success| parameter to indicate success or
failure; why not do that here?

[Łukasz Anforowicz, 2014/12/18 19:02:33]

Seemed cleaner this way (i.e. Timeout is more readable than "false" means in a
call to base::Bind;  OTOH, I can add a comment saying that "false" is a
"success" value + I can see that bool is shorter and therefore results in
cleaner declarations).  Anyway - I replaced AckOrTimeout with bool in the latest
patchset.

   void SetHostOfflineReason(
       const std::string& host_offline_reason,
-      const base::Closure& ack_callback);
+      const base::TimeDelta& timeout,
+      const base::Callback<void(AckOrTimeout)>& ack_callback);
 
   // SignalStrategy::Listener interface.
   void OnSignalStrategyStateChange(SignalStrategy::State state) override;
   bool OnSignalStrategyIncomingStanza(const buzz::XmlElement* stanza) override;
 
  private:
   FRIEND_TEST_ALL_PREFIXES(HeartbeatSenderTest,
                            DoSendStanzaWithExpectedSequenceId);
   FRIEND_TEST_ALL_PREFIXES(HeartbeatSenderTest, ProcessResponseSetInterval);
   FRIEND_TEST_ALL_PREFIXES(HeartbeatSenderTest,
                            ProcessResponseExpectedSequenceId);
   friend class HeartbeatSenderTest;
 
   void SendStanza();
   void ResendStanza();
   void DoSendStanza();
   void ProcessResponse(bool is_offline_heartbeat_response,
                        IqRequest* request,
                        const buzz::XmlElement* response);
   void SetInterval(int interval);
   void SetSequenceId(int sequence_id);
+  void CallHostOfflineReasonAckCallback(AckOrTimeout ack_or_timeout);
 
   // Helper methods used by DoSendStanza() to generate heartbeat stanzas.
   scoped_ptr<buzz::XmlElement> CreateHeartbeatMessage();
   scoped_ptr<buzz::XmlElement> CreateSignature();
 
   base::Closure on_heartbeat_successful_callback_;
   base::Closure on_unknown_host_id_error_;
   std::string host_id_;
   SignalStrategy* signal_strategy_;
-  scoped_refptr<RsaKeyPair> key_pair_;
+  scoped_refptr<const RsaKeyPair> key_pair_;
   std::string directory_bot_jid_;
   scoped_ptr<IqSender> iq_sender_;
   scoped_ptr<IqRequest> request_;
   int interval_ms_;
   base::RepeatingTimer<HeartbeatSender> timer_;
   base::OneShotTimer<HeartbeatSender> timer_resend_;
   int sequence_id_;
   bool sequence_id_was_set_;
   int sequence_id_recent_set_num_;
   bool heartbeat_succeeded_;
   int failed_startup_heartbeat_count_;
   std::string host_offline_reason_;
-  base::Closure host_offline_reason_ack_callback_;
+  base::Callback<void(AckOrTimeout)> host_offline_reason_ack_callback_;
+  base::OneShotTimer<HeartbeatSender> host_offline_reason_timeout_timer_;

[Lambros, 2014/12/16 02:14:05]

Maybe just host_offline_reason_timer_?

[Łukasz Anforowicz, 2014/12/17 18:02:04]

I think I'll just leave the current name - more accurate + not much worse in
terms of readability/length from the proposal.

 
   base::ThreadChecker thread_checker_;
 
   DISALLOW_COPY_AND_ASSIGN(HeartbeatSender);
 };
 
 }  // namespace remoting
 
 #endif  // REMOTING_HOST_HEARTBEAT_SENDER_H_