Omnibox results show correctly for Chrome Home

Omnibox results show correctly for Chrome Home

This change allows the omnibox's suggestions to show inside of the
bottom sheet for Chrome Home. To do this, a second results container
was added to the bottom sheet (the first being a child of the root
coordinator layout). The results container that is acutally used will
depend on whether or not Chrome Home is enabled.

The results container is also no longer responsible for fading out
the web contents when the omnibox is focused. This view is now an
independent child of the root coordinator layout so that it can
function with and without Chrome Home being enabled.

BUG=671361, 685661
Review-Url: https://codereview.chromium.org/2652883002
Cr-Commit-Position: refs/heads/master@{#447249}
Committed: https://chromium.googlesource.com/chromium/src/+/2f0183c3f049882c0de912cf468d01e8f79790ce

[mdjones, 2017-01-24 01:46:15]

mdjones@chromium.org changed reviewers:
+ dfalcantara@chromium.org, tedchoc@chromium.org

[mdjones, 2017-01-24 01:46:16]

ptal

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

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

https://codereview.chromium.org/2652883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2248:
if (mBottomSheet != null) {
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.

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

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

https://codereview.chromium.org/2652883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBar.java:84:
* Set the bottom sheet for Chrome Home. This will be null if Chrome Home is
disabled.
Would this even need to be called if Chrome Home isn't enabled?  wondering why
we need to pass null here

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

https://codereview.chromium.org/2652883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:1590:
mOmniboxResultsContainer.addView(mSuggestionList, 0);
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.

https://codereview.chromium.org/2652883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2254:
mOmniboxResultsContainer.setOnClickListener(this);
I ton' think we need this anymore right?

https://codereview.chromium.org/2652883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2257:
mFadingView.setBackgroundColor(CONTENT_OVERLAY_COLOR);
should we just set this in XML at this point?  looks like we didn't really need
to set it via code before.

https://codereview.chromium.org/2652883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2295:
mFadingView.getBackground().setAlpha(0);
since this doesn't trigger the fadeIn, it isn't getting set to visible.

Maybe the setVisibility calls should be in this method.

https://codereview.chromium.org/2652883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2301:
updateOmniboxResultsContainerVisibility(false);
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?

https://codereview.chromium.org/2652883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2303:
fadeOutOmniboxResultsContainerBackground();
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).

https://codereview.chromium.org/2652883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2312:
mFadingView.setOnClickListener(this);
I think you can just have the onclick listener set during inflation instead of
here.

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

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

https://codereview.chromium.org/2652883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBar.java:84:
* Set the bottom sheet for Chrome Home. This will be null if Chrome Home is
disabled.
On 2017/01/25 19:27:02, Ted C wrote:
> Would this even need to be called if Chrome Home isn't enabled?  wondering why
> we need to pass null here

It won't; updated doc.

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

https://codereview.chromium.org/2652883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:1590:
mOmniboxResultsContainer.addView(mSuggestionList, 0);
On 2017/01/25 19:27:02, Ted C wrote:
> 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.

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

https://codereview.chromium.org/2652883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2254:
mOmniboxResultsContainer.setOnClickListener(this);
On 2017/01/25 19:27:02, Ted C wrote:
> I ton' think we need this anymore right?

Done.

https://codereview.chromium.org/2652883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2257:
mFadingView.setBackgroundColor(CONTENT_OVERLAY_COLOR);
On 2017/01/25 19:27:02, Ted C wrote:
> should we just set this in XML at this point?  looks like we didn't really
need
> to set it via code before.

Done.

https://codereview.chromium.org/2652883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2295:
mFadingView.getBackground().setAlpha(0);
On 2017/01/25 19:27:02, Ted C wrote:
> since this doesn't trigger the fadeIn, it isn't getting set to visible.
> 
> Maybe the setVisibility calls should be in this method.

Done.

https://codereview.chromium.org/2652883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2301:
updateOmniboxResultsContainerVisibility(false);
On 2017/01/25 19:27:02, Ted C wrote:
> 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?

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.

https://codereview.chromium.org/2652883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2303:
fadeOutOmniboxResultsContainerBackground();
On 2017/01/25 19:27:02, Ted C wrote:
> 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).

Fixed.

https://codereview.chromium.org/2652883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2312:
mFadingView.setOnClickListener(this);
On 2017/01/25 19:27:02, Ted C wrote:
> I think you can just have the onclick listener set during inflation instead of
> here.

Moved up to where mFadingView is first acquired.

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

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

