arc: Add InstanceHelper::GetInstanceForMethod()

Many developers have added utility functions to check if the many Mojo
instances are ready and with the correct version, which seems redundant.
This change adds a function to do that (and log failures).

BUG=647853
TEST=cheets_SettingsBridge

Review-Url: https://codereview.chromium.org/2347293002
Cr-Commit-Position: refs/heads/master@{#419471}

chrome/browser/chromeos/arc/arc_wallpaper_service.cc

@@ -141,27 +141,12 @@ void ArcWallpaperService::OnDecodeImageFailed() {
 
 void ArcWallpaperService::OnWallpaperDataChanged() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  mojom::WallpaperInstance* instance =
-      GetWallpaperInstance(kMinOnWallpaperChangedVersion);
-  if (!instance)
+  auto* wallpaper_instance =
+      arc_bridge_service()->wallpaper()->GetInstanceForMethod(
+          "OnWallpaperChanged", kMinOnWallpaperChangedVersion);
+  if (!wallpaper_instance)
     return;
-  instance->OnWallpaperChanged();
-}
-
-mojom::WallpaperInstance* ArcWallpaperService::GetWallpaperInstance(
-    uint32_t min_version) {
-  uint32_t version = arc_bridge_service()->wallpaper()->version();
-  if (version < min_version) {
-    VLOG(1) << "ARC wallpaper instance is too old. required: " << min_version
-            << ", actual: " << version;
-    return nullptr;
-  }
-
-  mojom::WallpaperInstance* instance =
-      arc_bridge_service()->wallpaper()->instance();
-  if (!instance)
-    VLOG(2) << "ARC wallpaper instance is not ready.";
-  return instance;
+  wallpaper_instance->OnWallpaperChanged();
 }
 
 }  // namespace arc

chrome/browser/chromeos/arc/arc_wallpaper_service.h

@@ -48,8 +48,6 @@ class ArcWallpaperService
   void OnWallpaperDataChanged() override;
 
  private:
-  mojom::WallpaperInstance* GetWallpaperInstance(uint32_t min_version);
-
   mojo::Binding<mojom::WallpaperHost> binding_;
   DISALLOW_COPY_AND_ASSIGN(ArcWallpaperService);
 };

chrome/browser/task_manager/providers/arc/arc_process_task.cc

