Omnibox results show correctly for Chrome Home

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

Index: chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java b/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
index 8748a279e8341d0a8d7a543ca134d31c53bd8e3d..2f97601a5b89a1d7dd644f3c8f8ada12bac4e620 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
@@ -87,6 +87,7 @@ import org.chromium.chrome.browser.util.ColorUtils;
 import org.chromium.chrome.browser.util.FeatureUtilities;
 import org.chromium.chrome.browser.util.KeyNavigationUtil;
 import org.chromium.chrome.browser.util.ViewUtils;
+import org.chromium.chrome.browser.widget.BottomSheet;
 import org.chromium.chrome.browser.widget.TintedImageButton;
 import org.chromium.chrome.browser.widget.animation.AnimatorProperties;
 import org.chromium.chrome.browser.widget.animation.CancelAwareAnimatorListener;
@@ -152,6 +153,9 @@ public class LocationBarLayout extends FrameLayout implements OnClickListener,
     protected UrlBar mUrlBar;
     private ActionModeController mActionModeController;
 
+    /** A handle to the bottom sheet for chrome home. */
+    private BottomSheet mBottomSheet;
+
     private AutocompleteController mAutocomplete;
 
     protected ToolbarDataProvider mToolbarDataProvider;
@@ -189,6 +193,7 @@ public class LocationBarLayout extends FrameLayout implements OnClickListener,
     private Runnable mRequestSuggestions;
 
     private ViewGroup mOmniboxResultsContainer;
+    private View mFadingView;
     private ObjectAnimator mFadeInOmniboxBackgroundAnimator;
     private ObjectAnimator mFadeOutOmniboxBackgroundAnimator;
     private Animator mOmniboxBackgroundAnimator;
@@ -1080,7 +1085,7 @@ public class LocationBarLayout extends FrameLayout implements OnClickListener,
         if (mUrlFocusChangeListener != null) mUrlFocusChangeListener.onUrlFocusChange(hasFocus);
 
         updateOmniboxResultsContainer();
