Add DiscoveryNetworkMonitor implementation

chrome/browser/media/router/discovery/discovery_network_monitor.cc

+// Copyright (c) 2017 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/media/router/discovery/discovery_network_monitor.h"
+
+#include <unordered_set>
+
+#include "base/lazy_instance.h"
+#include "base/logging.h"
+#include "base/sha1.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
+#include "chrome/browser/media/router/discovery/discovery_network_list.h"
+#include "net/base/network_interfaces.h"
+
+namespace {
+
+using content::BrowserThread;
+
+std::string ComputeNetworkId(
+    const std::vector<DiscoveryNetworkInfo>& network_info_list) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  if (network_info_list.size() == 0) {
+    return DiscoveryNetworkMonitor::kNetworkIdDisconnected;
+  }
+  std::string network_ids;
+  for (const auto& network_info : network_info_list) {
+    network_ids += network_info.network_id;
+  }
+  if (network_ids.size() == 0) {
+    return DiscoveryNetworkMonitor::kNetworkIdUnknown;
+  }
+
+  std::vector<std::string> id_list;
+  std::transform(network_info_list.begin(), network_info_list.end(),
+                 std::back_inserter(id_list),
+                 [](const DiscoveryNetworkInfo& network_info) {
+                   return network_info.network_id;
+                 });
+  std::sort(id_list.begin(), id_list.end());

[mark a. foltz, 2017/05/26 21:38:54]

1. Suggest using std::stable_sort to preserve order of equivalents.

http://www.cplusplus.com/reference/algorithm/stable_sort/

2. Instead of transforming the original list and sorting, pass a Compare functor
to implement a custom comparison function.

3. ISTM that GetDiscoveryNetworkInfoList() could be responsible for returning
its results in a stable sorted order using the logic here, then this function is
purely about computing the id.

[btolsch, 2017/05/26 23:38:10]

1. Done.
2. Done.
3. Done.

+  std::string combined_ids;
+  for (const auto& id : id_list) {
+    combined_ids = combined_ids + "!" + id;
+  }
+
+  std::string hash = base::SHA1HashString(combined_ids);
+  return base::ToLowerASCII(base::HexEncode(hash.data(), hash.length()));
+}
+
+std::vector<DiscoveryNetworkInfo> GetNetworkInfo() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  return GetDiscoveryNetworkInfoList();
+}
+
+base::LazyInstance<DiscoveryNetworkMonitor>::Leaky g_discovery_monitor;
+
+}  // namespace
+
+DiscoveryNetworkMonitor::DiscoveryNetworkMonitor()
+    : network_id_(kNetworkIdDisconnected),
+      observers_(new base::ObserverListThreadSafe<Observer>(
+          base::ObserverListThreadSafe<
+              Observer>::NotificationType::NOTIFY_EXISTING_ONLY)) {
+  net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
+}
+
+DiscoveryNetworkMonitor::~DiscoveryNetworkMonitor() {
+  net::NetworkChangeNotifier::RemoveNetworkChangeObserver(this);
+}
+
+// static
+DiscoveryNetworkMonitor* DiscoveryNetworkMonitor::GetInstance() {
+  DiscoveryNetworkMonitor* monitor = g_discovery_monitor.Pointer();
+  if (!monitor->network_info_callback_) {
+    monitor->network_info_callback_ = base::Bind(&GetNetworkInfo);

[mark a. foltz, 2017/05/26 21:38:54]

I don't think this needs to be a base::Callback; can you just store a function
pointer to GetDiscoveryNetworkInfoList as network_info_function_?

[btolsch, 2017/05/26 23:38:10]

Done.

+  }
+  return monitor;
+}
+
+// static
+DiscoveryNetworkMonitor* DiscoveryNetworkMonitor::GetInstanceForTest(

[mark a. foltz, 2017/05/26 21:38:54]

Rather than have a separate factory function for testing, is it possible to have
a SetNetworkInfoFunctionForTest() function to override |network_info_callback_|?

[btolsch, 2017/05/26 23:38:10]

Done.

+    NetworkInfoCallback strategy) {
+  DiscoveryNetworkMonitor* monitor = g_discovery_monitor.Pointer();
+  monitor->network_info_callback_ = std::move(strategy);
+  return monitor;
+}
+
+void DiscoveryNetworkMonitor::RebindNetworkChangeObserverForTest() {
+  net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
+}
+
+void DiscoveryNetworkMonitor::AddObserver(Observer* const observer) {
+  observers_->AddObserver(observer);
+}
+
+void DiscoveryNetworkMonitor::RemoveObserver(Observer* const observer) {
+  observers_->RemoveObserver(observer);
+}
+
+void DiscoveryNetworkMonitor::Refresh(NetworkRefreshCompleteCallback callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  BrowserThread::PostTask(

[imcheng, 2017/05/26 23:49:01]

Can you just use PostTaskAndReply here?

[btolsch, 2017/05/30 09:54:30]

Done.

+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&DiscoveryNetworkMonitor::UpdateNetworkInfo,
+                 base::Unretained(this), callback));
+}
+
+const std::string& DiscoveryNetworkMonitor::GetNetworkId() const {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  return network_id_;
+}
+
+void DiscoveryNetworkMonitor::OnNetworkChanged(
+    net::NetworkChangeNotifier::ConnectionType) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);

[imcheng, 2017/05/26 23:49:01]

This means the DiscoveryNetworkMonitor constructor (or at least the
AddNetworkChangeObserver call) has to be called from the UI thread. Is that
necessary true?

[btolsch, 2017/05/30 09:54:30]

You're right, that's not necessary.  Removed.

Similarly, with the PostTaskAndReply change, it's actually not necessary to
restrict Refresh to the UI thread either.

+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&DiscoveryNetworkMonitor::UpdateNetworkInfo,
+                 base::Unretained(this), NetworkRefreshCompleteCallback()));
+}
+
+// static

[mark a. foltz, 2017/05/26 21:38:54]

Please reorder definitions in this file to match declaration order in the .h

[btolsch, 2017/05/26 23:38:10]

Done.

+constexpr char DiscoveryNetworkMonitor::kNetworkIdDisconnected[] =
+    "disconnected";
+// static
+constexpr char DiscoveryNetworkMonitor::kNetworkIdUnknown[] = "unknown";
+
+void DiscoveryNetworkMonitor::UpdateNetworkInfo(
+    const NetworkRefreshCompleteCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  auto network_info_list = network_info_callback_.Run();
+  auto network_id = ComputeNetworkId(network_info_list);
+
+  networks_.swap(network_info_list);
+  network_id_.swap(network_id);
+
+  if (networks_.size() != network_info_list.size() ||

[mark a. foltz, 2017/05/26 21:38:54]

Can you compare network_id and the previous value of |network_id_| instead?  I
think the point of network_id is to hash the contents of a specific network
list.

[btolsch, 2017/05/26 23:38:10]

As mentioned before, there's a collision risk but I will compare network IDs
instead.

+      !std::is_permutation(networks_.begin(), networks_.end(),
+                           network_info_list.begin())) {
+    observers_->Notify(FROM_HERE, &Observer::OnNetworksChanged,
+                       base::ConstRef(*this));
+  }
+  if (callback) {
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback);
+  }
+}