[Contextual Search] Fix Promo behavior.

[Contextual Search] Fix Promo behavior.

BUG=475715
BUG=477060
Committed: https://crrev.com/8d1bac7f28a0eb9651cbe3dd962334e3f920a6e1
Cr-Commit-Position: refs/heads/master@{#325537}

R=donnd@chromium.org, dtrainor@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/6a0a97e1f828b2da65c55a14f2c1e03a4203553d

[pedro (no code reviews), 2015-04-09 22:26:35]

pedrosimonetti@chromium.org changed reviewers:
+ donnd@chromium.org

[pedro (no code reviews), 2015-04-09 22:26:53]

Donn, please take a first look at this change.

[Donn Denman, 2015-04-09 23:58:44]

Sorry to ask that you change a bunch of names, but I think "Accepted" is much
less confusing than "OptIn" for this change.

https://codereview.chromium.org/1061063005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
(right):

https://codereview.chromium.org/1061063005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:153:
mManagementDelegate.setPreferenceState(enabled);
Do we need to check that the management delegate is not null here?

https://codereview.chromium.org/1061063005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java
(right):

https://codereview.chromium.org/1061063005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:74:
private boolean mIsAnimatingPromoOptIn;
I think it's confusing to use the term "OptIn" for the purpose of this CL, since
we use that for the other promo.  Maybe mIsAnimatingPromoAccepted instead?

https://codereview.chromium.org/1061063005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:381:
setPreferenceState(true);
I wonder if it wouldn't be better to change the preference state when the choice
is made, instead of at the end of the animation.

https://codereview.chromium.org/1061063005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java
(right):

https://codereview.chromium.org/1061063005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1287:
animatePromoOptIn();
As mentioned elsewhere, maybe set the preference to true here?  Then we can just
set it to <accepted>.

https://codereview.chromium.org/1061063005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagementDelegate.java
(right):

https://codereview.chromium.org/1061063005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagementDelegate.java:22:
* Sets the preference state.
Nit: Maybe add "to enabled or disabled" since the preference is a tri-state?

[pedro (no code reviews), 2015-04-14 20:42:26]

Hey Donn, please take another look.

https://codereview.chromium.org/1061063005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
(right):

https://codereview.chromium.org/1061063005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:153:
mManagementDelegate.setPreferenceState(enabled);
On 2015/04/09 23:58:43, Donn Denman wrote:
> Do we need to check that the management delegate is not null here?

Done.

https://codereview.chromium.org/1061063005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java
(right):

https://codereview.chromium.org/1061063005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:74:
private boolean mIsAnimatingPromoOptIn;
On 2015/04/09 23:58:43, Donn Denman wrote:
> I think it's confusing to use the term "OptIn" for the purpose of this CL,
since
> we use that for the other promo.  Maybe mIsAnimatingPromoAccepted instead?

Done.

https://codereview.chromium.org/1061063005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:381:
setPreferenceState(true);
On 2015/04/09 23:58:43, Donn Denman wrote:
> I wonder if it wouldn't be better to change the preference state when the
choice
> is made, instead of at the end of the animation.

We check whether or not to display the Promo by checking if the preference is
unset (which means user hasn't decided). If we set the preference before, the
Promo will disappear immediately, instead of animating.

https://codereview.chromium.org/1061063005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java
(right):

https://codereview.chromium.org/1061063005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1287:
animatePromoOptIn();
On 2015/04/09 23:58:43, Donn Denman wrote:
> As mentioned elsewhere, maybe set the preference to true here?  Then we can
just
> set it to <accepted>.

See my response in yours previous comment.

https://codereview.chromium.org/1061063005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagementDelegate.java
(right):

https://codereview.chromium.org/1061063005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagementDelegate.java:22:
* Sets the preference state.
On 2015/04/09 23:58:43, Donn Denman wrote:
> Nit: Maybe add "to enabled or disabled" since the preference is a tri-state?

Done.

[pedro (no code reviews), 2015-04-14 23:17:13]

Hey Donn, this is ready for a complete review. Please take a look at it.

[Donn Denman, 2015-04-14 23:50:04]

LGTM!

Do you think it's worth asking David T to double-check any of this?

https://codereview.chromium.org/1061063005/diff/60001/chrome/browser/android/...
File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right):

https://codereview.chromium.org/1061063005/diff/60001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:202: // take
a snapshot of the Promo View) it will always be available before
This is confusing to me.  If the promo will always be available before the
shadow, why do we need to check if it's ready?