-        if (hasFocus) updateOmniboxResultsContainerBackground(true);
+        if (hasFocus) updateFadingBackgroundView(true);
     }
 
     /**
@@ -1211,6 +1216,11 @@ public class LocationBarLayout extends FrameLayout implements OnClickListener,
     }
 
     @Override
+    public void setBottomSheet(BottomSheet sheet) {
+        mBottomSheet = sheet;
+    }
+
+    @Override
     public void setMenuButtonHelper(AppMenuButtonHelper helper) { }
 
     @Override
@@ -1577,7 +1587,7 @@ public class LocationBarLayout extends FrameLayout implements OnClickListener,
 
         // Ensure the results container is initialized and add the suggestion list to it.
         initOmniboxResultsContainer();
-        mOmniboxResultsContainer.addView(mSuggestionList);
+        mOmniboxResultsContainer.addView(mSuggestionList, 0);

[Ted C, 2017/01/25 19:27:02]

why 0?  isn't mSuggestionList now the only child of mOmniboxResultsContainer? 
And if that is the case, do we actually need mOmniboxResultsContainer at all
anymore?  Seems like we could just add mSuggestionList to
mOmniboxResultsContainer's parent directly (at least in the normal Chrome case).
 This seems to add one level of parent indirection that I don' think gains us
anything anymore.

[mdjones, 2017/01/26 00:34:39]

Originally I had the shadow as a child of the container; removed.

[mdjones, 2017/01/26 00:49:32]

Also both cases still have at least one view that they need to be behind (both
involving the toolbar shadow) so I think the container view still needs to stick
around.

 
         // Start with visibility GONE to ensure that show() is called. http://crbug.com/517438
         mSuggestionList.setVisibility(GONE);
@@ -1922,10 +1932,10 @@ public class LocationBarLayout extends FrameLayout implements OnClickListener,
         } else if (v == mMicButton) {
             RecordUserAction.record("MobileOmniboxVoiceSearch");
             startVoiceRecognition();
-        } else if (v == mOmniboxResultsContainer) {
+        } else if (v == mFadingView) {
             // This will only be triggered when no suggestion items are selected in the container.
             setUrlBarFocus(false);
-            updateOmniboxResultsContainerBackground(false);
+            updateFadingBackgroundView(false);
         }
     }
 
@@ -2233,11 +2243,18 @@ public class LocationBarLayout extends FrameLayout implements OnClickListener,
     private void initOmniboxResultsContainer() {
         if (mOmniboxResultsContainer != null) return;
 
-        ViewStub overlayStub =
-                (ViewStub) getRootView().findViewById(R.id.omnibox_results_container_stub);
+        // Use the omnibox results container in the bottom sheet if it exists.
+        int omniboxResultsContainerId = R.id.omnibox_results_container_stub;
+        if (mBottomSheet != null) {

[mdjones, 2017/01/24 01:51:50]

It probably seems weird to go through the trouble of getting a handle to the
bottom sheet for this purpose alone, but it will be used more later. It is also
a reliable way of testing if chrome home is enabled for any particular
ChromeActivity.

+            omniboxResultsContainerId = R.id.bottom_omnibox_results_container_stub;
+        }
+
+        ViewStub overlayStub = (ViewStub) getRootView().findViewById(omniboxResultsContainerId);
         mOmniboxResultsContainer = (ViewGroup) overlayStub.inflate();
-        mOmniboxResultsContainer.setBackgroundColor(CONTENT_OVERLAY_COLOR);
         mOmniboxResultsContainer.setOnClickListener(this);

[Ted C, 2017/01/25 19:27:02]

I ton' think we need this anymore right?

[mdjones, 2017/01/26 00:34:39]

Done.

+
+        mFadingView = getRootView().findViewById(R.id.fading_focus_target);
+        mFadingView.setBackgroundColor(CONTENT_OVERLAY_COLOR);

[Ted C, 2017/01/25 19:27:02]

should we just set this in XML at this point?  looks like we didn't really need
to set it via code before.

[mdjones, 2017/01/26 00:34:39]

Done.

     }
 
     private void updateOmniboxResultsContainer() {
@@ -2245,7 +2262,7 @@ public class LocationBarLayout extends FrameLayout implements OnClickListener,
             initOmniboxResultsContainer();
             updateOmniboxResultsContainerVisibility(true);
         } else if (mOmniboxResultsContainer != null) {
-            updateOmniboxResultsContainerBackground(false);
+            updateFadingBackgroundView(false);
         }
     }
 
@@ -2265,26 +2282,25 @@ public class LocationBarLayout extends FrameLayout implements OnClickListener,
     }
 
     /**
-     * Set the background of the omnibox results container.
+     * Update the fading background view that shows when the omnibox is focused.
      * @param visible Whether the background should be made visible.
      */
