Add more tests to arc_navigation_throttle.cc

chrome/browser/chromeos/arc/arc_navigation_throttle.cc

 // Copyright 2016 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/chromeos/arc/arc_navigation_throttle.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/logging.h"
@@ skipping 40 matching lines @@
   if (!current_url.is_valid() || current_url.is_empty()) {
     DVLOG(1) << "Unexpected URL: " << current_url << ", opening it in Chrome.";
     return false;
   }
 
   return !net::registry_controlled_domains::SameDomainOrHost(
       current_url, previous_url,
       net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
 }
 
+// Returns true if |handlers| contain one or more apps.
+bool IsAppAvailable(const mojo::Array<mojom::IntentHandlerInfoPtr>& handlers) {
+  return !handlers.empty() &&
+         !(handlers.size() == 1 && ArcIntentHelperBridge::IsIntentHelperPackage(

[Luis Héctor Chávez, 2016/10/24 20:24:34]

nit: I think it's clearer after applying de Morgan's law:

return !handlers.empty() &&
       (handlers.size() > 1 ||
        !ArcIntentHelperBridge::IsIntentHelperPackage(...));

[Yusuke Sato, 2016/10/24 22:27:16]

Done.

Applied yours and shuffled the order a little bit more to make the intention
clearer.

+                                       handlers[0]->package_name));
+}
+
+// Searches for a preferred app in |handlers| and returns its index. If not
+// found, returns |handlers.size()|.
+size_t FindPreferredApp(
+    const mojo::Array<mojom::IntentHandlerInfoPtr>& handlers,
+    const GURL& url_for_logging) {
+  for (size_t i = 0; i < handlers.size(); ++i) {
+    if (!handlers[i]->is_preferred)
+      continue;
+    if (ArcIntentHelperBridge::IsIntentHelperPackage(
+            handlers[i]->package_name)) {
+      // If Chrome browser was selected as the preferred app, we should't

[djacobo_, 2016/10/24 20:22:14]

nit: shouldn't*

[Yusuke Sato, 2016/10/24 22:27:16]

Done.

+      // create a throttle.
+      DVLOG(1)
+          << "Chrome browser is selected as the preferred app for this URL: "
+          << url_for_logging;

[Luis Héctor Chávez, 2016/10/24 20:24:34]

isn't this missing |.spec()|? Since it's not a trivial function (there's at
least one potential mutex operation), I'm not sure if we want to pass the spec
directly into the function or keep the const-reference to GURL as parameter.

[Yusuke Sato, 2016/10/24 22:27:16]

I'm not sure if I understand your concern.. could you elaborate a bit more?

operator<< for url_for_logging just returns const string& (without any mutex
operation).

[Luis Héctor Chávez, 2016/10/24 22:59:54]

This snippet of code used to log |url_for_logging.spec()|. We have two options:
pass in the |url_for_logging| directly or |url_for_logging.spec()|. The latter
looks cleaner, but its definition is not inlined:
https://cs.chromium.org/chromium/src/url/gurl.cc?q=GURL::spec()&sq=package:ch...

moreover, if it's empty it needs a mutex operation to evaluate the empty spec:
https://cs.chromium.org/chromium/src/url/gurl.cc?sq=package:chromium&dr=C&rcl...

so i'm not sure what option is better. After writing all this, it looks like
keeping the GURL is fine (but the spec() part still stands). Sorry for any
confusion.

[Yusuke Sato, 2016/10/24 23:25:17]

Let me use url_for_logging (w/o .spec()) for simplicity.

+    }
+    return i;
+  }
+  return handlers.size();  // not found
+}
+
+// Swaps Chrome app with any app in row |kMaxAppResults-1| iff its index is
+// bigger, thus ensuring the user can always see Chrome without scrolling.
+// When swap is needed, fills |out_indices| and returns true.
+bool IsSwapElementsNeeded(
+    const mojo::Array<mojom::IntentHandlerInfoPtr>& handlers,
+    std::pair<size_t, size_t>* out_indices) {
+  size_t chrome_app_index = 0;
+  for (size_t i = 0; i < handlers.size(); ++i) {
+    if (ArcIntentHelperBridge::IsIntentHelperPackage(

[Luis Héctor Chávez, 2016/10/24 20:24:34]

qq: is it possible that the intent helper package does not appear in the list at
all? (Based on the condition in L64, it seems like it).

IIUC if that happens, it will keep |chrome_app_index| as zero and therefore not
trigger L103, but it's probably better to clearly document this.

[Yusuke Sato, 2016/10/24 22:27:16]

On production, Chrome always appears actually. It's not my code but David's but
anyway, L64 is for detecting the case where |handlers| only have one element and
it is Chrome.
Having that said, documenting it is not a bad idea at all. Done with some
additional unit tests. Thanks for the suggestion.

+            handlers[i]->package_name)) {
+      chrome_app_index = i;
+      break;
+    }
+  }
+  if (chrome_app_index < ArcNavigationThrottle::kMaxAppResults)
+    return false;
+
+  *out_indices = std::make_pair(ArcNavigationThrottle::kMaxAppResults - 1,
+                                chrome_app_index);
+  return true;
+}
+
 }  // namespace
 
 ArcNavigationThrottle::ArcNavigationThrottle(
     content::NavigationHandle* navigation_handle,
     const ShowIntentPickerCallback& show_intent_picker_cb)
     : content::NavigationThrottle(navigation_handle),
       show_intent_picker_callback_(show_intent_picker_cb),
       previous_user_action_(CloseReason::INVALID),
       weak_ptr_factory_(this) {}
 