[pedro (no code reviews), 2015-04-15 04:48:02]

pedrosimonetti@chromium.org changed reviewers:
+ dtrainor@chromium.org

[pedro (no code reviews), 2015-04-15 04:48:35]

Hey David, sorry I forgot to add you as a reviewer. Please take a look at this
change.

[David Trainor- moved to gerrit, 2015-04-15 07:36:15]

small comment, but lgtm.

https://codereview.chromium.org/1061063005/diff/60001/chrome/browser/android/...
File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right):

https://codereview.chromium.org/1061063005/diff/60001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:205: // be
placed over the Shadow layer, making it not visible.
You can insert children at specific points in the child list.  That might be
simpler.

[pedro (no code reviews), 2015-04-16 19:05:29]

I was trying to fix a problem that would cause the Search Bar Shadow
to not be displayed. The problem is that the dynamic Promo resource
might not be available right away (in the first frame) which was
causing it to be placed after the shadow, obstructing it.

Now, we're always placing the Promo layer in the first position,
so this problem won't happen.

Please take another look.

https://codereview.chromium.org/1061063005/diff/60001/chrome/browser/android/...
File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right):

https://codereview.chromium.org/1061063005/diff/60001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:202: // take
a snapshot of the Promo View) it will always be available before
On 2015/04/14 23:50:04, Donn Denman wrote:
> This is confusing to me.  If the promo will always be available before the
> shadow, why do we need to check if it's ready?

It was supposed to read: "it will NOT always be available before". There was
still a potential bug in the code so I reimplemented a fix for this problem and
updated the comment.

See my patch comment for more details.

https://codereview.chromium.org/1061063005/diff/60001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:205: // be
placed over the Shadow layer, making it not visible.
On 2015/04/15 07:36:15, David Trainor wrote:
> You can insert children at specific points in the child list.  That might be
> simpler.

Good idea. I'm using InsertChild() now.

On the other hand, I think the existing API is not flexible. I wanted to place
the Promo layer immediately before the Search Bar shadow one, but there are no
InsertBefore() or InsertAfter() methods. I could do that with InsertChild() but
I would have to know the position of the Shadow layer, which is not possible to
do since the IndexOfChild() method is private.

See my patch comment for more details.

[Donn Denman, 2015-04-16 19:15:46]

On 2015/04/16 19:05:29, pedrosimonetti wrote:
> I was trying to fix a problem that would cause the Search Bar Shadow
> to not be displayed. The problem is that the dynamic Promo resource
> might not be available right away (in the first frame) which was
> causing it to be placed after the shadow, obstructing it.
> 
> Now, we're always placing the Promo layer in the first position,
> so this problem won't happen.
> 
> Please take another look.
> 
>
https://codereview.chromium.org/1061063005/diff/60001/chrome/browser/android/...
> File chrome/browser/android/compositor/layer/contextual_search_layer.cc
(right):
> 
>
https://codereview.chromium.org/1061063005/diff/60001/chrome/browser/android/...
> chrome/browser/android/compositor/layer/contextual_search_layer.cc:202: //
take
> a snapshot of the Promo View) it will always be available before
> On 2015/04/14 23:50:04, Donn Denman wrote:
> > This is confusing to me.  If the promo will always be available before the
> > shadow, why do we need to check if it's ready?
> 
> It was supposed to read: "it will NOT always be available before". There was
> still a potential bug in the code so I reimplemented a fix for this problem
and
> updated the comment.
> 
> See my patch comment for more details.
> 
>
https://codereview.chromium.org/1061063005/diff/60001/chrome/browser/android/...
> chrome/browser/android/compositor/layer/contextual_search_layer.cc:205: // be
> placed over the Shadow layer, making it not visible.
> On 2015/04/15 07:36:15, David Trainor wrote:
> > You can insert children at specific points in the child list.  That might be
> > simpler.
> 
> Good idea. I'm using InsertChild() now.
> 
> On the other hand, I think the existing API is not flexible. I wanted to place
> the Promo layer immediately before the Search Bar shadow one, but there are no
> InsertBefore() or InsertAfter() methods. I could do that with InsertChild()
but
> I would have to know the position of the Shadow layer, which is not possible
to
> do since the IndexOfChild() method is private.
> 
> See my patch comment for more details.

