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"
@@ skipping 77 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<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.
   //
   // See rem:host-offline-reason class-level comments for discussion
   // of allowed values for |host_offline_reason| string.
   //
   // |ack_callback| will be called once, when the bot acks
@@ skipping 24 matching lines @@
   void SetSequenceId(int sequence_id);
 
   // 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_;
+  const scoped_refptr<RsaKeyPair> key_pair_;

[Lambros, 2014/12/02 03:35:22]

Why change this?

[Łukasz Anforowicz, 2014/12/02 20:08:01]

The assumption here is that presence of "const" is good:
1) It allows the callers to pass "const" arguments.
2) If it compiles it is probably safe (although C++ doesn't give any formal
guarantees / allows to cast the constness away).

Allowing callers to pass "const" arguments is desirable, because this:
1A) enables their callers to pass "const" arguments
1B) const arguments are sometimes unavoidable (i.e. retrieved from another part
of the system that we cannot or don't want to change) and casting constness away
is yucky
1C) presence of "const" is a good code readability aid (const = I promise I
won't modify/delete this pointer).

Anyway - this is unrelated to core changes here, but this is a small, beneficial
refactoring, so I think this is ok.

[Lambros, 2014/12/03 03:20:24]

But this field is a value type (not a reference or pointer type). Callers can
still pass "const" arguments even if the field is not const - it will be
assigned by value.

Adding "const" will discourage future maintainers from making certain changes
(for example, moving the assignment of this field out of the ctor into an Init()
method), for no obvious benefit.

I suspect what you really want is scoped_refptr<const RsaKeyPair> ?

[Łukasz Anforowicz, 2014/12/03 17:54:21]

You're right.  I don't know what I was thinking.

   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::ThreadChecker thread_checker_;
 
   DISALLOW_COPY_AND_ASSIGN(HeartbeatSender);
 };
 
 }  // namespace remoting
 
 #endif  // REMOTING_HOST_HEARTBEAT_SENDER_H_