Custom Tabs: Preserve the offline bolt icon when showing an offline page.

Custom Tabs: Preserve the offline bolt icon when showing an offline page.

The security level gets updated after the custom tab is navigated to a
new page, and as a result the security icon is removed, because offline
pages are shown with a connection security level of NONE.  However, the
bolt icon is another state of the connection security level, so we need
to update the bolt if we don't think that there is any security.

BUG=653934
Committed: https://crrev.com/b0d23feb21be94dc0fce54f982e15b8262d49736
Cr-Commit-Position: refs/heads/master@{#425697}

[dewittj, 2016-10-13 16:44:12]

The CQ bit was checked by dewittj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 16:44:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2016-10-13 16:46:58]

dewittj@chromium.org changed reviewers:
+ fgorski@chromium.org, tedchoc@chromium.org

[dewittj, 2016-10-13 16:46:58]

PTAL, thanks.

[fgorski, 2016-10-13 17:04:28]

lgtm

[commit-bot: I haz the power, 2016-10-13 17:14:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-13 17:14:55]

Dry run: This issue passed the CQ dry run.

[Ted C, 2016-10-13 18:33:26]

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

[Ted C, 2016-10-13 18:33:27]

+yusufo since this is CCT related

https://codereview.chromium.org/2420663002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
(right):

https://codereview.chromium.org/2420663002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:501:
showOfflineBoltIfNecessary();
What happens if you navigate to an HTTPS page and then to an HTTP page (w/o any
offline pages involved)?

Since mShowsOfflinePage is already false in showOfflineBoltIfNecessary, it seems
like we wouldn't hide the lock icon.

[dewittj, 2016-10-13 20:33:56]

The CQ bit was checked by dewittj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 20:34:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2016-10-13 20:57:56]

Good catch.  I'm not sure a page would be downgraded like you suggest normally
without other signals saying it's a new page, but it's worth defending against.

I changed showOfflineBoltIfNecessary to report whether it made any changes, but
it feels ugly.  Since I am not super familiar with all the possible state
changes it seemed the safest way to make this change.  Suggestions accepted on
how to refactor this reliably.

[Ted C, 2016-10-13 21:06:25]

On 2016/10/13 20:57:56, dewittj wrote:
> Good catch.  I'm not sure a page would be downgraded like you suggest normally
> without other signals saying it's a new page, but it's worth defending
against.
> 
> I changed showOfflineBoltIfNecessary to report whether it made any changes,
but
> it feels ugly.  Since I am not super familiar with all the possible state
> changes it seemed the safest way to make this change.  Suggestions accepted on
> how to refactor this reliably.

lgtm

[Yusuf, 2016-10-13 21:18:52]

lgtm

https://codereview.chromium.org/2420663002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
(right):

https://codereview.chromium.org/2420663002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:501:
if (!showOfflineBoltIfNecessary()) {
would probably fit one line?

[dewittj, 2016-10-14 20:31:57]

Patchset #4 (id:60001) has been deleted

[dewittj, 2016-10-14 20:32:45]

sorry for all the churn, but there was a bug in the last patchset, so I
consolidated all the icon code.  I think it's much cleaner now - and less prone
to bugs.  What do you think?

[Ted C, 2016-10-14 20:37:59]

lgtm

https://codereview.chromium.org/2420663002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
(right):

https://codereview.chromium.org/2420663002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:514:
mSecurityButton.setImageDrawable(null);
To make this more correct, I would do this:

showSecurityButton = false;

https://codereview.chromium.org/2420663002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:522:
mAnimDelegate.showSecurityButton();
you don't need this line anymore right?

[dewittj, 2016-10-14 20:49:31]

https://codereview.chromium.org/2420663002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
(right):

https://codereview.chromium.org/2420663002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:514:
mSecurityButton.setImageDrawable(null);
On 2016/10/14 20:37:59, Ted C wrote:
> To make this more correct, I would do this:
> 
> showSecurityButton = false;

That will have slightly different behavior than before this patch, since it
would explicitly have called |showSecurityButton|, and after that change it will
call |hideSecurityButton|.  I can do that, but am not sure of the UI
implications.  Is that what you meant?

https://codereview.chromium.org/2420663002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:522:
mAnimDelegate.showSecurityButton();
On 2016/10/14 20:37:59, Ted C wrote:
> you don't need this line anymore right?

Done.

[dewittj, 2016-10-14 21:04:49]

The CQ bit was checked by dewittj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 21:05:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ted C, 2016-10-14 21:08:03]

