[Offline pages] Reinstating the offline icon on tablet and fixing verbose state/URL emphasis

[Offline pages] Reinstating the offline icon on tablet and fixing verbose state/URL emphasis

This patch addresses the following problems:
* Offline icon is not shown in omnibox on tablets
* Offline verbose status is sometimes shown with green padlock
* URL emphasis lags behind security icon update.

This fix does the following:
* Moves the offline icon from the navigation button to security button
* Ensures that navigation button is never shown on phones
* Calculates when to show: navigation/security/no button, to properly
  show or hide icon container.
* Moves verbose status visibility control to security button related
  code (as that is where we show offline stuff and would show verbose
  security states)

BUG=656088, 648129
R=tedchoc@chromium.org

Committed: https://crrev.com/666fbc655654767125b5d2e89cac26b378eb758c
Cr-Commit-Position: refs/heads/master@{#427209}

[fgorski, 2016-10-21 21:00:54]

PTAL.

[fgorski, 2016-10-21 21:02:58]

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

[commit-bot: I haz the power, 2016-10-21 21:03:47]

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-21 22:01:31]

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

[Ted C, 2016-10-21 22:01:31]

overall this seems very reasonable to me.

Adding yusufo@ in case I'm out next week and can't review the rest of this.

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

https://codereview.chromium.org/2438413002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:405:
protected enum LocationBarButton {
despite NavigationButtonType existing above, we actually shouldn't be adding any
new enums in java (in fact we should be getting rid of all existing ones...not
in the patch of course).

Look at using @IntDef to define grouped ints that are similar to enums.

Also, I would add Type to the name as I thought this was an Android button type
when I saw the variable defined above.

https://codereview.chromium.org/2438413002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:635:
mNavigationButtonType = DeviceFormFactor.isTablet(getContext())
this change seems unnecessary imo

https://codereview.chromium.org/2438413002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:882:
if (!mUrlHasFocus && (getSecurityLevel() != ConnectionSecurityLevel.NONE ||
isOffline)) {
on tablets, we actually want to show the (i) which is
ConnectionSecurityLevel.NONE

I think it needs to be something like this:

if (mUrlHasFocus) {
    return isTablet ? LocationBarButton.NAVIGATION_ICON :
LocationBarButton.NONE;
}
return getSecurityIconResource(getSecurityLevel(), !isTablet, isOffline) != 0 ?
        LocationBarButton.SECURITY_ICON : LocationBarButton.NONE;

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

https://codereview.chromium.org/2438413002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:512:
int id = LocationBarLayout.getSecurityIconResource(securityLevel, isSmallDevice,
false);
can we now simplify this with the block above?

[fgorski, 2016-10-21 23:06:37]

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

[commit-bot: I haz the power, 2016-10-21 23:07:09]

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-21 23:08:19]

PTAL

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

https://codereview.chromium.org/2438413002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:405:
protected enum LocationBarButton {
On 2016/10/21 22:01:31, Ted C wrote:
> despite NavigationButtonType existing above, we actually shouldn't be adding
any
> new enums in java (in fact we should be getting rid of all existing ones...not
> in the patch of course).
> 
> Look at using @IntDef to define grouped ints that are similar to enums.
> 
> Also, I would add Type to the name as I thought this was an Android button
type
> when I saw the variable defined above.

Done.

https://codereview.chromium.org/2438413002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:635:
mNavigationButtonType = DeviceFormFactor.isTablet(getContext())
On 2016/10/21 22:01:31, Ted C wrote:
> this change seems unnecessary imo

Done. You are right. I actually changed the code and changed it back in instead
of kicking the change out.

https://codereview.chromium.org/2438413002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:882:
if (!mUrlHasFocus && (getSecurityLevel() != ConnectionSecurityLevel.NONE ||
isOffline)) {
On 2016/10/21 22:01:31, Ted C wrote:
> on tablets, we actually want to show the (i) which is
> ConnectionSecurityLevel.NONE
> 
> I think it needs to be something like this:
> 
> if (mUrlHasFocus) {
>     return isTablet ? LocationBarButton.NAVIGATION_ICON :
> LocationBarButton.NONE;
> }
> return getSecurityIconResource(getSecurityLevel(), !isTablet, isOffline) != 0
?
>         LocationBarButton.SECURITY_ICON : LocationBarButton.NONE;

Done. Thanks that is a good catch. I was trying to cut down on the ifs here, and
it backfired.

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

https://codereview.chromium.org/2438413002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:512:
int id = LocationBarLayout.getSecurityIconResource(securityLevel, isSmallDevice,
false);
On 2016/10/21 22:01:31, Ted C wrote:
> can we now simplify this with the block above?

Done. I am attempting it as similar to the LocationBarLayout as possible in this
section.

I had to check the opaqueness to tint the icon properly thou. Let me know if
that makes sense.

[commit-bot: I haz the power, 2016-10-22 03:47:48]

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

[commit-bot: I haz the power, 2016-10-22 03:47:49]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Ted C, 2016-10-24 17:19:45]

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

https://codereview.chromium.org/2438413002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:1361:
mIsEmphasizingHttpsScheme = shouldEmphasizeHttpsScheme();
why this change?  we calculate shouldEmphasizeHttpsScheme above and were using
it previously.  Does something that happens after the early return cause this to
be changed from that value?  If so, we should document that as it is not
immediately clear.

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

https://codereview.chromium.org/2438413002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:515:
getResources(), ColorUtils.shouldUseOpaqueTextboxBackground(
why the change for the last param?  we never show the omnibox background in
custom tabs, so it is always opaque.  I think it should remain false in this
file.

[fgorski, 2016-10-24 22:34:08]

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

[fgorski, 2016-10-24 22:34:14]

PTAL.

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

https://codereview.chromium.org/2438413002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:1361:
mIsEmphasizingHttpsScheme = shouldEmphasizeHttpsScheme();
On 2016/10/24 17:19:45, Ted C (OOO 10.25 - 30) wrote:
> why this change?  we calculate shouldEmphasizeHttpsScheme above and were using
> it previously.  Does something that happens after the early return cause this
to
> be changed from that value?  If so, we should document that as it is not
> immediately clear.

No I think I was experimenting with removing the calculation above and then only
do it here. This was left in unintentionally.

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

https://codereview.chromium.org/2438413002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:515:
getResources(), ColorUtils.shouldUseOpaqueTextboxBackground(
On 2016/10/24 17:19:45, Ted C (OOO 10.25 - 30) wrote:
> why the change for the last param?  we never show the omnibox background in
> custom tabs, so it is always opaque.  I think it should remain false in this
> file.

For some reason when testing it on Friday I saw improperly dimmed icon.
Having retried it today setting it to false was correct.

Original code for handling the offline pin icon was incorrect, and that is
probably the version I tested with and started "fixing" the problem based on.

[commit-bot: I haz the power, 2016-10-24 22:35:14]

Dry run: 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-24 22:53:47]

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

[commit-bot: I haz the power, 2016-10-24 22:53:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ted C, 2016-10-24 22:57:21]

lgtm w/ null check

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

https://codereview.chromium.org/2438413002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:922:
if (viewToBeShown.getVisibility() == VISIBLE && viewToBeShown.getAlpha() == 1) {
in the NONE case, this will cause an NPE since viewToBeShown is null.  We should
early exit in that case as well.

[fgorski, 2016-10-24 23:09:54]

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

https://codereview.chromium.org/2438413002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:922:
if (viewToBeShown.getVisibility() == VISIBLE && viewToBeShown.getAlpha() == 1) {
On 2016/10/24 22:57:21, Ted C (OOO 10.25 - 30) wrote:
> in the NONE case, this will cause an NPE since viewToBeShown is null.  We
should
> early exit in that case as well.

Done. Based on offline discussion the return statement above handles that.

[fgorski, 2016-10-24 23:10:02]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2016-10-24 23:10:58]

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-25 01:16:24]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-25 01:18:59]

Description was changed from

==========
[Offline pages] Reinstating the offline icon on tablet and fixing verbose
state/URL emphasis

This patch addresses the following problems:
* Offline icon is not shown in omnibox on tablets
* Offline verbose status is sometimes shown with green padlock
* URL emphasis lags behind security icon update.

This fix does the following:
* Moves the offline icon from the navigation button to security button
* Ensures that navigation button is never shown on phones
* Calculates when to show: navigation/security/no button, to properly
  show or hide icon container.
* Moves verbose status visibility control to security button related
  code (as that is where we show offline stuff and would show verbose
  security states)

BUG=656088,648129
R=tedchoc@chromium.org
==========

to

==========
[Offline pages] Reinstating the offline icon on tablet and fixing verbose
state/URL emphasis

This patch addresses the following problems:
* Offline icon is not shown in omnibox on tablets
* Offline verbose status is sometimes shown with green padlock
* URL emphasis lags behind security icon update.

This fix does the following:
* Moves the offline icon from the navigation button to security button
* Ensures that navigation button is never shown on phones
* Calculates when to show: navigation/security/no button, to properly
  show or hide icon container.
* Moves verbose status visibility control to security button related
  code (as that is where we show offline stuff and would show verbose
  security states)

BUG=656088,648129
R=tedchoc@chromium.org

Committed: https://crrev.com/666fbc655654767125b5d2e89cac26b378eb758c
Cr-Commit-Position: refs/heads/master@{#427209}
==========

[commit-bot: I haz the power, 2016-10-25 01:19:00]

Patchset 3 (id:??) landed as
https://crrev.com/666fbc655654767125b5d2e89cac26b378eb758c
Cr-Commit-Position: refs/heads/master@{#427209}