-    private void updateOmniboxResultsContainerBackground(boolean visible) {
+    private void updateFadingBackgroundView(boolean visible) {
         if (getToolbarDataProvider() == null) return;
 
         NewTabPage ntp = getToolbarDataProvider().getNewTabPageForCurrentTab();
         boolean locationBarShownInNTP = ntp != null && ntp.isLocationBarShownInNTP();
         if (visible) {
             if (locationBarShownInNTP) {
-                mOmniboxResultsContainer.getBackground().setAlpha(0);
+                mFadingView.getBackground().setAlpha(0);

[Ted C, 2017/01/25 19:27:02]

since this doesn't trigger the fadeIn, it isn't getting set to visible.

Maybe the setVisibility calls should be in this method.

[mdjones, 2017/01/26 00:34:39]

Done.

             } else {
                 fadeInOmniboxResultsContainerBackground();
             }
         } else {
             if (locationBarShownInNTP) {
                 updateOmniboxResultsContainerVisibility(false);

[Ted C, 2017/01/25 19:27:02]

I think we need to figure out how the omnibox results and the fade in view are
treated.  The fact that changing the fading background view controls the omnibox
results seems weird to me.

Maybe we need a unified method that handles visibility changes of both?

[mdjones, 2017/01/26 00:34:39]

I agree, but I don't think this is something to do exactly at this moment. I'd
like to remove ownership of this view from this class so that the bottom sheet
can make use of it in a sane way. Both the sheet and the omnibox need to listen
to click events on that view and both can control its opacity. A better owner
for this view might be ChromeActivity that sends notifications to both the sheet
and omnibox.

-            } else {
-                fadeOutOmniboxResultsContainerBackground();
             }
+            fadeOutOmniboxResultsContainerBackground();

[Ted C, 2017/01/25 19:27:02]

I think this was in an else to avoid flickering on the NTP (fade out will cause
it to be jump to fully opaque and then hide).

[mdjones, 2017/01/26 00:34:39]

Fixed.

         }
     }
 
@@ -2293,12 +2309,21 @@ public class LocationBarLayout extends FrameLayout implements OnClickListener,
      */
     protected void fadeInOmniboxResultsContainerBackground() {
         if (mFadeInOmniboxBackgroundAnimator == null) {
+            mFadingView.setOnClickListener(this);

[Ted C, 2017/01/25 19:27:02]

I think you can just have the onclick listener set during inflation instead of
here.

[mdjones, 2017/01/26 00:34:39]

Moved up to where mFadingView is first acquired.

+
             mFadeInOmniboxBackgroundAnimator = ObjectAnimator.ofInt(
-                    getRootView().findViewById(R.id.omnibox_results_container).getBackground(),
-                    AnimatorProperties.DRAWABLE_ALPHA_PROPERTY, 0, 255);
+                    mFadingView.getBackground(), AnimatorProperties.DRAWABLE_ALPHA_PROPERTY, 0,
+                    255);
             mFadeInOmniboxBackgroundAnimator.setDuration(OMNIBOX_CONTAINER_BACKGROUND_FADE_MS);
             mFadeInOmniboxBackgroundAnimator.setInterpolator(
                     BakedBezierInterpolator.FADE_IN_CURVE);
+
+            mFadeInOmniboxBackgroundAnimator.addListener(new CancelAwareAnimatorListener() {
+                @Override
+                public void onStart(Animator animator) {
+                    mFadingView.setVisibility(View.VISIBLE);
+                }
+            });
         }
         runOmniboxResultsFadeAnimation(mFadeInOmniboxBackgroundAnimator);
     }
@@ -2306,8 +2331,8 @@ public class LocationBarLayout extends FrameLayout implements OnClickListener,
     private void fadeOutOmniboxResultsContainerBackground() {
         if (mFadeOutOmniboxBackgroundAnimator == null) {
             mFadeOutOmniboxBackgroundAnimator = ObjectAnimator.ofInt(
-                    getRootView().findViewById(R.id.omnibox_results_container).getBackground(),
-                    AnimatorProperties.DRAWABLE_ALPHA_PROPERTY, 255, 0);
+                    mFadingView.getBackground(), AnimatorProperties.DRAWABLE_ALPHA_PROPERTY, 255,
+                    0);
             mFadeOutOmniboxBackgroundAnimator.setDuration(OMNIBOX_CONTAINER_BACKGROUND_FADE_MS);
             mFadeOutOmniboxBackgroundAnimator.setInterpolator(
                     BakedBezierInterpolator.FADE_OUT_CURVE);
@@ -2315,6 +2340,7 @@ public class LocationBarLayout extends FrameLayout implements OnClickListener,
                 @Override
                 public void onEnd(Animator animator) {
                     updateOmniboxResultsContainerVisibility(false);
+                    mFadingView.setVisibility(View.GONE);
                 }
             });
         }