Android implementation of WebsiteSettingsUi

Android implementation of WebsiteSettingsUi


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

[Ted C, 2013-01-25 02:04:05]

NITS (for the most part)

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/res/la...
File chrome/android/java/res/layout/website_settings.xml (right):

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/res/la...
chrome/android/java/res/layout/website_settings.xml:1: <?xml version="1.0"
encoding="utf-8"?>
copyright?

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/res/la...
chrome/android/java/res/layout/website_settings.xml:4:
android:layout_width="fill_parent"
Sadly, I don't think you get warnings about it, but fill_parent has been
replaced by match_parent and is deprecated.

http://developer.android.com/reference/android/view/ViewGroup.LayoutParams.ht...

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/res/la...
chrome/android/java/res/layout/website_settings.xml:21:
android:layout_width="wrap_content"
indent here looks off as it's aligned with the above and this a child node.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/res/va...
File chrome/android/java/res/values/strings.xml (right):

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/res/va...
chrome/android/java/res/values/strings.xml:46: <!-- NO DESCRIPTION
[CHAR-LIMIT=32] -->
are these NO_DESCRIPTIONs on purpose?

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
File chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java
(right):

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:35:
CertificateFactory mCertificateFactory;
are these intentionally package scoped?  I suspect they should be private and
potentially final where applicable.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:41:
public static void showCert(Context context, byte[] der_data) {
all public methods should have javadoc.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:46:
public static void showCertChain(Context context, byte[][] der_data) {
der_data should be camel cased (and all other usages).

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:56:
R.dimen.certificate_viewer_padding) / 2;
why is it dividing by two?

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:68:
for (int i = 0; i < der_data.length; ++i) {
i++ in java land (applicable other places in this file)

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:103:
LinearLayout.LayoutParams.MATCH_PARENT));
should be indented 8 from the start of the line.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:110:
mCertificateFactory = CertificateFactory.getInstance("X.509");
should X.509 be a constant?

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:125:
X509Certificate x509 = (X509Certificate)cert;
space between the cast and the variable.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:156:
if (!value.isEmpty()) {
I would probably do if (value.isEmpty()) return;

then drop the braces.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:178:
static private String formatSerial(byte[] bytes) {
private before static.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:43:
ContentViewCore mContentViewCore;
private?

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:74:
h.setTypeface(Typeface.defaultFromStyle(Typeface.BOLD));
any reason it's not configured to be bold in the xml?

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:75:
if (headline == null || headline.length() == 0) h.setVisibility(View.GONE);
TextUtils.isEmpty(headline)

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:79:
if (description == null || description.length() == 0)
d.setVisibility(View.GONE);
same thing here about TextUtils.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:86:
void addDivider() {
private?

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:88:
divider.setLayoutParams(new LayoutParams(LayoutParams.MATCH_PARENT, 2));
should the 2 be in DP or pixels?

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:106:
HELP_URL);
I think this is only indented 7 instead of 8.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:110:
void addUrl(String label, String url) {
private?

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:121:
void show() {
private

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:126:
LinearLayout.LayoutParams.MATCH_PARENT));
same thing here too..should be indented 8 from previous line.

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/android/chr...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/android/chr...
chrome/browser/android/chrome_jni_registrar.cc:52:
WebsiteSettingsPopupAndroid::RegisterWebsiteSettingsPopupAndroid },
indenting is off here it looks like

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/ui/android/...
File chrome/browser/ui/android/website_settings_popup_android.cc (right):

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/ui/android/...
chrome/browser/ui/android/website_settings_popup_android.cc:67: jobject context,
indent is off.

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/ui/android/...
chrome/browser/ui/android/website_settings_popup_android.cc:75: jobject context,
same here too.

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/ui/android/...
chrome/browser/ui/android/website_settings_popup_android.cc:143: NULL,
description.obj());
indented too much

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/ui/android/...
chrome/browser/ui/android/website_settings_popup_android.cc:173: 
remove this extra line

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/ui/android/...
chrome/browser/ui/android/website_settings_popup_android.cc:174: void
WebsiteSettingsPopupAndroid::Destroy(JNIEnv* env, jobject obj) {
destroy should move up to match the ordering of the header.

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/ui/android/...
File chrome/browser/ui/android/website_settings_popup_android.h (right):

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/ui/android/...
chrome/browser/ui/android/website_settings_popup_android.h:23: jobject context,
indented 1 too many

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/ui/android/...
chrome/browser/ui/android/website_settings_popup_android.h:42: jobject context,
indenting is off.

[Yaron, 2013-01-25 19:25:17]

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/res/la...
File chrome/android/java/res/layout/website_settings.xml (right):

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/res/la...
chrome/android/java/res/layout/website_settings.xml:1: <?xml version="1.0"
encoding="utf-8"?>
On 2013/01/25 02:04:05, Ted C wrote:
> copyright?

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/res/la...
chrome/android/java/res/layout/website_settings.xml:4:
android:layout_width="fill_parent"
On 2013/01/25 02:04:05, Ted C wrote:
> Sadly, I don't think you get warnings about it, but fill_parent has been
> replaced by match_parent and is deprecated.
> 
>
http://developer.android.com/reference/android/view/ViewGroup.LayoutParams.ht...

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/res/la...
chrome/android/java/res/layout/website_settings.xml:21:
android:layout_width="wrap_content"
On 2013/01/25 02:04:05, Ted C wrote:
> indent here looks off as it's aligned with the above and this a child node.

there seems to be some inconsistencies in our xml files. I went for 2 indents
for children, and then lining up attributes with the first line.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/res/va...
File chrome/android/java/res/values/strings.xml (right):

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/res/va...
chrome/android/java/res/values/strings.xml:46: <!-- NO DESCRIPTION
[CHAR-LIMIT=32] -->
On 2013/01/25 02:04:05, Ted C wrote:
> are these NO_DESCRIPTIONs on purpose?

Changed to using native strings.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
File chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java
(right):

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:35:
CertificateFactory mCertificateFactory;
On 2013/01/25 02:04:05, Ted C wrote:
> are these intentionally package scoped?  I suspect they should be private and
> potentially final where applicable.

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:41:
public static void showCert(Context context, byte[] der_data) {
On 2013/01/25 02:04:05, Ted C wrote:
> all public methods should have javadoc.

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:46:
public static void showCertChain(Context context, byte[][] der_data) {
On 2013/01/25 02:04:05, Ted C wrote:
> der_data should be camel cased (and all other usages).

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:56:
R.dimen.certificate_viewer_padding) / 2;
On 2013/01/25 02:04:05, Ted C wrote:
> why is it dividing by two?

I tried removing and there's too much vertical spacing. Looks better as-is.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:68:
for (int i = 0; i < der_data.length; ++i) {
On 2013/01/25 02:04:05, Ted C wrote:
> i++ in java land (applicable other places in this file)

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:103:
LinearLayout.LayoutParams.MATCH_PARENT));
On 2013/01/25 02:04:05, Ted C wrote:
> should be indented 8 from the start of the line.

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:110:
mCertificateFactory = CertificateFactory.getInstance("X.509");
On 2013/01/25 02:04:05, Ted C wrote:
> should X.509 be a constant?

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:125:
X509Certificate x509 = (X509Certificate)cert;
On 2013/01/25 02:04:05, Ted C wrote:
> space between the cast and the variable.

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:156:
if (!value.isEmpty()) {
On 2013/01/25 02:04:05, Ted C wrote:
> I would probably do if (value.isEmpty()) return;
> 
> then drop the braces.

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:178:
static private String formatSerial(byte[] bytes) {
On 2013/01/25 02:04:05, Ted C wrote:
> private before static.

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:43:
ContentViewCore mContentViewCore;
On 2013/01/25 02:04:05, Ted C wrote:
> private?

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:74:
h.setTypeface(Typeface.defaultFromStyle(Typeface.BOLD));
On 2013/01/25 02:04:05, Ted C wrote:
> any reason it's not configured to be bold in the xml?

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:75:
if (headline == null || headline.length() == 0) h.setVisibility(View.GONE);
On 2013/01/25 02:04:05, Ted C wrote:
> TextUtils.isEmpty(headline)

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:79:
if (description == null || description.length() == 0)
d.setVisibility(View.GONE);
On 2013/01/25 02:04:05, Ted C wrote:
> same thing here about TextUtils.

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:86:
void addDivider() {
On 2013/01/25 02:04:05, Ted C wrote:
> private?

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:88:
divider.setLayoutParams(new LayoutParams(LayoutParams.MATCH_PARENT, 2));
On 2013/01/25 02:04:05, Ted C wrote:
> should the 2 be in DP or pixels?

I guess dp?

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:106:
HELP_URL);
On 2013/01/25 02:04:05, Ted C wrote:
> I think this is only indented 7 instead of 8.

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:110:
void addUrl(String label, String url) {
On 2013/01/25 02:04:05, Ted C wrote:
> private?

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:121:
void show() {
On 2013/01/25 02:04:05, Ted C wrote:
> private

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:126:
LinearLayout.LayoutParams.MATCH_PARENT));
On 2013/01/25 02:04:05, Ted C wrote:
> same thing here too..should be indented 8 from previous line.

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/android/chr...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/android/chr...
chrome/browser/android/chrome_jni_registrar.cc:52:
WebsiteSettingsPopupAndroid::RegisterWebsiteSettingsPopupAndroid },
On 2013/01/25 02:04:05, Ted C wrote:
> indenting is off here it looks like

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/ui/android/...
File chrome/browser/ui/android/website_settings_popup_android.cc (right):

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/ui/android/...
chrome/browser/ui/android/website_settings_popup_android.cc:67: jobject context,
On 2013/01/25 02:04:05, Ted C wrote:
> indent is off.

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/ui/android/...
chrome/browser/ui/android/website_settings_popup_android.cc:75: jobject context,
On 2013/01/25 02:04:05, Ted C wrote:
> same here too.

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/ui/android/...
chrome/browser/ui/android/website_settings_popup_android.cc:143: NULL,
description.obj());
On 2013/01/25 02:04:05, Ted C wrote:
> indented too much

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/ui/android/...
chrome/browser/ui/android/website_settings_popup_android.cc:173: 
On 2013/01/25 02:04:05, Ted C wrote:
> remove this extra line

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/ui/android/...
chrome/browser/ui/android/website_settings_popup_android.cc:174: void
WebsiteSettingsPopupAndroid::Destroy(JNIEnv* env, jobject obj) {
On 2013/01/25 02:04:05, Ted C wrote:
> destroy should move up to match the ordering of the header.

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/ui/android/...
File chrome/browser/ui/android/website_settings_popup_android.h (right):

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/ui/android/...
chrome/browser/ui/android/website_settings_popup_android.h:23: jobject context,
On 2013/01/25 02:04:05, Ted C wrote:
> indented 1 too many

Done.

https://codereview.chromium.org/12045081/diff/2001/chrome/browser/ui/android/...
chrome/browser/ui/android/website_settings_popup_android.h:42: jobject context,
On 2013/01/25 02:04:05, Ted C wrote:
> indenting is off.

Done.

[Ted C, 2013-01-25 19:33:26]

lgtm, awesome to have this be upstreamed...and couple final nits :-)

https://codereview.chromium.org/12045081/diff/9001/chrome/android/java/res/la...
File chrome/android/java/res/layout/website_settings.xml (right):

https://codereview.chromium.org/12045081/diff/9001/chrome/android/java/res/la...
chrome/android/java/res/layout/website_settings.xml:17: <LinearLayout
xmlns:android="http://schemas.android.com/apk/res/android"
don't need the xmlns: here...it's only needed for the outer most element.

https://codereview.chromium.org/12045081/diff/9001/chrome/android/java/src/or...
File chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java
(right):

https://codereview.chromium.org/12045081/diff/9001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:197: 
remove this blank line

[Yaron, 2013-01-25 19:48:50]

+the right Markus
+sky for trivial chrome/*.gypi changes

[sky, 2013-01-25 21:13:49]

LGTM - feel free to TBR gyp changes

https://codereview.chromium.org/12045081/diff/16001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/12045081/diff/16001/chrome/chrome_browser.gyp...
chrome/chrome_browser.gypi:3100:
'android/java/src/org/chromium/chrome/browser/database/SQLiteCursor.java',
Move this one to the right spot.

[markusheintz_, 2013-01-25 22:12:18]

Only two nits

LGTM

https://codereview.chromium.org/12045081/diff/16001/chrome/browser/ui/android...
File chrome/browser/ui/android/website_settings_popup_android.cc (right):

https://codereview.chromium.org/12045081/diff/16001/chrome/browser/ui/android...
chrome/browser/ui/android/website_settings_popup_android.cc:81: 
nit: I don't think we need this empty line.

https://codereview.chromium.org/12045081/diff/16001/chrome/browser/ui/android...
File chrome/browser/ui/android/website_settings_popup_android.h (right):

https://codereview.chromium.org/12045081/diff/16001/chrome/browser/ui/android...
chrome/browser/ui/android/website_settings_popup_android.h:23: jobject context,
Please indent one space less. Same at the two lines below.

[commit-bot: I haz the power, 2013-01-26 00:07:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yfriedman@chromium.org/12045081/17002

[commit-bot: I haz the power, 2013-01-26 04:20:10]

Change committed as 179027