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

remoting/host/minimum_heartbeat_supporter.h

Index: remoting/host/minimum_heartbeat_supporter.h
diff --git a/remoting/host/minimum_heartbeat_supporter.h b/remoting/host/minimum_heartbeat_supporter.h
new file mode 100644
index 0000000000000000000000000000000000000000..c6d77a07ddb2bd0e137fdacc7f1f51b4c65e8f0a
--- /dev/null
+++ b/remoting/host/minimum_heartbeat_supporter.h
@@ -0,0 +1,102 @@
+// Copyright 2014 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_MINIMUM_HEARTBEAT_SUPPORTER_H_
+#define REMOTING_HOST_MINIMUM_HEARTBEAT_SUPPORTER_H_
+
+#include <string>
+
+#include "base/callback.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "remoting/base/auto_thread_task_runner.h"
+#include "remoting/base/rsa_key_pair.h"
+#include "remoting/host/ack_or_timeout_reporter.h"
+#include "remoting/signaling/xmpp_signal_strategy.h"
+
+namespace base {
+class TimeDelta;
+}
+
+namespace net {
+class NetworkChangeNotifier;
+}
+
+namespace remoting {
+
+class ChromotingHostContext;
+class DnsBlackholeChecker;
+class HeartbeatSender;
+class OAuthTokenGetter;
+class SignalStrategy;
+class SignalingConnector;
+
+// MinimumHeartbeatSupporter has 2 functions:

[Sergey Ulanov, 2014/12/15 22:39:19]

I think you can easily separate these two features. There is no reason they need
to be part of the same class. You can have MinimumHeartbeatSupporter just keep
all the pieces that are necessary for XMPP connection and then separate class
(e.g. OfflineReasonSender) that sends offline state on shutdown. That way
MinimumHeartbeatSupporter wouldn't need to be ref-counted and you would pass its
ovnership to OfflineReasonSender when shutting down the host. 

[Łukasz Anforowicz, 2014/12/16 00:59:52]

I think ownership management is orthogonal to having 2
functions/responsibilities - even with a separate OfflineReasonSender, it should
be the MinimumHeartbeatSupporter that keeps the network_task_runner alive,
right?

I am working on removing ref-counting, following the pattern below suggested by
Wez:
scoped_ptr<T> x_;
x_.release()->DoWorkAndDelete();

I don't understand what we would gain (after ref-counting is removed) by having
a separate OfflineReasonSender - the separate object would just forward calls to
heartbeat_sender and to minimum_heartbeat_supporter (aka
host_signaling_manager).

+// 1. Keep sending regular heartbeats to the service.
+// 2. Keep the host process alive while sending host-offline-reason heartbeat.
+class MinimumHeartbeatSupporter

[Sergey Ulanov, 2014/12/15 22:39:19]

IMO that's not very descriptive name. It's essentially a holder for all the
pieces that we need for signaling on the host. Maybe call it HostSignaling?

[Łukasz Anforowicz, 2014/12/16 00:59:52]

I'll go ahead with HostSignalingManager.

+    : public base::RefCounted<MinimumHeartbeatSupporter> {
+ public:
+  // TODO(lukasza): Refactor to limit the number of parameters below.

[Sergey Ulanov, 2014/12/15 22:39:19]

Do you really need this Create() method? I suggest just making the constructor
public and keep the code that creates HeartbeatSender, SignalStrategy, etc. in
HostProcess class.

[Łukasz Anforowicz, 2014/12/16 00:59:52]

Without the Create method, the heartbeat_sender, signaling_connector,
network_change_notifier, dns_blackhole_checker would no longer be internal
implementation details of MinimumHeartbeatSupporter (right now only
minimum_heartbeat_supporter.h/.cc refers to these 4 things - I think this
encapsulation is desirable and I don't want to reintroduce these dependencies
back to HostProcess).

+  // Probably necessitates refactoring HostProcess to extract a new
+  // class to read and store config/policy/cmdline values.
+  static scoped_refptr<MinimumHeartbeatSupporter> Create(
+      const base::Closure& on_heartbeat_successful_callback,
+      const base::Closure& on_unknown_host_id_error_callback,
+      const base::Closure& on_auth_failed_callback,
+      const ChromotingHostContext& host_context,
+      const XmppSignalStrategy::XmppServerConfig& xmpp_server_config,
+      const std::string& talkgadget_prefix,
+      const std::string& host_id,
+      const scoped_refptr<const RsaKeyPair>& key_pair,
+      const std::string& directory_bot_jid,
+      const std::string& oauth_refresh_token,
+      bool use_service_account);
+
+  // Gets signal strategy used for talking to the Chromoting bot.
+  // Return value is valid until |this| gets deleted.
+  SignalStrategy* signal_strategy() { return signal_strategy_.get(); }
+
+  // Kicks off sending a heartbeat containing a host-offline-reason attribute.
+  //
+  // Ensures that |this| (and transitively the whole process) is kept alive
+  // while waiting for an ack from the bot (this is subject to |timeout|).
+  void SendHostOfflineReason(const std::string& host_offline_reason,
+                             const base::TimeDelta& timeout);
+
+ private:
+  // Ref-counting helpers.
+  friend class base::RefCounted<MinimumHeartbeatSupporter>;
+  ~MinimumHeartbeatSupporter();
+
+  MinimumHeartbeatSupporter(
+      scoped_refptr<AutoThreadTaskRunner> network_task_runner,
+      scoped_ptr<net::NetworkChangeNotifier> network_change_notifier,
+      scoped_ptr<XmppSignalStrategy> signal_strategy,

[Sergey Ulanov, 2014/12/15 22:39:19]

s/XmppSignalStrategy/SignalStrategy/. There is nothing in this class that
require |signal_strategy| to use XMPP.

[Łukasz Anforowicz, 2014/12/16 00:59:52]

Good point.  Done.

+      scoped_ptr<SignalingConnector> signaling_connector,
+      scoped_ptr<HeartbeatSender> heartbeat_sender);
+
+  void OnAckOrTimeout(AckOrTimeout ack_or_timeout);
+
+  // Order of fields below is important for destructing them in the right order.
+  // - |heartbeat_sender_| and |signaling_connector_| have to be destructed
+  //   before |signal_strategy_| because their destructors need to call
+  //   signal_strategy_->RemoveListener(this)
+  // - |signaling_connector_| has to be destructed before
+  //   |network_change_notifier_| because its destructor needs to deregister
+  //   network change notifications
+  // - |network_task_runner_| is used by all the other fields and has to be
+  //   destructed last.
+  scoped_refptr<AutoThreadTaskRunner> network_task_runner_;
+  scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;
+  scoped_ptr<XmppSignalStrategy> signal_strategy_;
+  scoped_ptr<SignalingConnector> signaling_connector_;
+  scoped_ptr<HeartbeatSender> heartbeat_sender_;
+
+  DISALLOW_COPY_AND_ASSIGN(MinimumHeartbeatSupporter);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_MINIMUM_HEARTBEAT_SUPPORTER_H_