Migrate UrlFormatter#fixupUrl to GURL

Also adds unit tests for the UrlFormatter java wrapper which didn't
previously exist.

Bug: 783819
Change-Id: Ib98bd5daa64a48bb06d6593185f53f49ad22b3c7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2036730
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Chris Palmer <palmer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738268}

chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkEditActivity.java

@@ -20,6 +20,7 @@ import org.chromium.chrome.browser.bookmarks.BookmarkBridge.BookmarkModelObserve
 import org.chromium.components.bookmarks.BookmarkId;
 import org.chromium.components.browser_ui.widget.TintedDrawable;
 import org.chromium.components.url_formatter.UrlFormatter;
+import org.chromium.url.GURL;
 
 /**
  * The activity that enables the user to modify the title, url and parent folder of a bookmark.
@@ -136,8 +137,7 @@ public class BookmarkEditActivity extends SynchronousInitializationActivity {
     @Override
     protected void onStop() {
         if (mModel.doesBookmarkExist(mBookmarkId)) {
-            final String originalUrl =
-                    mModel.getBookmarkById(mBookmarkId).getUrl();
+            final GURL originalUrl = new GURL(mModel.getBookmarkById(mBookmarkId).getUrl());
             final String title = mTitleEditText.getTrimmedText();
             final String url = mUrlEditText.getTrimmedText();
 
@@ -147,9 +147,9 @@ public class BookmarkEditActivity extends SynchronousInitializationActivity {
 
             if (!mUrlEditText.isEmpty()
                     && mModel.getBookmarkById(mBookmarkId).isUrlEditable()) {
-                String fixedUrl = UrlFormatter.fixupUrl(url);
-                if (fixedUrl != null && !fixedUrl.equals(originalUrl)) {
-                    mModel.setBookmarkUrl(mBookmarkId, fixedUrl);
+                GURL fixedUrl = UrlFormatter.fixupUrl(url);
+                if (fixedUrl.isValid() && !fixedUrl.equals(originalUrl)) {
+                    mModel.setBookmarkUrl(mBookmarkId, fixedUrl.getSpec());
                 }
             }
         }

chrome/android/java/src/org/chromium/chrome/browser/init/StartupTabPreloader.java

@@ -33,6 +33,7 @@ import org.chromium.content_public.browser.WebContents;
 import org.chromium.content_public.common.Referrer;
 import org.chromium.network.mojom.ReferrerPolicy;
 import org.chromium.ui.base.WindowAndroid;
+import org.chromium.url.GURL;
 
 /**
  * This class attempts to preload the tab if the url is known from the intent when the profile
@@ -168,13 +169,13 @@ public class StartupTabPreloader implements ProfileManager.Observer, Destroyable
 
     private void loadTab() {
         Intent intent = mIntentSupplier.get();
-        String url = UrlFormatter.fixupUrl(getUrlFromIntent(intent));
+        GURL url = UrlFormatter.fixupUrl(getUrlFromIntent(intent));
 
         ChromeTabCreator chromeTabCreator =
                 (ChromeTabCreator) mTabCreatorManager.getTabCreator(false);
         WebContents webContents = WebContentsFactory.createWebContents(false, false);
 
-        mLoadUrlParams = new LoadUrlParams(url);
+        mLoadUrlParams = new LoadUrlParams(url.getValidSpecOrEmpty());
         String referrer = IntentHandler.getReferrerUrlIncludingExtraHeaders(intent);
         if (referrer != null && !referrer.isEmpty()) {
             mLoadUrlParams.setReferrer(new Referrer(referrer, ReferrerPolicy.DEFAULT));

chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchActivity.java

@@ -48,6 +48,7 @@ import org.chromium.content_public.common.ContentUrlConstants;
 import org.chromium.ui.base.ActivityKeyboardVisibilityDelegate;
 import org.chromium.ui.base.ActivityWindowAndroid;
 import org.chromium.ui.modaldialog.ModalDialogManager;
+import org.chromium.url.GURL;
 
 /** Queries the user's default search engine and shows autocomplete suggestions. */
 public class SearchActivity extends AsyncInitializationActivity
@@ -324,8 +325,8 @@ public class SearchActivity extends AsyncInitializationActivity
         if (TextUtils.isEmpty(url)) return;
 
         // Fix up the URL and send it to the full browser.
-        String fixedUrl = UrlFormatter.fixupUrl(url);
-        Intent intent = new Intent(Intent.ACTION_VIEW, Uri.parse(fixedUrl));
+        GURL fixedUrl = UrlFormatter.fixupUrl(url);
+        Intent intent = new Intent(Intent.ACTION_VIEW, Uri.parse(fixedUrl.getValidSpecOrEmpty()));
         intent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK | Intent.FLAG_ACTIVITY_NEW_DOCUMENT);
         intent.setClass(this, ChromeLauncherActivity.class);
         IntentHandler.addTrustedIntentExtras(intent);

