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

remoting/host/host_signaling_manager.cc

+// 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.
+
+#include "remoting/host/host_signaling_manager.h"
+
+#include "base/bind.h"
+#include "base/time/time.h"
+#include "net/base/network_change_notifier.h"
+#include "net/socket/client_socket_factory.h"
+#include "remoting/base/auto_thread_task_runner.h"
+#include "remoting/base/logging.h"
+#include "remoting/base/url_request_context_getter.h"
+#include "remoting/host/chromoting_host_context.h"
+#include "remoting/host/dns_blackhole_checker.h"
+#include "remoting/host/heartbeat_sender.h"
+#include "remoting/host/signaling_connector.h"
+#include "remoting/signaling/xmpp_signal_strategy.h"
+
+namespace remoting {
+
+HostSignalingManager::HostSignalingManager(
+    scoped_refptr<AutoThreadTaskRunner> network_task_runner,
+    scoped_ptr<net::NetworkChangeNotifier> network_change_notifier,
+    scoped_ptr<SignalStrategy> signal_strategy,
+    scoped_ptr<SignalingConnector> signaling_connector,
+    scoped_ptr<HeartbeatSender> heartbeat_sender)
+    : network_task_runner_(network_task_runner),
+      network_change_notifier_(network_change_notifier.Pass()),
+      signal_strategy_(signal_strategy.Pass()),
+      signaling_connector_(signaling_connector.Pass()),
+      heartbeat_sender_(heartbeat_sender.Pass()),
+      self_(nullptr) {
+}
+
+scoped_ptr<HostSignalingManager> HostSignalingManager::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,

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

You only use network_task_runner() and url_context_getter() so you should just
pass those in directly, rather than the entire context.

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

I was passing them separately in the first draft, but 1) I prefer smaller number
of consolidated arguments especially since 2) they are passed by *const*-ref and
3) Lambros already complained in his feedback that this method has too many
parameters.

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

It has too many parameters in part because of the callbacks not being wrapped
into an interface, and not using things like e.g. OAuthCredentials to
encapsulate groups of related parameters, though.

Also consider whether any of these parameters are really only needed for
testing, or for special-cases, in which case those would be good candidates for
separate setter functions to configure.

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

Point taken on OAuthCredentials - it will let me remove oauth_refresh_token and
use_service_account from the parameters list (and replace them with
oauth_credentials).

I am not convinced about callbacks-vs-interface and not sure if there is
anything to be done with the remaining parameters.  Let's chat about it?
 
All of the parameters are used in the main control path (i.e. all of them were
originally in HostProcess and used to construct the fields that are now hidden
behind HostSignalingManager).  Therefore I think separate setters (or some kind
of a "builder" pattern) are not necessary and would only complicate things
unnecessarily.

