Percent-encode illegal characters in Android page info popup URL

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

 // Copyright 2013 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.chrome.browser;
 
 import android.app.Dialog;
 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;
 import android.text.style.ForegroundColorSpan;
 import android.text.style.StyleSpan;
 import android.util.AttributeSet;
 import android.view.Gravity;
 import android.view.LayoutInflater;
 import android.view.View;
 import android.view.View.OnClickListener;
@@ skipping 135 matching lines @@
             if (maxLines != mCurrentMaxLines) {
                 setMaxLines(maxLines);
                 return true;
             }
             return false;
         }
     }
 
     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;
 
     // A pointer to the C++ object for this UI.
     private final long mNativeWebsiteSettingsPopup;
 
     // The outer container, filled with the layout from website_settings.xml.
     private final LinearLayout mContainer;
 
@@ skipping 95 matching lines @@
         mDialog.setOnDismissListener(new DialogInterface.OnDismissListener() {
             @Override
             public void onDismiss(DialogInterface dialog) {
                 assert mNativeWebsiteSettingsPopup != 0;
                 webContentsObserver.destroy();
                 nativeDestroy(mNativeWebsiteSettingsPopup);
             }
         });
 
         // 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);
         } catch (URISyntaxException e) {
             mParsedUrl = null;
             mIsInternalPage = false;
         }
         mSecurityLevel = ToolbarModel.getSecurityLevelForWebContents(mWebContents);
 
         SpannableStringBuilder urlBuilder = new SpannableStringBuilder(mFullUrl);
@@ skipping 243 matching lines @@
         new WebsiteSettingsPopup(context, profile, webContents);
     }
 
     private static native long nativeInit(WebsiteSettingsPopup popup, WebContents webContents);
 
     private native void nativeDestroy(long nativeWebsiteSettingsPopupAndroid);
 
     private native void nativeOnPermissionSettingChanged(long nativeWebsiteSettingsPopupAndroid,
             int type, int setting);
 }