[Offline pages] Ensuring verbose Offline status does not show next to the security padlock

[Offline pages] Ensuring verbose Offline status does not show next to the security padlock

* Ensuring the verbose status is updated when the padlock animation ends
* Ensuring the URL is properly emphasized when that happens as well

BUG=648129
Committed: https://crrev.com/083092ef6ec00129ebf7d755b323a1cb90cdb659
Cr-Commit-Position: refs/heads/master@{#425140}

[fgorski, 2016-10-12 21:06:40]

Patchset #1 (id:1) has been deleted

[fgorski, 2016-10-12 21:06:50]

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

[commit-bot: I haz the power, 2016-10-12 21:07:28]

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

[fgorski, 2016-10-12 21:07:45]

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

[fgorski, 2016-10-12 21:07:46]

Hey, Ted, this may not look the nicest, but I am not aware of a better place to
put the code. Please review and advise if changes are necessary.

thanks.
Filip

[commit-bot: I haz the power, 2016-10-12 21:42:49]

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

[commit-bot: I haz the power, 2016-10-12 21:42:50]

Dry run: This issue passed the CQ dry run.

[Ted C, 2016-10-12 23:46:22]

https://codereview.chromium.org/2412273002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
(right):

https://codereview.chromium.org/2412273002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:678:
setUrlToPageUrl();
why setUrlToPageUrl?  That doesn't look right to me.

https://codereview.chromium.org/2412273002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:686:
if (animation == mSecurityButtonShowAnimator) {
should we ever be showing the verbose state during this animation?

Wondering if we should trigger updateVerboseStatusVisibility here and on the
end.

Then just have 

&& mNavigationButton.getVisibility() == VISIBLE
&& !mSecurityButton.getVisibility() == INVISIBLE

in the update method.

[fgorski, 2016-10-13 17:58:36]

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

[commit-bot: I haz the power, 2016-10-13 17:59:05]

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

[fgorski, 2016-10-13 18:04:35]

PTAL

https://codereview.chromium.org/2412273002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
(right):

https://codereview.chromium.org/2412273002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:678:
setUrlToPageUrl();
On 2016/10/12 23:46:22, Ted C wrote:
> why setUrlToPageUrl?  That doesn't look right to me.

If we don't we can get padlock shown with a scheme stripped URL in situation,
where the offline page is reloaded to an online version, which is https://

Per the second comment I am moving the whole code outside of if and straight to
the animation end.

Keeping the setUrlToPageUrl() after the updateVerboseStatusVisibility() with a
comment, but if you prefer not to have it, I am fine with removing it.

I can make that call from inside updateVerboseStatusVisibility() if you think
that's better, but that is inconsistent with what the method is meant to do, and
may cause problems in other places.

https://codereview.chromium.org/2412273002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:686:
if (animation == mSecurityButtonShowAnimator) {
On 2016/10/12 23:46:22, Ted C wrote:
> should we ever be showing the verbose state during this animation?
> 
> Wondering if we should trigger updateVerboseStatusVisibility here and on the
> end.
> 
> Then just have 
> 
> && mNavigationButton.getVisibility() == VISIBLE
> && !mSecurityButton.getVisibility() == INVISIBLE
> 
> in the update method.

Done, with exception of the last &&. Please check the change and let me know if
that is what you where thinking about.

[commit-bot: I haz the power, 2016-10-13 18:37:44]

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

[commit-bot: I haz the power, 2016-10-13 18:37:45]

Dry run: This issue passed the CQ dry run.

[Ted C, 2016-10-13 20:03:35]

On 2016/10/13 18:04:35, fgorski wrote:
> PTAL
> 
>
https://codereview.chromium.org/2412273002/diff/20001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
> (right):
> 
>
https://codereview.chromium.org/2412273002/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:678:
> setUrlToPageUrl();
> On 2016/10/12 23:46:22, Ted C wrote:
> > why setUrlToPageUrl?  That doesn't look right to me.
> 
> If we don't we can get padlock shown with a scheme stripped URL in situation,
> where the offline page is reloaded to an online version, which is https://
> 
> Per the second comment I am moving the whole code outside of if and straight
to
> the animation end.
> 
> Keeping the setUrlToPageUrl() after the updateVerboseStatusVisibility() with a
> comment, but if you prefer not to have it, I am fine with removing it.
> 
> I can make that call from inside updateVerboseStatusVisibility() if you think
> that's better, but that is inconsistent with what the method is meant to do,
and
> may cause problems in other places.
> 
>
https://codereview.chromium.org/2412273002/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:686:
> if (animation == mSecurityButtonShowAnimator) {
> On 2016/10/12 23:46:22, Ted C wrote:
> > should we ever be showing the verbose state during this animation?
> > 
> > Wondering if we should trigger updateVerboseStatusVisibility here and on the
> > end.
> > 
> > Then just have 
> > 
> > && mNavigationButton.getVisibility() == VISIBLE
> > && !mSecurityButton.getVisibility() == INVISIBLE
> > 
> > in the update method.
> 
> Done, with exception of the last &&. Please check the change and let me know
if
> that is what you where thinking about.

lgtm...yeah the last && was wrong as I started with == VISIBLE and then switched
it.  submit away and thanks for the explanation for setUrlToPageUrl.  I was
also concerned that could affect users if they had focused the omnibox, but I 
see we already check for that.

[fgorski, 2016-10-13 20:04:18]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2016-10-13 20:04:46]

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-13 20:10:53]

Committed patchset #2 (id:40001)

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

Description was changed from

==========
[Offline pages] Ensuring verbose Offline status does not show next to the
security padlock

* Ensuring the verbose status is updated when the padlock animation ends
* Ensuring the URL is properly emphasized when that happens as well

BUG=648129
==========

to

==========
[Offline pages] Ensuring verbose Offline status does not show next to the
security padlock

* Ensuring the verbose status is updated when the padlock animation ends
* Ensuring the URL is properly emphasized when that happens as well

BUG=648129

Committed: https://crrev.com/083092ef6ec00129ebf7d755b323a1cb90cdb659
Cr-Commit-Position: refs/heads/master@{#425140}
==========

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

Patchset 2 (id:??) landed as
https://crrev.com/083092ef6ec00129ebf7d755b323a1cb90cdb659
Cr-Commit-Position: refs/heads/master@{#425140}

[fgorski, 2016-10-17 23:08:02]

A revert of this CL (patchset #2 id:40001) has been created in
https://codereview.chromium.org/2423153004/ by fgorski@chromium.org.

The reason for reverting is: Reverting per offline discussion with tedchoc@ as
this patch introduced bug 655923.