Open |current_url| in Chrome when |previous_url| is empty

to prevent the disambig window from being shown when it shouldn't be.

BUG=640393
TEST=see repro steps in b/31043273
TEST=try

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

chrome/browser/chromeos/BUILD.gn

@@ -1351,6 +1351,7 @@ source_set("unit_tests") {
     "accessibility/magnification_manager_unittest.cc",
     "accessibility/spoken_feedback_event_rewriter_unittest.cc",
     "arc/arc_auth_service_unittest.cc",
+    "arc/arc_navigation_throttle_unittest.cc",
     "arc/arc_policy_bridge_unittest.cc",
     "attestation/attestation_ca_client_unittest.cc",
     "attestation/attestation_policy_observer_unittest.cc",

chrome/browser/chromeos/arc/arc_navigation_throttle.cc

@@ -21,6 +21,7 @@
 #include "content/public/browser/web_contents.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "ui/base/page_transition_types.h"
+#include "url/gurl.h"
 
 namespace arc {
 
@@ -34,6 +35,27 @@ scoped_refptr<ActivityIconLoader> GetIconLoader() {
   return arc_service_manager ? arc_service_manager->icon_loader() : nullptr;
 }
 
+// Compares the host name of the referrer and target URL to decide whether
+// the navigation needs to be overriden.
+bool ShouldOverrideUrlLoading(const GURL& previous_url,
+                              const GURL& current_url) {
+  // When the navigation is initiated in a web page where sending a referrer
+  // is disabled, |previous_url| can be empty. In this case, we should open
+  // it in the desktop browser.
+  if (!previous_url.is_valid() || previous_url.is_empty())
+    return false;
+
+  // Also check |current_url| just in case.
+  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);
+}
+
 }  // namespace
 
 ArcNavigationThrottle::ArcNavigationThrottle(
@@ -84,7 +106,9 @@ ArcNavigationThrottle::HandleRequest() {
   if (ShouldIgnoreNavigation(navigation_handle()->GetPageTransition()))
     return content::NavigationThrottle::PROCEED;
 
-  if (!ShouldOverrideUrlLoading(navigation_handle()))
+  const GURL previous_url = navigation_handle()->GetReferrer().url;
+  const GURL current_url = navigation_handle()->GetURL();
+  if (!ShouldOverrideUrlLoading(previous_url, current_url))
     return content::NavigationThrottle::PROCEED;
 
   arc::ArcServiceManager* arc_service_manager = arc::ArcServiceManager::Get();
@@ -249,13 +273,11 @@ void ArcNavigationThrottle::OnIntentPickerClosed(
                             static_cast<int>(CloseReason::SIZE));
 }
 
-bool ArcNavigationThrottle::ShouldOverrideUrlLoading(
-    content::NavigationHandle* navigation_handle) {
-  GURL previous_url = navigation_handle->GetReferrer().url;
-  GURL current_url = navigation_handle->GetURL();
-  return !net::registry_controlled_domains::SameDomainOrHost(
-      current_url, previous_url,
-      net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
+// static
+bool ArcNavigationThrottle::ShouldOverrideUrlLoadingForTesting(
+    const GURL& previous_url,
+    const GURL& current_url) {
+  return ShouldOverrideUrlLoading(previous_url, current_url);
 }
 
 }  // namespace arc

chrome/browser/chromeos/arc/arc_navigation_throttle.h

@@ -17,6 +17,8 @@
 #include "content/public/browser/navigation_throttle.h"
 #include "ui/gfx/image/image.h"
 
+class GURL;
+
 namespace content {
 class NavigationHandle;
 class WebContents;
@@ -54,6 +56,9 @@ class ArcNavigationThrottle : public content::NavigationThrottle {
                         const ShowIntentPickerCallback& show_intent_picker_cb);
   ~ArcNavigationThrottle() override;
 
+  static bool ShouldOverrideUrlLoadingForTesting(const GURL& previous_url,
+                                                 const GURL& current_url);
+
  private:
   // content::Navigation implementation:
   NavigationThrottle::ThrottleCheckResult WillStartRequest() override;
@@ -67,10 +72,6 @@ class ArcNavigationThrottle : public content::NavigationThrottle {
   void OnIntentPickerClosed(mojo::Array<mojom::UrlHandlerInfoPtr> handlers,
                             size_t selected_app_index,
                             CloseReason close_reason);
-  // Compares the host name of the referrer and target URL to decide whether
-  // the navigation needs to be overriden.
-  bool ShouldOverrideUrlLoading(content::NavigationHandle* navigation_handle);
-
   // A callback object that allow us to display an IntentPicker when Run() is
   // executed, it also allow us to report the user's selection back to
   // OnIntentPickerClosed().

chrome/browser/chromeos/arc/arc_navigation_throttle_unittest.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 "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace arc {
+
+TEST(ArcNavigationThrottleTest, TestShouldOverrideUrlLoading) {
+  // A navigation within the same domain shouldn't be overridden.
+  EXPECT_FALSE(ArcNavigationThrottle::ShouldOverrideUrlLoadingForTesting(
+      GURL("http://google.com"), GURL("http://google.com/")));
+  EXPECT_FALSE(ArcNavigationThrottle::ShouldOverrideUrlLoadingForTesting(
+      GURL("http://google.com"), GURL("http://a.google.com/")));
+  EXPECT_FALSE(ArcNavigationThrottle::ShouldOverrideUrlLoadingForTesting(
+      GURL("http://a.google.com"), GURL("http://google.com/")));
+  EXPECT_FALSE(ArcNavigationThrottle::ShouldOverrideUrlLoadingForTesting(
+      GURL("http://a.google.com"), GURL("http://b.google.com/")));
+  EXPECT_FALSE(ArcNavigationThrottle::ShouldOverrideUrlLoadingForTesting(
+      GURL("http://a.google.com"), GURL("http://b.c.google.com/")));
+  EXPECT_FALSE(ArcNavigationThrottle::ShouldOverrideUrlLoadingForTesting(
+      GURL("http://a.b.google.com"), GURL("http://c.google.com/")));
+
+  // If either of two paramters is empty, the function should return false.
+  EXPECT_FALSE(ArcNavigationThrottle::ShouldOverrideUrlLoadingForTesting(
+      GURL(), GURL("http://a.google.com/")));
+  EXPECT_FALSE(ArcNavigationThrottle::ShouldOverrideUrlLoadingForTesting(
+      GURL("http://a.google.com/"), GURL()));
+  EXPECT_FALSE(ArcNavigationThrottle::ShouldOverrideUrlLoadingForTesting(
+      GURL(), GURL()));
+
+  // A navigation not within the same domain can be overridden.
+  EXPECT_TRUE(ArcNavigationThrottle::ShouldOverrideUrlLoadingForTesting(
+      GURL("http://www.google.com"), GURL("http://www.not-google.com/")));
+  EXPECT_TRUE(ArcNavigationThrottle::ShouldOverrideUrlLoadingForTesting(
+      GURL("http://www.not-google.com"), GURL("http://www.google.com/")));
+}
+
+}  // namespace arc