@@ skipping 85 matching lines @@
       url.spec(), base::Bind(&ArcNavigationThrottle::OnAppCandidatesReceived,
                              weak_ptr_factory_.GetWeakPtr()));
   return content::NavigationThrottle::DEFER;
 }
 
 // We received the array of app candidates to handle this URL (even the Chrome
 // app is included).
 void ArcNavigationThrottle::OnAppCandidatesReceived(
     mojo::Array<mojom::IntentHandlerInfoPtr> handlers) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  if (handlers.empty() ||
-      (handlers.size() == 1 && ArcIntentHelperBridge::IsIntentHelperPackage(
-                                   handlers[0]->package_name))) {
+  if (!IsAppAvailable(handlers)) {
     // This scenario shouldn't be accesed as ArcNavigationThrottle is created
     // iff there are ARC apps which can actually handle the given URL.
     DVLOG(1) << "There are no app candidates for this URL: "
              << navigation_handle()->GetURL().spec();

[Yusuke Sato, 2016/10/24 23:25:17]

Removed this .spec() call too to be consistent with L52 and L82.

     navigation_handle()->Resume();
     return;
   }
 
   // If one of the apps is marked as preferred, use it right away without
   // showing the UI.
-  for (size_t i = 0; i < handlers.size(); ++i) {
-    if (!handlers[i]->is_preferred)
-      continue;
-    if (ArcIntentHelperBridge::IsIntentHelperPackage(
-            handlers[i]->package_name)) {
-      // If Chrome browser was selected as the preferred app, we should't
-      // create a throttle.
-      DVLOG(1)
-          << "Chrome browser is selected as the preferred app for this URL: "
-          << navigation_handle()->GetURL().spec();
-    }
-    std::string package_name = handlers[i]->package_name;
+  const size_t index =
+      FindPreferredApp(handlers, navigation_handle()->GetURL());
+  if (index != handlers.size()) {
+    const std::string package_name = handlers[index]->package_name;
     OnIntentPickerClosed(std::move(handlers), package_name,
                          CloseReason::PREFERRED_ACTIVITY_FOUND);
     return;
   }
 
-  // Swap Chrome app with any app in row |kMaxAppResults-1| iff its index is
-  // bigger, thus ensuring the user can always see Chrome without scrolling.
-  size_t chrome_app_index = 0;
-  for (size_t i = 0; i < handlers.size(); ++i) {
-    if (ArcIntentHelperBridge::IsIntentHelperPackage(
-            handlers[i]->package_name)) {
-      chrome_app_index = i;
-      break;
-    }
-  }
-
-  if (chrome_app_index >= kMaxAppResults)
-    std::swap(handlers[kMaxAppResults - 1], handlers[chrome_app_index]);
+  std::pair<size_t, size_t> indices;
+  if (IsSwapElementsNeeded(handlers, &indices))
+    std::swap(handlers[indices.first], handlers[indices.second]);
 
   scoped_refptr<ActivityIconLoader> icon_loader = GetIconLoader();
   if (!icon_loader) {
     LOG(ERROR) << "Cannot get an instance of ActivityIconLoader";
     navigation_handle()->Resume();
     return;
   }
   std::vector<ActivityIconLoader::ActivityName> activities;
-  for (const auto& handler : handlers) {
+  for (const auto& handler : handlers)
     activities.emplace_back(handler->package_name, handler->activity_name);
-  }
   icon_loader->GetActivityIcons(
       activities,
       base::Bind(&ArcNavigationThrottle::OnAppIconsReceived,
                  weak_ptr_factory_.GetWeakPtr(), base::Passed(&handlers)));
 }
 
 void ArcNavigationThrottle::OnAppIconsReceived(
     mojo::Array<mojom::IntentHandlerInfoPtr> handlers,
     std::unique_ptr<ActivityIconLoader::ActivityToIconsMap> icons) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ skipping 21 matching lines @@
     std::string selected_app_package,
     CloseReason close_reason) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   const GURL& url = navigation_handle()->GetURL();
   content::NavigationHandle* handle = navigation_handle();
   previous_user_action_ = close_reason;
 
   // Make sure that the instance at least supports HandleUrl.
   auto* instance = ArcIntentHelperBridge::GetIntentHelperInstance(
       "HandleUrl", kMinVersionForHandleUrl);
