Add Permissions to the PageInfo dialog on Android

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 c3d3ab9c3433dea915ec353ab7c48ccffd3380eb..30a035cb16406ce17469d1f33a05004c48196baf 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
@@ -5,6 +5,8 @@
 package org.chromium.chrome.browser;
 
 import android.app.Dialog;
+import android.content.ClipData;
+import android.content.ClipboardManager;
 import android.content.Context;
 import android.content.DialogInterface;
 import android.graphics.Color;
@@ -12,14 +14,22 @@ import android.graphics.drawable.ColorDrawable;
 import android.text.Spannable;
 import android.text.SpannableStringBuilder;
 import android.text.style.ForegroundColorSpan;
-import android.text.style.StyleSpan;
 import android.view.Gravity;
 import android.view.LayoutInflater;
+import android.view.View;
+import android.view.View.OnClickListener;
 import android.view.ViewGroup;
 import android.view.Window;
+import android.widget.AdapterView;
+import android.widget.AdapterView.OnItemSelectedListener;
+import android.widget.ArrayAdapter;
+import android.widget.Button;
+import android.widget.ImageView;
 import android.widget.LinearLayout;
 import android.widget.ScrollView;
+import android.widget.Spinner;
 import android.widget.TextView;
+import android.widget.Toast;
 
 import org.chromium.base.CalledByNative;
 import org.chromium.base.CommandLine;
@@ -28,10 +38,14 @@ import org.chromium.chrome.R;
 import org.chromium.content.browser.WebContentsObserverAndroid;
 import org.chromium.content_public.browser.WebContents;
 
+import java.util.Arrays;
+
 /**
  * Java side of Android implementation of the website settings UI.
+ * TODO(sashab): Rename this, and all its resources, to PageInfo* and page_info_* instead of
+ * WebsiteSettings* and website_settings_*. Do this on the C++ side as well.

[Ted C, 2014/10/09 01:25:14]

For TODO's, align all subsequent lines with the start of the first line
("Rename" in this case).

[sashab, 2014/10/14 02:53:18]

Done.

  */