@@ -20,6 +20,8 @@ namespace task_manager {
 
 namespace {
 
+constexpr uint32_t kKillProcessMinInstanceVersion = 1;
+
 base::string16 MakeTitle(const std::string& process_name,
                          arc::mojom::ProcessState process_state) {
   int name_template = IDS_TASK_MANAGER_ARC_PREFIX;
@@ -126,18 +128,12 @@ bool ArcProcessTask::IsKillable() {
 }
 
 void ArcProcessTask::Kill() {
-  arc::mojom::ProcessInstance* arc_process_instance =
-      arc::ArcBridgeService::Get()->process()->instance();
-  if (!arc_process_instance) {
-    LOG(ERROR) << "ARC process instance is not ready.";
-    return;
-  }
-  if (arc::ArcBridgeService::Get()->process()->version() < 1) {
-    LOG(ERROR) << "ARC KillProcess IPC is unavailable.";
+  auto* process_instance =
+      arc::ArcBridgeService::Get()->process()->GetInstanceForMethod(
+          "KillProcess", kKillProcessMinInstanceVersion);
+  if (!process_instance)
     return;
-  }
-  arc_process_instance->KillProcess(nspid_,
-                                    "Killed manually from Task Manager");
+  process_instance->KillProcess(nspid_, "Killed manually from Task Manager");
 }
 
 void ArcProcessTask::OnInstanceReady() {

chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc

@@ -354,17 +354,10 @@ void ArcAppListPrefs::SetNotificationsEnabled(const std::string& app_id,
     return;
   }
 
-  arc::mojom::AppInstance* app_instance = app_instance_holder_->instance();
-  if (!app_instance) {
-    // AppInstance should be ready since we have app_id in ready_apps_.
-    NOTREACHED();
-    return;
-  }
-
-  if (app_instance_holder_->version() < kSetNotificationsEnabledMinVersion) {
-    VLOG(2) << "app version is too small to set notifications enabled.";
+  auto* app_instance = app_instance_holder_->GetInstanceForMethod(
+      "SetNotificationsEnabled", kSetNotificationsEnabledMinVersion);
+  if (!app_instance)
     return;
-  }
 
   SetNotificationsEnabledDeferred(prefs_).Remove(app_id);
   app_instance->SetNotificationsEnabled(app_info->package_name, enabled);
@@ -1145,7 +1138,6 @@ std::vector<std::string> ArcAppListPrefs::GetPackagesFromPrefs(
       prefs_->GetDictionary(prefs::kArcPackages);
   for (base::DictionaryValue::Iterator package(*package_prefs);
        !package.IsAtEnd(); package.Advance()) {
-
     bool uninstalled = false;
     package_prefs->GetBoolean(kSystem, &uninstalled);
     if (installed != !uninstalled)

chrome/browser/ui/app_list/arc/arc_app_utils.cc

@@ -31,14 +31,14 @@ constexpr int kNexus5Width = 410;
 constexpr int kNexus5Height = 690;
 
 // Minimum required versions.
-constexpr int kMinVersion = 0;
-constexpr int kCanHandleResolutionMinVersion = 1;
-constexpr int kSendBroadcastMinVersion = 1;
-constexpr int kUninstallPackageMinVersion = 2;
-constexpr int kTaskSupportMinVersion = 3;
-constexpr int kShowPackageInfoMinVersion = 5;
-constexpr int kRemoveIconMinVersion = 9;
-constexpr int kShowPackageInfoOnPageMinVersion = 10;
+constexpr uint32_t kMinVersion = 0;
+constexpr uint32_t kCanHandleResolutionMinVersion = 1;
+constexpr uint32_t kSendBroadcastMinVersion = 1;
+constexpr uint32_t kUninstallPackageMinVersion = 2;
+constexpr uint32_t kTaskSupportMinVersion = 3;
+constexpr uint32_t kShowPackageInfoMinVersion = 5;
+constexpr uint32_t kRemoveIconMinVersion = 9;
+constexpr uint32_t kShowPackageInfoOnPageMinVersion = 10;
 
 // Service name strings.
 constexpr char kCanHandleResolutionStr[] = "get resolution capability";
@@ -51,7 +51,7 @@ constexpr char kUninstallPackageStr[] = "uninstall package";
 
 // Helper function which returns the AppInstance. Create related logs when error
 // happens.
-arc::mojom::AppInstance* GetAppInstance(int required_version,
+arc::mojom::AppInstance* GetAppInstance(uint32_t required_version,
                                         const std::string& service_name) {
   arc::ArcBridgeService* bridge_service = arc::ArcBridgeService::Get();
   if (!bridge_service) {
@@ -60,21 +60,8 @@ arc::mojom::AppInstance* GetAppInstance(int required_version,
     return nullptr;
   }
 
-  arc::mojom::AppInstance* app_instance = bridge_service->app()->instance();
-  if (!app_instance) {
-    VLOG(2) << "Request to " << service_name
-            << " when mojom::app_instance is not ready.";
-    return nullptr;
-  }
-
-  int bridge_version = bridge_service->app()->version();
-  if (bridge_version < required_version) {
-    VLOG(2) << "Request to " << service_name << " when Arc version "
-            << bridge_version << " does not support it.";
-    return nullptr;
-  }
-
-  return app_instance;
+  return bridge_service->app()->GetInstanceForMethod(service_name.c_str(),
+                                                     required_version);
 }
 
 void PrioritizeArcInstanceCallback(bool success) {
@@ -304,16 +291,11 @@ void ShowTalkBackSettings() {
     return;
   }
 
-  arc::mojom::IntentHelperInstance *intent_helper_instance =
-      bridge_service->intent_helper()->instance();
-  if (!intent_helper_instance) {
-    VLOG(2) << "ARC intent helper instance is not ready";
+  auto* intent_helper_instance =
+      bridge_service->intent_helper()->GetInstanceForMethod(
+          "SendBroadcast", kSendBroadcastMinVersion);
+  if (!intent_helper_instance)
     return;
-  }
-  if (bridge_service->intent_helper()->version() < kSendBroadcastMinVersion) {
-    VLOG(2) << "ARC intent helper instance is too old";
-    return;
-  }
 
   intent_helper_instance->SendBroadcast(
           "org.chromium.arc.intent_helper.SHOW_TALKBACK_SETTINGS",

components/arc/bluetooth/arc_bluetooth_bridge.h

@@ -331,8 +331,6 @@ class ArcBluetoothBridge
       device::BluetoothDevice* device) const;
 
   void SendCachedDevicesFound() const;
-  bool HasBluetoothInstance() const;
-  bool CheckBluetoothInstanceVersion(uint32_t version_need) const;
 
   device::BluetoothRemoteGattCharacteristic* FindGattCharacteristic(
       mojom::BluetoothAddressPtr remote_addr,

components/arc/ime/arc_ime_bridge_impl.cc

@@ -16,8 +16,8 @@
 namespace arc {
 namespace {
 
-constexpr int kMinVersionForOnKeyboardsBoundsChanging = 3;
-constexpr int kMinVersionForExtendSelectionAndDelete = 4;
+constexpr uint32_t kMinVersionForOnKeyboardsBoundsChanging = 3;
+constexpr uint32_t kMinVersionForExtendSelectionAndDelete = 4;
 
 ui::TextInputType ConvertTextInputType(arc::mojom::TextInputType ipc_type) {
   // The two enum types are similar, but intentionally made not identical.
@@ -94,64 +94,49 @@ void ArcImeBridgeImpl::OnInstanceReady() {
 
 void ArcImeBridgeImpl::SendSetCompositionText(
     const ui::CompositionText& composition) {
-  mojom::ImeInstance* ime_instance = bridge_service_->ime()->instance();
-  if (!ime_instance) {
-    LOG(ERROR) << "ArcImeInstance method called before being ready.";
+  auto* ime_instance =
+      bridge_service_->ime()->GetInstanceForMethod("SetCompositionText");
+  if (!ime_instance)
     return;
-  }
 
   ime_instance->SetCompositionText(base::UTF16ToUTF8(composition.text),
                                    ConvertSegments(composition));
 }
 
 void ArcImeBridgeImpl::SendConfirmCompositionText() {
-  mojom::ImeInstance* ime_instance = bridge_service_->ime()->instance();
-  if (!ime_instance) {
-    LOG(ERROR) << "ArcImeInstance method called before being ready.";
+  auto* ime_instance =
+      bridge_service_->ime()->GetInstanceForMethod("ConfirmCompositionText");
+  if (!ime_instance)
     return;
-  }
 
   ime_instance->ConfirmCompositionText();
 }
 
 void ArcImeBridgeImpl::SendInsertText(const base::string16& text) {
-  mojom::ImeInstance* ime_instance = bridge_service_->ime()->instance();
-  if (!ime_instance) {
-    LOG(ERROR) << "ArcImeInstance method called before being ready.";
+  auto* ime_instance =
+      bridge_service_->ime()->GetInstanceForMethod("InsertText");
+  if (!ime_instance)
     return;
-  }
 
   ime_instance->InsertText(base::UTF16ToUTF8(text));
 }
 
 void ArcImeBridgeImpl::SendOnKeyboardBoundsChanging(
     const gfx::Rect& new_bounds) {
-  mojom::ImeInstance* ime_instance = bridge_service_->ime()->instance();
-  if (!ime_instance) {
-    LOG(ERROR) << "ArcImeInstance method called before being ready.";
-    return;
-  }
-  if (bridge_service_->ime()->version() <
-      kMinVersionForOnKeyboardsBoundsChanging) {
-    LOG(ERROR) << "ArcImeInstance is too old for OnKeyboardsBoundsChanging.";
+  auto* ime_instance = bridge_service_->ime()->GetInstanceForMethod(
+      "OnKeyboardBoundsChanging", kMinVersionForOnKeyboardsBoundsChanging);
+  if (!ime_instance)
     return;
-  }
 
   ime_instance->OnKeyboardBoundsChanging(new_bounds);
 }
 
 void ArcImeBridgeImpl::SendExtendSelectionAndDelete(
     size_t before, size_t after) {
-  mojom::ImeInstance* ime_instance = bridge_service_->ime()->instance();
-  if (!ime_instance) {
-    LOG(ERROR) << "ArcImeInstance method called before being ready.";
-    return;
-  }
-  if (bridge_service_->ime()->version() <
-      kMinVersionForExtendSelectionAndDelete) {
-    LOG(ERROR) << "ArcImeInstance is too old for ExtendSelectionAndDelete.";
+  auto* ime_instance = bridge_service_->ime()->GetInstanceForMethod(
+      "ExtendSelectionAndDelete", kMinVersionForExtendSelectionAndDelete);
+  if (!ime_instance)
     return;
-  }
 
   ime_instance->ExtendSelectionAndDelete(before, after);
 }

components/arc/instance_holder.h

@@ -42,6 +42,31 @@ class InstanceHolder {
   T* instance() const { return instance_; }
   uint32_t version() const { return version_; }
 
+  // Gets the Mojo interface that's intended to call for
+  // |method_name_for_logging|, but only if its reported version is at least
+  // |min_version|. Returns nullptr if the instance is either not ready or does
+  // not have the requested version, and logs appropriately.
+  T* GetInstanceForMethod(const char* method_name_for_logging,
+                          uint32_t min_version) {
+    if (!instance_) {
+      VLOG(1) << "Instance for " << T::Name_ << "::" << method_name_for_logging
+              << " not available.";
+      return nullptr;
+    }
+    if (version_ < min_version) {
+      VLOG(1) << "Instance for " << T::Name_ << "::" << method_name_for_logging
+              << " version mismatch. Expected " << min_version << " got "
+              << version_;
+      return nullptr;
+    }
+    return instance_;
+  }
+
+  // Same as the above, but for the version zero.
+  T* GetInstanceForMethod(const char* method_name_for_logging) {
+    return GetInstanceForMethod(method_name_for_logging, 0u);
+  }
+
   // Adds or removes observers. This can only be called on the thread that this
   // class was created on. RemoveObserver does nothing if |observer| is not in
   // the list.

components/arc/net/arc_net_host_impl.cc

@@ -27,7 +27,10 @@
 
 namespace {
 
-const int kGetNetworksListLimit = 100;
+constexpr int kGetNetworksListLimit = 100;
+constexpr uint32_t kScanCompletedMinInstanceVersion = 1;
+constexpr uint32_t kDefaultNetworkChangedMinInstanceVersion = 2;
+constexpr uint32_t kWifiEnabledStateChanged = 3;
 
 chromeos::NetworkStateHandler* GetStateHandler() {
   return chromeos::NetworkHandler::Get()->network_state_handler();
@@ -572,16 +575,12 @@ void ArcNetHostImpl::StartScan() {
 }
 
 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()->GetInstanceForMethod(
+      "ScanCompleted", kScanCompletedMinInstanceVersion);
+  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(
@@ -605,26 +604,23 @@ void ArcNetHostImpl::GetDefaultNetwork(
 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()->GetInstanceForMethod(
+      "DefaultNetworkChanged", kDefaultNetworkChangedMinInstanceVersion);
+  if (!net_instance)
     return;
-  }
-  arc_bridge_service()->net()->instance()->DefaultNetworkChanged(
-      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.";
-    return;
-  }
-
   if (!network) {
     VLOG(1) << "No default network";
-    arc_bridge_service()->net()->instance()->DefaultNetworkChanged(nullptr,
-                                                                   nullptr);
+    auto* net_instance = arc_bridge_service()->net()->GetInstanceForMethod(
+        "DefaultNetworkChanged", kDefaultNetworkChangedMinInstanceVersion);
+    if (net_instance)
+      net_instance->DefaultNetworkChanged(nullptr, nullptr);
     return;
   }
 
@@ -638,14 +634,14 @@ void ArcNetHostImpl::DefaultNetworkChanged(
 }
 
 void ArcNetHostImpl::DeviceListChanged() {
-  if (arc_bridge_service()->net()->version() < 3) {
-    VLOG(1) << "ArcBridgeService does not support DeviceListChanged.";
+  auto* net_instance = arc_bridge_service()->net()->GetInstanceForMethod(
+      "WifiEnabledStateChanged", kWifiEnabledStateChanged);
+  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() {

components/arc/storage_manager/arc_storage_manager.cc

@@ -40,49 +40,35 @@ ArcStorageManager* ArcStorageManager::Get() {
 }
 
 bool ArcStorageManager::OpenPrivateVolumeSettings() {
-  auto* storage_manager_instance = GetStorageManagerInstance();
-  if (!storage_manager_instance) {
+  auto* storage_manager_instance =
+      arc_bridge_service()->storage_manager()->GetInstanceForMethod(
+          "OpenPrivateVolumeSettings", kMinInstanceVersion);
+  if (!storage_manager_instance)
     return false;
-  }
   storage_manager_instance->OpenPrivateVolumeSettings();
   return true;
 }
 
 bool ArcStorageManager::GetApplicationsSize(
     const GetApplicationsSizeCallback& callback) {
-  auto* storage_manager_instance = GetStorageManagerInstance();
-  if (!storage_manager_instance) {
+  auto* storage_manager_instance =
+      arc_bridge_service()->storage_manager()->GetInstanceForMethod(
+          "GetApplicationsSize", kMinInstanceVersion);
+  if (!storage_manager_instance)
     return false;
-  }
   storage_manager_instance->GetApplicationsSize(callback);
   return true;
 }
 
 bool ArcStorageManager::DeleteApplicationsCache(
     const base::Callback<void()>& callback) {
-  auto* storage_manager_instance = GetStorageManagerInstance();
-  if (!storage_manager_instance) {
+  auto* storage_manager_instance =
+      arc_bridge_service()->storage_manager()->GetInstanceForMethod(
+          "DeleteApplicationsCache", kMinInstanceVersion);
+  if (!storage_manager_instance)
     return false;
-  }
   storage_manager_instance->DeleteApplicationsCache(callback);
   return true;
 }
 
-mojom::StorageManagerInstance* ArcStorageManager::GetStorageManagerInstance() {
-  auto* bridge_service = arc_bridge_service();
-  auto* storage_manager_instance =
-      bridge_service->storage_manager()->instance();
-  if (!storage_manager_instance) {
-    DLOG(WARNING) << "ARC storage manager instance is not ready.";
-    return nullptr;
-  }
-  auto storage_manager_version = bridge_service->storage_manager()->version();
-  if (storage_manager_version < kMinInstanceVersion) {
-    DLOG(ERROR) << "ARC storage manager instance (version "
-                << storage_manager_version << ") is too old.";
-    return nullptr;
-  }
-  return storage_manager_instance;
-}
-
 }  // namespace arc

components/arc/storage_manager/arc_storage_manager.h

@@ -39,8 +39,6 @@ class ArcStorageManager : public ArcService {
   bool DeleteApplicationsCache(const base::Callback<void()>& callback);
 
  private:
-  mojom::StorageManagerInstance* GetStorageManagerInstance();
-
   DISALLOW_COPY_AND_ASSIGN(ArcStorageManager);
 };
 

ui/arc/notification/arc_notification_manager.cc

@@ -4,6 +4,9 @@
 
 #include "ui/arc/notification/arc_notification_manager.h"
 
+#include <memory>
+#include <utility>
+
 #include "ash/common/system/toast/toast_manager.h"
 #include "ash/common/wm_shell.h"
 #include "base/memory/ptr_util.h"
@@ -207,20 +210,10 @@ void ArcNotificationManager::CreateNotificationWindow(const std::string& key) {
   }
 
   auto* notifications_instance =
-      arc_bridge_service()->notifications()->instance();
-  // On shutdown, the ARC channel may quit earlier then notifications.
-  if (!notifications_instance) {
-    VLOG(2) << "Request to create window for ARC Notification (key: " << key
-            << "), but the ARC channel has already gone.";
+      arc_bridge_service()->notifications()->GetInstanceForMethod(
+          "CreateNotificationWindow", kMinVersionNotificationWindow);
+  if (!notifications_instance)
     return;
-  }
-
-  if (arc_bridge_service()->notifications()->version() <
-      kMinVersionNotificationWindow) {
-    VLOG(2)
-        << "NotificationInstance does not support CreateNotificationWindow.";
-    return;
-  }
 
   notifications_instance->CreateNotificationWindow(key);
 }
@@ -233,19 +226,10 @@ void ArcNotificationManager::CloseNotificationWindow(const std::string& key) {
   }
 
   auto* notifications_instance =
-      arc_bridge_service()->notifications()->instance();
-  // On shutdown, the ARC channel may quit earlier then notifications.
-  if (!notifications_instance) {
-    VLOG(2) << "Request to close window for ARC Notification (key: " << key
-            << "), but the ARC channel has already gone.";
+      arc_bridge_service()->notifications()->GetInstanceForMethod(
+          "CloseNotificationWindow", kMinVersionNotificationWindow);
+  if (!notifications_instance)
     return;
-  }
-
-  if (arc_bridge_service()->notifications()->version() <
-      kMinVersionNotificationWindow) {
-    VLOG(2) << "NotificationInstance does not support CloseNotificationWindow.";
-    return;
-  }
 
   notifications_instance->CloseNotificationWindow(key);
 }