Add connection info popup within Page Info on Android.

Add connection info popup within Page Info on Android.

Partial revert of 00e86ade3b71d372ebe49c3e24da75361e841dc7

This is a revived version of WebsiteSettingsPopupLegacy, which contains
detailed connection and certificate information, similar to the desktop
website settings popup. The popup is accessed through a 'Details' link
in the page info popup.

BUG=425158
Committed: https://crrev.com/f21c52aeafa701b18ed505347ee0e7a7d07e5d53
Cr-Commit-Position: refs/heads/master@{#327429}

[tsergeant, 2015-04-24 01:48:09]

Patchset #2 (id:20001) has been deleted

[tsergeant, 2015-04-24 02:59:11]

tsergeant@chromium.org changed reviewers:
+ tedchoc@chromium.org

[tsergeant, 2015-04-24 02:59:11]

tedchoc@, can you please review?

[Ted C, 2015-04-24 21:55:39]

On 2015/04/24 02:59:11, tsergeant wrote:
> tedchoc@, can you please review?

Did you end up needing to change anything in connection_info ||
certificate_viewer (c++ or java)?  I don't know if I need to be reviewing those
files or not.  If there are no changes and it is a clean revert, then I think
this looks fine.

But if you did need to modify those files, then I'll go over them in more
detail.

[tsergeant, 2015-04-27 00:24:23]

On 2015/04/24 21:55:39, Ted C wrote:
> On 2015/04/24 02:59:11, tsergeant wrote:
> > tedchoc@, can you please review?
> 
> Did you end up needing to change anything in connection_info ||
> certificate_viewer (c++ or java)?  I don't know if I need to be reviewing
those
> files or not.  If there are no changes and it is a clean revert, then I think
> this looks fine.
> 
> But if you did need to modify those files, then I'll go over them in more
> detail.

Oops, I was meaning to explain that but forgot =/

* connection_info_popup_android.{h,cc} are unchanged apart from the rename from
website_settings_popup_legacy
* certificate_viewer_android.cc is untouched
* ConnectionInfoPopup.java and CertificateViewer.java have had some minor
changes to padding (see screenshots on bug)
* resource_id.h was changed to consolidate down the unused icons
* Most other files were unchanged apart from the rename

[Ted C, 2015-04-27 19:04:01]

lgtm w/ some small cleanups

The reverted files are in dire need of cleanup as well, but i'd rather have this
be a cleaner revert than trying to mangle them too much.

https://codereview.chromium.org/1100283002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/drawable/connection_info_reset_cert_decisions.xml
(right):

https://codereview.chromium.org/1100283002/diff/40001/chrome/android/java/res...
chrome/android/java/res/drawable/connection_info_reset_cert_decisions.xml:3:
<!-- Copyright 2014 The Chromium Authors. All rights reserved.
2015 now :-)...for any new files might as well update them since it will be hard
to see this as a true revert anymore.

https://codereview.chromium.org/1100283002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/connection_info.xml (right):

https://codereview.chromium.org/1100283002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/connection_info.xml:3: Copyright (c) 2013 The
Chromium Authors. All rights reserved.
no (c) here either

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

https://codereview.chromium.org/1100283002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:503:
messageBuilder.append(" ");
I "assume" this is fine since we do similar things above about all languages. 
It's a separate sentence, do I think it is ok, but still makes me wary.

https://codereview.chromium.org/1100283002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:507:
mContext.getResources().getColor(R.color.website_settings_popup_text_link));
can the span not be applied prior to appending the text?  just wondering if you
can build the detailsText as a Spannable string and just plop it on the end.

https://codereview.chromium.org/1100283002/diff/40001/chrome/browser/android/...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/1100283002/diff/40001/chrome/browser/android/...
chrome/browser/android/chrome_jni_registrar.cc:124: bool
RegisterCertificateViewer(JNIEnv* env);
I think you should just include the .h for the certificateviewer instead of this
type of forward declare (as it is really out of place)

