Redesign GUI of Website Setting Popup Dialog

Redesign GUI of Website Setting Popup Dialog

Implement the new design of SSL certificate dialog. Remove underline in hyperlink textview and add paddings around textviews. Refer to https://drive.google.com/a/google.com/file/d/0B6x6iYCtKinEUElmYWRWVlh5RlE/view for design specification.

BUG=394564
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285181

[Ian Wen, 2014-07-22 19:19:37]

Hi Aurimas, please help me do a code review. :)

[Ian Wen, 2014-07-22 20:50:43]

Screenshots are located at
https://code.google.com/p/chromium/issues/detail?id=394564#c9

[aurimas (slooooooooow), 2014-07-22 22:05:37]

https://codereview.chromium.org/409003002/diff/40001/chrome/android/java/res/...
File chrome/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/409003002/diff/40001/chrome/android/java/res/...
chrome/android/java/res/values/dimens.xml:9: <dimen
name="certificate_viewer_padding">24dp</dimen>
Rename to certificate_viewer_padding_wide since you are adding the second one.

https://codereview.chromium.org/409003002/diff/40001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/409003002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:35:
private static final String LINK_COLOR_BLUE = "#2AAFE0";
Please move this to colors.xml

https://codereview.chromium.org/409003002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:84:
/** Adds certificate section, which contains an icon, a headline, and a
description. */
Change this to be a proper javadoc to make it look like:
/**
 * Adds certificate section, which contains an icon, a headline, and a
description.
 */

(please also fix other comments in this file)

