Omnibox results show correctly for Chrome Home

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 69 matching lines @@
 import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.chrome.browser.toolbar.ActionModeController;
 import org.chromium.chrome.browser.toolbar.ActionModeController.ActionBarDelegate;
 import org.chromium.chrome.browser.toolbar.ToolbarActionModeCallback;
 import org.chromium.chrome.browser.toolbar.ToolbarDataProvider;
 import org.chromium.chrome.browser.toolbar.ToolbarPhone;
 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;
 import org.chromium.components.security_state.ConnectionSecurityLevel;
 import org.chromium.content_public.browser.LoadUrlParams;
 import org.chromium.content_public.browser.WebContents;
 import org.chromium.ui.UiUtils;
 import org.chromium.ui.base.DeviceFormFactor;
 import org.chromium.ui.base.PageTransition;
 import org.chromium.ui.base.WindowAndroid;
@@ skipping 45 matching lines @@
             CollectionUtil.newHashSet("file", "javascript", "data");
 
     protected ImageView mNavigationButton;
     protected TintedImageButton mSecurityButton;
     protected TextView mVerboseStatusTextView;
     protected TintedImageButton mDeleteButton;
     protected TintedImageButton mMicButton;
     protected UrlBar mUrlBar;
     private ActionModeController mActionModeController;
 
+    /** A handle to the bottom sheet for chrome home. */
+    private BottomSheet mBottomSheet;
+
     private AutocompleteController mAutocomplete;
 
     protected ToolbarDataProvider mToolbarDataProvider;
     private UrlFocusChangeListener mUrlFocusChangeListener;
 
     protected boolean mNativeInitialized;
 
     private final List<Runnable> mDeferredNativeRunnables = new ArrayList<Runnable>();
 
     // The type of the navigation button currently showing.
@@ skipping 17 matching lines @@
     private boolean mIgnoreOmniboxItemSelection = true;
 
     private String mOriginalUrl = "";
 
     private WindowAndroid mWindowAndroid;
     private WindowDelegate mWindowDelegate;
 
     private Runnable mRequestSuggestions;
 
     private ViewGroup mOmniboxResultsContainer;
+    private View mFadingView;
     private ObjectAnimator mFadeInOmniboxBackgroundAnimator;
     private ObjectAnimator mFadeOutOmniboxBackgroundAnimator;
     private Animator mOmniboxBackgroundAnimator;
 
     private boolean mSuggestionsShown;
     private boolean mUrlHasFocus;
     protected boolean mUrlFocusChangeInProgress;
     private boolean mUrlFocusedFromFakebox;
     private boolean mUrlFocusedWithoutAnimations;
 
@@ skipping 871 matching lines @@
 
     /**
      * Handle and run any necessary animations that are triggered off focusing the UrlBar.
      * @param hasFocus Whether focus was gained.
      */
     protected void handleUrlFocusAnimation(boolean hasFocus) {
         if (hasFocus) mUrlFocusedWithoutAnimations = false;
         if (mUrlFocusChangeListener != null) mUrlFocusChangeListener.onUrlFocusChange(hasFocus);
 
         updateOmniboxResultsContainer();
-        if (hasFocus) updateOmniboxResultsContainerBackground(true);
+        if (hasFocus) updateFadingBackgroundView(true);
     }
 
     /**
      * Make a zero suggest request if native is loaded, the URL bar has focus, and the
      * current tab is not incognito.
      */
     private void startZeroSuggest() {
         // Reset "edited" state in the omnibox if zero suggest is triggered -- new edits
         // now count as a new session.
         mHasStartedNewOmniboxEditSession = false;
@@ skipping 110 matching lines @@
 
         mUrlBar.setOnFocusChangeListener(new View.OnFocusChangeListener() {
             @Override
             public void onFocusChange(View v, final boolean hasFocus) {
                 onUrlFocusChange(hasFocus);
             }
         });
     }
 
     @Override