https://codereview.chromium.org/2652883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:1590:
mOmniboxResultsContainer.addView(mSuggestionList, 0);
On 2017/01/26 00:34:39, mdjones wrote:
> On 2017/01/25 19:27:02, Ted C wrote:
> > 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.
> 
> Originally I had the shadow as a child of the container; removed.

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.

[mdjones, 2017-01-27 15:58:56]

Description was changed from

==========
Omnibox results show correctly for Chrome Home

This change allows the omnibox's suggestions to show inside of the
bottom sheet for Chrome Home. To do this, a second results container
was added to the bottom sheet (the first being a child of the root
coordinator layout). The results container that is acutally used will
depend on whether or not Chrome Home is enabled.

The results container is also no longer responsible for fading out
the web contents when the omnibox is focused. This view is now an
independent child of the root coordinator layout so that it can
function with and without Chrome Home being enabled.

BUG=671361
==========

to

==========
Omnibox results show correctly for Chrome Home

This change allows the omnibox's suggestions to show inside of the
bottom sheet for Chrome Home. To do this, a second results container
was added to the bottom sheet (the first being a child of the root
coordinator layout). The results container that is acutally used will
depend on whether or not Chrome Home is enabled.

The results container is also no longer responsible for fading out
the web contents when the omnibox is focused. This view is now an
independent child of the root coordinator layout so that it can
function with and without Chrome Home being enabled.

BUG=671361, 685661
==========

[Ted C, 2017-01-27 19:54:41]

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