chrome/android/java/src/org/chromium/chrome/browser/settings/homepage/HomepageEditor.java

@@ -18,6 +18,7 @@ import org.chromium.chrome.R;
 import org.chromium.chrome.browser.partnercustomizations.HomepageManager;
 import org.chromium.chrome.browser.settings.SettingsUtils;
 import org.chromium.components.url_formatter.UrlFormatter;
+import org.chromium.url.GURL;
 
 /**
  * Provides the Java-UI for editing the homepage preference.
@@ -77,8 +78,8 @@ public class HomepageEditor extends Fragment implements TextWatcher {
         mSaveButton.setOnClickListener(new View.OnClickListener() {
             @Override
             public void onClick(View v) {
-                mHomepageManager.setPrefHomepageCustomUri(
-                        UrlFormatter.fixupUrl(mHomepageUrlEdit.getText().toString()));
+                GURL url = UrlFormatter.fixupUrl(mHomepageUrlEdit.getText().toString());
+                mHomepageManager.setPrefHomepageCustomUri(url.getValidSpecOrEmpty());
                 mHomepageManager.setPrefHomepageUseDefaultUri(false);
                 getActivity().finish();
             }

chrome/android/java/src/org/chromium/chrome/browser/tabmodel/ChromeTabCreator.java

@@ -34,6 +34,7 @@ import org.chromium.content_public.browser.WebContents;
 import org.chromium.content_public.common.Referrer;
 import org.chromium.ui.base.PageTransition;
 import org.chromium.ui.base.WindowAndroid;
+import org.chromium.url.GURL;
 
 /**
  * This class creates various kinds of new tabs and adds them to the right {@link TabModel}.
@@ -109,8 +110,10 @@ public class ChromeTabCreator extends TabCreatorManager.TabCreator {
             TraceEvent.begin("ChromeTabCreator.createNewTab");
             int parentId = parent != null ? parent.getId() : Tab.INVALID_TAB_ID;
 
+            GURL url = UrlFormatter.fixupUrl(loadUrlParams.getUrl());
+
             // Sanitize the url.
-            loadUrlParams.setUrl(UrlFormatter.fixupUrl(loadUrlParams.getUrl()));
+            loadUrlParams.setUrl(url.getValidSpecOrEmpty());
             loadUrlParams.setTransitionType(getTransitionType(type, intent));
 
             // Check if the tab is being created asynchronously.

components/BUILD.gn

@@ -325,6 +325,7 @@ test("components_unittests") {
       "//components/signin/core/browser/android:java",
       "//components/signin/public/android:java",
       "//components/spellcheck/browser/android:java",
+      "//components/url_formatter/android:native_java_unittests_java",
       "//components/variations/android:variations_java",
       "//content/public/android:content_java",
       "//content/public/browser",

components/url_formatter/BUILD.gn

@@ -54,7 +54,10 @@ jumbo_static_library("url_formatter") {
   public_deps = [ "//third_party/icu" ]
 
   if (is_android) {
-    deps += [ "android:jni_headers" ]
+    deps += [
+      "android:jni_headers",
+      "//url:gurl_android",
+    ]
   }
 }
 
@@ -84,4 +87,12 @@ jumbo_source_set("unit_tests") {
     "//ui/gfx",
     "//url",
   ]
+
+  if (is_android) {
+    sources += [ "url_formatter_android_unittest.cc" ]
+    deps += [
+      "android:native_j_unittests_jni_headers",
+      "android:native_java_unittests_java",
+    ]
+  }
 }

components/url_formatter/android/BUILD.gn

@@ -8,6 +8,7 @@ android_library("url_formatter_java") {
   deps = [
     "//base:base_java",
     "//base:jni_java",
+    "//url:gurl_java",
   ]
   annotation_processor_deps = [ "//base/android/jni_generator:jni_processor" ]
 
@@ -21,3 +22,23 @@ generate_jni("jni_headers") {
   sources =
       [ "java/src/org/chromium/components/url_formatter/UrlFormatter.java" ]
 }
+
+android_library("native_java_unittests_java") {
+  testonly = true
+  deps = [
+    ":url_formatter_java",
+    "//base:base_java",
+    "//third_party/junit",
+    "//url:gurl_java",
+  ]
+  sources = [ "native_java_unittests/src/org/chromium/components/url_formatter/UrlFormatterUnitTest.java" ]
+
+  annotation_processor_deps = [ "//base/android/jni_generator:jni_processor" ]
+}
+
+# See https://bugs.chromium.org/p/chromium/issues/detail?id=908819 for why we
+# can't put 'java' in the name here.
+generate_jni("native_j_unittests_jni_headers") {
+  testonly = true
+  sources = [ "native_java_unittests/src/org/chromium/components/url_formatter/UrlFormatterUnitTest.java" ]
+}

components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java

@@ -9,24 +9,29 @@ import android.text.TextUtils;
 import androidx.annotation.VisibleForTesting;
 
 import org.chromium.base.annotations.JNINamespace;
+import org.chromium.base.annotations.MainDex;
 import org.chromium.base.annotations.NativeMethods;
+import org.chromium.url.GURL;
 
 /**
  * Wrapper for utilities in url_formatter.
  */
 @JNINamespace("url_formatter::android")