+    public void setBottomSheet(BottomSheet sheet) {
+        mBottomSheet = sheet;
+    }
+
+    @Override
     public void setMenuButtonHelper(AppMenuButtonHelper helper) { }
 
     @Override
     public View getMenuAnchor() {
         return null;
     }
 
     /**
      * Sets the URL focus change listner that will be notified when the URL gains or loses focus.
      * @param listener The listener to be registered.
@@ skipping 346 matching lines @@
                 });
             }
         };
         getRootView().findViewById(R.id.control_container)
                 .addOnLayoutChangeListener(suggestionListResizer);
 
         mSuggestionList = new OmniboxSuggestionsList(getContext());
 
         // 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);
         mSuggestionList.setAdapter(mSuggestionListAdapter);
         mSuggestionList.setClipToPadding(false);
         mSuggestionListAdapter.setSuggestionDelegate(new OmniboxSuggestionDelegate() {
             @Override
             public void onSelection(OmniboxSuggestion suggestion, int position) {
                 mSuggestionSelectionInProgress = true;
                 String suggestionMatchUrl = updateSuggestionUrlIfNeeded(
@@ skipping 324 matching lines @@
             if (currentTab != null && currentTab.getWebContents() != null) {
                 Activity activity = currentTab.getWindowAndroid().getActivity().get();
                 if (activity != null) {
                     WebsiteSettingsPopup.show(
                             activity, currentTab, null, WebsiteSettingsPopup.OPENED_FROM_TOOLBAR);
                 }
             }
         } 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);
         }
     }
 
     @Override
     public void onSuggestionsReceived(List<OmniboxSuggestion> newSuggestions,
             String inlineAutocompleteText) {
         // This is a callback from a listener that is set up by onNativeLibraryReady,
         // so can only be called once the native side is set up.
         assert mNativeInitialized : "Suggestions received before native side intialialized";
 
@@ skipping 287 matching lines @@
      */
     @Override
     public Tab getCurrentTab() {
         if (mToolbarDataProvider == null) return null;
         return mToolbarDataProvider.getTab();
     }
 
     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() {
         if (mSuggestionsShown || mUrlHasFocus) {
             initOmniboxResultsContainer();
             updateOmniboxResultsContainerVisibility(true);
         } else if (mOmniboxResultsContainer != null) {
-            updateOmniboxResultsContainerBackground(false);
+            updateFadingBackgroundView(false);
         }
     }
 
     private void updateOmniboxResultsContainerVisibility(boolean visible) {
         boolean currentlyVisible = mOmniboxResultsContainer.getVisibility() == VISIBLE;
         if (currentlyVisible == visible) return;
 
         ChromeActivity activity = (ChromeActivity) mWindowAndroid.getActivity().get();
 
         if (visible) {
             mOmniboxResultsContainer.setVisibility(VISIBLE);
             if (activity != null) activity.addViewObscuringAllTabs(mOmniboxResultsContainer);
         } else {
             mOmniboxResultsContainer.setVisibility(INVISIBLE);
             if (activity != null) activity.removeViewObscuringAllTabs(mOmniboxResultsContainer);
         }
     }
 
     /**
-     * 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.

         }
     }
 
     /**
      * Trigger a fade in of the omnibox results background.
      */
     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);
     }
 
     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);
             mFadeOutOmniboxBackgroundAnimator.addListener(new CancelAwareAnimatorListener() {
                 @Override
                 public void onEnd(Animator animator) {
                     updateOmniboxResultsContainerVisibility(false);
+                    mFadingView.setVisibility(View.GONE);
                 }
             });
         }
         runOmniboxResultsFadeAnimation(mFadeOutOmniboxBackgroundAnimator);
     }
 
     private void runOmniboxResultsFadeAnimation(Animator fadeAnimation) {
         if (mOmniboxBackgroundAnimator == fadeAnimation
                 && mOmniboxBackgroundAnimator.isRunning()) {
             return;
@@ skipping 175 matching lines @@
     public View getContainerView() {
         return this;
     }
 
     @Override
     public void setTitleToPageTitle() { }
 
     @Override
     public void setShowTitle(boolean showTitle) { }
 }