Add support for highlighting menu items

Add support for highlighting menu items

Add the ability to highlight menu items through the AppMenuButtonHelper.
This will allow components to draw attention to the menu button and,
once the menu is open, a particular menu entry.

BUG=705687
Review-Url: https://codereview.chromium.org/2779543005
Cr-Commit-Position: refs/heads/master@{#464300}
Committed: https://chromium.googlesource.com/chromium/src/+/3ab64ddd22e7af80cccc96562b6cf64b594be723

[David Trainor- moved to gerrit, 2017-03-27 21:41:48]

Description was changed from

==========
Attempt to optimize and clean up the background logic


Add support for highlighting menu items

Add the ability to highlight menu items through the AppMenuButtonHelper.
This will allow components to draw attention to the menu button and,
once the menu is open, a particular menu entry.

BUG=705687
==========

to

==========
Add support for highlighting menu items

Add the ability to highlight menu items through the AppMenuButtonHelper.
This will allow components to draw attention to the menu button and,
once the menu is open, a particular menu entry.

BUG=705687
==========

[David Trainor- moved to gerrit, 2017-03-27 21:58:56]

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

[David Trainor- moved to gerrit, 2017-03-27 21:58:57]

tedchoc@ ptal.  Still need to focus the menu item and throttle the framerate. 
Might be in separate patches though.  dfalcantara@ as cc to throttle.

https://codereview.chromium.org/2779543005/diff/1/chrome/android/java/res/val...
File chrome/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/2779543005/diff/1/chrome/android/java/res/val...
chrome/android/java/res/values/colors.xml:198: <color
name="app_menu_highlight_color_light">#4285F4</color>
These are existing colors already.  Will move.

https://codereview.chromium.org/2779543005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java
(right):

https://codereview.chromium.org/2779543005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:106:
private Drawable mCachedStandardBackgroundDrawable;
Remove.

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

https://codereview.chromium.org/2779543005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:2186:
// TODO(dtrainor): Why is the badge only shown on light?
TODO(me) need to ask about this :).

[Ted C, 2017-03-31 19:01:47]

https://codereview.chromium.org/2779543005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java
(right):

https://codereview.chromium.org/2779543005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:447:
if (tag == null || !(tag instanceof MenuItemViewHolder)) return;
when isn't the tag a MenuItemViewHolder?  It seems like it is the root of all of
the tags.

Why would tag be null?

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

https://codereview.chromium.org/2779543005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java:698:
setAppMenuButtonHighlight(highlight, true /* useLightDrawable*/);
should we expose uselightdrawable (mUseLightColorAssets for tablets, and
mUseLightToolbarDrawables for phones) to use here instead of true?

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

https://codereview.chromium.org/2779543005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:2184:
super.setMenuButtonHighlight(highlight);
doesn't super also call setAppMenuButtonHighlight?

Should this be:

if (mTabSwitcherState == STATIC_TAB) return;

At the beginning?

https://codereview.chromium.org/2779543005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:2186:
// TODO(dtrainor): Why is the badge only shown on light?
I don't understand this TODO.

[David Trainor- moved to gerrit, 2017-04-11 18:43:04]