+@MainDex
 public final class UrlFormatter {
     /**
      * Refer to url_formatter::FixupURL.
      *
-     * Given a URL-like string, returns a real URL or null. For example:
+     * Given a URL-like string, returns a possibly-invalid GURL. For example:
      *  - "google.com" -> "http://google.com/"
      *  - "about:" -> "chrome://version/"
      *  - "//mail.google.com:/" -> "file:///mail.google.com:/"
-     *  - "..." -> null
+     *  - "0x100.0" -> "http://0x100.0/" (invalid)
      */
-    public static String fixupUrl(String uri) {
-        return TextUtils.isEmpty(uri) ? null : UrlFormatterJni.get().fixupUrl(uri);
+    public static GURL fixupUrl(String uri) {
+        if (TextUtils.isEmpty(uri)) return GURL.emptyGURL();
+        GURL.ensureNativeInitializedForGURL();
+        return UrlFormatterJni.get().fixupUrl(uri);
     }
 
     /**
@@ -125,7 +130,7 @@ public final class UrlFormatter {
     @VisibleForTesting
     @NativeMethods
     public interface Natives {
-        String fixupUrl(String url);
+        GURL fixupUrl(String url);
         String formatUrlForDisplayOmitScheme(String url);
         String formatUrlForDisplayOmitHTTPScheme(String url);
         String formatUrlForDisplayOmitSchemeOmitTrivialSubdomains(String url);

components/url_formatter/android/native_java_unittests/src/org/chromium/components/url_formatter/UrlFormatterUnitTest.java

+// Copyright 2020 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.
+
+package org.chromium.components.url_formatter;
+
+import org.junit.Assert;
+
+import org.chromium.base.annotations.CalledByNative;
+import org.chromium.base.annotations.CalledByNativeJavaTest;
+
+/**
+ * Unit tests for {@link UrlFormatter}.
+ *
+ * These tests are basic sanity checks to ensure the plumbing is working correctly. The wrapped
+ * functions are tested much more thoroughly elsewhere.
+ */
+public class UrlFormatterUnitTest {
+    @CalledByNative
+    private UrlFormatterUnitTest() {}
+
+    @CalledByNativeJavaTest
+    public void testFixupUrl() {
+        Assert.assertEquals("http://google.com/", UrlFormatter.fixupUrl("google.com").getSpec());
+        Assert.assertEquals("chrome://version/", UrlFormatter.fixupUrl("about:").getSpec());
+        Assert.assertEquals("file:///mail.google.com:/",
+                UrlFormatter.fixupUrl("//mail.google.com:/").getSpec());
+        Assert.assertFalse(UrlFormatter.fixupUrl("0x100.0").isValid());
+    }
+}

components/url_formatter/url_formatter_android.cc

@@ -10,6 +10,7 @@
 #include "components/url_formatter/elide_url.h"
 #include "components/url_formatter/url_fixer.h"
 #include "components/url_formatter/url_formatter.h"
+#include "url/android/gurl_android.h"
 #include "url/gurl.h"
 
 using base::android::JavaParamRef;
@@ -27,16 +28,14 @@ namespace url_formatter {
 
 namespace android {
 
-static ScopedJavaLocalRef<jstring> JNI_UrlFormatter_FixupUrl(
+static ScopedJavaLocalRef<jobject> JNI_UrlFormatter_FixupUrl(
     JNIEnv* env,
     const JavaParamRef<jstring>& url) {
   DCHECK(url);
   GURL fixed_url = url_formatter::FixupURL(
       base::android::ConvertJavaStringToUTF8(env, url), std::string());
 
-  return fixed_url.is_valid()
-             ? base::android::ConvertUTF8ToJavaString(env, fixed_url.spec())
-             : ScopedJavaLocalRef<jstring>();
+  return url::GURLAndroid::FromNativeGURL(env, fixed_url);
 }
 
 static ScopedJavaLocalRef<jstring>

components/url_formatter/url_formatter_android_unittest.cc

+// Copyright 2020 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 "base/android/jni_android.h"
+#include "components/url_formatter/android/native_j_unittests_jni_headers/UrlFormatterUnitTest_jni.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using base::android::AttachCurrentThread;
+
+class UrlFormatterUnitTest : public ::testing::Test {
+ public:
+  UrlFormatterUnitTest()
+      : j_test_(Java_UrlFormatterUnitTest_Constructor(AttachCurrentThread())) {}
+
+  const base::android::ScopedJavaGlobalRef<jobject>& j_test() {
+    return j_test_;
+  }
+
+ private:
+  base::android::ScopedJavaGlobalRef<jobject> j_test_;
+};
+
+JAVA_TESTS(UrlFormatterUnitTest, j_test())

tools/android/eclipse/.classpath

@@ -187,6 +187,7 @@ to the classpath for downstream development. See "additional_entries" below.
     <classpathentry kind="src" path="components/sync/test/android/javatests/src"/>
     <classpathentry kind="src" path="components/test/android/browsertests_apk/src"/>
     <classpathentry kind="src" path="components/url_formatter/android/java/src"/>
+    <classpathentry kind="src" path="components/url_formatter/android/native_java_unittests/src"/>
     <classpathentry kind="src" path="components/variations/android/java/src"/>
     <classpathentry kind="src" path="components/variations/android/junit/src"/>
     <classpathentry kind="src" path="components/version_info/android/java/src"/>

url/android/gurl_android.cc

@@ -67,6 +67,14 @@ std::unique_ptr<GURL> GURLAndroid::ToNativeGURL(
       reinterpret_cast<GURL*>(Java_GURL_toNativeGURL(env, j_gurl)));
 }
 
+// static
+ScopedJavaLocalRef<jobject> GURLAndroid::FromNativeGURL(JNIEnv* env,
+                                                        const GURL& gurl) {
+  ScopedJavaLocalRef<jobject> j_gurl = Java_GURL_Constructor(env);
+  InitFromGURL(env, gurl, j_gurl);
+  return j_gurl;
+}
+
 static void JNI_GURL_GetOrigin(JNIEnv* env,
                                const JavaParamRef<jstring>& j_spec,
                                jboolean is_valid,

url/android/gurl_android.h

@@ -17,6 +17,9 @@ class GURLAndroid {
   static std::unique_ptr<GURL> ToNativeGURL(
       JNIEnv* env,
       const base::android::JavaRef<jobject>& j_gurl);
+  static base::android::ScopedJavaLocalRef<jobject> FromNativeGURL(
+      JNIEnv* env,
+      const GURL& gurl);
 };
 
 }  // namespace url

url/android/java/src/org/chromium/url/GURL.java

@@ -34,21 +34,39 @@ public class GURL {
     private boolean mIsValid;
     private Parsed mParsed;
 
+    private static class Holder { private static GURL sEmptyGURL = new GURL(""); }
+
+    public static GURL emptyGURL() {
+        return Holder.sEmptyGURL;
+    }
+
     /**
      * Create a new GURL.
      *
      * @param uri The string URI representation to parse into a GURL.
      */
     public GURL(String uri) {
+        ensureNativeInitializedForGURL();
+        GURLJni.get().init(uri, this);
+    }
+
+    @CalledByNative
+    protected GURL() {}
+
+    /**
+     * Ensures that the native library is sufficiently loaded for GURL usage.
+     *
+     * This function is public so that GURL-related usage like the UrlFormatter also counts towards
+     * the "Startup.Android.GURLEnsureMainDexInitialized" histogram.
+     */
+    public static void ensureNativeInitializedForGURL() {
+        if (LibraryLoader.getInstance().isInitialized()) return;
         long time = SystemClock.elapsedRealtime();
         LibraryLoader.getInstance().ensureMainDexInitialized();
         RecordHistogram.recordTimesHistogram("Startup.Android.GURLEnsureMainDexInitialized",
                 SystemClock.elapsedRealtime() - time);
-        GURLJni.get().init(uri, this);
     }
 
-    protected GURL() {}
-
     @CalledByNative
     private void init(String spec, boolean isValid, Parsed parsed) {
         mSpec = spec;
@@ -79,6 +97,14 @@ public class GURL {
         return "";
     }
 
+    /**
+     * @return Either a valid Spec (see {@link #getSpec}), or an empty string.
+     */
+    public String getValidSpecOrEmpty() {
+        if (isValid()) return mSpec;
+        return "";
+    }
+
     /**
      * See native GURL::possibly_invalid_spec().
      */