+    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) {
+  DCHECK(host_context.network_task_runner()->BelongsToCurrentThread());
+
+  // Create a NetworkChangeNotifier for use by the signaling connector.

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

This comment seems to add nothing?

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

I am just copy&pasting from the code that was in HostProcess class.  Somebody
else thought that this comment was useful (when inserting it in the first
place).  I can remove, if you feel strongly about this.

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

Given that none of the code below is commented, I'd prefer we remove this for
consistency, and because it adds nothing.

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

Done.

+  scoped_ptr<net::NetworkChangeNotifier> network_change_notifier(
+      net::NetworkChangeNotifier::Create());
+
+  scoped_ptr<XmppSignalStrategy> signal_strategy(new XmppSignalStrategy(

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

nit: Here & below, IIRC style guide prefers wrapping at the outermost point that
makes sense, e.g. in this case immediately after signal_strategy( unless that
makes the wrapping much more cumbersome.

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

IIRC Lambros complained that this fits on a single line so I folded.  This also
passes git cl format, so I'll just leave it as is.

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

Acknowledged.

+      net::ClientSocketFactory::GetDefaultFactory(),
+      host_context.url_request_context_getter(), xmpp_server_config));
+
+  scoped_ptr<DnsBlackholeChecker> dns_blackhole_checker(new DnsBlackholeChecker(
+      host_context.url_request_context_getter(), talkgadget_prefix));
+
+  scoped_ptr<SignalingConnector> signaling_connector(new SignalingConnector(
+      signal_strategy.get(), dns_blackhole_checker.Pass(),
+      on_auth_failed_callback));
+
+  if (!oauth_refresh_token.empty()) {

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

What does it mean for the OAuth refresh token to be empty? What happens then?

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

I don't know.  Just copy&pasting existing code from HostProcess.  If we suspect
that something is wrong (or simply suspicious/smelly) here then I'd prefer to
tackle this separately from this changelist.

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

Acknowledged. Can you file a cleanup bug for us to review the code for that?

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

Done - crbug.com/446646

+    scoped_ptr<OAuthTokenGetter::OAuthCredentials> oauth_credentials(
+        new OAuthTokenGetter::OAuthCredentials(xmpp_server_config.username,
+                                               oauth_refresh_token,
+                                               use_service_account));
+
+    scoped_ptr<OAuthTokenGetter> oauth_token_getter(
+        new OAuthTokenGetter(oauth_credentials.Pass(),
+                             host_context.url_request_context_getter(), false));
+
+    signaling_connector->EnableOAuth(oauth_token_getter.Pass());
+  }
+
+  scoped_ptr<HeartbeatSender> heartbeat_sender(new HeartbeatSender(
+      on_heartbeat_successful_callback, on_unknown_host_id_error_callback,
+      host_id, signal_strategy.get(), key_pair, directory_bot_jid));
+
+  return scoped_ptr<HostSignalingManager>(new HostSignalingManager(
+      host_context.network_task_runner(), network_change_notifier.Pass(),
+      signal_strategy.Pass(), signaling_connector.Pass(),
+      heartbeat_sender.Pass()));
+}
+
+HostSignalingManager::~HostSignalingManager() {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+
+  HOST_LOG << "HostSignalingManager destroyed.";

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

Is this really important enough to put in the host log? Maybe [D]VLOG(2) it?

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

I hoped that this would help diagnose the case of a hang host (when you might
not have an opportunity to restart with more verbose logging).  OTOH, I don't
feel strongly about this (and admittedly I don't have much experience judging
how much logging is too little vs too much).

I'll leave it in for now, but please push back if you think I should change to
VLOG(2).

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

I think it's fine to remove this since you already log the ACK and timeout
cases.

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

Done.

+}
+
+void HostSignalingManager::SendHostOfflineReasonAndDelete(
+    const std::string& host_offline_reason,
+    const base::TimeDelta& timeout) {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+
+  HOST_LOG << "SendHostOfflineReason: trying to send " << host_offline_reason
+           << " to the bot";
+
+  self_ = this;  // We own ourselves until the ack-or-timeout callback fires.

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

This has no effect, though..?

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

Yes.  I don't know what I was thinking... :-(

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

Acknowledged.

+  heartbeat_sender_->SetHostOfflineReason(
+      host_offline_reason, timeout,
+      base::Bind(&HostSignalingManager::OnAckOrTimeout,
+                 base::Unretained(this)));
+}
+
+void HostSignalingManager::OnAckOrTimeout(
+    HeartbeatSender::AckOrTimeout ack_or_timeout) {

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

As noted elsewhere, suggest re-expressing this as a simple |bool success|

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

Done.

+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  switch (ack_or_timeout) {
+    case HeartbeatSender::Ack:
+      HOST_LOG << "SendHostOfflineReason: got ack";
+      break;
+    case HeartbeatSender::Timeout:
+      HOST_LOG << "SendHostOfflineReason: timed out";
+      break;
+    default:

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

You're already handling all the possible values, so get rid of the "default"
case.

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

N/A after the switch to bool.

+      NOTREACHED();
+      break;
+  }
+
+  delete self_;

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

delete this

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

Right.

+}
+
+}  // namespace remoting