Percent-encode illegal characters in Android page info popup URL

chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java

Index: chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java b/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
index 6b6d07a3fa6620310bc2b4716e1851141934c4e4..e8721fff1ff3b99c347fd18c4f6b913fcefa427a 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
@@ -9,6 +9,7 @@ import android.content.Context;
 import android.content.DialogInterface;
 import android.graphics.Color;
 import android.graphics.drawable.ColorDrawable;
+import android.net.Uri;
 import android.text.Layout;
 import android.text.Spannable;
 import android.text.SpannableStringBuilder;
@@ -164,6 +165,9 @@ public class WebsiteSettingsPopup implements OnClickListener, OnItemSelectedList
 
     private static final int MAX_TABLET_DIALOG_WIDTH_DP = 400;
 
+    // The set of characters which have syntactic meaning in a URL.

[Matt Giuca, 2015/03/19 03:12:57]

// This is the "reserved" character set from RFC 3986, plus the '%' character.

[tsergeant, 2015/03/24 05:33:47]

Done.

[Matt Giuca, 2015/03/24 06:28:37]

nit: I meant to append my extra comment, not replace (your comment was also
helpful). Feel free to put back, or just leave as-is.

+    private static final String URI_RESERVED_CHARACTERS = "!*'();:@&=+$,/?#[]%";
+
     private final Context mContext;
     private final Profile mProfile;
     private final WebContents mWebContents;
@@ -279,7 +283,10 @@ public class WebsiteSettingsPopup implements OnClickListener, OnItemSelectedList
         });
 
         // Work out the URL and connection message.
-        mFullUrl = mWebContents.getVisibleUrl();
+        // Clean up the URL by percent-encoding anything which is not
+        // allowed in a URL (spaces, ASCII control characters, non-ASCII Unicode).

[Matt Giuca, 2015/03/19 03:12:57]

You are also allowing the following: "<>\^`{|}
(Those characters are in neither the reserved nor unreserved sets, and are
therefore illegal in URLs.) Assuming my understanding of Uri.encode is correct
--- that it escapes all characters other than URI_RESERVED_CHARACTERS. So you
should mention that in the comment.

Also, explain the purpose of not simply calling encode with no |allow| argument:
// Do not use the default allowed character set of Uri.encode, as that will
escape reserved syntactic characters (eg, '&'). We must only escape illegal
characters.

(Feel free to rephrase.)

+        mFullUrl = Uri.encode(mWebContents.getVisibleUrl(), URI_RESERVED_CHARACTERS);

[Matt Giuca, 2015/03/19 03:12:57]

Would it make sense to break this line out into a separate testable function?

You could write a couple of tests that try URLs with characters from all the
different sets (reserved, unreserved, space, control, non-ASCII and the above
special illegal characters), to not only show that it behaves correctly, but
also demonstrate the purpose of this encoding.

[tsergeant, 2015/03/24 05:33:47]

Done.

+
         try {
             mParsedUrl = new URI(mFullUrl);
             mIsInternalPage = UrlUtilities.isInternalScheme(mParsedUrl);