https://codereview.chromium.org/2420663002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
(right):

https://codereview.chromium.org/2420663002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:514:
mSecurityButton.setImageDrawable(null);
On 2016/10/14 20:49:30, dewittj wrote:
> On 2016/10/14 20:37:59, Ted C wrote:
> > To make this more correct, I would do this:
> > 
> > showSecurityButton = false;
> 
> That will have slightly different behavior than before this patch, since it
> would explicitly have called |showSecurityButton|, and after that change it
will
> call |hideSecurityButton|.  I can do that, but am not sure of the UI
> implications.  Is that what you meant?

Yeah, I'm suggesting a change in behavior.  It definitely seems wrong as it was.

[commit-bot: I haz the power, 2016-10-14 21:46:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-14 21:46:29]

Dry run: This issue passed the CQ dry run.

[dewittj, 2016-10-14 21:58:13]

The CQ bit was checked by dewittj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 21:58:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2016-10-14 21:59:16]

On 2016/10/14 21:08:03, Ted C wrote:
>
https://codereview.chromium.org/2420663002/diff/80001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
> (right):
> 
>
https://codereview.chromium.org/2420663002/diff/80001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:514:
> mSecurityButton.setImageDrawable(null);
> On 2016/10/14 20:49:30, dewittj wrote:
> > On 2016/10/14 20:37:59, Ted C wrote:
> > > To make this more correct, I would do this:
> > > 
> > > showSecurityButton = false;
> > 
> > That will have slightly different behavior than before this patch, since it
> > would explicitly have called |showSecurityButton|, and after that change it
> will
> > call |hideSecurityButton|.  I can do that, but am not sure of the UI
> > implications.  Is that what you meant?
> 
> Yeah, I'm suggesting a change in behavior.  It definitely seems wrong as it
was.

Done.  I removed setting the drawable to |null| since it seemed better to
gradually animate it out than to set drawable to null instantly and then animate
it out.

[commit-bot: I haz the power, 2016-10-14 22:25:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-14 22:25:38]

Dry run: This issue passed the CQ dry run.

[dewittj, 2016-10-17 15:59:39]

The CQ bit was checked by dewittj@chromium.org

[dewittj, 2016-10-17 15:59:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
fgorski@chromium.org, yusufo@chromium.org, tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2420663002/#ps120001
(title: "Hide security button if no drawable is available.")

[commit-bot: I haz the power, 2016-10-17 15:59:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2016-10-17 16:15:17]

The CQ bit was unchecked by dewittj@chromium.org

[dewittj, 2016-10-17 16:20:26]

The CQ bit was checked by dewittj@chromium.org

[commit-bot: I haz the power, 2016-10-17 16:20:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 16:35:05]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-10-17 16:38:51]

Description was changed from

==========
Custom Tabs: Preserve the offline bolt icon when showing an offline page.

The security level gets updated after the custom tab is navigated to a
new page, and as a result the security icon is removed, because offline
pages are shown with a connection security level of NONE.  However, the
bolt icon is another state of the connection security level, so we need
to update the bolt if we don't think that there is any security.

BUG=653934
==========

to

==========
Custom Tabs: Preserve the offline bolt icon when showing an offline page.

The security level gets updated after the custom tab is navigated to a
new page, and as a result the security icon is removed, because offline
pages are shown with a connection security level of NONE.  However, the
bolt icon is another state of the connection security level, so we need
to update the bolt if we don't think that there is any security.

BUG=653934

Committed: https://crrev.com/b0d23feb21be94dc0fce54f982e15b8262d49736
Cr-Commit-Position: refs/heads/master@{#425697}
==========

[commit-bot: I haz the power, 2016-10-17 16:38:52]

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