arc: Add InstanceHelper::GetInstanceForMethod()

components/arc/net/arc_net_host_impl.cc

 // Copyright 2015 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 "components/arc/net/arc_net_host_impl.h"
 
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "chromeos/login/login_state.h"
 #include "chromeos/network/managed_network_configuration_handler.h"
 #include "chromeos/network/network_connection_handler.h"
 #include "chromeos/network/network_handler.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
 #include "chromeos/network/network_type_pattern.h"
 #include "chromeos/network/network_util.h"
 #include "chromeos/network/onc/onc_utils.h"
 #include "components/arc/arc_bridge_service.h"
 
 namespace {
 
-const int kGetNetworksListLimit = 100;
+constexpr int kGetNetworksListLimit = 100;
+constexpr uint32_t kDefaultMinInstanceVersion = 0;

[Yusuke Sato, 2016/09/16 23:58:49]

same, remove?

[Luis Héctor Chávez, 2016/09/17 00:30:53]

Acknowledged.

+constexpr uint32_t kDefaultNetworkChangedMinInstanceVersion = 2;
+constexpr uint32_t kDeviceListChangedMinInstanceVersion = 3;

[Yusuke Sato, 2016/09/16 23:58:49]

I think this should be renamed to kWifiEnabledStateChanged.

[Luis Héctor Chávez, 2016/09/17 00:30:53]

Done.

 
 chromeos::NetworkStateHandler* GetStateHandler() {
   return chromeos::NetworkHandler::Get()->network_state_handler();
 }
 
 chromeos::ManagedNetworkConfigurationHandler* GetManagedConfigurationHandler() {
   return chromeos::NetworkHandler::Get()
       ->managed_network_configuration_handler();
 }
 
@@ skipping 524 matching lines @@
         chromeos::network_handler::ErrorCallback());
     callback.Run(true);
   }
 }
 
 void ArcNetHostImpl::StartScan() {
   GetStateHandler()->RequestScan();
 }
 
 void ArcNetHostImpl::ScanCompleted(const chromeos::DeviceState* /*unused*/) {
-  if (!arc_bridge_service()->net()->instance()) {
-    VLOG(2) << "NetInstance not ready yet";
+  auto* net_instance = arc_bridge_service()->net()->GetInstanceForVersion(
+      kDefaultMinInstanceVersion, "ScanCompleted");
+  if (!net_instance)
     return;
-  }
-  if (arc_bridge_service()->net()->version() < 1) {
-    VLOG(1) << "NetInstance does not support ScanCompleted.";
-    return;
-  }
 
-  arc_bridge_service()->net()->instance()->ScanCompleted();
+  net_instance->ScanCompleted();
 }
 
 void ArcNetHostImpl::GetDefaultNetwork(
     const GetDefaultNetworkCallback& callback) {
   const chromeos::NetworkState* default_network =
       GetStateHandler()->DefaultNetwork();
   if (!default_network) {
     VLOG(1) << "GetDefaultNetwork: no default network";
     callback.Run(nullptr, nullptr);
     return;
   }
   VLOG(1) << "GetDefaultNetwork: default network is "
           << default_network->path();
   std::string user_id_hash = chromeos::LoginState::Get()->primary_user_hash();
   GetManagedConfigurationHandler()->GetProperties(
       user_id_hash, default_network->path(),
       base::Bind(&GetDefaultNetworkSuccessCallback, callback),
       base::Bind(&GetDefaultNetworkFailureCallback, callback));
 }
 
 void ArcNetHostImpl::DefaultNetworkSuccessCallback(
     const std::string& service_path,
     const base::DictionaryValue& dictionary) {
-  if (!arc_bridge_service()->net()->instance()) {
-    VLOG(2) << "NetInstance is null.";
+  auto* net_instance = arc_bridge_service()->net()->GetInstanceForVersion(
+      kDefaultMinInstanceVersion, "GetDefaultNetwork");

[Yusuke Sato, 2016/09/16 23:58:49]

* Not sure why kDefaultMinInstanceVersion is used here. I believe this should be
kDefaultNetworkChangedMinInstanceVersion.
* Also, the second param should be "DefaultNetworkChanged"?

[Luis Héctor Chávez, 2016/09/17 00:30:53]

The thing is that there are two "optional" parameters to this function: the
version (default to 0), and the method name (default to nullptr).  I didn't want
to create 4 very similar functions, so adding the version=0 was a compromise.

I can also move the version to a template parameter (with a default of 0 so it
can be omitted), but i'm not sure if that's more or less readable.

+  if (!net_instance)
     return;
-  }
-  arc_bridge_service()->net()->instance()->DefaultNetworkChanged(
-      TranslateONCConfiguration(&dictionary),
-      TranslateONCConfiguration(&dictionary));
+
+  net_instance->DefaultNetworkChanged(TranslateONCConfiguration(&dictionary),
+                                      TranslateONCConfiguration(&dictionary));
 }
 
 void ArcNetHostImpl::DefaultNetworkChanged(
     const chromeos::NetworkState* network) {
-  if (arc_bridge_service()->net()->version() < 2) {
-    VLOG(1) << "ArcBridgeService does not support DefaultNetworkChanged.";
+  auto* net_instance = arc_bridge_service()->net()->GetInstanceForVersion(

[Yusuke Sato, 2016/09/16 23:58:49]

I think you can move this to line 624.

[Luis Héctor Chávez, 2016/09/17 00:30:54]

Done.

+      kDefaultNetworkChangedMinInstanceVersion, "DefaultNetworkChanged");
+  if (!net_instance)
+    return;
+
+  if (!network) {
+    VLOG(1) << "No default network";
+    net_instance->DefaultNetworkChanged(nullptr, nullptr);
     return;
   }
 
-  if (!network) {
-    VLOG(1) << "No default network";
-    arc_bridge_service()->net()->instance()->DefaultNetworkChanged(nullptr,
-                                                                   nullptr);
-    return;
-  }
-
   VLOG(1) << "New default network: " << network->path();
   std::string user_id_hash = chromeos::LoginState::Get()->primary_user_hash();
   GetManagedConfigurationHandler()->GetProperties(
       user_id_hash, network->path(),
       base::Bind(&arc::ArcNetHostImpl::DefaultNetworkSuccessCallback,
                  weak_factory_.GetWeakPtr()),
       base::Bind(&DefaultNetworkFailureCallback));
 }
 
 void ArcNetHostImpl::DeviceListChanged() {
-  if (arc_bridge_service()->net()->version() < 3) {
-    VLOG(1) << "ArcBridgeService does not support DeviceListChanged.";
+  auto* net_instance = arc_bridge_service()->net()->GetInstanceForVersion(
+      kDeviceListChangedMinInstanceVersion, "DeviceListChanged");

[Yusuke Sato, 2016/09/16 23:58:49]

      kWifiEnabledStateChanged, "WifiEnabledStateChanged");

[Luis Héctor Chávez, 2016/09/17 00:30:53]

Done.

+  if (!net_instance)
     return;
-  }
 
   bool is_enabled = GetStateHandler()->IsTechnologyEnabled(
       chromeos::NetworkTypePattern::WiFi());
-  arc_bridge_service()->net()->instance()->WifiEnabledStateChanged(is_enabled);
+  net_instance->WifiEnabledStateChanged(is_enabled);
 }
 
 void ArcNetHostImpl::OnShuttingDown() {
   GetStateHandler()->RemoveObserver(this, FROM_HERE);
 }
 
 }  // namespace arc