I don't see a new patch, did you forget to upload it?

[Donn Denman, 2015-04-16 19:18:04]

On 2015/04/16 19:15:46, Donn Denman wrote:
> On 2015/04/16 19:05:29, pedrosimonetti wrote:
> > I was trying to fix a problem that would cause the Search Bar Shadow
> > to not be displayed. The problem is that the dynamic Promo resource
> > might not be available right away (in the first frame) which was
> > causing it to be placed after the shadow, obstructing it.
> > 
> > Now, we're always placing the Promo layer in the first position,
> > so this problem won't happen.
> > 
> > Please take another look.
> > 
> >
>
https://codereview.chromium.org/1061063005/diff/60001/chrome/browser/android/...
> > File chrome/browser/android/compositor/layer/contextual_search_layer.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/1061063005/diff/60001/chrome/browser/android/...
> > chrome/browser/android/compositor/layer/contextual_search_layer.cc:202: //
> take
> > a snapshot of the Promo View) it will always be available before
> > On 2015/04/14 23:50:04, Donn Denman wrote:
> > > This is confusing to me.  If the promo will always be available before the
> > > shadow, why do we need to check if it's ready?
> > 
> > It was supposed to read: "it will NOT always be available before". There was
> > still a potential bug in the code so I reimplemented a fix for this problem
> and
> > updated the comment.
> > 
> > See my patch comment for more details.
> > 
> >
>
https://codereview.chromium.org/1061063005/diff/60001/chrome/browser/android/...
> > chrome/browser/android/compositor/layer/contextual_search_layer.cc:205: //
be
> > placed over the Shadow layer, making it not visible.
> > On 2015/04/15 07:36:15, David Trainor wrote:
> > > You can insert children at specific points in the child list.  That might
be
> > > simpler.
> > 
> > Good idea. I'm using InsertChild() now.
> > 
> > On the other hand, I think the existing API is not flexible. I wanted to
place
> > the Promo layer immediately before the Search Bar shadow one, but there are
no
> > InsertBefore() or InsertAfter() methods. I could do that with InsertChild()
> but
> > I would have to know the position of the Shadow layer, which is not possible
> to
> > do since the IndexOfChild() method is private.
> > 
> > See my patch comment for more details.
> 
> I don't see a new patch, did you forget to upload it?
Never mind -- I see the changes now!

[Donn Denman, 2015-04-16 19:21:57]