https://codereview.chromium.org/2652883002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2197:
private void initOmniboxResultsContainer() {
I would split this into two methods

initOmniboxResultsContainer

initFadeOverlayView

Or maybe we can just drop Container from the method as it is now two (or maybe
initOmniboxResultsUi)

Then each of my other comments might be resolved by unifying on a naming that
doesn't imply a single container.

But yeah, in general, let's try to separate the logic of the two views as best
we can to make the transition to it not living in this class easier.

https://codereview.chromium.org/2652883002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2230:
if (activity != null)
activity.addViewObscuringAllTabs(mOmniboxResultsContainer);
the mFadingView is actually the one that should be ObscuringAllTabs

https://codereview.chromium.org/2652883002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2232:
mFadingView.setVisibility(View.GONE);
I was wrong, and I feel wrong, but I now agree with your original change to have
this live in the animator code below.

https://codereview.chromium.org/2652883002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2250:
mFadingView.setAlpha(0);
Still wrapping my head around the two views thing, but I actually think we can
just set it to GONE in this state.  We only needed it to be visible before
because it also held the suggestions.

https://codereview.chromium.org/2652883002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2256:
updateOmniboxResultsContainerVisibility(false);
this should be a no-op if we set it to gone.  maybe just set it to GONE here
instead.

And based on my comment below, maybe this calls onFadeOutOverlayGone here to
allow the same code path?

https://codereview.chromium.org/2652883002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2268:
mFadeInOmniboxBackgroundAnimator = ObjectAnimator.ofFloat(mFadingView, ALPHA, 0,
1);
oooh...now that we're not animating a drawable.  let's not specify the start
value (0 in this case).  Then it will use the current value and it will never
jump to unneeded states.

https://codereview.chromium.org/2652883002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2285:
updateOmniboxResultsContainerVisibility(false);
I think if we want to split the views, I'd like to have very clear splits
between them (i.e. this animator is for the fading background, it shouldn't talk
directly to the results container).

Maybe we add a method, onFadeOutOverlayGone that is in LocationBarLayout which
can call this.

If we want to move where it isn't owned by LocationBarLayout, I think we really
need to separate the two now.

https://codereview.chromium.org/2652883002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2292:
private void runOmniboxResultsFadeAnimation(Animator fadeAnimation) {
let's rename this to runFadeOverlayAnimation

and probably mOmniboxBackgroundAnimator to mFadeOverlayAnimator.

[mdjones, 2017-01-30 21:57:16]

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

https://codereview.chromium.org/2652883002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2197:
private void initOmniboxResultsContainer() {
On 2017/01/27 19:54:41, Ted C wrote:
> I would split this into two methods
> 
> initOmniboxResultsContainer
> 
> initFadeOverlayView
> 
> Or maybe we can just drop Container from the method as it is now two (or maybe
> initOmniboxResultsUi)
> 
> Then each of my other comments might be resolved by unifying on a naming that
> doesn't imply a single container.
> 
> But yeah, in general, let's try to separate the logic of the two views as best
> we can to make the transition to it not living in this class easier.

Separated logic.

https://codereview.chromium.org/2652883002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2230:
if (activity != null)
activity.addViewObscuringAllTabs(mOmniboxResultsContainer);
On 2017/01/27 19:54:41, Ted C wrote:
> the mFadingView is actually the one that should be ObscuringAllTabs

Done.

https://codereview.chromium.org/2652883002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2232:
mFadingView.setVisibility(View.GONE);
On 2017/01/27 19:54:41, Ted C wrote:
> I was wrong, and I feel wrong, but I now agree with your original change to
have
> this live in the animator code below.

Done, I also refactored the visibility code a bit so it takes a single path.

https://codereview.chromium.org/2652883002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2250:
mFadingView.setAlpha(0);
On 2017/01/27 19:54:41, Ted C wrote:
> Still wrapping my head around the two views thing, but I actually think we can
> just set it to GONE in this state.  We only needed it to be visible before
> because it also held the suggestions.

Done.

https://codereview.chromium.org/2652883002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2256:
updateOmniboxResultsContainerVisibility(false);
On 2017/01/27 19:54:41, Ted C wrote:
> this should be a no-op if we set it to gone.  maybe just set it to GONE here
> instead.
> 
> And based on my comment below, maybe this calls onFadeOutOverlayGone here to
> allow the same code path?

Done, all of this logic has migrated to [show/hide]FadingOverlay().

https://codereview.chromium.org/2652883002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2268:
mFadeInOmniboxBackgroundAnimator = ObjectAnimator.ofFloat(mFadingView, ALPHA, 0,
1);
On 2017/01/27 19:54:41, Ted C wrote:
> oooh...now that we're not animating a drawable.  let's not specify the start
> value (0 in this case).  Then it will use the current value and it will never
> jump to unneeded states.

Done.

https://codereview.chromium.org/2652883002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2285:
updateOmniboxResultsContainerVisibility(false);
On 2017/01/27 19:54:41, Ted C wrote:
> I think if we want to split the views, I'd like to have very clear splits
> between them (i.e. this animator is for the fading background, it shouldn't
talk
> directly to the results container).
> 
> Maybe we add a method, onFadeOutOverlayGone that is in LocationBarLayout which
> can call this.
> 
> If we want to move where it isn't owned by LocationBarLayout, I think we
really
> need to separate the two now.

Done.

https://codereview.chromium.org/2652883002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2292:
private void runOmniboxResultsFadeAnimation(Animator fadeAnimation) {
On 2017/01/27 19:54:41, Ted C wrote:
> let's rename this to runFadeOverlayAnimation
> 
> and probably mOmniboxBackgroundAnimator to mFadeOverlayAnimator.

Done.

[Ted C, 2017-01-31 07:40:46]

lgtm - can you double check this on tablets before landing?

[mdjones, 2017-01-31 16:28:47]

On 2017/01/31 07:40:46, Ted C wrote:
> lgtm - can you double check this on tablets before landing?

Tablet looks good.

[mdjones, 2017-01-31 16:29:04]

The CQ bit was checked by mdjones@chromium.org

[commit-bot: I haz the power, 2017-01-31 16:29:30]

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, 2017-01-31 17:05:22]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485880144045320,
"parent_rev": "e256b3108ccfdf178866619d2438502ffebda8d6", "commit_rev":
"2f0183c3f049882c0de912cf468d01e8f79790ce"}

[commit-bot: I haz the power, 2017-01-31 17:05:56]

Description was changed from

==========
Omnibox results show correctly for Chrome Home

This change allows the omnibox's suggestions to show inside of the
bottom sheet for Chrome Home. To do this, a second results container
was added to the bottom sheet (the first being a child of the root
coordinator layout). The results container that is acutally used will
depend on whether or not Chrome Home is enabled.

The results container is also no longer responsible for fading out
the web contents when the omnibox is focused. This view is now an
independent child of the root coordinator layout so that it can
function with and without Chrome Home being enabled.

BUG=671361, 685661
==========

to

==========
Omnibox results show correctly for Chrome Home

This change allows the omnibox's suggestions to show inside of the
bottom sheet for Chrome Home. To do this, a second results container
was added to the bottom sheet (the first being a child of the root
coordinator layout). The results container that is acutally used will
depend on whether or not Chrome Home is enabled.

The results container is also no longer responsible for fading out
the web contents when the omnibox is focused. This view is now an
independent child of the root coordinator layout so that it can
function with and without Chrome Home being enabled.

BUG=671361, 685661

Review-Url: https://codereview.chromium.org/2652883002
Cr-Commit-Position: refs/heads/master@{#447249}
Committed:
https://chromium.googlesource.com/chromium/src/+/2f0183c3f049882c0de912cf468d...
==========

[commit-bot: I haz the power, 2017-01-31 17:05:58]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/2f0183c3f049882c0de912cf468d...