Use FormatUrl in Android PageInfoPopup

This adds a new UrlFormatter.formatUrlForDisplayWithNormalEscaping()
function, and uses it in the Android PageInfoPopup to make its URL
display correctly de-punycode URLs that are safe to convert back to
Unicode for display.

This also adds url_formatter unittests for the specific usage of
FormatUrl that the new function uses under a variety of URL
conditions.

Bug: 818839
Change-Id: Id0f50a6a37be7309c684ef180acc78f84895d729
Reviewed-on: https://chromium-review.googlesource.com/1015229
Commit-Queue: Christopher Thompson <cthomp@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554611}

chrome/android/java/src/org/chromium/chrome/browser/page_info/PageInfoPopup.java

@@ -25,6 +25,7 @@ import android.text.style.TextAppearanceSpan;
 
 import org.chromium.base.ApiCompatibilityUtils;
 import org.chromium.base.StrictModeContext;
+import org.chromium.base.VisibleForTesting;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.base.metrics.RecordUserAction;
@@ -54,6 +55,7 @@ import org.chromium.chrome.browser.vr_shell.UiUnsupportedMode;
 import org.chromium.chrome.browser.vr_shell.VrShellDelegate;
 import org.chromium.components.location.LocationUtils;
 import org.chromium.components.security_state.ConnectionSecurityLevel;
+import org.chromium.components.url_formatter.UrlFormatter;
 import org.chromium.content_public.browser.WebContents;
 import org.chromium.content_public.browser.WebContentsObserver;
 import org.chromium.ui.base.DeviceFormFactor;