https://codereview.chromium.org/409003002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:127:
if (mCertificateLayout != null) {
In what cases will mCertificateLayout will be null? Should we make it into an
assertion instead of an if check?

https://codereview.chromium.org/409003002/diff/40001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/409003002/diff/40001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9144: +          Certificate Information
Change back to "Certificate Information" as Android is sentence-case and not
title-case.

[Ian Wen, 2014-07-23 00:05:41]

https://codereview.chromium.org/409003002/diff/40001/chrome/android/java/res/...
File chrome/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/409003002/diff/40001/chrome/android/java/res/...
chrome/android/java/res/values/dimens.xml:9: <dimen
name="certificate_viewer_padding">24dp</dimen>
On 2014/07/22 22:05:36, aurimas wrote:
> Rename to certificate_viewer_padding_wide since you are adding the second one.

Not renamed as thin is used in XML.

https://codereview.chromium.org/409003002/diff/40001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/409003002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:35:
private static final String LINK_COLOR_BLUE = "#2AAFE0";
On 2014/07/22 22:05:36, aurimas wrote:
> Please move this to colors.xml

Done.

https://codereview.chromium.org/409003002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:84:
/** Adds certificate section, which contains an icon, a headline, and a
description. */
On 2014/07/22 22:05:36, aurimas wrote:
> Change this to be a proper javadoc to make it look like:
> /**
>  * Adds certificate section, which contains an icon, a headline, and a
> description.
>  */
> 
> (please also fix other comments in this file)

Done.

https://codereview.chromium.org/409003002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:127:
if (mCertificateLayout != null) {
On 2014/07/22 22:05:36, aurimas wrote:
> In what cases will mCertificateLayout will be null? Should we make it into an
> assertion instead of an if check?

Done.

https://codereview.chromium.org/409003002/diff/40001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/409003002/diff/40001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9144: +          Certificate Information
On 2014/07/22 22:05:36, aurimas wrote:
> Change back to "Certificate Information" as Android is sentence-case and not
> title-case.

Done.

[aurimas (slooooooooow), 2014-07-23 00:13:32]

https://codereview.chromium.org/409003002/diff/40001/chrome/android/java/res/...
File chrome/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/409003002/diff/40001/chrome/android/java/res/...
chrome/android/java/res/values/dimens.xml:9: <dimen
name="certificate_viewer_padding">24dp</dimen>
On 2014/07/23 00:05:41, Ian Wen wrote:
> On 2014/07/22 22:05:36, aurimas wrote:
> > Rename to certificate_viewer_padding_wide since you are adding the second
one.
> 
> Not renamed as thin is used in XML.

I do not understand? I am suggesting to rename certificate_viewer_padding to
certificate_viewer_padding_wide and keep certificate_viewer_padding_thin as it
is.

[Ian Wen, 2014-07-23 00:38:16]

https://codereview.chromium.org/409003002/diff/40001/chrome/android/java/res/...
File chrome/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/409003002/diff/40001/chrome/android/java/res/...
chrome/android/java/res/values/dimens.xml:9: <dimen
name="certificate_viewer_padding">24dp</dimen>
On 2014/07/23 00:13:31, aurimas wrote:
> On 2014/07/23 00:05:41, Ian Wen wrote:
> > On 2014/07/22 22:05:36, aurimas wrote:
> > > Rename to certificate_viewer_padding_wide since you are adding the second
> one.
> > 
> > Not renamed as thin is used in XML.
> 
> I do not understand? I am suggesting to rename certificate_viewer_padding to
> certificate_viewer_padding_wide and keep certificate_viewer_padding_thin as it
> is.

Done.

[aurimas (slooooooooow), 2014-07-23 01:17:40]

lgtm % comments

https://chromiumcodereview.appspot.com/409003002/diff/80001/chrome/android/ja...
File chrome/android/java/res/layout/website_settings.xml (right):

https://chromiumcodereview.appspot.com/409003002/diff/80001/chrome/android/ja...
chrome/android/java/res/layout/website_settings.xml:7: <LinearLayout
xmlns:android="http://schemas.android.com/apk/res/android"
Please format this xml file. It is not properly indented.

https://chromiumcodereview.appspot.com/409003002/diff/80001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://chromiumcodereview.appspot.com/409003002/diff/80001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:149:
mMoreInfoLink.setTextSize(12);
Extract 12 as a constant.

[Ian Wen, 2014-07-23 01:30:07]

https://codereview.chromium.org/409003002/diff/80001/chrome/android/java/res/...
File chrome/android/java/res/layout/website_settings.xml (right):

https://codereview.chromium.org/409003002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings.xml:7: <LinearLayout
xmlns:android="http://schemas.android.com/apk/res/android"
On 2014/07/23 01:17:40, aurimas wrote:
> Please format this xml file. It is not properly indented.

Done.

https://codereview.chromium.org/409003002/diff/80001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/409003002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:149:
mMoreInfoLink.setTextSize(12);
On 2014/07/23 01:17:40, aurimas wrote:
> Extract 12 as a constant.

Done.

[David Trainor- moved to gerrit, 2014-07-23 17:17:10]

On 2014/07/23 01:30:07, Ian Wen wrote:
>
https://codereview.chromium.org/409003002/diff/80001/chrome/android/java/res/...
> File chrome/android/java/res/layout/website_settings.xml (right):
> 
>
https://codereview.chromium.org/409003002/diff/80001/chrome/android/java/res/...
> chrome/android/java/res/layout/website_settings.xml:7: <LinearLayout
> xmlns:android="http://schemas.android.com/apk/res/android"
> On 2014/07/23 01:17:40, aurimas wrote:
> > Please format this xml file. It is not properly indented.
> 
> Done.
> 
>
https://codereview.chromium.org/409003002/diff/80001/chrome/android/java/src/...
> File
> chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
> (right):
> 
>
https://codereview.chromium.org/409003002/diff/80001/chrome/android/java/src/...
>
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:149:
> mMoreInfoLink.setTextSize(12);
> On 2014/07/23 01:17:40, aurimas wrote:
> > Extract 12 as a constant.
> 
> Done.

Pre-review comments:
- Do we need xxhdpi drawables?
- Should any of those drawables be mirrored for rtl languages?

[David Trainor- moved to gerrit, 2014-07-23 17:23:55]

https://codereview.chromium.org/409003002/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/409003002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:37:
private static final int DESCRIPTION_TEXT_SIZE = 12;
can you add a unit?  DESCRIPTION_TEXT_SIZE_SP?

https://codereview.chromium.org/409003002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:92:
mCertificateLayout = (ViewGroup)
section.findViewById(R.id.website_settings_text_layout);
should we assert that this is not already set?  Same below?

https://codereview.chromium.org/409003002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:127:
mCertificateViewer =  new TextView(mContext);
remove extra space?

https://codereview.chromium.org/409003002/diff/100001/chrome/browser/ui/andro...
File chrome/browser/ui/android/website_settings_popup_android.cc (right):

https://codereview.chromium.org/409003002/diff/100001/chrome/browser/ui/andro...
chrome/browser/ui/android/website_settings_popup_android.cc:138:
Java_WebsiteSettingsPopup_setCertificateViewer(
Can we just pass the cert label to the function above?  And treat an empty/null
string as if it isn't there?  They you wouldn't have to save the section
anymore.

[Ian Wen, 2014-07-23 17:25:29]

On 2014/07/23 17:17:10, David Trainor wrote:
> On 2014/07/23 01:30:07, Ian Wen wrote:
> >
>
https://codereview.chromium.org/409003002/diff/80001/chrome/android/java/res/...
> > File chrome/android/java/res/layout/website_settings.xml (right):
> > 
> >
>
https://codereview.chromium.org/409003002/diff/80001/chrome/android/java/res/...
> > chrome/android/java/res/layout/website_settings.xml:7: <LinearLayout
> > xmlns:android="http://schemas.android.com/apk/res/android"
> > On 2014/07/23 01:17:40, aurimas wrote:
> > > Please format this xml file. It is not properly indented.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/409003002/diff/80001/chrome/android/java/src/...
> > File
> >
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/409003002/diff/80001/chrome/android/java/src/...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:149:
> > mMoreInfoLink.setTextSize(12);
> > On 2014/07/23 01:17:40, aurimas wrote:
> > > Extract 12 as a constant.
> > 
> > Done.
> 
> Pre-review comments:
> - Do we need xxhdpi drawables?
> - Should any of those drawables be mirrored for rtl languages?

1. The UX designer insisted on using XXhdpi assets
(https://code.google.com/p/chromium/issues/detail?id=394564#c12). Originally we
were not using page_info icons in xxhdpi.

2. The new icons
(https://drive.google.com/a/google.com/folderview?id=0B6x6iYCtKinEcHcyU3VFZXpv...)
are just flattened version of previous ones. I think if there were no problem
with old icons, new icons should not cause any RTL issues. Plus most of the
icons are symmetric.

[aurimas (slooooooooow), 2014-07-23 17:36:19]

On 2014/07/23 17:25:29, Ian Wen wrote:
> On 2014/07/23 17:17:10, David Trainor wrote:
> > On 2014/07/23 01:30:07, Ian Wen wrote:
> > >
> >
>
https://codereview.chromium.org/409003002/diff/80001/chrome/android/java/res/...
> > > File chrome/android/java/res/layout/website_settings.xml (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/409003002/diff/80001/chrome/android/java/res/...
> > > chrome/android/java/res/layout/website_settings.xml:7: <LinearLayout
> > > xmlns:android="http://schemas.android.com/apk/res/android"
> > > On 2014/07/23 01:17:40, aurimas wrote:
> > > > Please format this xml file. It is not properly indented.
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/409003002/diff/80001/chrome/android/java/src/...
> > > File
> > >
> chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/409003002/diff/80001/chrome/android/java/src/...
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:149:
> > > mMoreInfoLink.setTextSize(12);
> > > On 2014/07/23 01:17:40, aurimas wrote:
> > > > Extract 12 as a constant.
> > > 
> > > Done.
> > 
> > Pre-review comments:
> > - Do we need xxhdpi drawables?
> > - Should any of those drawables be mirrored for rtl languages?
> 
> 1. The UX designer insisted on using XXhdpi assets
> (https://code.google.com/p/chromium/issues/detail?id=394564#c12). Originally
we
> were not using page_info icons in xxhdpi.
We should have xxhdpi assets. We have been adding this density with the latest
UI refresh.

> 2. The new icons
>
(https://drive.google.com/a/google.com/folderview?id=0B6x6iYCtKinEcHcyU3VFZXpv...)
> are just flattened version of previous ones. I think if there were no problem
> with old icons, new icons should not cause any RTL issues. Plus most of the
> icons are symmetric.


Don't forget to update
./content/public/android/java/res/drawable-hdpi/pageinfo_warning_major.png

[Ian Wen, 2014-07-23 17:43:47]

We do not use pageinfo_warning_major.png in Clank. It seems to be only used
on desktop or chromOS.


On Wed, Jul 23, 2014 at 10:36 AM, <aurimas@chromium.org> wrote:

> On 2014/07/23 17:25:29, Ian Wen wrote:
>
>> On 2014/07/23 17:17:10, David Trainor wrote:
>> > On 2014/07/23 01:30:07, Ian Wen wrote:
>> > >
>> >
>>
>
> https://codereview.chromium.org/409003002/diff/80001/
> chrome/android/java/res/layout/website_settings.xml
>
>> > > File chrome/android/java/res/layout/website_settings.xml (right):
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/409003002/diff/80001/
> chrome/android/java/res/layout/website_settings.xml#newcode7
>
>> > > chrome/android/java/res/layout/website_settings.xml:7: <LinearLayout
>> > > xmlns:android="http://schemas.android.com/apk/res/android"
>> > > On 2014/07/23 01:17:40, aurimas wrote:
>> > > > Please format this xml file. It is not properly indented.
>> > >
>> > > Done.
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/409003002/diff/80001/
> chrome/android/java/src/org/chromium/chrome/browser/
> WebsiteSettingsPopup.java
>
>> > > File
>> > >
>> chrome/android/java/src/org/chromium/chrome/browser/
>> WebsiteSettingsPopup.java
>> > > (right):
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/409003002/diff/80001/
> chrome/android/java/src/org/chromium/chrome/browser/
> WebsiteSettingsPopup.java#newcode149
>
>> > >
>> >
>>
>
> chrome/android/java/src/org/chromium/chrome/browser/
> WebsiteSettingsPopup.java:149:
>
>> > > mMoreInfoLink.setTextSize(12);
>> > > On 2014/07/23 01:17:40, aurimas wrote:
>> > > > Extract 12 as a constant.
>> > >
>> > > Done.
>> >
>> > Pre-review comments:
>> > - Do we need xxhdpi drawables?
>> > - Should any of those drawables be mirrored for rtl languages?
>>
>
>  1. The UX designer insisted on using XXhdpi assets
>> (https://code.google.com/p/chromium/issues/detail?id=394564#c12).
>> Originally
>>
> we
>
>> were not using page_info icons in xxhdpi.
>>
> We should have xxhdpi assets. We have been adding this density with the
> latest
> UI refresh.
>
>
>  2. The new icons
>>
>
> (https://drive.google.com/a/google.com/folderview?id=
> 0B6x6iYCtKinEcHcyU3VFZXpvaDQ&usp=sharing)
>
>> are just flattened version of previous ones. I think if there were no
>> problem
>> with old icons, new icons should not cause any RTL issues. Plus most of
>> the
>> icons are symmetric.
>>
>
>
> Don't forget to update
> ./content/public/android/java/res/drawable-hdpi/pageinfo_warning_major.png
>
> https://codereview.chromium.org/409003002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ian Wen, 2014-07-23 18:28:26]

https://codereview.chromium.org/409003002/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/409003002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:37:
private static final int DESCRIPTION_TEXT_SIZE = 12;
On 2014/07/23 17:23:54, David Trainor wrote:
> can you add a unit?  DESCRIPTION_TEXT_SIZE_SP?

Done.

https://codereview.chromium.org/409003002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:92:
mCertificateLayout = (ViewGroup)
section.findViewById(R.id.website_settings_text_layout);
On 2014/07/23 17:23:54, David Trainor wrote:
> should we assert that this is not already set?  Same below?

Done.

https://codereview.chromium.org/409003002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:127:
mCertificateViewer =  new TextView(mContext);
On 2014/07/23 17:23:54, David Trainor wrote:
> remove extra space?

Done.

https://codereview.chromium.org/409003002/diff/100001/chrome/browser/ui/andro...
File chrome/browser/ui/android/website_settings_popup_android.cc (right):

https://codereview.chromium.org/409003002/diff/100001/chrome/browser/ui/andro...
chrome/browser/ui/android/website_settings_popup_android.cc:138:
Java_WebsiteSettingsPopup_setCertificateViewer(
On 2014/07/23 17:23:54, David Trainor wrote:
> Can we just pass the cert label to the function above?  And treat an
empty/null
> string as if it isn't there?  They you wouldn't have to save the section
> anymore.

Done.

[David Trainor- moved to gerrit, 2014-07-23 19:46:37]

lgtm

[Ian Wen, 2014-07-23 20:15:07]

The CQ bit was checked by ianwen@chromium.org

[commit-bot: I haz the power, 2014-07-23 20:17:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ianwen@chromium.org/409003002/140001

[Yaron, 2014-07-23 20:43:27]

lgtm

just 2 nits

https://codereview.chromium.org/409003002/diff/140001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/409003002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:139:
assert mCertificateLayout != null;
This is extraneous. You'll just crash on the next line and it'll be obvious why.

https://codereview.chromium.org/409003002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:158:
assert mDescriptionLayout != null;
same here

[Ian Wen, 2014-07-23 21:03:19]

The CQ bit was unchecked by ianwen@chromium.org

[Ian Wen, 2014-07-23 21:03:19]

The CQ bit was checked by ianwen@chromium.org

[commit-bot: I haz the power, 2014-07-23 21:03:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ianwen@chromium.org/409003002/140001

[Ian Wen, 2014-07-23 21:09:08]

The CQ bit was checked by ianwen@chromium.org

[commit-bot: I haz the power, 2014-07-23 21:11:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ianwen@chromium.org/409003002/160001

[commit-bot: I haz the power, 2014-07-24 10:59:10]

Change committed as 285181