Replaced the current WebsiteSettings dialog with a new PageInfo dialog

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 d4d0cf541b3bbf78b4503825166831c1f2ba2a18..77c9c43be27f3c01cf6c561655c23ccd3e69df61 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
@@ -7,77 +7,88 @@ package org.chromium.chrome.browser;
 import android.app.Dialog;
 import android.content.Context;
 import android.content.DialogInterface;
-import android.content.Intent;
 import android.graphics.Color;
-import android.provider.Browser;
-import android.text.TextUtils;
+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.Button;
-import android.widget.ImageView;
 import android.widget.LinearLayout;
 import android.widget.ScrollView;
 import android.widget.TextView;
 
-import org.chromium.base.ApiCompatibilityUtils;
 import org.chromium.base.CalledByNative;
+import org.chromium.base.CommandLine;
 import org.chromium.chrome.R;
 import org.chromium.content.browser.WebContentsObserverAndroid;
+import org.chromium.content.common.ContentSwitches;
 import org.chromium.content_public.browser.WebContents;
-
-import java.net.URISyntaxException;
+import org.chromium.ui.gfx.PageInfoConnectionType;
 
 /**
  * Java side of Android implementation of the website settings UI.
  */
-public class WebsiteSettingsPopup implements OnClickListener {
-    private static final String HELP_URL =
-            "http://www.google.com/support/chrome/bin/answer.py?answer=95617";
-    private static final int DESCRIPTION_TEXT_SIZE_SP = 12;
+public class WebsiteSettingsPopup {
     private final Context mContext;
-    private final Dialog mDialog;
-    private final LinearLayout mContainer;
     private final WebContents mWebContents;
-    private final int mPaddingWide, mPaddingThin;
-    private final long mNativeWebsiteSettingsPopup;
-    private TextView mCertificateViewer, mMoreInfoLink;
-    private ViewGroup mCertificateLayout, mDescriptionLayout;
-    private Button mResetCertDecisionsButton;
-    private String mLinkUrl;
 
+    // A pointer to the C++ object for this UI.
+    final long mNativeWebsiteSettingsPopup;

[Ted C, 2014/09/03 20:44:48]

can this be private?

[sashab, 2014/09/19 02:29:47]

Done.

+
+    // The outer container, filled with the layout from website_settings.xml.
+    private final LinearLayout mContainer;
+
+    // UI elements in the dialog.
+    private final TextView mUrlTitle, mUrlConnectionMessage;

[Ted C, 2014/09/03 20:44:48]

the previous file set a bad precedent, but I would put each on their own
separate line.

[sashab, 2014/09/19 02:29:48]

Done.

+
+    // The dialog the container is placed in.
+    private final Dialog mDialog;
+
+    /**
+     * Creates the WebsiteSettingsPopup, but does not display it. Also
+     * initializes the corresponding C++ object and saves a pointer to it.
+     */
     private WebsiteSettingsPopup(Context context, WebContents webContents) {
         mContext = context;
         mWebContents = webContents;
 
-        mContainer = new LinearLayout(mContext);
-        mContainer.setOrientation(LinearLayout.VERTICAL);
-        mContainer.setBackgroundColor(Color.WHITE);
-        mPaddingWide = (int) context.getResources().getDimension(
-                R.dimen.certificate_viewer_padding_wide);
-        mPaddingThin = (int) context.getResources().getDimension(
-                R.dimen.certificate_viewer_padding_thin);
-        mContainer.setPadding(mPaddingWide, mPaddingWide + mPaddingThin, mPaddingWide,
-                mPaddingWide);
+        // Find the container and all it's important subviews.
+        mContainer = (LinearLayout) LayoutInflater.from(mContext).inflate(
+                R.layout.website_settings, null);
+
+        mUrlTitle = (TextView) mContainer
+                .findViewById(R.id.website_settings_url);
+        mUrlConnectionMessage = (TextView) mContainer
+                .findViewById(R.id.website_settings_permission_message);
 
+        // Create the dialog.
         mDialog = new Dialog(mContext);
         mDialog.requestWindowFeature(Window.FEATURE_NO_TITLE);
         mDialog.setCanceledOnTouchOutside(true);
+
+        // Place the dialog at the top of the screen, and remove its border.
+        Window window = mDialog.getWindow();
+        window.setGravity(Gravity.TOP);
+        window.setBackgroundDrawable(new ColorDrawable(Color.TRANSPARENT));
+
         // This needs to come after other member initialization.
         mNativeWebsiteSettingsPopup = nativeInit(this, webContents);
-        final WebContentsObserverAndroid webContentsObserver =
-                new WebContentsObserverAndroid(mWebContents) {
-            @Override
+        final WebContentsObserverAndroid webContentsObserver = new WebContentsObserverAndroid(
+                mWebContents) {
+                @Override

[Ted C, 2014/09/03 20:44:48]

this should be -4 indented

[sashab, 2014/09/19 02:29:48]

I think the provided formatter I was using in Eclipse was misconfigured. Sorry
about that :)

             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/09/03 20:44:48]

why reformat this comment?  it looks like it all fit in 100 chars before and now
it's oddly formatted now.

[sashab, 2014/09/19 02:29:48]

Sorry, auto-format by Eclipse. Fixed.

                 // showing is stale so dismiss the dialog.
                 mDialog.dismiss();
             }
         };
         mDialog.setOnDismissListener(new DialogInterface.OnDismissListener() {
-            @Override
+                @Override

[Ted C, 2014/09/03 20:44:48]

-4 indent here too

[sashab, 2014/09/19 02:29:48]

Done.

             public void onDismiss(DialogInterface dialog) {
                 assert mNativeWebsiteSettingsPopup != 0;
                 webContentsObserver.detachFromWebContents();
@@ -87,155 +98,106 @@ public class WebsiteSettingsPopup implements OnClickListener {
     }
 
     /**
-     * Adds certificate section, which contains an icon, a headline, a
-     * description and a label for certificate info link.
+     * Sets the URL in the title to: |scheme|://|domain||path|. Also colors
+     * different parts of the URL depending on |connection_type|.
+     * |connection_type| should be a valid PageInfoConnectionType.

[Ted C, 2014/09/03 20:44:48]

the |variable| syntax is more for C++, it's fine here as these are private
methods but I wouldn't use it for public stuff.

[sashab, 2014/09/19 02:29:48]

Is there any way I should distinguish them? I'd rather have it all in the same
style. Just removed the |'s for now.

      */
     @CalledByNative
-    private void addCertificateSection(int enumeratedIconId, String headline, String description,
-            String label) {
-        View section = addSection(enumeratedIconId, headline, description);
-        assert mCertificateLayout == null;
-        mCertificateLayout = (ViewGroup) section.findViewById(R.id.website_settings_text_layout);
-        if (label != null && !label.isEmpty()) {
-            setCertificateViewer(label);
+    private void setURLTitle(String scheme, String domain, String path, int connection_type) {
+        boolean make_domain_bold = false;

[Ted C, 2014/09/03 20:44:48]

s/make_domain_bold/makeDomainBold

same camel casing comment for the rest of these variables.

[sashab, 2014/09/19 02:29:48]

Done.

+        int scheme_color_id = R.color.website_settings_popup_url_scheme_broken;
+        if (connection_type == PageInfoConnectionType.CONNECTION_UNKNOWN) {

[Ted C, 2014/09/03 20:44:48]

I would probably use a switch here for connectionType instead of the if/else ifs

[sashab, 2014/09/19 02:29:47]

Good idea, done.

+            scheme_color_id = R.color.website_settings_popup_url_scheme_http;
+            make_domain_bold = true;
+        } else if (connection_type == PageInfoConnectionType.CONNECTION_ENCRYPTED) {
+            scheme_color_id = R.color.website_settings_popup_url_scheme_https;
+        } else if (connection_type == PageInfoConnectionType.CONNECTION_MIXED_CONTENT) {
+            scheme_color_id = R.color.website_settings_popup_url_scheme_mixed;
+            make_domain_bold = true;
+        } else if (connection_type == PageInfoConnectionType.CONNECTION_UNENCRYPTED) {
+            scheme_color_id = R.color.website_settings_popup_url_scheme_http;
+            make_domain_bold = true;
+        } else if (connection_type == PageInfoConnectionType.CONNECTION_ENCRYPTED_ERROR) {
+            scheme_color_id = R.color.website_settings_popup_url_scheme_broken;
+            make_domain_bold = true;
+        } else if (connection_type == PageInfoConnectionType.CONNECTION_INTERNAL_PAGE) {
+            scheme_color_id = R.color.website_settings_popup_url_scheme_http;
         }

[Ted C, 2014/09/03 20:44:48]

I would have a else/default case where you:

assert false : "Unexpected connection type: " + connectionType;

[sashab, 2014/09/19 02:29:47]

Done.

-    }
-
-    /**
-     * Adds Description section, which contains an icon, a headline, and a
-     * description. Most likely headline for description is empty
-     */
-    @CalledByNative
-    private void addDescriptionSection(int enumeratedIconId, String headline, String description) {
-        View section = addSection(enumeratedIconId, headline, description);
-        assert mDescriptionLayout == null;
-        mDescriptionLayout = (ViewGroup) section.findViewById(R.id.website_settings_text_layout);
-    }
-
-    private View addSection(int enumeratedIconId, String headline, String description) {
-        View section = LayoutInflater.from(mContext).inflate(R.layout.website_settings, null);
-        ImageView i = (ImageView) section.findViewById(R.id.website_settings_icon);
-        int drawableId = ResourceId.mapToDrawableId(enumeratedIconId);
-        i.setImageResource(drawableId);
 
-        TextView h = (TextView) section.findViewById(R.id.website_settings_headline);
-        h.setText(headline);
-        if (TextUtils.isEmpty(headline)) h.setVisibility(View.GONE);
+        final SpannableStringBuilder sb = new SpannableStringBuilder(
+                scheme + "://" + domain + path);
 
-        TextView d = (TextView) section.findViewById(R.id.website_settings_description);
-        d.setText(description);
-        d.setTextSize(DESCRIPTION_TEXT_SIZE_SP);
-        if (TextUtils.isEmpty(description)) d.setVisibility(View.GONE);
+        ForegroundColorSpan scheme_color_span = new ForegroundColorSpan(
+                mContext.getResources().getColor(scheme_color_id));
+        sb.setSpan(scheme_color_span, 0, scheme.length(), Spannable.SPAN_INCLUSIVE_INCLUSIVE);
 
-        mContainer.addView(section);
-        return section;
-    }
+        int domain_start_index = scheme.length() + 3;
+        final ForegroundColorSpan domain_color_span = new ForegroundColorSpan(

[Ted C, 2014/09/03 20:44:48]

I would probably drop the finals in this method as they aren't super consistent

[sashab, 2014/09/19 02:29:47]

Done.

+                mContext.getResources().getColor(R.color.website_settings_popup_url_domain));
+        sb.setSpan(domain_color_span, domain_start_index, domain_start_index + domain.length(),
+                Spannable.SPAN_INCLUSIVE_INCLUSIVE);
 
-    private void setCertificateViewer(String label) {
-        assert mCertificateViewer == null;
-        mCertificateViewer = new TextView(mContext);
-        mCertificateViewer.setText(label);
-        mCertificateViewer.setTextColor(
-                mContext.getResources().getColor(R.color.website_settings_popup_text_link));
-        mCertificateViewer.setTextSize(DESCRIPTION_TEXT_SIZE_SP);
-        mCertificateViewer.setOnClickListener(this);
-        mCertificateViewer.setPadding(0, mPaddingWide, 0, mPaddingWide);
-        mCertificateLayout.addView(mCertificateViewer);
-    }
+        if (make_domain_bold) {
+            final StyleSpan bold_style_span = new StyleSpan(android.graphics.Typeface.BOLD);
+            sb.setSpan(bold_style_span, domain_start_index, domain_start_index + domain.length(),
+                    Spannable.SPAN_INCLUSIVE_INCLUSIVE);
+        }
 
-    @CalledByNative
-    private void addResetCertDecisionsButton(String label) {
-        assert mNativeWebsiteSettingsPopup != 0;
-        assert mResetCertDecisionsButton == null;
-
-        mResetCertDecisionsButton = new Button(mContext);
-        mResetCertDecisionsButton.setText(label);
-        ApiCompatibilityUtils.setBackgroundForView(mResetCertDecisionsButton,
-                mContext.getResources().getDrawable(
-                        R.drawable.website_settings_reset_cert_decisions));
-        mResetCertDecisionsButton.setTextColor(
-                mContext.getResources().getColor(
-                R.color.website_settings_popup_reset_cert_decisions_button));
-        mResetCertDecisionsButton.setTextSize(DESCRIPTION_TEXT_SIZE_SP);
-        mResetCertDecisionsButton.setOnClickListener(this);
-
-        LinearLayout container = new LinearLayout(mContext);
-        container.setOrientation(LinearLayout.VERTICAL);
-        container.addView(mResetCertDecisionsButton);
-        container.setPadding(0, 0, 0, mPaddingWide);
-        mContainer.addView(container);
+        mUrlTitle.setText(sb);
     }
 
+    /**
+     * Sets the connection message displayed at the top of the dialog to
+     * |connection_message| (e.g. "Could not securely connect to this site").
+     */
     @CalledByNative
-    private void addMoreInfoLink(String linkText) {
-        addUrl(linkText, HELP_URL);
-    }
-
-    /** Adds a section containing a description and a hyperlink. */
-    private void addUrl(String label, String url) {
-        mMoreInfoLink = new TextView(mContext);
-        mLinkUrl = url;
-        mMoreInfoLink.setText(label);
-        mMoreInfoLink.setTextColor(
-                mContext.getResources().getColor(R.color.website_settings_popup_text_link));
-        mMoreInfoLink.setTextSize(DESCRIPTION_TEXT_SIZE_SP);
-        mMoreInfoLink.setPadding(0, mPaddingWide + mPaddingThin, 0, mPaddingWide);
-        mMoreInfoLink.setOnClickListener(this);
-        mDescriptionLayout.addView(mMoreInfoLink);
+    private void setConnectionMessage(String connection_message) {
+        mUrlConnectionMessage.setText(connection_message);
     }
 
     /** Displays the WebsiteSettingsPopup. */
     @CalledByNative
     private void showDialog() {
+        // Wrap the dialog in a ScrollView in case the content is too long.
         ScrollView scrollView = new ScrollView(mContext);
+        scrollView.setLayoutParams(new LinearLayout.LayoutParams(

[Ted C, 2014/09/03 20:44:48]

you shouldn't need to set the layout params here as you're respecifying them
down below in the addContentView calls.

[sashab, 2014/09/19 02:29:48]

Aren't these the layout params for the scrollView's parent? It needs to stretch
to fill the surrounding container. Removed for now because I think I
misunderstand what these do, since it still seems to work.

+                LinearLayout.LayoutParams.MATCH_PARENT,
+                LinearLayout.LayoutParams.MATCH_PARENT));
         scrollView.addView(mContainer);
-        mDialog.addContentView(scrollView,
-                new LinearLayout.LayoutParams(LinearLayout.LayoutParams.MATCH_PARENT,
-                        LinearLayout.LayoutParams.MATCH_PARENT));
-        mDialog.show();
-    }
+        mDialog.addContentView(scrollView, new LinearLayout.LayoutParams(
+                LinearLayout.LayoutParams.MATCH_PARENT,
+                LinearLayout.LayoutParams.MATCH_PARENT));
 
-    @Override
-    public void onClick(View v) {
-        mDialog.dismiss();
-        if (mResetCertDecisionsButton == v) {
-            nativeResetCertDecisions(mNativeWebsiteSettingsPopup, mWebContents);
-        } else if (mCertificateViewer == v) {
-            byte[][] certChain = nativeGetCertificateChain(mWebContents);
-            if (certChain == null) {
-                // The WebContents may have been destroyed/invalidated. If so,
-                // ignore this request.
-                return;
-            }
-            CertificateViewer.showCertificateChain(mContext, certChain);
-        } else if (mMoreInfoLink == v) {
-            try {
-                Intent i = Intent.parseUri(mLinkUrl, Intent.URI_INTENT_SCHEME);
-                i.putExtra(Browser.EXTRA_CREATE_NEW_TAB, true);
-                i.putExtra(Browser.EXTRA_APPLICATION_ID, mContext.getPackageName());
-                mContext.startActivity(i);
-            } catch (URISyntaxException ex) {}
-        }
+        // Make the dialog fill the width of the screen. This must be called
+        // after addContentView, or it won't fully fill to the edge.
+        Window window = mDialog.getWindow();
+        window.setLayout(ViewGroup.LayoutParams.MATCH_PARENT,

[Ted C, 2014/09/03 20:44:48]

what are the plans for tablet?  I'm doubtful we want this to go full width on a
10" screen

[sashab, 2014/09/19 02:29:47]

I don't have mocks for a tablet version yet... What do we usually do? And how
can I make this only go full-width on a tablet?

+                ViewGroup.LayoutParams.WRAP_CONTENT);
+
+        mDialog.show();
     }
 
     /**
-     * Shows a WebsiteSettings dialog for the provided WebContents.
-     *
-     * The popup adds itself to the view hierarchy which owns the reference while it's
+     * Shows a WebsiteSettings dialog for the provided WebContents. The popup
+     * adds itself to the view hierarchy which owns the reference while it's
      * visible.
      *
      * @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.
+     * @param webContents The WebContents for which to show Website information.
+     *            This information is retrieved for the visible entry.

[Ted C, 2014/09/03 20:44:48]

align this with "The WebContents" above.

[sashab, 2014/09/19 02:29:47]

Done.

      */
     @SuppressWarnings("unused")
     public static void show(Context context, WebContents webContents) {
-        new WebsiteSettingsPopup(context, webContents);
+        if (CommandLine.getInstance().hasSwitch(
+                ContentSwitches.ENABLE_NEW_WEBSITE_SETTINGS)) {
+            new WebsiteSettingsPopup(context, webContents);
+        } else {
+            WebsiteSettingsPopupLegacy.show(context, webContents);
+        }
     }
 
-    private static native long nativeInit(WebsiteSettingsPopup popup, WebContents webContents);
+    private static native long nativeInit(WebsiteSettingsPopup popup,
+            WebContents webContents);
+
     private native void nativeDestroy(long nativeWebsiteSettingsPopupAndroid);
-    private native void nativeResetCertDecisions(
-            long nativeWebsiteSettingsPopupAndroid, WebContents webContents);
-    private native byte[][] nativeGetCertificateChain(WebContents webContents);
 }