@@ -169,7 +171,7 @@ public class PageInfoPopup implements ModalDialogView.Controller {
      * @param offlinePageState         State of the tab showing offline page.
      * @param publisher                The name of the content publisher, if any.
      */
-    private PageInfoPopup(Activity activity, Tab tab, String offlinePageUrl,
+    protected PageInfoPopup(Activity activity, Tab tab, String offlinePageUrl,
             String offlinePageCreationDate, @OfflinePageState int offlinePageState,
             String publisher) {
         mContext = activity;
@@ -213,7 +215,7 @@ public class PageInfoPopup implements ModalDialogView.Controller {
         }
         mSecurityLevel = SecurityStateModel.getSecurityLevelForWebContents(mTab.getWebContents());
 
-        String displayUrl = mFullUrl;
+        String displayUrl = UrlFormatter.formatUrlForCopy(mFullUrl);
         if (isShowingOfflinePage()) {
             displayUrl = OfflinePageUtils.stripSchemeFromOnlineUrl(mFullUrl);
         }
@@ -618,6 +620,11 @@ public class PageInfoPopup implements ModalDialogView.Controller {
         }
     }
 
+    @VisibleForTesting
+    public PageInfoView getPageInfoViewForTesting() {
+        return mView;
+    }
+
     /**
      * Shows a PageInfo dialog for the provided Tab. The popup adds itself to the view
      * hierarchy which owns the reference while it's visible.

chrome/android/java/src/org/chromium/chrome/browser/page_info/PageInfoView.java

@@ -25,6 +25,7 @@ import android.widget.LinearLayout;
 import android.widget.TextView;
 
 import org.chromium.base.ApiCompatibilityUtils;
+import org.chromium.base.VisibleForTesting;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.widget.TintedDrawable;
 
@@ -385,4 +386,9 @@ public class PageInfoView extends FrameLayout implements OnClickListener, OnLong
 
         return animation;
     }
+
+    @VisibleForTesting
+    public String getUrlTitleForTesting() {
+        return mUrlTitle.getText().toString();
+    }
 }

chrome/android/javatests/DEPS

@@ -47,4 +47,7 @@ specific_include_rules = {
   "AuthenticatorTest\.java": [
     "+content/public/android/java/src/org/chromium/content/common/ContentSwitches.java",
   ],
+  "PageInfoPopupTest\.java": [
+    "+content/public/android/java/src/org/chromium/content/common/ContentSwitches.java",
+  ],
 }

chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/TrustedCdnPublisherUrlTest.java

@@ -295,7 +295,7 @@ public class TrustedCdnPublisherUrlTest {
                 () -> { Assert.assertNull(tab.getTrustedCdnPublisherUrl()); });
 
         String testUrl = mWebServer.getResponseUrl("/test.html");
-        String expectedUrl = UrlFormatter.formatUrlForDisplay(testUrl);
+        String expectedUrl = UrlFormatter.formatUrlForDisplayOmitScheme(testUrl);
 
         CriteriaHelper.pollUiThread(Criteria.equals(expectedUrl, () -> {
             UrlBar urlBar = newActivity.findViewById(R.id.url_bar);

chrome/android/javatests/src/org/chromium/chrome/browser/page_info/PageInfoPopupTest.java

@@ -4,8 +4,12 @@
 
 package org.chromium.chrome.browser.page_info;
 
+import android.support.test.InstrumentationRegistry;
 import android.support.test.filters.MediumTest;
 
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -18,17 +22,34 @@ import org.chromium.chrome.browser.ChromeActivity;
 import org.chromium.chrome.browser.ChromeSwitches;
 import org.chromium.chrome.test.ChromeActivityTestRule;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
+import org.chromium.content.common.ContentSwitches;
+import org.chromium.net.test.EmbeddedTestServer;
+import org.chromium.ui.base.PageTransition;
 
 /**
  * Tests for PageInfoPopup.
  */
 @RunWith(ChromeJUnit4ClassRunner.class)
-@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
+@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE,
+        ContentSwitches.HOST_RESOLVER_RULES + "=MAP * 127.0.0.1"})
 public class PageInfoPopupTest {
     @Rule
     public ChromeActivityTestRule<ChromeActivity> mActivityTestRule =
             new ChromeActivityTestRule<>(ChromeActivity.class);
 
+    private EmbeddedTestServer mTestServer;
+
+    @Before
+    public void setUp() throws Exception {
+        mActivityTestRule.startMainActivityOnBlankPage();
+        mTestServer = EmbeddedTestServer.createAndStartServer(InstrumentationRegistry.getContext());
+    }
+
+    @After
+    public void tearDown() throws Exception {
+        mTestServer.stopAndDestroyServer();
+    }
+
     /**
      * Tests that PageInfoPopup can be instantiated and shown.
      */
@@ -37,14 +58,33 @@ public class PageInfoPopupTest {
     @Feature({"PageInfoPopup"})
     @RetryOnFailure
     public void testShow() throws InterruptedException {
-        mActivityTestRule.startMainActivityOnBlankPage();
-        ThreadUtils.runOnUiThreadBlocking(new Runnable() {
-            @Override
-            public void run() {
+        ThreadUtils.runOnUiThreadBlocking(() -> {
             PageInfoPopup.show(mActivityTestRule.getActivity(),
                     mActivityTestRule.getActivity().getActivityTab(), null,
                     PageInfoPopup.OPENED_FROM_MENU);
+        });
     }
+
+    /**
+     * Tests that PageInfoPopup converts safe URLs to Unicode.
+     */
+    @Test
+    @MediumTest
+    @Feature({"PageInfoPopup"})
+    @RetryOnFailure
+    public void testPageInfoUrl() throws InterruptedException {
+        String testUrl = mTestServer.getURLWithHostName("xn--allestrungen-9ib.ch", "/");
+        mActivityTestRule.loadUrlInTab(
+                testUrl, PageTransition.TYPED, mActivityTestRule.getActivity().getActivityTab());
+        ThreadUtils.runOnUiThreadBlocking(() -> {
+            PageInfoPopup pageInfo = new PageInfoPopup(mActivityTestRule.getActivity(),
+                    mActivityTestRule.getActivity().getActivityTab(), null, null,
+                    PageInfoPopup.NOT_OFFLINE_PAGE, null);
+            PageInfoView pageInfoView = pageInfo.getPageInfoViewForTesting();
+            // Test that the title contains the Unicode hostname rather than strict equality, as
+            // the test server will be bound to a random port.
+            Assert.assertTrue(
+                    pageInfoView.getUrlTitleForTesting().contains("http://allestörungen.ch"));
         });
     }
 }

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

@@ -30,40 +30,34 @@ public final class UrlFormatter {
 
     /**
      * Builds a String representation of <code>uri</code> suitable for display to the user, omitting
-     * the scheme if it is "http://", the username and password, and trailing slash on a bare
-     * hostname.
+     * the scheme, the username and password, and trailing slash on a bare hostname.
+     *
+     * The IDN hostname is turned to Unicode if the Unicode representation is deemed safe.
+     * For more information, see <code>url_formatter::FormatUrl(const GURL&)</code>.
      *
      * Some examples:
      *  - "http://user:password@example.com/" -> "example.com"
-     *  - "https://example.com/path" -> "https://example.com/path"
+     *  - "https://example.com/path" -> "example.com/path"
      *  - "http://www.xn--frgbolaget-q5a.se" -> "www.färgbolaget.se"
      *
-     * The IDN hostname is turned to Unicode if the Unicode representation is deemed safe.
-     * For more information, see <code>url_formatter::FormatUrl(const GURL&)</code>.
-     *
      * @param uri URI to format.
      * @return Formatted URL.
      */
-    public static String formatUrlForDisplay(URI uri) {
-        return formatUrlForDisplay(uri.toString());
-    }
-
-    /**
-     * @see #formatUrlForDisplay(URI)
-     */
-    public static String formatUrlForDisplay(String uri) {
-        return nativeFormatUrlForDisplay(uri);
+    public static String formatUrlForDisplayOmitScheme(String uri) {
+        return nativeFormatUrlForDisplayOmitScheme(uri);
     }
 
     /**
-     * Builds a String representation of <code>uri</code> suitable for display to the user, omitting
-     * the scheme, the username and password, and trailing slash on a bare hostname.
+     * Builds a String representation of <code>uri</code> suitable for copying to the clipboard.
+     * It does not omit any components, and it performs normal escape decoding. Spaces are left
+     * escaped. The IDN hostname is turned to Unicode if the Unicode representation is deemed safe.
+     * For more information, see <code>url_formatter::FormatUrl(const GURL&)</code>.
+     *
      * @param uri URI to format.
      * @return Formatted URL.
-     * @see #formatUrlForDisplay(URI)
      */
-    public static String formatUrlForDisplayOmitScheme(String uri) {
-        return nativeFormatUrlForDisplayOmitScheme(uri);
+    public static String formatUrlForCopy(String uri) {
+        return nativeFormatUrlForCopy(uri);
     }
 
     /**
@@ -95,8 +89,8 @@ public final class UrlFormatter {
     }
 
     private static native String nativeFixupUrl(String url);
-    private static native String nativeFormatUrlForDisplay(String url);
     private static native String nativeFormatUrlForDisplayOmitScheme(String url);
+    private static native String nativeFormatUrlForCopy(String url);
     private static native String nativeFormatUrlForSecurityDisplay(String url);
     private static native String nativeFormatUrlForSecurityDisplayOmitScheme(String url);
 }

components/url_formatter/url_formatter_android.cc

@@ -40,26 +40,28 @@ static ScopedJavaLocalRef<jstring> JNI_UrlFormatter_FixupUrl(
              : ScopedJavaLocalRef<jstring>();
 }
 
-static ScopedJavaLocalRef<jstring> JNI_UrlFormatter_FormatUrlForDisplay(
+static ScopedJavaLocalRef<jstring>
+JNI_UrlFormatter_FormatUrlForDisplayOmitScheme(
     JNIEnv* env,
     const JavaParamRef<jclass>& clazz,
     const JavaParamRef<jstring>& url) {
   return base::android::ConvertUTF16ToJavaString(
       env, url_formatter::FormatUrl(
-               JNI_UrlFormatter_ConvertJavaStringToGURL(env, url)));
+               JNI_UrlFormatter_ConvertJavaStringToGURL(env, url),
+               url_formatter::kFormatUrlOmitDefaults |
+                   url_formatter::kFormatUrlOmitHTTPS,
+               net::UnescapeRule::SPACES, nullptr, nullptr, nullptr));
 }
 
-static ScopedJavaLocalRef<jstring>
-JNI_UrlFormatter_FormatUrlForDisplayOmitScheme(
+static ScopedJavaLocalRef<jstring> JNI_UrlFormatter_FormatUrlForCopy(
     JNIEnv* env,
     const JavaParamRef<jclass>& clazz,
     const JavaParamRef<jstring>& url) {
   return base::android::ConvertUTF16ToJavaString(
       env, url_formatter::FormatUrl(
                JNI_UrlFormatter_ConvertJavaStringToGURL(env, url),
-               url_formatter::kFormatUrlOmitDefaults |
-                   url_formatter::kFormatUrlOmitHTTPS,
-               net::UnescapeRule::SPACES, nullptr, nullptr, nullptr));
+               url_formatter::kFormatUrlOmitNothing, net::UnescapeRule::NORMAL,
+               nullptr, nullptr, nullptr));
 }
 
 static ScopedJavaLocalRef<jstring> JNI_UrlFormatter_FormatUrlForSecurityDisplay(