[Ted C, 2015-04-28 01:17:40]

https://codereview.chromium.org/1100283002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java
(right):

https://codereview.chromium.org/1100283002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:87:
(int)
mContext.getResources().getDimension(R.dimen.connection_info_padding_spinner);
I suspect you'll find this value to be dependent on OS version and even
manufacturer, so it might not always end up doing what you want.

Unless this is overwriting the padding with something that does line up, but if
this was compounding with an internal padding around some drawable inside I
suspect you'll find it might not work out always.

[tsergeant, 2015-04-28 03:15:00]

I've addressed the comments - there ended up being a reasonable number of
changes, so I'd appreciate it if you could give it another once over before I
commit.

> The reverted files are in dire need of cleanup as well, but i'd rather have
this
> be a cleaner revert than trying to mangle them too much.

Agreed. In theory, we'll be improving/replacing this dialog later this year
anyway, so I'm not sure there's much value in spending time cleaning the
existing code.

https://codereview.chromium.org/1100283002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/drawable/connection_info_reset_cert_decisions.xml
(right):

https://codereview.chromium.org/1100283002/diff/40001/chrome/android/java/res...
chrome/android/java/res/drawable/connection_info_reset_cert_decisions.xml:3:
<!-- Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/04/27 19:04:01, Ted C wrote:
> 2015 now :-)...for any new files might as well update them since it will be
hard
> to see this as a true revert anymore.

Done.

https://codereview.chromium.org/1100283002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/connection_info.xml (right):

https://codereview.chromium.org/1100283002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/connection_info.xml:3: Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2015/04/27 19:04:01, Ted C wrote:
> no (c) here either

Done.

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

https://codereview.chromium.org/1100283002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:507:
mContext.getResources().getColor(R.color.website_settings_popup_text_link));
On 2015/04/27 19:04:01, Ted C wrote:
> can the span not be applied prior to appending the text?  just wondering if
you
> can build the detailsText as a Spannable string and just plop it on the end.

Done.

https://codereview.chromium.org/1100283002/diff/40001/chrome/browser/android/...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/1100283002/diff/40001/chrome/browser/android/...
chrome/browser/android/chrome_jni_registrar.cc:124: bool
RegisterCertificateViewer(JNIEnv* env);
On 2015/04/27 19:04:01, Ted C wrote:
> I think you should just include the .h for the certificateviewer instead of
this
> type of forward declare (as it is really out of place)

Done. There wasn't a .h file for the Android specific code (which I guess was
why this was here in the first place), so I added one with just this function.

https://codereview.chromium.org/1100283002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java
(right):

https://codereview.chromium.org/1100283002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:87:
(int)
mContext.getResources().getDimension(R.dimen.connection_info_padding_spinner);
On 2015/04/28 01:17:40, Ted C wrote:
> I suspect you'll find this value to be dependent on OS version and even
> manufacturer, so it might not always end up doing what you want.
> 
> Unless this is overwriting the padding with something that does line up, but
if
> this was compounding with an internal padding around some drawable inside I
> suspect you'll find it might not work out always.

Good point - this was intended to compound with the existing padding. It's a
pretty small misalignment in a low-priority UI, so I might just change this back
to how it was.

[Ted C, 2015-04-28 18:53:05]