-public class WebsiteSettingsPopup {
+public class WebsiteSettingsPopup implements OnClickListener, OnItemSelectedListener {
     private final Context mContext;
     private final WebContents mWebContents;
 
@@ -44,10 +58,18 @@ public class WebsiteSettingsPopup {
     // UI elements in the dialog.
     private final TextView mUrlTitle;
     private final TextView mUrlConnectionMessage;
+    private final LinearLayout mPermissionsList;
+    private final Button mCopyUrlButton;
+    private final Button mSiteSettingsButton;
+    private final Button mDoneButton;
 
     // The dialog the container is placed in.
     private final Dialog mDialog;
 
+    // The full URL from the URL bar, which is copied to the user's clipboard when they select 'Copy
+    // URL'.
+    private String mFullUrl;
+
     /**
      * Creates the WebsiteSettingsPopup, but does not display it. Also
      * initializes the corresponding C++ object and saves a pointer to it.
@@ -63,7 +85,21 @@ public class WebsiteSettingsPopup {
         mUrlTitle = (TextView) mContainer
                 .findViewById(R.id.website_settings_url);
         mUrlConnectionMessage = (TextView) mContainer
-                .findViewById(R.id.website_settings_permission_message);
+                .findViewById(R.id.website_settings_connection_message);
+        mPermissionsList = (LinearLayout) mContainer
+                .findViewById(R.id.website_settings_permissions_list);
+
+        mCopyUrlButton = (Button) mContainer.findViewById(R.id.website_settings_copy_url_button);
+        mCopyUrlButton.setOnClickListener(this);
+        mSiteSettingsButton = (Button) mContainer
+                .findViewById(R.id.website_settings_site_settings_button);
+        mSiteSettingsButton.setOnClickListener(this);
+        mDoneButton = (Button) mContainer.findViewById(R.id.website_settings_done_button);
+        mDoneButton.setOnClickListener(this);
+
+        // Hide the Site Settings button until there's a link to take it to.
+        // TODO(sashab,finnur): Make this button visible once Site Settings is working.
+        mSiteSettingsButton.setVisibility(View.GONE);

[Ted C, 2014/10/09 01:25:15]

move this up with the mSiteSettingsButton configuration.  And I would spaces
between the setup of mCopyUrlButton, mSiteSettingsButton, and mDoneButton
blocks.

[sashab, 2014/10/14 02:53:18]

Done.

 
         // Create the dialog.
         mDialog = new Dialog(mContext);
@@ -81,7 +117,8 @@ public class WebsiteSettingsPopup {
                 mWebContents) {
             @Override
             public void navigationEntryCommitted() {
-                // If a navigation is committed (e.g. from in-page redirect), the data we're
+                // If a navigation is committed (e.g. from in-page redirect),
+                // the data we're

[Ted C, 2014/10/09 01:25:15]

this looks like it fit fine before on 100 char lines, so this is a mistake???

[sashab, 2014/10/14 02:53:18]

Weird... Done.

                 // showing is stale so dismiss the dialog.
                 mDialog.dismiss();
             }
@@ -97,33 +134,67 @@ public class WebsiteSettingsPopup {
     }
 
     /**
+     * Returns the Image resource of the icon to use for the given permission.
+     * Permission must be a valid ContentSettingsType viewable in the PageInfo
+     * dialog.
+     */
+    private int getImageResourceForPermission(int permission) {
+        switch (permission) {
+            case ContentSettingsType.CONTENT_SETTINGS_TYPE_AUTOMATIC_DOWNLOADS:
+                return R.drawable.page_info_auto_downloads;
+                // TODO(sashab): Check if this needs to be removed - manually

[Ted C, 2014/10/09 01:25:14]

should this be above the return?

[sashab, 2014/10/14 02:53:18]

Yes, but I've now actually confirmed that this doesn't exist on mobile. Replaced
this list with only the 6 that we are using, so no comments required.

+                // setting downloads may
+                // not be available on mobile.

[Ted C, 2014/10/09 01:25:14]

put this on the previous line

[sashab, 2014/10/14 02:53:18]

Done.

+            case ContentSettingsType.CONTENT_SETTINGS_TYPE_FULLSCREEN:

[Ted C, 2014/10/09 01:25:15]

We don't use this on mobile.

[sashab, 2014/10/14 02:53:18]

Done.

+                // TODO(sashab): Check if this needs to be removed - manually
+                // setting fullscreen may
+                // not be available on mobile.
+                return R.drawable.page_info_fullscreen;
+            case ContentSettingsType.CONTENT_SETTINGS_TYPE_IMAGES:
+                return R.drawable.page_info_image;
+            case ContentSettingsType.CONTENT_SETTINGS_TYPE_JAVASCRIPT:
+                return R.drawable.page_info_javascript;
+            case ContentSettingsType.CONTENT_SETTINGS_TYPE_GEOLOCATION:
+                return R.drawable.page_info_location;
+            case ContentSettingsType.CONTENT_SETTINGS_TYPE_MEDIASTREAM:
+                return R.drawable.page_info_media;
+            case ContentSettingsType.CONTENT_SETTINGS_TYPE_NOTIFICATIONS:
+                return R.drawable.page_info_notification;
+            case ContentSettingsType.CONTENT_SETTINGS_TYPE_PLUGINS:
+                return R.drawable.page_info_plugins;
+            case ContentSettingsType.CONTENT_SETTINGS_TYPE_POPUPS:
+                return R.drawable.page_info_popups;
+            default:
+                assert false : "Icon requested for invalid permission: " + permission;
+                return -1;
+        }
+    }
+
+    /**
      * Sets the URL in the title to: (scheme)://(domain)(path). Also colors
-     * different parts of the URL depending on connectionType.
-     * connectionType should be a valid PageInfoConnectionType.
+     * different parts of the URL depending on connectionType. connectionType
+     * should be a valid PageInfoConnectionType.
      */
     @CalledByNative
-    private void setURLTitle(String scheme, String domain, String path, int connectionType) {
-        boolean makeDomainBold = false;
+    private void setURL(String fullUrl, String scheme, String domain, String path,
+            int connectionType) {
+        mFullUrl = fullUrl;
         int schemeColorId = R.color.website_settings_popup_url_scheme_broken;
         switch (connectionType) {
             case PageInfoConnectionType.CONNECTION_UNKNOWN:
                 schemeColorId = R.color.website_settings_popup_url_scheme_http;
-                makeDomainBold = true;
                 break;
             case PageInfoConnectionType.CONNECTION_ENCRYPTED:
                 schemeColorId = R.color.website_settings_popup_url_scheme_https;
                 break;
             case PageInfoConnectionType.CONNECTION_MIXED_CONTENT:
                 schemeColorId = R.color.website_settings_popup_url_scheme_mixed;
-                makeDomainBold = true;
                 break;
             case PageInfoConnectionType.CONNECTION_UNENCRYPTED:
                 schemeColorId = R.color.website_settings_popup_url_scheme_http;
-                makeDomainBold = true;
                 break;
             case PageInfoConnectionType.CONNECTION_ENCRYPTED_ERROR:
                 schemeColorId = R.color.website_settings_popup_url_scheme_broken;
-                makeDomainBold = true;
                 break;
             case PageInfoConnectionType.CONNECTION_INTERNAL_PAGE:
                 schemeColorId = R.color.website_settings_popup_url_scheme_http;
@@ -132,24 +203,21 @@ public class WebsiteSettingsPopup {
                 assert false : "Unexpected connection type: " + connectionType;
         }
 
+        // The scheme should be one color, the ://(domain) as another color, and
+        // the path as a third color.
         SpannableStringBuilder sb = new SpannableStringBuilder(scheme + "://" + domain + path);
 
         ForegroundColorSpan schemeColorSpan = new ForegroundColorSpan(
                 mContext.getResources().getColor(schemeColorId));
         sb.setSpan(schemeColorSpan, 0, scheme.length(), Spannable.SPAN_INCLUSIVE_INCLUSIVE);
 
-        int domainStartIndex = scheme.length() + 3;
+        int domainStartIndex = scheme.length();

[Ted C, 2014/10/09 01:25:14]

this should really be schemeEndIndex now

[sashab, 2014/10/14 02:53:17]

Done.

+        int domainEndIndex = domainStartIndex + domain.length() + 3;

[Ted C, 2014/10/09 01:25:15]

Out of curiosity, what about pages like about:blank?  Will the scheme divider be
: or will it be expanded to ://?  Just wondering if 3 isn't flexible enough.

[sashab, 2014/10/14 02:53:18]

Good point. Also confirmed with UI that we actually should be displaying the
whole URL, not just the 'compact' version like I've done here. Revamped this
function to just take a schemeEndIndex and domainEndIndex and color it
accordingly.

         ForegroundColorSpan domainColorSpan = new ForegroundColorSpan(
                 mContext.getResources().getColor(R.color.website_settings_popup_url_domain));
-        sb.setSpan(domainColorSpan, domainStartIndex, domainStartIndex + domain.length(),
+        sb.setSpan(domainColorSpan, domainStartIndex, domainEndIndex,
                 Spannable.SPAN_INCLUSIVE_INCLUSIVE);
 
-        if (makeDomainBold) {
-            StyleSpan boldStyleSpan = new StyleSpan(android.graphics.Typeface.BOLD);
-            sb.setSpan(boldStyleSpan, domainStartIndex, domainStartIndex + domain.length(),
-                    Spannable.SPAN_INCLUSIVE_INCLUSIVE);
-        }
-
         mUrlTitle.setText(sb);
     }
 
@@ -162,6 +230,49 @@ public class WebsiteSettingsPopup {
         mUrlConnectionMessage.setText(connectionMessage);
     }
 
+    /**
+     * Adds a new row for the given permission. - name is the title of the
+     * permission to display to the user. - type is a ContentSettingsType used
+     * to identify the permission; this is what is returned when the permission
+     * is changed. - settingsNames and settingsValues are the list of available
+     * options the user can select; the name is the value that is displayed and
+     * the value is the value returned when it is changed. - currentSettingIndex
+     * is the index of the currently selected setting in
+     * settingsNames/settingsValues.

[sashab, 2014/10/14 02:53:18]

Went through and changed all these comments to be aligned to 100 chars instead
of 80. Not sure why this happened.

Also added JavaDoc-style comments where appropriate.

+     */
+    @CalledByNative
+    private void addPermissionSection(String name, int type, String[] settingsNames,
+            int[] settingsValues, int currentSettingIndex) {
+        LinearLayout permission_row = (LinearLayout) LayoutInflater.from(mContext).inflate(

[Ted C, 2014/10/09 01:25:14]

always use camelCased names in java

[sashab, 2014/10/14 02:53:18]

Done.

+                R.layout.website_settings_permission_row, null);
+
+        ImageView permission_icon = (ImageView) permission_row.findViewById(
+                R.id.website_settings_permission_icon);
+        permission_icon.setImageResource(getImageResourceForPermission(type));
+
+        TextView permission_name = (TextView) permission_row.findViewById(
+                R.id.website_settings_permission_name);
+        permission_name.setText(name);
+
+        Spinner permission_spinner = (Spinner) permission_row.findViewById(
+                R.id.website_settings_permission_spinner);
+
+        // Save the permission type in a tag inside the spinner.
+        permission_spinner.setTag(R.id.website_settings_popup_permission_type, type);
+        permission_spinner.setTag(R.id.website_settings_popup_permission_spinner_values,
+                settingsValues);
+
+        ArrayAdapter<String> adapter = new ArrayAdapter<String>(mContext,

[Ted C, 2014/10/09 01:25:15]

Instead of using tags, I think it would be slightly nicer to have a simple class
that was:

private static final class PagePermissionEntry {
    public final String name;
    public final int value;
    public final int type;

    PagePermission(String name, int value, int type);

    public String toString() {
        return name;
    }
}

Then this would be an adapter of those and onItemSelected would just operate on
a single unit.

[sashab, 2014/10/14 02:53:18]

Nice. Done.

Also changed the place these are set to be purely in Java - agreed with UI that
these will always be fixed options (only Allow and Block), so no need to pass
these parameters (settingsNames/settingsValues) through anymore.

+                R.drawable.website_settings_permission_spinner_item, Arrays.asList(settingsNames));
+        adapter.setDropDownViewResource(
+                R.drawable.website_settings_permission_spinner_dropdown_item);
+        permission_spinner.setAdapter(adapter);
+        permission_spinner.setSelection(currentSettingIndex, false);
+        permission_spinner.setOnItemSelectedListener(this);
+
+        mPermissionsList.addView(permission_row);
+    }
+
     /** Displays the WebsiteSettingsPopup. */
     @CalledByNative
     private void showDialog() {
@@ -181,6 +292,38 @@ public class WebsiteSettingsPopup {
         mDialog.show();
     }
 
+    @Override
+    public void onItemSelected(AdapterView<?> parent, View view, int pos, long id) {
+        int[] settingsValues = (int[]) parent
+                .getTag(R.id.website_settings_popup_permission_spinner_values);
+        int settingsType = (Integer) parent.getTag(R.id.website_settings_popup_permission_type);
+        nativeOnPermissionSettingChanged(mNativeWebsiteSettingsPopup, settingsType,
+                settingsValues[pos]);
+    }
+
+    @Override
+    public void onNothingSelected(AdapterView<?> parent) {
+        // Do nothing intentionally.
+    }
+
+    @Override
+    public void onClick(View view) {
+        if (view == mCopyUrlButton) {
+            ClipboardManager clipboardManager = (ClipboardManager) mContext.getSystemService(

[Ted C, 2014/10/09 01:25:14]

Clipboard.java is slightly safer to compensate for some crashier old builds of
android.

(i.e. new Clipboard(mContext).setText(...))

And in most cases I see we are just passing the value for the label as well, so
we might just do that here to be consistent (or you need to pull "url" out as a
constant).

[sashab, 2014/10/14 02:53:18]

Thanks for this; done, and set the label to the value (I can see it done
everywhere else as well).

+                    Context.CLIPBOARD_SERVICE);
+            ClipData clip = ClipData.newPlainText("url", mFullUrl);
+            clipboardManager.setPrimaryClip(clip);
+            Toast.makeText(mContext, R.string.page_info_copy_success_text, Toast.LENGTH_SHORT)

[Ted C, 2014/10/09 01:25:15]

This toast isn't consistent with the rest of the copy actions we do in chrome
(nor the aosp part of android).  Wondering what the justification is for doing
it here?

Is it because we are not dismissing the popup if they click copy URL?  Should we
be and not using this toast?

[sashab, 2014/10/14 02:53:18]

Removed the toast; it was my own idea (and you're right - it's inconsistent :-)
). Confirming with UI what kind of feedback we want to give the user when they
click 'Copy URL'.

+                    .show();
+        } else if (view == mSiteSettingsButton) {
+            // TODO(sashab,finnur): Make this open the Website Settings dialog.
+            assert false : "No Website Settings here!";
+            mDialog.dismiss();
+        } else if (view == mDoneButton) {
+            mDialog.dismiss();
+        }
+    }
+
     /**
      * Shows a WebsiteSettings dialog for the provided WebContents. The popup
      * adds itself to the view hierarchy which owns the reference while it's
@@ -188,7 +331,7 @@ public class WebsiteSettingsPopup {
      *
      * @param context Context which is used for launching a dialog.
      * @param webContents The WebContents for which to show Website information.
-     *                    This information is retrieved for the visible entry.
+     *            This information is retrieved for the visible entry.
      */
     @SuppressWarnings("unused")
     public static void show(Context context, WebContents webContents) {
@@ -203,4 +346,7 @@ public class WebsiteSettingsPopup {
             WebContents webContents);
 
     private native void nativeDestroy(long nativeWebsiteSettingsPopupAndroid);
+
+    private native void nativeOnPermissionSettingChanged(long nativeWebsiteSettingsPopupAndroid,
+            int type, int setting);
 }