[Contextual Search] Animate thumbnail appearance

[Contextual Search] Animate thumbnail appearance

BUG=644932
Committed: https://crrev.com/e7cb300f89916774b569fca4caa135eb3122ee22
Cr-Commit-Position: refs/heads/master@{#420646}

[Theresa, 2016-09-22 20:29:11]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 20:29:48]

Dry run: 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, 2016-09-22 22:19:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 22:19:16]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Theresa, 2016-09-23 01:38:40]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[Theresa, 2016-09-23 01:38:53]

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

[Theresa, 2016-09-23 01:38:54]

ptal

[commit-bot: I haz the power, 2016-09-23 01:39:12]

Dry run: 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, 2016-09-23 02:31:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 02:31:14]

Dry run: This issue passed the CQ dry run.

[Donn Denman, 2016-09-23 04:05:23]

LGTM, with a just a few nits and suggestions.

I don't feel very strong on animation and layers, so you might want to get
another reviewer since this is a pretty big change.

Also, cc Peter?

https://codereview.chromium.org/2363843002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchImageControl.java
(right):

https://codereview.chromium.org/2363843002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchImageControl.java:32:
private OverlayPanelAnimation mOverlayPanelAnimation;
nit: make this final too?

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

https://codereview.chromium.org/2363843002/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:465:
thumbnail_size_ * (1.f - thumbnail_visibility_percentage);
Why is the thumbnail_size_ involved here?

https://codereview.chromium.org/2363843002/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:490: } else {
Nit suggestion: I would think that this else clause wouldn't be needed, because
all the calculations would just pin to these hard-coded values.  Then you
wouldn't need the "If" either.  You could replace them with DCHECKS if you want
to be sure.

[Donn Denman, 2016-09-23 04:08:22]

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

https://codereview.chromium.org/2363843002/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:465:
thumbnail_size_ * (1.f - thumbnail_visibility_percentage);
On 2016/09/23 04:05:23, Donn Denman wrote:
> Why is the thumbnail_size_ involved here?

Elaborating my question ... maybe the available space instead, e.g. bar height
would be better?  I'm worried that a small or large image might affect the
positioning and where it animates to/from.

[Theresa, 2016-09-23 04:39:57]

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

https://codereview.chromium.org/2363843002/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:465:
thumbnail_size_ * (1.f - thumbnail_visibility_percentage);
On 2016/09/23 04:08:21, Donn Denman wrote:
> On 2016/09/23 04:05:23, Donn Denman wrote:
> > Why is the thumbnail_size_ involved here?
> 
> Elaborating my question ... maybe the available space instead, e.g. bar height
> would be better?  I'm worried that a small or large image might affect the
> positioning and where it animates to/from. 

Good question.

The thumbnail and 'G' icon slide through their parent container, which acts as a
36dp x 36dp window. thumbnail_size_ (36dp) is used to set bounds on the
container and is therefore the available space. There's a video linked in the
bug of what this looks like. 

The thumbnail itself is bounded to 36dp x 36dp, so it can't be larger than the
available space. If it takes up the full space, the top offset for the thumbnail
should change from 36dp to 0dp.

I do need to modify this for smaller images, though, since the code below
vertically or horizontally centers images that have a minimum side < 36dp. I'll
use two variables to keep track of the thumbnail top and side margin. This code
will animate the y-position from (36dp + top_margin) to (top_margin). The side
margin will be applied instead of 0.f for the x-position

[Theresa, 2016-09-23 15:45:46]

twellington@chromium.org changed reviewers:
+ mdjones@chromium.org

[Theresa, 2016-09-23 15:45:47]

+mdjones for ContextualSearchSceneLayer.java OWNERS

Will you also take a look at the stuff in contextual_search_layers.cc to make
sure I'm not doing anything crazy with the cc layers.

https://codereview.chromium.org/2363843002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchImageControl.java
(right):

https://codereview.chromium.org/2363843002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchImageControl.java:32:
private OverlayPanelAnimation mOverlayPanelAnimation;
On 2016/09/23 04:05:23, Donn Denman wrote:
> nit: make this final too?

Done.

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

https://codereview.chromium.org/2363843002/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:465:
thumbnail_size_ * (1.f - thumbnail_visibility_percentage);
On 2016/09/23 04:39:57, Theresa Wellington wrote:
> On 2016/09/23 04:08:21, Donn Denman wrote:
> > On 2016/09/23 04:05:23, Donn Denman wrote:
> > > Why is the thumbnail_size_ involved here?
> > 
> > Elaborating my question ... maybe the available space instead, e.g. bar
height
> > would be better?  I'm worried that a small or large image might affect the
> > positioning and where it animates to/from. 
> 
> Good question.
> 
> The thumbnail and 'G' icon slide through their parent container, which acts as
a
> 36dp x 36dp window. thumbnail_size_ (36dp) is used to set bounds on the
> container and is therefore the available space. There's a video linked in the
> bug of what this looks like. 
> 
> The thumbnail itself is bounded to 36dp x 36dp, so it can't be larger than the
> available space. If it takes up the full space, the top offset for the
thumbnail
> should change from 36dp to 0dp.
> 
> I do need to modify this for smaller images, though, since the code below
> vertically or horizontally centers images that have a minimum side < 36dp.
I'll
> use two variables to keep track of the thumbnail top and side margin. This
code
> will animate the y-position from (36dp + top_margin) to (top_margin). The side
> margin will be applied instead of 0.f for the x-position

Done. I also added some comments so hopefully this is more clear.

https://codereview.chromium.org/2363843002/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:490: } else {
On 2016/09/23 04:05:23, Donn Denman wrote:
> Nit suggestion: I would think that this else clause wouldn't be needed,
because
> all the calculations would just pin to these hard-coded values.  Then you
> wouldn't need the "If" either.  You could replace them with DCHECKS if you
want
> to be sure.

Done.

[Theresa, 2016-09-23 15:46:20]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 15:46:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mdjones, 2016-09-23 16:07:40]

lgtm

[Theresa, 2016-09-23 16:09:53]

The CQ bit was unchecked by twellington@chromium.org

[Theresa, 2016-09-23 16:09:54]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2016-09-23 16:09:55]

The patchset sent to the CQ was uploaded after l-g-t-m from donnd@chromium.org
Link to the patchset: https://codereview.chromium.org/2363843002/#ps40001
(title: "Changes from donnd@ review")

[commit-bot: I haz the power, 2016-09-23 16:10:03]

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, 2016-09-23 17:34:08]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-23 17:35:27]

Description was changed from

==========
[Contextual Search] Animate thumbnail appearance

BUG=644932
==========

to

==========
[Contextual Search] Animate thumbnail appearance

BUG=644932

Committed: https://crrev.com/e7cb300f89916774b569fca4caa135eb3122ee22
Cr-Commit-Position: refs/heads/master@{#420646}
==========

[commit-bot: I haz the power, 2016-09-23 17:35:28]

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