On 2015/04/16 19:18:04, Donn Denman wrote:
> On 2015/04/16 19:15:46, Donn Denman wrote:
> > On 2015/04/16 19:05:29, pedrosimonetti wrote:
> > > I was trying to fix a problem that would cause the Search Bar Shadow
> > > to not be displayed. The problem is that the dynamic Promo resource
> > > might not be available right away (in the first frame) which was
> > > causing it to be placed after the shadow, obstructing it.
> > > 
> > > Now, we're always placing the Promo layer in the first position,
> > > so this problem won't happen.
> > > 
> > > Please take another look.
> > > 
> > >
> >
>
https://codereview.chromium.org/1061063005/diff/60001/chrome/browser/android/...
> > > File chrome/browser/android/compositor/layer/contextual_search_layer.cc
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1061063005/diff/60001/chrome/browser/android/...
> > > chrome/browser/android/compositor/layer/contextual_search_layer.cc:202: //
> > take
> > > a snapshot of the Promo View) it will always be available before
> > > On 2015/04/14 23:50:04, Donn Denman wrote:
> > > > This is confusing to me.  If the promo will always be available before
the
> > > > shadow, why do we need to check if it's ready?
> > > 
> > > It was supposed to read: "it will NOT always be available before". There
was
> > > still a potential bug in the code so I reimplemented a fix for this
problem
> > and
> > > updated the comment.
> > > 
> > > See my patch comment for more details.
> > > 
> > >
> >
>
https://codereview.chromium.org/1061063005/diff/60001/chrome/browser/android/...
> > > chrome/browser/android/compositor/layer/contextual_search_layer.cc:205: //
> be
> > > placed over the Shadow layer, making it not visible.
> > > On 2015/04/15 07:36:15, David Trainor wrote:
> > > > You can insert children at specific points in the child list.  That
might
> be
> > > > simpler.
> > > 
> > > Good idea. I'm using InsertChild() now.
> > > 
> > > On the other hand, I think the existing API is not flexible. I wanted to
> place
> > > the Promo layer immediately before the Search Bar shadow one, but there
are
> no
> > > InsertBefore() or InsertAfter() methods. I could do that with
InsertChild()
> > but
> > > I would have to know the position of the Shadow layer, which is not
possible
> > to
> > > do since the IndexOfChild() method is private.
> > > 
> > > See my patch comment for more details.
> > 
> > I don't see a new patch, did you forget to upload it?
> Never mind -- I see the changes now!

LGTM, just one trivial nit.

[Donn Denman, 2015-04-16 19:22:12]

lgtm

https://codereview.chromium.org/1061063005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java
(right):

https://codereview.chromium.org/1061063005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:235:
// Prevent the fling gesture to move the Panel from PEEKED to MAXIMIZED if the
Panel
Nit: change "to move" ==> "from moving".

[David Trainor- moved to gerrit, 2015-04-16 20:51:46]

lgtm

https://codereview.chromium.org/1061063005/diff/60001/chrome/browser/android/...
File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right):

https://codereview.chromium.org/1061063005/diff/60001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:205: // be
placed over the Shadow layer, making it not visible.
On 2015/04/16 19:05:29, pedrosimonetti wrote:
> On 2015/04/15 07:36:15, David Trainor wrote:
> > You can insert children at specific points in the child list.  That might be
> > simpler.
> 
> Good idea. I'm using InsertChild() now.
> 
> On the other hand, I think the existing API is not flexible. I wanted to place
> the Promo layer immediately before the Search Bar shadow one, but there are no
> InsertBefore() or InsertAfter() methods. I could do that with InsertChild()
but
> I would have to know the position of the Shadow layer, which is not possible
to
> do since the IndexOfChild() method is private.
> 
> See my patch comment for more details.

Yeah, I wonder if we could make that public.  I don't see any reason not to :). 
But I'm fine with this way too, as long as putting the promo at 0 works :).

[pedro (no code reviews), 2015-04-16 21:12:22]

Donn, PTAL

https://codereview.chromium.org/1061063005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java
(right):

https://codereview.chromium.org/1061063005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:235:
// Prevent the fling gesture to move the Panel from PEEKED to MAXIMIZED if the
Panel
On 2015/04/16 19:22:12, Donn Denman wrote:
> Nit: change "to move" ==> "from moving".

Done.

[pedro (no code reviews), 2015-04-16 22:01:58]

The CQ bit was checked by pedrosimonetti@chromium.org

[pedro (no code reviews), 2015-04-16 22:01:59]

The patchset sent to the CQ was uploaded after l-g-t-m from donnd@chromium.org,
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/1061063005/#ps140001
(title: "Sync & rebase")

[commit-bot: I haz the power, 2015-04-16 22:06:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1061063005/140001

[commit-bot: I haz the power, 2015-04-16 23:00:20]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-04-16 23:02:18]

Patchset 8 (id:??) landed as
https://crrev.com/8d1bac7f28a0eb9651cbe3dd962334e3f920a6e1
Cr-Commit-Position: refs/heads/master@{#325537}

[David Trainor- moved to gerrit, 2015-04-17 20:56:07]

Committed patchset #8 (id:140001) manually as
6a0a97e1f828b2da65c55a14f2c1e03a4203553d (presubmit successful).