I think this should block the release, or at least get approval from UI review to ship with out if it slips.

Who added the initial offline page icon on phones? I'm not familiar with any of the code for that button/text.

fgorski@ - it looks like you added the offline button here: https://chromium.googlesource.com/chromium/src/+/d9950d124acec85375d5b2eb37ff9c86c8d13e63

Does it make sense for you to own this bug?



I took a quick look at the LocationBarLayout.java code -- this is probably what's happening:
#updateLoadingState() calls updateNavigationButton() which should set up the offline lightning bolt. Then it calls #updateSecurityIcon() which calls #getSecurityIconResource(). That method returns 0 on phones since phones are a small device and R.drawable.omnibox_info on tablets. The info icon is probably showing on top of the offline icon.

Seems the right fix is to not call updateSecurityIcon() if we are showing an offline page?

Not sure this is a blocker, since OIB is still correct and there is a verbose "Offline" chip next to the icon, and that icon is always going to be an "info" one, so overall the UX is not terrible, just a bit incorrect.

I did add the offline icon and it was there, therefore marking this bug as a regression.

I'll follow the lead with updateSecurityIcon. Would returning 0 be reasonable for tablets if we are showing an offline page?

That sounds reasonable to me.

I think the CL that caused this regression was "New origin security indicator icons" https://crrev.com/f7c39acbbe817c4717613619e5fd90eddf9bf9cf. We used to always return '0' for ConnectionsecurityLevel.NONE but now we return the info icon on tablets. palmer@ authored that CL but it looks like he's OOO for a few weeks.

Ted, as the reviewer for that CL, do you think it's okay to change LocationBarLayout#getSecurityIconResource() to return 0 instead of the info icon if the security level is NONE and an offline page is showing?

I am working on a patch for this, trying multiple things, but my machine died on Friday and I am not in the office today. I pointed Ted to that change as part of review of another patch.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/666fbc655654767125b5d2e89cac26b378eb758c

commit 666fbc655654767125b5d2e89cac26b378eb758c
Author: fgorski <fgorski@chromium.org>
Date: Tue Oct 25 01:15:50 2016

[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

Review-Url: https://codereview.chromium.org/2438413002
Cr-Commit-Position: refs/heads/master@{#427209}

[modify] https://crrev.com/666fbc655654767125b5d2e89cac26b378eb758c/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
[modify] https://crrev.com/666fbc655654767125b5d2e89cac26b378eb758c/chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
[modify] https://crrev.com/666fbc655654767125b5d2e89cac26b378eb758c/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarModelImpl.java
[modify] https://crrev.com/666fbc655654767125b5d2e89cac26b378eb758c/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappUrlBar.java

Your change meets the bar and is auto-approved for M55 (branch: 2883)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d6275b7f774be01f878380c8b934f13fc277686c

commit d6275b7f774be01f878380c8b934f13fc277686c
Author: Filip Gorski <fgorski@chromium.org>
Date: Tue Oct 25 21:19:52 2016

[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

Review-Url: https://codereview.chromium.org/2438413002
Cr-Commit-Position: refs/heads/master@{#427209}
(cherry picked from commit 666fbc655654767125b5d2e89cac26b378eb758c)

Review URL: https://codereview.chromium.org/2452753003 .

Cr-Commit-Position: refs/branch-heads/2883@{#297}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/d6275b7f774be01f878380c8b934f13fc277686c/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
[modify] https://crrev.com/d6275b7f774be01f878380c8b934f13fc277686c/chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
[modify] https://crrev.com/d6275b7f774be01f878380c8b934f13fc277686c/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarModelImpl.java
[modify] https://crrev.com/d6275b7f774be01f878380c8b934f13fc277686c/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappUrlBar.java

Verified in  M55-55.0.2883.28 build

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d6275b7f774be01f878380c8b934f13fc277686c

commit d6275b7f774be01f878380c8b934f13fc277686c
Author: Filip Gorski <fgorski@chromium.org>
Date: Tue Oct 25 21:19:52 2016

[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

Review-Url: https://codereview.chromium.org/2438413002
Cr-Commit-Position: refs/heads/master@{#427209}
(cherry picked from commit 666fbc655654767125b5d2e89cac26b378eb758c)

Review URL: https://codereview.chromium.org/2452753003 .

Cr-Commit-Position: refs/branch-heads/2883@{#297}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/d6275b7f774be01f878380c8b934f13fc277686c/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
[modify] https://crrev.com/d6275b7f774be01f878380c8b934f13fc277686c/chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
[modify] https://crrev.com/d6275b7f774be01f878380c8b934f13fc277686c/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarModelImpl.java
[modify] https://crrev.com/d6275b7f774be01f878380c8b934f13fc277686c/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappUrlBar.java

[Automated comment] removing mislabelled merge-merged-2840

[Automated comment] removing mislabelled merge-merged-2840