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

chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.chrome.browser.omnibox;
 
 import static org.chromium.chrome.browser.toolbar.ToolbarPhone.URL_FOCUS_CHANGE_ANIMATION_DURATION_MS;
 
 import android.Manifest;
 import android.animation.Animator;
@@ skipping 653 matching lines @@
         mNavigationButton.setVisibility(VISIBLE);
         mSecurityButton.setVisibility(INVISIBLE);
 
         setLayoutTransition(null);
 
         AnimatorListenerAdapter iconChangeAnimatorListener = new AnimatorListenerAdapter() {
             @Override
             public void onAnimationEnd(Animator animation) {
                 if (animation == mSecurityButtonShowAnimator) {
                     mNavigationButton.setVisibility(INVISIBLE);
+                    // This is done specifically not to show offline page verbose status with the
+                    // icon. It should be properly solved, when omnibox verbose status animation is
+                    // implemented. See: http://crbug.com/648129
+                    updateVerboseStatusVisibility();
+                    setUrlToPageUrl();

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

why setUrlToPageUrl?  That doesn't look right to me.

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

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.

                 } else if (animation == mNavigationIconShowAnimator) {
                     mSecurityButton.setVisibility(INVISIBLE);
                 }
             }
 
             @Override
             public void onAnimationStart(Animator animation) {
                 if (animation == mSecurityButtonShowAnimator) {

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

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 18:04:35]

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

                     mSecurityButton.setVisibility(VISIBLE);
                 } else if (animation == mNavigationIconShowAnimator) {
                     mNavigationButton.setVisibility(VISIBLE);
                 }
             }
         };
 
         mSecurityButtonShowAnimator = new AnimatorSet();
         mSecurityButtonShowAnimator.playTogether(
                 ObjectAnimator.ofFloat(mNavigationButton, ALPHA, 0),
@@ skipping 690 matching lines @@
 
         updateVerboseStatusVisibility();
         updateLocationBarIconContainerVisibility();
     }
 
     /**
      * Update visibility of the verbose status based on the button type and focus state of the
      * omnibox.
      */
     private void updateVerboseStatusVisibility() {
-        boolean verboseStatusVisible =
-                mNavigationButtonType == NavigationButtonType.OFFLINE && !mUrlHasFocus;
+        boolean verboseStatusVisible = mNavigationButtonType == NavigationButtonType.OFFLINE
+                && !mUrlHasFocus && mNavigationButton.getVisibility() == VISIBLE;
 
         int verboseStatusVisibility = verboseStatusVisible ? VISIBLE : GONE;
 
         mVerboseStatusTextView.setTextColor(ApiCompatibilityUtils.getColor(getResources(),
                 mUseDarkColors ? R.color.locationbar_status_color
                         : R.color.locationbar_status_color_light));
         mVerboseStatusTextView.setVisibility(verboseStatusVisibility);
 
         View separator = findViewById(R.id.location_bar_verbose_status_separator);
         separator.setBackgroundColor(ApiCompatibilityUtils.getColor(getResources(), mUseDarkColors
@@ skipping 1063 matching lines @@
     public View getContainerView() {
         return this;
     }
 
     @Override
     public void setTitleToPageTitle() { }
 
     @Override
     public void setShowTitle(boolean showTitle) { }
 }