Fade theme color in tab switcher

Fade theme color in tab switcher

This change allows updates to a page's theme color to smoothly
transition in the tab switcher.

BUG=606612
Committed: https://crrev.com/71c93015be244e6befed4add93ca7bfe2560c790
Cr-Commit-Position: refs/heads/master@{#404444}

[mdjones, 2016-06-30 02:03:20]

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

[mdjones, 2016-06-30 02:03:20]

The animation logic is similar to that in ToolbarPhone but is not easily shared.
ptal

[Ted C, 2016-06-30 18:55:27]

https://codereview.chromium.org/2114513002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/components/LayoutTab.java
(right):

https://codereview.chromium.org/2114513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/components/LayoutTab.java:227:
if (mToolbarBackgroundColor != toolbarBackgroundColor && updateHost != null) {
Although it pains me to have to say this, should we be using ChromeAnimation
here instead?

That seems to be how LayoutTabs are animated, and I think we should be
consistent with that.

Then they look to be triggerde by something like:
addToAnimation(sourceLayoutTab, LayoutTab.Property.BORDER_ALPHA, 0.0f, 1.0f,
        BACKGROUND_STEP1_DURATION, 0, false,
BakedBezierInterpolator.TRANSFORM_CURVE);

[mdjones, 2016-07-01 01:59:23]

https://codereview.chromium.org/2114513002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/components/LayoutTab.java
(right):

https://codereview.chromium.org/2114513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/components/LayoutTab.java:227:
if (mToolbarBackgroundColor != toolbarBackgroundColor && updateHost != null) {
On 2016/06/30 18:55:26, Ted C wrote:
> Although it pains me to have to say this, should we be using ChromeAnimation
> here instead?
> 
> That seems to be how LayoutTabs are animated, and I think we should be
> consistent with that.
> 
> Then they look to be triggerde by something like:
> addToAnimation(sourceLayoutTab, LayoutTab.Property.BORDER_ALPHA, 0.0f, 1.0f,
>         BACKGROUND_STEP1_DURATION, 0, false,
> BakedBezierInterpolator.TRANSFORM_CURVE);

Updated to use ChromeAnimation, thought I'll be adding some compositor folks to
make sure this looks sane to them too (namely the changes to layout.java).

[Ted C, 2016-07-01 16:41:03]

https://codereview.chromium.org/2114513002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/components/LayoutTab.java
(right):

https://codereview.chromium.org/2114513002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/components/LayoutTab.java:160:
mAnimations = new ChromeAnimation<ChromeAnimation.Animatable<?>>();
Why not leave it as null?

https://codereview.chromium.org/2114513002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/components/LayoutTab.java:232:
if (mToolbarBackgroundColor != toolbarBackgroundColor && updateHost != null) {
any idea why the update host would be null?

https://codereview.chromium.org/2114513002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/components/LayoutTab.java:246:
mAnimations.start();
start() isn't reentrant, so I'm not sure whether we should support appending to
an existing one.  I think we should just clobber an existing one if it exists
instead of the == null check above.

if we do that, change mAnimations to something like mCurrentAnimation.

https://codereview.chromium.org/2114513002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/components/LayoutTab.java:248:
updateHost.requestUpdate();
instead of passing the host, I would probably change this to return a boolean to
say whether this requires an update as a result of this init from host. 
Also...I'm a "bit" surprised that this doesn't happen automatically when you are
initializing tabs.

https://codereview.chromium.org/2114513002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/components/LayoutTab.java:821:
mVisible = visible;
I was thinking we might want to force the animations to finish if it isn't
visible (to avoid churning non-visible frames), but I guess that could cause
problems if you are dealing with a tab right on the cusp of visibility and the
animation just jumps to the finish if you wiggle your finger.

https://codereview.chromium.org/2114513002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java
(right):

https://codereview.chromium.org/2114513002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:1860:
.setDuration(ColorUtils.THEME_COLOR_TRANSITION_DURATION);
There is also a value in CustomTabToolbar for the same

[mdjones, 2016-07-01 18:10:04]

https://codereview.chromium.org/2114513002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/components/LayoutTab.java
(right):

https://codereview.chromium.org/2114513002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/components/LayoutTab.java:160:
mAnimations = new ChromeAnimation<ChromeAnimation.Animatable<?>>();
On 2016/07/01 16:41:02, Ted C wrote:
> Why not leave it as null?

Done.

https://codereview.chromium.org/2114513002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/components/LayoutTab.java:232:
if (mToolbarBackgroundColor != toolbarBackgroundColor && updateHost != null) {
On 2016/07/01 16:41:02, Ted C wrote:
> any idea why the update host would be null?

In the current context it wouldn't be. I guess its ok to let it fail if used
incorrectly (this is now removed anyway).

https://codereview.chromium.org/2114513002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/components/LayoutTab.java:246:
mAnimations.start();
On 2016/07/01 16:41:02, Ted C wrote:
> start() isn't reentrant, so I'm not sure whether we should support appending
to
> an existing one.  I think we should just clobber an existing one if it exists
> instead of the == null check above.
> 
> if we do that, change mAnimations to something like mCurrentAnimation.

Done.

https://codereview.chromium.org/2114513002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/components/LayoutTab.java:248:
updateHost.requestUpdate();
On 2016/07/01 16:41:03, Ted C wrote:
> instead of passing the host, I would probably change this to return a boolean
to
> say whether this requires an update as a result of this init from host. 
> Also...I'm a "bit" surprised that this doesn't happen automatically when you
are
> initializing tabs.

Fixed; the problem is that any other animations have likely completed by the
time this is added and updates will stop. requestUpdate is needed to get the
ball rolling again.

https://codereview.chromium.org/2114513002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/components/LayoutTab.java:821:
mVisible = visible;
On 2016/07/01 16:41:03, Ted C wrote:
> I was thinking we might want to force the animations to finish if it isn't
> visible (to avoid churning non-visible frames), but I guess that could cause
> problems if you are dealing with a tab right on the cusp of visibility and the
> animation just jumps to the finish if you wiggle your finger.

Done.

https://codereview.chromium.org/2114513002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java
(right):

https://codereview.chromium.org/2114513002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:1860:
.setDuration(ColorUtils.THEME_COLOR_TRANSITION_DURATION);
On 2016/07/01 16:41:03, Ted C wrote:
> There is also a value in CustomTabToolbar for the same

Done.

[Ted C, 2016-07-01 18:13:22]

lgtm

[mdjones, 2016-07-01 18:26:00]

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

[mdjones, 2016-07-01 18:26:01]

+changwan for specific compositor owner to take a look.

[Changwan Ryu, 2016-07-07 02:09:03]

https://codereview.chromium.org/2114513002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
(right):

https://codereview.chromium.org/2114513002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:938:
requestUpdate();
requestUpdate() should still be called even when there is no layout animations
left if layouttab has some animation. Probably you didn't see a glitch because
there were other layout animation effects running at the same time.

How about calling requestUpdate() right before returning finished when necessary
(finished == false)?

https://codereview.chromium.org/2114513002/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/util/ColorUtils.java
(right):

https://codereview.chromium.org/2114513002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/util/ColorUtils.java:21:
public static final long THEME_COLOR_TRANSITION_DURATION = 250;
I'm not quite familiar with this file, but I feel that it's awkward to put this
constant here, as it's more concerned about the toolbar's animation effect
rather than about generic color feature.

[mdjones, 2016-07-07 23:32:55]

https://codereview.chromium.org/2114513002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
(right):

https://codereview.chromium.org/2114513002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:938:
requestUpdate();
On 2016/07/07 02:09:03, Changwan Ryu wrote:
> requestUpdate() should still be called even when there is no layout animations
> left if layouttab has some animation. Probably you didn't see a glitch because
> there were other layout animation effects running at the same time.
> 
> How about calling requestUpdate() right before returning finished when
necessary
> (finished == false)?

Done.

https://codereview.chromium.org/2114513002/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/util/ColorUtils.java
(right):

https://codereview.chromium.org/2114513002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/util/ColorUtils.java:21:
public static final long THEME_COLOR_TRANSITION_DURATION = 250;
On 2016/07/07 02:09:03, Changwan Ryu wrote:
> I'm not quite familiar with this file, but I feel that it's awkward to put
this
> constant here, as it's more concerned about the toolbar's animation effect
> rather than about generic color feature.

Moved to ToolbarPhone since it is a phone-only feature.

[Changwan Ryu, 2016-07-08 00:35:10]

lgtm

[mdjones, 2016-07-08 16:18:58]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2016-07-08 16:18:58]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2114513002/#ps100001
(title: "address comments & fix coloring invisible LayoutTab")

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

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-07-08 18:19:12]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-08 18:19:14]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 18:20:51]

Description was changed from

==========
Fade theme color in tab switcher

This change allows updates to a page's theme color to smoothly
transition in the tab switcher.

BUG=606612
==========

to

==========
Fade theme color in tab switcher

This change allows updates to a page's theme color to smoothly
transition in the tab switcher.

BUG=606612

Committed: https://crrev.com/71c93015be244e6befed4add93ca7bfe2560c790
Cr-Commit-Position: refs/heads/master@{#404444}
==========

[commit-bot: I haz the power, 2016-07-08 18:20:52]

Patchset 6 (id:??) landed as
https://crrev.com/71c93015be244e6befed4add93ca7bfe2560c790
Cr-Commit-Position: refs/heads/master@{#404444}