On 2015/04/28 03:15:00, tsergeant wrote:
> I've addressed the comments - there ended up being a reasonable number of
> changes, so I'd appreciate it if you could give it another once over before I
> commit.
> 
> > The reverted files are in dire need of cleanup as well, but i'd rather have
> this
> > be a cleaner revert than trying to mangle them too much.
> 
> Agreed. In theory, we'll be improving/replacing this dialog later this year
> anyway, so I'm not sure there's much value in spending time cleaning the
> existing code.
> 
>
https://codereview.chromium.org/1100283002/diff/40001/chrome/android/java/res...
> File chrome/android/java/res/drawable/connection_info_reset_cert_decisions.xml
> (right):
> 
>
https://codereview.chromium.org/1100283002/diff/40001/chrome/android/java/res...
> chrome/android/java/res/drawable/connection_info_reset_cert_decisions.xml:3:
> <!-- Copyright 2014 The Chromium Authors. All rights reserved.
> On 2015/04/27 19:04:01, Ted C wrote:
> > 2015 now :-)...for any new files might as well update them since it will be
> hard
> > to see this as a true revert anymore.
> 
> Done.
> 
>
https://codereview.chromium.org/1100283002/diff/40001/chrome/android/java/res...
> File chrome/android/java/res/layout/connection_info.xml (right):
> 
>
https://codereview.chromium.org/1100283002/diff/40001/chrome/android/java/res...
> chrome/android/java/res/layout/connection_info.xml:3: Copyright (c) 2013 The
> Chromium Authors. All rights reserved.
> On 2015/04/27 19:04:01, Ted C wrote:
> > no (c) here either
> 
> Done.
> 
>
https://codereview.chromium.org/1100283002/diff/40001/chrome/android/java/src...
> File
> chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
> (right):
> 
>
https://codereview.chromium.org/1100283002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:507:
> mContext.getResources().getColor(R.color.website_settings_popup_text_link));
> On 2015/04/27 19:04:01, Ted C wrote:
> > can the span not be applied prior to appending the text?  just wondering if
> you
> > can build the detailsText as a Spannable string and just plop it on the end.
> 
> Done.
> 
>
https://codereview.chromium.org/1100283002/diff/40001/chrome/browser/android/...
> File chrome/browser/android/chrome_jni_registrar.cc (right):
> 
>
https://codereview.chromium.org/1100283002/diff/40001/chrome/browser/android/...
> chrome/browser/android/chrome_jni_registrar.cc:124: bool
> RegisterCertificateViewer(JNIEnv* env);
> On 2015/04/27 19:04:01, Ted C wrote:
> > I think you should just include the .h for the certificateviewer instead of
> this
> > type of forward declare (as it is really out of place)
> 
> Done. There wasn't a .h file for the Android specific code (which I guess was
> why this was here in the first place), so I added one with just this function.
> 
>
https://codereview.chromium.org/1100283002/diff/60001/chrome/android/java/src...
> File
chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java
> (right):
> 
>
https://codereview.chromium.org/1100283002/diff/60001/chrome/android/java/src...
> chrome/android/java/src/org/chromium/chrome/browser/CertificateViewer.java:87:
> (int)
> mContext.getResources().getDimension(R.dimen.connection_info_padding_spinner);
> On 2015/04/28 01:17:40, Ted C wrote:
> > I suspect you'll find this value to be dependent on OS version and even
> > manufacturer, so it might not always end up doing what you want.
> > 
> > Unless this is overwriting the padding with something that does line up, but
> if
> > this was compounding with an internal padding around some drawable inside I
> > suspect you'll find it might not work out always.
> 
> Good point - this was intended to compound with the existing padding. It's a
> pretty small misalignment in a low-priority UI, so I might just change this
back
> to how it was.

lgtm

[tsergeant, 2015-04-29 00:32:32]

The CQ bit was checked by tsergeant@chromium.org

[commit-bot: I haz the power, 2015-04-29 00:34:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1100283002/80001

[tsergeant, 2015-04-29 02:30:12]

The CQ bit was unchecked by tsergeant@chromium.org

[tsergeant, 2015-04-29 02:30:27]

The CQ bit was checked by tsergeant@chromium.org

[commit-bot: I haz the power, 2015-04-29 02:31:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1100283002/80001

[commit-bot: I haz the power, 2015-04-29 03:27:05]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-04-29 03:28:00]

Patchset 4 (id:??) landed as
https://crrev.com/f21c52aeafa701b18ed505347ee0e7a7d07e5d53
Cr-Commit-Position: refs/heads/master@{#327429}