Bootstrapping device lister plus tests

chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc

Index: chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc
diff --git a/chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc b/chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1d0ab3b2f9627b4fc18342533e258c22e4e22255
--- /dev/null
+++ b/chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc
@@ -0,0 +1,178 @@
+// 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 "chrome/browser/local_discovery/wifi/bootstrapping_device_lister.h"
+
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/message_loop/message_loop_proxy.h"
+
+namespace local_discovery {
+
+namespace wifi {
+
+namespace {
+
+const char kPrivetSuffix[] = "prv";
+// 3 for prv, 3 for type, one for connection status.
+const size_t kPrivetCharactersAfterSeparator = 7;
+
+const struct {
+  const char* short_name;

[Vitaly Buka (NO REVIEWS), 2014/05/22 05:51:36]

const char* const

[Noam Samuel, 2014/05/22 18:43:59]

Done.

+  const char* long_name;
+} kPrivetShortNames[] = {
+      {"cam", "camera"},

[Vitaly Buka (NO REVIEWS), 2014/05/22 05:51:36]

I thing it should be 2 spaces

[Noam Samuel, 2014/05/22 18:43:59]

I'd be reluctant to make any changes clang-format will just reverse again.

+      {"pri", "printer"},
+      {NULL, NULL}  // Sentinel value
+};
+
+const char* kPrivetDeviceLongName = "device";

[Vitaly Buka (NO REVIEWS), 2014/05/22 05:51:36]

const char kPrivetDeviceLongName[]

[Noam Samuel, 2014/05/22 18:43:59]

Done.

+
+std::string ExpandDeviceKind(const std::string& device_kind_short) {
+  int i = 0;
+
+  while (kPrivetShortNames[i].short_name) {

[Vitaly Buka (NO REVIEWS), 2014/05/22 05:51:36]

for loop is better

[Noam Samuel, 2014/05/22 18:43:59]

Done.

+    if (device_kind_short == kPrivetShortNames[i].short_name) {
+      return kPrivetShortNames[i].long_name;
+    }
+
+    i++;
+  }
+  return kPrivetDeviceLongName;
+}
+
+BootstrappingDeviceDescription::ConnectionStatus GetConnectionStatus(
+    char signifier) {
+  if (signifier == 'O')
+    return BootstrappingDeviceDescription::ONLINE;
+  if (signifier == 'F')
+    return BootstrappingDeviceDescription::OFFLINE;
+  if (signifier == 'C')
+    return BootstrappingDeviceDescription::CONNECTING;
+  if (signifier == 'N')
+    return BootstrappingDeviceDescription::NOT_CONFIGURED;
+  if (signifier == 'L')
+    return BootstrappingDeviceDescription::LOCAL_ONLY;
+
+  LOG(WARNING) << "Unknown WiFi connection state signifier " << signifier;
+  return BootstrappingDeviceDescription::NOT_CONFIGURED;
+}
+
+// Return whether or not the ssid ends with ".privet" and has two periods
+bool IsPrivetSSID(const std::string& ssid) {
+  bool ends_with_privet =

[Vitaly Buka (NO REVIEWS), 2014/05/22 05:51:36]

EndsWith()

[Noam Samuel, 2014/05/22 18:43:59]

Done.

+      (ssid.length() >= sizeof(kPrivetSuffix) - 1 &&
+       ssid.rfind(kPrivetSuffix) == ssid.length() - sizeof(kPrivetSuffix) + 1);
+
+  if (!ends_with_privet)
+    return false;
+
+  size_t at_pos = ssid.rfind('@');
+
+  if (at_pos == std::string::npos)

[Vitaly Buka (NO REVIEWS), 2014/05/22 05:51:36]

if (ssid[ssid.length() - kPrivetCharactersAfterSeparator - 1] != '@' &&
    /*maybe*/ count of '@' != 1)
return false

[Noam Samuel, 2014/05/22 18:43:59]

Done.

+    return false;
+
+  if (ssid.length() - at_pos != kPrivetCharactersAfterSeparator + 1)
+    return false;

[Vitaly Buka (NO REVIEWS), 2014/05/22 05:51:36]

or maybe just use base::SplitString

[Noam Samuel, 2014/05/22 18:43:59]

Did the first one.

+
+  return true;
+}
+
+void FillBootstrappingDeviceDescription(
+    const std::string internal_id,

[Vitaly Buka (NO REVIEWS), 2014/05/22 05:51:36]

string&

[Noam Samuel, 2014/05/22 18:43:59]

Done.

+    const std::string ssid,
+    BootstrappingDeviceDescription* description) {
+  description->device_network_id = internal_id;
+  description->device_ssid = ssid;
+
+  size_t at_pos = ssid.rfind('@');

[Vitaly Buka (NO REVIEWS), 2014/05/22 05:51:36]

base::SplitString ?

[Noam Samuel, 2014/05/22 18:43:59]

We don't really use the split strings directly, so I'm not sure it entirely
makes sense here, since we'd have to get substrs anyways.

+
+  description->device_name = ssid.substr(0, at_pos);
+  description->device_kind = ExpandDeviceKind(ssid.substr(at_pos + 1, 3));
+  description->connection_status = GetConnectionStatus(ssid.at(at_pos + 4));
+}
+
+}  // namespace
+
+BootstrappingDeviceDescription::BootstrappingDeviceDescription() {
+}

[Vitaly Buka (NO REVIEWS), 2014/05/22 05:51:36]

: connection_status(SOMETHING)

[Noam Samuel, 2014/05/22 18:43:59]

Done.

+
+BootstrappingDeviceDescription::~BootstrappingDeviceDescription() {
+}
+
+BootstrappingDeviceLister::BootstrappingDeviceLister(
+    WifiManager* wifi_manager,
+    const UpdateCallback& update_callback)
+    : wifi_manager_(wifi_manager),
+      update_callback_(update_callback),
+      started_(false),
+      weak_factory_(this) {
+}
+
+void BootstrappingDeviceLister::Start() {
+  DCHECK(!started_);
+
+  started_ = true;
+
+  network_watcher_ = wifi_manager_->CreateNetworkListWatcher(base::Bind(

[Vitaly Buka (NO REVIEWS), 2014/05/22 05:51:36]

break before base::Bind

[Noam Samuel, 2014/05/22 18:43:59]

Again, clang-format reverts this change when I run it. Unless this is really
bothersome, I'd prefer to keep the formatting clang-format uses.

+      &BootstrappingDeviceLister::OnGotSSIDList, base::Unretained(this)));
+
+  network_watcher_->Start();
+
+  wifi_manager_->GetSSIDList(base::Bind(
+      &BootstrappingDeviceLister::OnGotSSIDList, weak_factory_.GetWeakPtr()));
+}
+
+void BootstrappingDeviceLister::OnGotSSIDList(
+    const std::vector<NetworkProperties>& ssids) {
+  ActiveDeviceMap active_devices;
+
+  for (std::vector<NetworkProperties>::const_iterator i = ssids.begin();
+       i != ssids.end();

[Vitaly Buka (NO REVIEWS), 2014/05/22 05:51:36]

usually size_t i = 0 ... for are shorter

[Noam Samuel, 2014/05/22 18:43:59]

Done.

+       i++) {
+    if (IsPrivetSSID(i->ssid)) {
+      active_devices[i->ssid] = i->internal_id;
+    }
+  }
+
+  base::WeakPtr<BootstrappingDeviceLister> weak_this =
+      weak_factory_.GetWeakPtr();
+
+  // Find new or changed SSIDs
+  for (ActiveDeviceMap::iterator i = active_devices.begin();
+       weak_this && i != active_devices.end();

[Vitaly Buka (NO REVIEWS), 2014/05/22 05:51:36]

same

[Noam Samuel, 2014/05/22 18:43:59]

This is iteration over a map.

+       i++) {
+    ActiveDeviceMap::iterator found = active_devices_.find(i->first);
+    // Send an update if the network was not last available, or if its internal
+    // ID has changed.
+    if (found == active_devices_.end() || found->second != i->second) {
+      BootstrappingDeviceDescription description;
+      FillBootstrappingDeviceDescription(i->second, i->first, &description);
+      update_callback_.Run(true, description);
+    }
+  }
+
+  // Swap active_devices_ and active_devices now so we're not iterating over and
+  // instance variable when the object may be deleted. Also, because we need to
+  // swap them at some point.
+  active_devices_.swap(active_devices);
+
+  // Find removed SSIDs
+  for (ActiveDeviceMap::iterator i = active_devices.begin();
+       weak_this && i != active_devices.end();
+       i++) {
+    if (active_devices_.find(i->first) == active_devices_.end()) {
+      BootstrappingDeviceDescription description;
+      FillBootstrappingDeviceDescription(i->second, i->first, &description);
+      update_callback_.Run(false, description);
+    }
+  }
+}
+
+BootstrappingDeviceLister::~BootstrappingDeviceLister() {
+}
+
+}  // namespace wifi
+
+}  // namespace local_discovery