Clean up wake on wifi handling

chrome/browser/chromeos/net/wake_on_wifi_manager.cc

Index: chrome/browser/chromeos/net/wake_on_wifi_manager.cc
diff --git a/chrome/browser/chromeos/net/wake_on_wifi_manager.cc b/chrome/browser/chromeos/net/wake_on_wifi_manager.cc
new file mode 100644
index 0000000000000000000000000000000000000000..30710d27e131535f1f7a951719449f3bbcd25d41
--- /dev/null
+++ b/chrome/browser/chromeos/net/wake_on_wifi_manager.cc
@@ -0,0 +1,201 @@
+// Copyright (c) 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 "chrome/browser/chromeos/net/wake_on_wifi_manager.h"
+
+#include <string>
+
+#include "base/bind.h"
+#include "base/command_line.h"
+#include "base/logging.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/services/gcm/gcm_profile_service.h"
+#include "chrome/browser/services/gcm/gcm_profile_service_factory.h"
+#include "chromeos/network/device_state.h"
+#include "chromeos/network/network_device_handler.h"
+#include "chromeos/network/network_handler.h"
+#include "chromeos/network/network_state_handler.h"
+#include "chromeos/network/network_type_pattern.h"
+#include "components/gcm_driver/gcm_connection_observer.h"
+#include "components/gcm_driver/gcm_driver.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_source.h"
+#include "net/base/ip_endpoint.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
+
+namespace chromeos {
+
+namespace {

[stevenjb, 2014/11/13 22:20:17]

nit: WS

[Chirantan Ekbote, 2014/11/14 02:47:43]

Done.

+const char kWakeOnNone[] = "none";
+const char kWakeOnPacket[] = "packet";
+const char kWakeOnSsid[] = "ssid";
+const char kWakeOnPacketAndSsid[] = "packet_and_ssid";
+
+std::string WakeOnWifiFeatureToString(
+    chromeos::WakeOnWifiManager::WakeOnWifiFeature feature) {
+  switch (feature) {
+    case chromeos::WakeOnWifiManager::WAKE_ON_NONE:
+      return kWakeOnNone;
+    case chromeos::WakeOnWifiManager::WAKE_ON_PACKET:
+      return kWakeOnPacket;
+    case chromeos::WakeOnWifiManager::WAKE_ON_SSID:
+      return kWakeOnSsid;
+    case chromeos::WakeOnWifiManager::WAKE_ON_PACKET_AND_SSID:
+      return kWakeOnPacketAndSsid;
+    default:

[stevenjb, 2014/11/13 22:20:16]

No default so the compiler can catch missing enums. Put the NOTREACHED outside
the switch.

[Chirantan Ekbote, 2014/11/14 02:47:43]

Done.

+      NOTREACHED() << "Unknown wake on wifi feature: " << feature;
+  }
+
+  return std::string();
+}
+
+// Weak pointer.  This class is owned by ChromeBrowserMainPartsChromeos.
+chromeos::WakeOnWifiManager* g_wake_on_wifi_manager = NULL;

[stevenjb, 2014/11/13 22:20:16]

Rather than exposing a weak global pointer and making Preferences depend on this
class, I think you can use PrefChangeRegistrar to register and observe changes
to the preference within this class.

[Chirantan Ekbote, 2014/11/13 23:12:37]

PrefChangeRegistrar still needs a PrefService before any observers can be
registered.  So I would need to hook this into ProfileImpl if we don't want it
in Preferences.

+
+}  // namespace
+
+// Simple class that listens for a connection to the GCM server and passes the
+// connection information down to shill.  Each profile gets its own instance of
+// this class.

[stevenjb, 2014/11/13 22:20:17]

I believe that the correct behavior is that there should only be one instance of
this for the primary user profile. All Shill configuration applies to the
primary user.

[Chirantan Ekbote, 2014/11/13 23:12:37]

So I don't know much about how profiles work.  Is the primary user the one who
signs in from the login screen?  Assuming it is then this is what I think you
are saying:

- A user signs in via the login screen and we configure shill with that user's
preferences.  We also set up a GCMConnectionObserver to observe connections to
the GCM server and send those down to shill.
- The user then adds another account via the status area.  We ignore this
profiles's wake on wifi preferences and we don't observe this profile's
connection to the GCM server.

So even if the primary profile chooses to wake on GCM packets we still will not
wake up for packets sent to the secondary profile.  It seems to me that what we
should do is a combination of the two: respect the primary profile's preference
but still wake on packets for the secondary profile if the primary profile has
enabled it.

[stevenjb, 2014/11/13 23:24:46]

Yes, "primary" refers to the profile used to initially sign in.

I see, GCMConnectionObserver is per-profile. So yes, you are correct, we want a
single Manager instance using the preference associated with the primary
profile, with an observer for each profile (primary and secondary). Apologies
for my confusion.

+class WakeOnWifiManager::WakeOnPacketConnectionObserver
+    : public gcm::GCMConnectionObserver {
+ public:
+  explicit WakeOnPacketConnectionObserver(Profile* profile)
+      : profile_(profile),
+        ip_endpoint_(net::IPEndPoint()) {
+    gcm::GCMProfileServiceFactory::GetForProfile(profile_)
+        ->driver()
+        ->AddConnectionObserver(this);
+  }
+
+  ~WakeOnPacketConnectionObserver() override {
+    if (!(ip_endpoint_ == net::IPEndPoint()))
+      OnDisconnected();
+
+    gcm::GCMProfileServiceFactory::GetForProfile(profile_)
+        ->driver()
+        ->RemoveConnectionObserver(this);
+  }
+
+  // gcm::GCMConnectionObserver overrides.

[stevenjb, 2014/11/13 22:20:16]

When inlining overrides, they need to be spaced apart (i.e. WS between line 93
and 94), so it is also better to put a blank line after the base class comment.

[Chirantan Ekbote, 2014/11/14 02:47:43]

Done.

+  void OnConnected(const net::IPEndPoint& ip_endpoint) override {
+    ip_endpoint_ = ip_endpoint;
+
+    NetworkHandler::Get()
+        ->network_device_handler()
+        ->AddWifiWakeOnPacketConnection(
+            ip_endpoint_,
+            base::Bind(&base::DoNothing),
+            network_handler::ErrorCallback());
+  }
+  void OnDisconnected() override {
+    if (ip_endpoint_ == net::IPEndPoint()) {
+      LOG(WARNING) << "Received GCMConnectionObserver::OnDisconnected without "
+                   << "a valid IPEndPoint.";
+      return;
+    }
+
+    NetworkHandler::Get()
+        ->network_device_handler()
+        ->RemoveWifiWakeOnPacketConnection(
+            ip_endpoint_,
+            base::Bind(&base::DoNothing),
+            network_handler::ErrorCallback());
+
+    ip_endpoint_ = net::IPEndPoint();
+  }
+
+ private:
+  Profile* profile_;
+  net::IPEndPoint ip_endpoint_;
+
+  DISALLOW_COPY_AND_ASSIGN(WakeOnPacketConnectionObserver);
+};
+
+// static
+WakeOnWifiManager* WakeOnWifiManager::Get() {
+  DCHECK(g_wake_on_wifi_manager);
+  return g_wake_on_wifi_manager;
+}
+
+WakeOnWifiManager::WakeOnWifiManager() {
+  DCHECK(!g_wake_on_wifi_manager);
+  g_wake_on_wifi_manager = this;
+
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_PROFILE_ADDED,
+                 content::NotificationService::AllBrowserContextsAndSources());
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_PROFILE_DESTROYED,
+                 content::NotificationService::AllBrowserContextsAndSources());
+
+  NetworkHandler::Get()
+      ->network_device_handler()
+      ->RemoveAllWifiWakeOnPacketConnections(
+          base::Bind(&base::DoNothing),
+          network_handler::ErrorCallback());
+}
+
+WakeOnWifiManager::~WakeOnWifiManager() {
+  DCHECK(g_wake_on_wifi_manager);
+  g_wake_on_wifi_manager = NULL;
+}
+
+void WakeOnWifiManager::OnPreferenceChanged(
+    WakeOnWifiManager::WakeOnWifiFeature feature) {
+  const DeviceState* device =
+      NetworkHandler::Get()->network_state_handler()->GetDeviceStateByType(
+          NetworkTypePattern::WiFi());
+  if (!device)
+    return;
+
+  std::string feature_string(WakeOnWifiFeatureToString(feature));
+  if (feature_string.empty())
+    return;

[stevenjb, 2014/11/13 22:20:17]

DCHECK instead since this should never be empty.

[Chirantan Ekbote, 2014/11/14 02:47:43]

Done.

+
+  NetworkHandler::Get()->network_device_handler()->SetDeviceProperty(
+      device->path(),
+      shill::kWakeOnWiFiFeaturesEnabledProperty,
+      base::StringValue(feature_string),
+      base::Bind(&base::DoNothing),
+      network_handler::ErrorCallback());
+}
+
+void WakeOnWifiManager::Observe(int type,
+                                const content::NotificationSource& source,
+                                const content::NotificationDetails& details) {
+  switch (type) {
+    case chrome::NOTIFICATION_PROFILE_ADDED: {
+      OnProfileAdded(content::Source<Profile>(source).ptr());
+      break;
+    }
+    case chrome::NOTIFICATION_PROFILE_DESTROYED: {
+      OnProfileDestroyed(content::Source<Profile>(source).ptr());
+      break;
+    }
+    default:
+      NOTREACHED();
+  }
+}
+
+void WakeOnWifiManager::OnProfileAdded(Profile* profile) {
+  if (connection_observers_.find(profile) != connection_observers_.end())
+    return;
+
+  connection_observers_[profile] =
+      linked_ptr<WakeOnWifiManager::WakeOnPacketConnectionObserver>(
+          new WakeOnWifiManager::WakeOnPacketConnectionObserver(profile));
+}
+
+void WakeOnWifiManager::OnProfileDestroyed(Profile* profile) {
+  const auto iter = connection_observers_.find(profile);
+  if (iter == connection_observers_.end())
+    return;
+
+  connection_observers_.erase(iter);
+}

[stevenjb, 2014/11/13 22:20:16]

We shouldn't need any of the above, there should just be the one connection
observer for the primary profile.

(Note: as written, if this class is constructed after the primary profile
exists, no PROFILE_ADDED notification will occur and so no observer will be
created, but again I think there should only be one observer for the primary
profile).

[Chirantan Ekbote, 2014/11/13 23:12:37]

This is why I construct this class in PreMainMessageLoopRun, which I think runs
after the browser process is created but before any profiles are created.  If
that's not actually how it works then I need to fix this.

[stevenjb, 2014/11/13 23:24:46]

OK, I think that is valid, but if we make that sort of assumption we should
document or ideally CHECK in the constructor that we are not logged in, e.g.
CHECK(!LoginState::IsUserLoggedIn()).

+
+}  // namespace chromeos