Clean up wake on wifi handling

chrome/browser/chromeos/net/wake_on_wifi_manager.h

+// Copyright (c) 2014 The Chromium Authors. All rights reserved.

[Lei Zhang, 2014/11/18 01:57:54]

nit: no (c), same in .cc file.

[Chirantan Ekbote, 2014/11/18 20:37:39]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_CHROMEOS_NET_WAKE_ON_WIFI_MANAGER_H_
+#define CHROME_BROWSER_CHROMEOS_NET_WAKE_ON_WIFI_MANAGER_H_
+
+#include <map>
+
+#include "base/macros.h"
+#include "base/memory/linked_ptr.h"

[Lei Zhang, 2014/11/18 01:57:54]

Can you use base/containers/scoped_ptr_hash_map.h and avoid using linked_ptrs?
See http://crbug.com/137767

[Chirantan Ekbote, 2014/11/18 02:16:17]

Ok

[Chirantan Ekbote, 2014/11/18 20:37:39]

Done.

+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+
+class Profile;
+
+namespace chromeos {
+
+// This class is responsible for managing the various wake-on-wifi related bits
+// of functionality in chrome.  It is responsible for communicating the user's
+// preferences to shill as well as listening for connections to the Google GCM
+// servers and sending that connection information down to shill.  This class is
+// owned by ChromeBrowserMainPartsChromeos.

[Lei Zhang, 2014/11/18 01:57:55]

Is this class single-threaded? It runs on the UI thread, right?

[Chirantan Ekbote, 2014/11/18 02:16:17]

Yes, this class is not thread-safe.  I will add a comment to that effect.

[Lei Zhang, 2014/11/18 02:22:38]

Please sprinkle in a few DCHECK_CURRENTLY_ON() calls to enforce this.

[Chirantan Ekbote, 2014/11/18 20:37:39]

Done.

+class WakeOnWifiManager : public content::NotificationObserver {
+ public:
+  enum WakeOnWifiFeature {
+    WAKE_ON_NONE            = 0,
+    WAKE_ON_PACKET          = 1,
+    WAKE_ON_SSID            = 2,
+    WAKE_ON_PACKET_AND_SSID = 3,
+  };
+
+  static WakeOnWifiManager* Get();

[Lei Zhang, 2014/11/18 01:57:55]

Do you really need this and |g_wake_on_wifi_manager| ? Can the caller just go
through |g_browser_process| ?

[Chirantan Ekbote, 2014/11/18 02:16:17]

I'm not sure I follow what you mean.  How do you get to BrowserMainParts from
|g_browser_process|?  From what I can tell it is a private member variable of
BrowserMainLoop with no accessor.  |g_wake_on_wifi_manager| is local to the .cc
file and I don't know how Get() can access the pointer to the class without it.

[Lei Zhang, 2014/11/18 02:22:38]

Sorry, I was thinking of BrowserProcess owning this. Nevermind.

+
+  WakeOnWifiManager();
+  ~WakeOnWifiManager() override;
+
+  // Should be called whenever the user changes their preference for the

[michaelpg, 2014/11/18 01:31:59]

nit: whenever the *primary* user changes their preference

(although technically the UI shouldn't allow non-primary users to change their
preference.)

[Chirantan Ekbote, 2014/11/18 01:54:35]

Done.

+  // wake-on-wifi features that should be enabled.
+  void OnPreferenceChanged(WakeOnWifiFeature feature);
+
+  // content::NotificationObserver override.
+  void Observe(int type,
+               const content::NotificationSource& source,
+               const content::NotificationDetails& details) override;
+
+ private:
+  void OnProfileAdded(Profile* profile);
+  void OnProfileDestroyed(Profile* profile);
+
+  class WakeOnPacketConnectionObserver;
+  std::map<Profile*, linked_ptr<WakeOnPacketConnectionObserver> >

[michaelpg, 2014/11/18 01:31:59]

nit: use >> without space

[Chirantan Ekbote, 2014/11/18 01:54:35]

Done.

+      connection_observers_;
+
+  content::NotificationRegistrar registrar_;
+
+  DISALLOW_COPY_AND_ASSIGN(WakeOnWifiManager);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_NET_WAKE_ON_WIFI_MANAGER_H_