Replace update menu badge

Replace update menu badge

Replace the current update menu badge with a different animated badge.

BUG=586325
Committed: https://crrev.com/601699b4a642b051a4cddf63732cb3c84ab44359
Cr-Commit-Position: refs/heads/master@{#375619}

[Theresa, 2016-02-12 18:02:29]

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

[Theresa, 2016-02-12 18:02:30]

ptal

[gone, 2016-02-12 18:41:50]

Did you already run pngcrush on these things, btw?

[Theresa, 2016-02-12 19:58:10]

On 2016/02/12 18:41:50, dfalcantara wrote:
> Did you already run pngcrush on these things, btw?

Nope, I'll do that.

[gone, 2016-02-13 00:26:16]

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/toolbar.xml (right):

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/toolbar.xml:84: android:background="@null" />
Given that these two things have similar attributes everywhere, should there be
a common <style>?

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/values-v17/styles.xml (right):

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/res...
chrome/android/java/res/values-v17/styles.xml:440: <item
name="android:layout_width">48dp</item>
Any idea what's going on here?  This style sets a layout_width that gets
overridden by both the menu_button and the menu_badge Views.

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

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarPhone.java:393:
mMenuBadge.setImageResource(R.drawable.badge_update_light);
Maybe this should mimic what's above, where it uses the ternary operator? 
Barring that, add a comment saying the default is dark.

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarPhone.java:451:
mMenuBadgeAnimatorSet.addListener(new AnimatorListenerAdapter() {
Can this be combined with the three other AnimatorListenerAdapters above?  Maybe
with a createBadgeAnimatorListener() or something?

Could you take that even further and avoid duplicating the code, too?  Seems
like the same fields are added to both Toolbar subclasses, e.g., and the code
looks like it does fairly similar things.

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java
(right):

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java:722:
mMenuBadgeAnimatorSet.cancel();
Does this need to null check mMenuBadgeAnimatorSet?  AFAICT this function is
always called.

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

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:1021:
: mTabSwitcherAnimationMenuBadgeDarkDrawable;
Maybe move this badgeDrawable call outside the if {}, then just check if
badgeDrawable != null?

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java
(right):

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java:372:
mMenuBadge.setVisibility(View.INVISIBLE);
Set back to GONE pretty please.

[Theresa, 2016-02-16 18:21:54]

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/toolbar.xml (right):

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/toolbar.xml:84: android:background="@null" />
On 2016/02/13 00:26:15, dfalcantara wrote:
> Given that these two things have similar attributes everywhere, should there
be
> a common <style>?

I made 3 new styles. I could go either way on having a few shared styles for the
menu button and update icon vs 1 shared style for the update icon across the two
toolbars and location bar. I think this all gets better when we don't have the
document mode toolbar (2 unique styles for the buttons vs 3).

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/values-v17/styles.xml (right):

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/res...
chrome/android/java/res/values-v17/styles.xml:440: <item
name="android:layout_width">48dp</item>
On 2016/02/13 00:26:15, dfalcantara wrote:
> Any idea what's going on here?  This style sets a layout_width that gets
> overridden by both the menu_button and the menu_badge Views.

The layout_width values are different for tablets and document mode vs not on
phones to get the menu button positioned where its supposed to go.

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

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarPhone.java:393:
mMenuBadge.setImageResource(R.drawable.badge_update_light);
On 2016/02/13 00:26:15, dfalcantara wrote:
> Maybe this should mimic what's above, where it uses the ternary operator? 
> Barring that, add a comment saying the default is dark.

Done.

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarPhone.java:435:
if (!mIsMenuBadgeAnimationRunning && showMenuButtonInOmnibox()) {
To document this change from patchset 3, if the animation to show the badge is
running when a call to remove the badge comes in (e.g. open the overlow menu
while the animation to show the badge is running), I think we should just remove
the badge (visibility == gone), rather than do nothing.

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarPhone.java:451:
mMenuBadgeAnimatorSet.addListener(new AnimatorListenerAdapter() {
On 2016/02/13 00:26:15, dfalcantara wrote:
> Can this be combined with the three other AnimatorListenerAdapters above? 
Maybe
> with a createBadgeAnimatorListener() or something?
> 
> Could you take that even further and avoid duplicating the code, too?  Seems
> like the same fields are added to both Toolbar subclasses, e.g., and the code
> looks like it does fairly similar things.

LocationBarPhone isn't a Toolbar subclass which is why there's some duplicate
code. As discussed offline, since duplicate code is a result of document mode
and it's disappearing soon anyway, so going to leave it as is.

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java
(right):

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java:722:
mMenuBadgeAnimatorSet.cancel();
On 2016/02/13 00:26:16, dfalcantara wrote:
> Does this need to null check mMenuBadgeAnimatorSet?  AFAICT this function is
> always called.

Added a null check. I don't think we can have mIsMenuBadgeAnimationRunning ==
true and mMenuBadgeAnimatorSet == null, but better safe than crashing.

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

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:1021:
: mTabSwitcherAnimationMenuBadgeDarkDrawable;
On 2016/02/13 00:26:16, dfalcantara wrote:
> Maybe move this badgeDrawable call outside the if {}, then just check if
> badgeDrawable != null?

Done.

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java
(right):

https://codereview.chromium.org/1694673002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java:372:
mMenuBadge.setVisibility(View.INVISIBLE);
On 2016/02/13 00:26:16, dfalcantara wrote:
> Set back to GONE pretty please.

Done.

[gone, 2016-02-16 18:33:52]

lgtm

[Theresa, 2016-02-16 18:45:51]

The CQ bit was checked by twellington@chromium.org

[commit-bot: I haz the power, 2016-02-16 18:46:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1694673002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1694673002/100001

[commit-bot: I haz the power, 2016-02-16 19:14:32]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-16 22:53:47]

Description was changed from

==========
Replace update menu badge

Replace the current update menu badge with a different animated badge.

BUG=586325
==========

to

==========
Replace update menu badge

Replace the current update menu badge with a different animated badge.

BUG=586325

Committed: https://crrev.com/601699b4a642b051a4cddf63732cb3c84ab44359
Cr-Commit-Position: refs/heads/master@{#375619}
==========

[commit-bot: I haz the power, 2016-02-16 22:53:48]

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