-  size_t selected_app_index = handlers.size();
+  // Since we are selecting an app by its package name, we need to locate it
+  // on the |handlers| structure before sending the IPC to ARC.
+  const size_t selected_app_index = GetAppIndex(handlers, selected_app_package);
   if (!instance) {
     close_reason = CloseReason::ERROR;
   } else if (close_reason == CloseReason::JUST_ONCE_PRESSED ||
              close_reason == CloseReason::ALWAYS_PRESSED ||
              close_reason == CloseReason::PREFERRED_ACTIVITY_FOUND) {
-    // Since we are selecting an app by its package name, we need to locate it
-    // on the |handlers| structure before sending the IPC to ARC.
-    for (size_t i = 0; i < handlers.size(); ++i) {
-      if (handlers[i]->package_name == selected_app_package) {
-        selected_app_index = i;
-        break;
-      }
-    }
-
     if (selected_app_index == handlers.size())
       close_reason = CloseReason::ERROR;
   }
 
   switch (close_reason) {
     case CloseReason::ERROR:
     case CloseReason::DIALOG_DEACTIVATED: {
       // If the user fails to select an option from the list, or the UI returned
       // an error or if |selected_app_index| is not a valid index, then resume
       // the navigation in Chrome.
@@ skipping 30 matching lines @@
       return;
     }
   }
 
   UMA_HISTOGRAM_ENUMERATION("Arc.IntentHandlerAction",
                             static_cast<int>(close_reason),
                             static_cast<int>(CloseReason::SIZE));
 }
 
 // static
+size_t ArcNavigationThrottle::GetAppIndex(
+    const mojo::Array<mojom::IntentHandlerInfoPtr>& handlers,
+    const std::string& selected_app_package) {
+  for (size_t i = 0; i < handlers.size(); ++i) {
+    if (handlers[i]->package_name == selected_app_package)
+      return i;
+  }
+  return handlers.size();
+}
+
+// static
 bool ArcNavigationThrottle::ShouldOverrideUrlLoadingForTesting(
     const GURL& previous_url,
     const GURL& current_url) {
   return ShouldOverrideUrlLoading(previous_url, current_url);
 }
 
+// static
+bool ArcNavigationThrottle::IsAppAvailableForTesting(
+    const mojo::Array<mojom::IntentHandlerInfoPtr>& handlers) {
+  return IsAppAvailable(handlers);
+}
+
+// static
+size_t ArcNavigationThrottle::FindPreferredAppForTesting(
+    const mojo::Array<mojom::IntentHandlerInfoPtr>& handlers) {
+  return FindPreferredApp(handlers, GURL());
+}
+
+// static
+bool ArcNavigationThrottle::IsSwapElementsNeededForTesting(
+    const mojo::Array<mojom::IntentHandlerInfoPtr>& handlers,
+    std::pair<size_t, size_t>* out_indices) {
+  return IsSwapElementsNeeded(handlers, out_indices);
+}
+
 }  // namespace arc