Always use arc::InstanceHolder<T>::GetInstanceForMethod

chrome/browser/chromeos/arc/arc_navigation_throttle.cc

Index: chrome/browser/chromeos/arc/arc_navigation_throttle.cc
diff --git a/chrome/browser/chromeos/arc/arc_navigation_throttle.cc b/chrome/browser/chromeos/arc/arc_navigation_throttle.cc
index 4db4b12a3b23a32ff0badce1e5f90f7602386134..ae7f574fe6c1e60912ccdffd467c264ff4ee390d 100644
--- a/chrome/browser/chromeos/arc/arc_navigation_throttle.cc
+++ b/chrome/browser/chromeos/arc/arc_navigation_throttle.cc
@@ -27,7 +27,9 @@ namespace arc {
 
 namespace {
 
-constexpr int kMinInstanceVersion = 7;
+constexpr uint32_t kMinVersionForHandleUrl = 2;
+constexpr uint32_t kMinVersionForRequestUrlHandlerList = 2;
+constexpr uint32_t kMinVersionForAddPreferredPackage = 7;
 
 scoped_refptr<ActivityIconLoader> GetIconLoader() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -111,9 +113,9 @@ ArcNavigationThrottle::HandleRequest() {
   if (!ShouldOverrideUrlLoading(previous_url, current_url))
     return content::NavigationThrottle::PROCEED;
 
-  arc::ArcServiceManager* arc_service_manager = arc::ArcServiceManager::Get();
+  ArcServiceManager* arc_service_manager = ArcServiceManager::Get();
   DCHECK(arc_service_manager);
-  scoped_refptr<arc::LocalActivityResolver> local_resolver =
+  scoped_refptr<LocalActivityResolver> local_resolver =
       arc_service_manager->activity_resolver();
   if (local_resolver->ShouldChromeHandleUrl(url)) {
     // Allow navigation to proceed if there isn't an android app that handles
@@ -121,11 +123,11 @@ ArcNavigationThrottle::HandleRequest() {
     return content::NavigationThrottle::PROCEED;
   }
 
-  mojom::IntentHelperInstance* bridge_instance =
-      arc::ArcIntentHelperBridge::GetIntentHelperInstance(kMinInstanceVersion);
-  if (!bridge_instance)
+  auto* instance = ArcIntentHelperBridge::GetIntentHelperInstance(
+      "RequestUrlHandlerList", kMinVersionForRequestUrlHandlerList);
+  if (!instance)
     return content::NavigationThrottle::PROCEED;
-  bridge_instance->RequestUrlHandlerList(
+  instance->RequestUrlHandlerList(
       url.spec(), base::Bind(&ArcNavigationThrottle::OnAppCandidatesReceived,
                              weak_ptr_factory_.GetWeakPtr()));
   return content::NavigationThrottle::DEFER;
@@ -227,11 +229,11 @@ void ArcNavigationThrottle::OnIntentPickerClosed(
 
   previous_user_action_ = close_reason;
 
-  mojom::IntentHelperInstance* bridge =
-      arc::ArcIntentHelperBridge::GetIntentHelperInstance(kMinInstanceVersion);
-  if (!bridge || selected_app_index >= handlers.size()) {
+  // Make sure that the instance at least supports HandleUrl.
+  auto* instance = ArcIntentHelperBridge::GetIntentHelperInstance(
+      "HandleUrl", kMinVersionForHandleUrl);

[Luis Héctor Chávez, 2016/09/23 22:51:03]

nit: Given that version 7 was added more than three months ago, I think in this
case it's safe to just check for AddPreferredPackage.

[Yusuke Sato, 2016/09/24 00:15:22]

Yes that's true, but in terms of safety, probably 90% of version checks we have
right now are unnecessary by now.

My feeling is that removing or consolidating version checks based on practical
safety is somewhat complicated, and "always check, period" type of simpler
guideline would probably work better. Based on this, I'd keep two version checks
here to encourage developers to always check versions. WDYT?

[Luis Héctor Chávez, 2016/09/24 02:00:00]

Fair enough.

+  if (!instance || selected_app_index >= handlers.size())
     close_reason = CloseReason::ERROR;
-  }
 
   switch (close_reason) {
     case CloseReason::ERROR:
@@ -244,7 +246,12 @@ void ArcNavigationThrottle::OnIntentPickerClosed(
       break;
     }
     case CloseReason::ALWAYS_PRESSED: {
-      bridge->AddPreferredPackage(handlers[selected_app_index]->package_name);
+      // Call AddPreferredPackage if it is supported.
+      if (ArcIntentHelperBridge::GetIntentHelperInstance(
+              "AddPreferredPackage", kMinVersionForAddPreferredPackage)) {
+        instance->AddPreferredPackage(
+            handlers[selected_app_index]->package_name);
+      }
       // fall through.
     }
     case CloseReason::JUST_ONCE_PRESSED:
@@ -253,8 +260,8 @@ void ArcNavigationThrottle::OnIntentPickerClosed(
               handlers[selected_app_index]->package_name)) {
         handle->Resume();
       } else {
-        bridge->HandleUrl(url.spec(),
-                          handlers[selected_app_index]->package_name);
+        instance->HandleUrl(url.spec(),
+                            handlers[selected_app_index]->package_name);
         handle->CancelDeferredNavigation(
             content::NavigationThrottle::CANCEL_AND_IGNORE);
         if (handle->GetWebContents()->GetController().IsInitialNavigation())