ptal... sorry this is hard to test remotely.  cleaned up what I had working
yesterday.  will check that it works tonight :(.

https://codereview.chromium.org/2779543005/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/PulseDrawable.java
(right):

https://codereview.chromium.org/2779543005/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/PulseDrawable.java:26:
public class PulseDrawable extends Drawable {
These should be in a better, more generic place.  Do you have a preference?

[David Trainor- moved to gerrit, 2017-04-12 05:35:20]

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

[commit-bot: I haz the power, 2017-04-12 05:35:41]

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, 2017-04-12 06:35:16]

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

[commit-bot: I haz the power, 2017-04-12 06:35:17]

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

[Ted C, 2017-04-12 18:05:57]

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java
(right):

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:446:
private void checkHighlightDrawable(View view, boolean isIcon, int itemId) {
I think maybe this should be:

highlightItemIfNecessary or something

This name is not what I expected this method would do.

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:465:
background = background.getConstantState().newDrawable(resources);
Why aren't we reusing the background as is?

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/PulseDrawable.java
(right):

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/PulseDrawable.java:64:
protected PulseState mState;
why protected?

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/PulseDrawable.java:85:
public void setColor(@ColorInt int color) {
I'd rather this not be configurable right now.  I'd rather never get to a state
where we
use anything but one color

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/PulseDrawable.java:95:
setBounds(mOriginalBounds);
Should we only call this if !mOriginalBounds.isEmpty()?

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/PulseDrawable.java:209:
/** The original color to draw (may not include any alpha updates. */
may not, or does not?  also missing trailing )

I thought this always was the original color and never had the alpha modified.

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/PulseDrawable.java:256:
private final Runnable mNextFrame = new Runnable() {
I'd put this above the constructor as it is a local variable.

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/PulseDrawableFactory.java
(right):

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/PulseDrawableFactory.java:25:
public static PulseDrawable createHighlight() {
Any reason these aren't statics in PulseDrawable?

Then we could have the constructor be private and these could be the only way
you interact with that class (then you don't have to think about what the
painter is or anything).

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/TabSwitcherCallout.java
(right):

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/TabSwitcherCallout.java:37:
// if (!isTabSwitcherCalloutNecessary()) return null;
?

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java
(right):

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java:661:
public abstract boolean useLightDrawables();
javadoc

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java:813:
* @param highlighting     Whether or not the menu button should be highlighted.
too much space after highlighting

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java:826:
mHighlightDrawable.setColor(color);
ah...so we already use different colors.  Can we just say dark or light here
instead allowing passing colors?

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java
(right):

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:2191:
System.out.println("dtrainor: tab switcher anim " +
isTabSwitcherAnimationRunning() + ", "
?

[David Trainor- moved to gerrit, 2017-04-12 18:56:19]

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

[commit-bot: I haz the power, 2017-04-12 18:57:27]

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

[David Trainor- moved to gerrit, 2017-04-12 18:59:37]

addressed comments.  can try the trick with the background drawable later
tonight.  ptal.

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java
(right):

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:446:
private void checkHighlightDrawable(View view, boolean isIcon, int itemId) {
On 2017/04/12 18:05:57, Ted C wrote:
> I think maybe this should be:
> 
> highlightItemIfNecessary or something
> 
> This name is not what I expected this method would do.

Done.

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:465:
background = background.getConstantState().newDrawable(resources);
On 2017/04/12 18:05:56, Ted C wrote:
> Why aren't we reusing the background as is?

I can probably make it work.  Here's why it's breaking:
1. LayerDrawable calls Drawable#setCallback(...) on the background Drawable as
it's added, effectively stealing ownership but not telling anyone.
2. View.setBackground(LayerDrawable) calls Drawable#setCallback(null) when it
drops it's internal reference to the old background.  Sadly this detaches the
Drawable from LayerDrawable and not the View... as the callback was already
swapped to the View.

A solution might be:

view.setBackground(null);

// build the LayerDrawable, reuse the background.

view.setBackground(drawable);

I'll test this tonight

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/PulseDrawable.java
(right):

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/PulseDrawable.java:64:
protected PulseState mState;
On 2017/04/12 18:05:57, Ted C wrote:
> why protected?

Ah I was planning on overriding this class for the sub-types... but found out I
couldn't easily do that because of the constant state.

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/PulseDrawable.java:85:
public void setColor(@ColorInt int color) {
On 2017/04/12 18:05:57, Ted C wrote:
> I'd rather this not be configurable right now.  I'd rather never get to a
state
> where we
> use anything but one color

I can have it set light/dark if you'd like.  We only need two colors internally.

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/PulseDrawable.java:95:
setBounds(mOriginalBounds);
On 2017/04/12 18:05:57, Ted C wrote:
> Should we only call this if !mOriginalBounds.isEmpty()?

Yes good point.

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/PulseDrawable.java:209:
/** The original color to draw (may not include any alpha updates. */
On 2017/04/12 18:05:57, Ted C wrote:
> may not, or does not?  also missing trailing )
> 
> I thought this always was the original color and never had the alpha modified.

It depends on whether or not the color passed in had alpha to begin with.

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/PulseDrawable.java:256:
private final Runnable mNextFrame = new Runnable() {
On 2017/04/12 18:05:57, Ted C wrote:
> I'd put this above the constructor as it is a local variable.

Done.

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/PulseDrawableFactory.java
(right):

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/PulseDrawableFactory.java:25:
public static PulseDrawable createHighlight() {
On 2017/04/12 18:05:57, Ted C wrote:
> Any reason these aren't statics in PulseDrawable?
> 
> Then we could have the constructor be private and these could be the only way
> you interact with that class (then you don't have to think about what the
> painter is or anything).

Sure sounds good.

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/TabSwitcherCallout.java
(right):

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/TabSwitcherCallout.java:37:
// if (!isTabSwitcherCalloutNecessary()) return null;
On 2017/04/12 18:05:57, Ted C wrote:
> ?

Doesn't everyone want to see the tab switcher callout!?

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java
(right):

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java:661:
public abstract boolean useLightDrawables();
On 2017/04/12 18:05:57, Ted C wrote:
> javadoc

Done.

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java:813:
* @param highlighting     Whether or not the menu button should be highlighted.
On 2017/04/12 18:05:57, Ted C wrote:
> too much space after highlighting

Done.

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java:826:
mHighlightDrawable.setColor(color);
On 2017/04/12 18:05:57, Ted C wrote:
> ah...so we already use different colors.  Can we just say dark or light here
> instead allowing passing colors?

Yeah good idea

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java
(right):

https://codereview.chromium.org/2779543005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:2191:
System.out.println("dtrainor: tab switcher anim " +
isTabSwitcherAnimationRunning() + ", "
On 2017/04/12 18:05:57, Ted C wrote:
> ?

Sorry... logging from figuring out why this wasn't showing in the switcher :/.

[Ted C, 2017-04-12 20:44:08]

lgtm

[David Trainor- moved to gerrit, 2017-04-13 00:08:44]

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

[commit-bot: I haz the power, 2017-04-13 00:09:32]

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, 2017-04-13 01:19:00]

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

[commit-bot: I haz the power, 2017-04-13 01:19:01]

Dry run: This issue passed the CQ dry run.

[David Trainor- moved to gerrit, 2017-04-13 03:52:02]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2017-04-13 03:52:48]

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

[David Trainor- moved to gerrit, 2017-04-13 04:07:45]

The CQ bit was unchecked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2017-04-13 04:07:56]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2017-04-13 04:08:25]

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-04-13 04:50:39]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492056476520090,
"parent_rev": "7333f8119912a21c35d48e14503f26ad45f43763", "commit_rev":
"3ab64ddd22e7af80cccc96562b6cf64b594be723"}

[commit-bot: I haz the power, 2017-04-13 04:51:37]

Description was changed from

==========
Add support for highlighting menu items

Add the ability to highlight menu items through the AppMenuButtonHelper.
This will allow components to draw attention to the menu button and,
once the menu is open, a particular menu entry.

BUG=705687
==========

to

==========
Add support for highlighting menu items

Add the ability to highlight menu items through the AppMenuButtonHelper.
This will allow components to draw attention to the menu button and,
once the menu is open, a particular menu entry.

BUG=705687

Review-Url: https://codereview.chromium.org/2779543005
Cr-Commit-Position: refs/heads/master@{#464300}
Committed:
https://chromium.googlesource.com/chromium/src/+/3ab64ddd22e7af80cccc96562b6c...
==========

[commit-bot: I haz the power, 2017-04-13 04:51:39]

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