Add update menu item and app menu icon badge

Add update menu item and app menu icon badge

Adds an update menu item and badge, controlled by finch (or command line).

BUG=566085
Committed: https://crrev.com/018d2db49cda296c0932d7935ec9dbf1e3e3e40f
Cr-Commit-Position: refs/heads/master@{#365696}

[Theresa, 2015-12-08 03:11:53]

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

[Theresa, 2015-12-08 03:11:54]

ptal

I have a couple of TODOs in ToolbarPhone.java that I plan to address tomorrow.

I also want to ask the group (via email) about:
 - chrome://flags for testers
 - whether we should hide the badge for accessibility; if not if the string I
have is appropriate
 - default summary string (I think this is still TBD)
 - command line flags to set the newest version in OmahaClient (for more robust
manual code path testing)
 
 I plan to ask these questions after the patchset has some review, in case other
questions arise.

[Theresa, 2015-12-08 17:59:37]

Add some comments for things I need to do as a result of tryjobs & discussion
with sgabriel@

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/res...
File chrome/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/res...
chrome/android/java/res/values/dimens.xml:59: <dimen
name="menu_badge_background_size">16dp</dimen>
The icon size w/out the border is 16x16; the background size should change to
17x17.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java
(right):

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:71:
public static synchronized UpdateMenuItemHelper getInstance() {
Fix this (findbugs error for making the method synchronized)

[gone, 2015-12-08 22:24:25]

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
File chrome/android/java/res/layout-sw600dp/toolbar.xml (right):

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/res/layout-sw600dp/toolbar.xml:52: />
nit: should go on the same line as the last attribute

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/res/layout-sw600dp/toolbar.xml:58: <FrameLayout
1) Does it make sense to pull this FrameLayout + children thing out into its own
Layout file, then include it from the three locations I see so far?

2) nit: could you add some newlines to break up this monolithic block of stuff? 
The attributes are indented at the same level as the children and makes it hard
to parse.

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/res/layout-sw600dp/toolbar.xml:61:
android:id="@+id/menu_btn_wrapper" >
nit: menu_button_wrapper?  I think the btn_ designation is just for filenames,
and the IDs above use forward_button / btn_forward as well.

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/res/layout-sw600dp/toolbar.xml:77:
android:contentDescription="@string/accessibility_toolbar_update_badge"
I don't know if it makes sense to apply the accessibility description to the
tiny badge; does it make sense to apply a common accessibility string to the
FrameLayout instead?  One that changes depending on whether an update is
available?

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
File chrome/android/java/res/layout/update_menu_item.xml (right):

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/res/layout/update_menu_item.xml:12:
android:orientation="horizontal" >
----------------------------------------------
Newlines.

<Woody looking awed>  <Buzz, arm outstretched>

Newlines everywhere.
----------------------------------------------

Seriously, though: newlines between children?

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/res/layout/update_menu_item.xml:13: <LinearLayout
nit: indentation's wonky.

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/res/layout/update_menu_item.xml:23:
android:textColor="#DC5554"
I think convention is to use lowercase instead of caps for hex colors.  Also,
move the layout_* params right below the ID.

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/res/layout/update_menu_item.xml:34: <view
Can't you just use
<org.chromium.chrome.browser.appmenu.AppMenUItemIcon
    etc
    />

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:378: if
(UpdateMenuItemHelper.getInstance().shouldShowMenuItem(activity)
does it make more sense to record whether the UpdateMenuItemHelper thinks the
MenuItem is visible?  that would prevent the logic from drifting apart,
especially because UpdateMenuItemHelper doesn't know what overview mode is (and
I'm not sure it's checked before showing the menu item in the AppMenuAdapter).

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java
(right):

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:53:

nit: either remove the newline here or add newlines between all of the above
MENU_ITEM values.

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:148:
if (convertView == null) {
if (!(convertView instanceof CustomMenuItemViewHolder)) maybe?  handles the null
check and avoids incorrectly casting something.

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java
(right):

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java:716:
}
Is this bit necessary?  DocumentActivity is a ChromeActivity and goes through
onDeferredStartup, etc.

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java
(right):

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:30:
private static final String TAG = "OmahaUpdateMenuItem";
should be UpdateMenuItemHelper

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:32:
// Command line flag for testing.
Move to ChromeSwitches.java

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:37:
private static final String FIELD_TRIAL_NAME = "UpateMenuItem";
Is "Upate" intentional?  Saw it in the histogram too.

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:55:
// Whether OmahaClient has already been checked for an update., and volunteer my
time
/methinks your email window focus was incorrect

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:71:
public static synchronized UpdateMenuItemHelper getInstance() {
On 2015/12/08 17:59:37, Theresa Wellington wrote:
> Fix this (findbugs error for making the method synchronized)

Any reason why you're using a singleton?  Can it just be a member of the
ChromeApplication?

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:83:
* Checks if the OmahaClient knows about an update.
{@link OmahaClient}

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:87:
if (getBooleanParam(ENABLE_UPDATE_MENU_ITEM)) {
early exit?

if (!getBooleanParam(ENABLE_UPDATE_MENU_ITEM) || mAlreadyCheckedForUpdates)
return;

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:111:
recordUpdateHistogram();
should recordUpdateHistogram() be the thing checking if
getClickedUpdateMenuItem() was called?  this is (mostly) duplicated with the
call below.

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:142:
if (!mAlreadyCheckedForUpdates) {
this last bit is exactly the same as shouldShowToolbarBadge's bottom half. 
consolidate?

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:192:
public void onMenuItemClicked(ChromeActivity activity) {
early exit to avoid indentation?

if (mUpdateUrl == null) return;

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:218:
public void onAppMenuDismissed() {
maybe onMenuDismissed for consistency with the calling function?

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:238:
* Gets a boolean Finch parameter, assuming the <paramName>="true" format.  Also
checks for a
Don't know if we're supposed to be using "Finch".  Can you find the public name?

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarPhone.java
(right):

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarPhone.java:385:
public void showAppMenuBadge() {
showAppMenuUpdateBadge?

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/toolbar/Toolbar.java
(right):

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/Toolbar.java:49: *
Show the update badge on the app menu button.
* Show the update badge on the app menu button.
* Will have no effect if there is no app menu button for the current Activity?

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java
(right):

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java:99:
mMenuButtonWrapper  = findViewById(R.id.menu_btn_wrapper);
nit: one space

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java
(right):

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:992:
mMenuBadge.setImageAlpha(previousMenuBadgeAlpha);
nit: rearrange for clarity?

int previousVisibility = mMenuBadge.getVisibility();
int previousMenuBadgeAlpha = mMenuBadge.getImageAlpha();

mMenuBadge.setVisibility(View.VISIBLE);
mMenuBadge.setImageAlpha(rgbAlpha);
drawChild(canvas, mMenuBadge, SystemClock.uptimeMillis());
mMenuBadge.setVisibility(previousVisibility);
mMenuBadge.setImageAlpha(previousMenuBadgeAlpha);

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:1992:
mMenuBadge.setVisibility(View.INVISIBLE);
View.GONE?

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:2000:
protected View getMenuButton() {
getMenuButtonWrapper()?  Will get confusing because there's also an mMenuButton.

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/java/strings/android_chrome_strings.grd:1759: <message
name="IDS_MENU_UPDATE" desc="">
You need the descriptions; translators use that to deal with nuance.

https://chromiumcodereview.appspot.com/1505913003/diff/20001/testing/variatio...
File testing/variations/fieldtrial_testing_config_android.json (right):

https://chromiumcodereview.appspot.com/1505913003/diff/20001/testing/variatio...
testing/variations/fieldtrial_testing_config_android.json:323: "UpateMenuItem":
[
*UpdateMenuItem

https://chromiumcodereview.appspot.com/1505913003/diff/20001/tools/metrics/hi...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/1505913003/diff/20001/tools/metrics/hi...
tools/metrics/histograms/histograms.xml:65376: +  <int value="2" label="Not
clicked"/>
nit: Feels like "Not clicked" should be 0.

[gone, 2015-12-08 22:27:54]

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
File
chrome/android/javatests/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelperTest.java
(right):

https://chromiumcodereview.appspot.com/1505913003/diff/20001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelperTest.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
Guessing you'll delete the OmahaUpdateInfoBarTest after this?

[Theresa, 2015-12-10 03:53:19]

Still working on some bugs in UpdateMenuItemHelper related to switching the
drawable for the app menu button. Also need to address the other TODO note about
adjusting for urlExpansionPercentage. Will uploading a new patch set sometime
tomorrow.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout-sw600dp/toolbar.xml (right):

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout-sw600dp/toolbar.xml:52: />
On 2015/12/08 22:24:23, dfalcantara wrote:
> nit: should go on the same line as the last attribute

Done.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout-sw600dp/toolbar.xml:58: <FrameLayout
On 2015/12/08 22:24:23, dfalcantara wrote:
> 1) Does it make sense to pull this FrameLayout + children thing out into its
own
> Layout file, then include it from the three locations I see so far?
> 
> 2) nit: could you add some newlines to break up this monolithic block of
stuff? 
> The attributes are indented at the same level as the children and makes it
hard
> to parse.

1. All of the menu_buttons have different layout_width and padding so extracting
and the FrameLayout for reuse between both toolbar.xml files and
location_bar.xml will be difficult, but I'm not totally opposed to doing that. I
did extract some of the menu_badge style into a styles.xml.

Another idea, suggested by newt@, is to make a new class that extends
TintedImageButton, override the onDraw method and draw the badge in the correct
spot (if it should be shown).

I'm most inclined to leave the code how it is since the badge is experimental
(and less work is better), but can easily be talked into changing it.

2. done (are there too many now?)

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout-sw600dp/toolbar.xml:61:
android:id="@+id/menu_btn_wrapper" >
On 2015/12/08 22:24:23, dfalcantara wrote:
> nit: menu_button_wrapper?  I think the btn_ designation is just for filenames,
> and the IDs above use forward_button / btn_forward as well.

Done.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout-sw600dp/toolbar.xml:77:
android:contentDescription="@string/accessibility_toolbar_update_badge"
On 2015/12/08 22:24:23, dfalcantara wrote:
> I don't know if it makes sense to apply the accessibility description to the
> tiny badge; does it make sense to apply a common accessibility string to the
> FrameLayout instead?  One that changes depending on whether an update is
> available?

I don't think views inherit contentDescriptions from their parent (if I move the
contentDescription from the TintedImageButton to the FrameLayout I get "Button
28, unlabeled"). I could add code to set it on the menu_button, but I don't
think that's necessary.

I mostly added a content description to the badge because lint complained that I
didn't have one. I don't think the accessibility string needs to change when the
menu is badged, so I made the badge non-focusable, labeled it as not important
for accessibility and set the contentDescription to null. A tap on the menu
button reads "More options button", which is accurate (and concise) whether a
badge is present or not.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout/update_menu_item.xml (right):

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/update_menu_item.xml:12:
android:orientation="horizontal" >
On 2015/12/08 22:24:23, dfalcantara wrote:
> ----------------------------------------------
> Newlines.
> 
> <Woody looking awed>  <Buzz, arm outstretched>
> 
> Newlines everywhere.
> ----------------------------------------------
> 
> Seriously, though: newlines between children?
> 

Done.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/update_menu_item.xml:13: <LinearLayout
On 2015/12/08 22:24:23, dfalcantara wrote:
> nit: indentation's wonky.

Done.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/update_menu_item.xml:23:
android:textColor="#DC5554"
On 2015/12/08 22:24:23, dfalcantara wrote:
> I think convention is to use lowercase instead of caps for hex colors.  Also,
> move the layout_* params right below the ID.

Done.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/update_menu_item.xml:34: <view
On 2015/12/08 22:24:23, dfalcantara wrote:
> Can't you just use
> <org.chromium.chrome.browser.appmenu.AppMenUItemIcon
>     etc
>     />

Done. Here and in menu_item.xml

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/res...
File chrome/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/res...
chrome/android/java/res/values/dimens.xml:59: <dimen
name="menu_badge_background_size">16dp</dimen>
On 2015/12/08 17:59:37, Theresa Wellington wrote:
> The icon size w/out the border is 16x16; the background size should change to
> 17x17.

Done.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:378: if
(UpdateMenuItemHelper.getInstance().shouldShowMenuItem(activity)
On 2015/12/08 22:24:23, dfalcantara wrote:
> does it make more sense to record whether the UpdateMenuItemHelper thinks the
> MenuItem is visible?  that would prevent the logic from drifting apart,
> especially because UpdateMenuItemHelper doesn't know what overview mode is
(and
> I'm not sure it's checked before showing the menu item in the AppMenuAdapter).

AppMenuPropertiesDelegate is doing the checking for whether or not the update
menu item should be visible. It knows about whether the regular menu or the
overview menu is showing in prepareMenu.

I only want to log when the menu item is visible (this is for click-through
rate), so I would prefer not to log whether the UpdateMenuItemHelper things the
MenuItem is visible (because that class doesn't know about overview mode and
doesn't really need to). I changed it to actually check the menu for visibility.
Does that work?

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

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:53:

On 2015/12/08 22:24:23, dfalcantara wrote:
> nit: either remove the newline here or add newlines between all of the above
> MENU_ITEM values.

Done.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:148:
if (convertView == null) {
On 2015/12/08 22:24:23, dfalcantara wrote:
> if (!(convertView instanceof CustomMenuItemViewHolder)) maybe?  handles the
null
> check and avoids incorrectly casting something.

Added to the check here and to the other cases. The null check is still needed
since convertView.getTag() holds the class.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java
(right):

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java:716:
}
On 2015/12/08 22:24:23, dfalcantara wrote:
> Is this bit necessary?  DocumentActivity is a ChromeActivity and goes through
> onDeferredStartup, etc.

I added this because I was seeing the first DocumentActivity tab get badged, but
subsequent ones not getting badged. I changed the logic in
checkForUpdateOnBackgroundThread to call
activity.onCheckForUpdate(mUpdateAvaible) before returning if
mAlreadyCheckedForUpdates is true, which makes this code unneccessary (removed).

The slight down side to removing this line is it takes slightly longer for the
badge to show up, but I think it makes the code cleaner so I'm okay with that.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java
(right):

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:30:
private static final String TAG = "OmahaUpdateMenuItem";
On 2015/12/08 22:24:23, dfalcantara wrote:
> should be UpdateMenuItemHelper

Done.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:32:
// Command line flag for testing.
On 2015/12/08 22:24:24, dfalcantara wrote:
> Move to ChromeSwitches.java

Done.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:37:
private static final String FIELD_TRIAL_NAME = "UpateMenuItem";
On 2015/12/08 22:24:24, dfalcantara wrote:
> Is "Upate" intentional?  Saw it in the histogram too.

Nope. Fixed here and in fieldtrial_testing_config_android.json; double checked
the histograms and I think they are all correct.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:55:
// Whether OmahaClient has already been checked for an update., and volunteer my
time
On 2015/12/08 22:24:24, dfalcantara wrote:
> /methinks your email window focus was incorrect
Definitely. How embarrassing - should have done another pass on this not at
9:30p, sorry!

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:71:
public static synchronized UpdateMenuItemHelper getInstance() {
On 2015/12/08 22:24:24, dfalcantara wrote:
> On 2015/12/08 17:59:37, Theresa Wellington wrote:
> > Fix this (findbugs error for making the method synchronized)
> 
> Any reason why you're using a singleton?  Can it just be a member of the
> ChromeApplication?

Since this class is used in a lot of places that don't have a direct link to the
activity (and therefore a way to call getChromeApplication()), I prefer a
singleton to a bunch of calls to context.getApplicationContext() cast to
ChromeApplication. This isn't a strong preference, so I'm okay with moving it to
ChromeApplication if that makes more sense.

For background, aurimas@ has a TODO comment in ChromeApplication to replace
mUpdateInfoBarHelper w/ a static instance, so I figured I'd follow suit. It got
added during upstreaming/merging (I think because newt@ said to delete or inline
the function (probably because it's only used in ChromeActivity).

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:83:
* Checks if the OmahaClient knows about an update.
On 2015/12/08 22:24:24, dfalcantara wrote:
> {@link OmahaClient}

Done.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:87:
if (getBooleanParam(ENABLE_UPDATE_MENU_ITEM)) {
On 2015/12/08 22:24:24, dfalcantara wrote:
> early exit?
> 
> if (!getBooleanParam(ENABLE_UPDATE_MENU_ITEM) || mAlreadyCheckedForUpdates)
> return;

Done (mostly). The check for mAlreadyCheckedForUpdates is later.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:111:
recordUpdateHistogram();
On 2015/12/08 22:24:23, dfalcantara wrote:
> should recordUpdateHistogram() be the thing checking if
> getClickedUpdateMenuItem() was called?  this is (mostly) duplicated with the
> call below.

Done.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:142:
if (!mAlreadyCheckedForUpdates) {
On 2015/12/08 22:24:24, dfalcantara wrote:
> this last bit is exactly the same as shouldShowToolbarBadge's bottom half. 
> consolidate?

Done.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:192:
public void onMenuItemClicked(ChromeActivity activity) {
On 2015/12/08 22:24:24, dfalcantara wrote:
> early exit to avoid indentation?
> 
> if (mUpdateUrl == null) return;

Done.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:218:
public void onAppMenuDismissed() {
On 2015/12/08 22:24:24, dfalcantara wrote:
> maybe onMenuDismissed for consistency with the calling function?

Done.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:238:
* Gets a boolean Finch parameter, assuming the <paramName>="true" format.  Also
checks for a
On 2015/12/08 22:24:23, dfalcantara wrote:
> Don't know if we're supposed to be using "Finch".  Can you find the public
name?

I changed it to the class name (VariationsAssociatedData).

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

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarPhone.java:385:
public void showAppMenuBadge() {
On 2015/12/08 22:24:24, dfalcantara wrote:
> showAppMenuUpdateBadge?

Done.

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

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/Toolbar.java:49: *
Show the update badge on the app menu button.
On 2015/12/08 22:24:24, dfalcantara wrote:
> * Show the update badge on the app menu button.
> * Will have no effect if there is no app menu button for the current Activity?

Done.

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

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java:99:
mMenuButtonWrapper  = findViewById(R.id.menu_btn_wrapper);
On 2015/12/08 22:24:24, dfalcantara wrote:
> nit: one space

Done.

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

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:992:
mMenuBadge.setImageAlpha(previousMenuBadgeAlpha);
On 2015/12/08 22:24:24, dfalcantara wrote:
> nit: rearrange for clarity?
> 
> int previousVisibility = mMenuBadge.getVisibility();
> int previousMenuBadgeAlpha = mMenuBadge.getImageAlpha();
> 
> mMenuBadge.setVisibility(View.VISIBLE);
> mMenuBadge.setImageAlpha(rgbAlpha);
> drawChild(canvas, mMenuBadge, SystemClock.uptimeMillis());
> mMenuBadge.setVisibility(previousVisibility);
> mMenuBadge.setImageAlpha(previousMenuBadgeAlpha);

Done.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:1992:
mMenuBadge.setVisibility(View.INVISIBLE);
On 2015/12/08 22:24:24, dfalcantara wrote:
> View.GONE?

Done.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:2000:
protected View getMenuButton() {
On 2015/12/08 22:24:24, dfalcantara wrote:
> getMenuButtonWrapper()?  Will get confusing because there's also an
mMenuButton.

Done. Changed the logic in ToolbarLayout and deleted this method. All the
classes that call getMenuButton are changing the visibility, which needs to be
done on both the menu button and badge.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:1759: <message
name="IDS_MENU_UPDATE" desc="">
On 2015/12/08 22:24:24, dfalcantara wrote:
> You need the descriptions; translators use that to deal with nuance.

Done.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelperTest.java
(right):

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelperTest.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2015/12/08 22:27:54, dfalcantara wrote:
> Guessing you'll delete the OmahaUpdateInfoBarTest after this?

Yes.

https://codereview.chromium.org/1505913003/diff/20001/testing/variations/fiel...
File testing/variations/fieldtrial_testing_config_android.json (right):

https://codereview.chromium.org/1505913003/diff/20001/testing/variations/fiel...
testing/variations/fieldtrial_testing_config_android.json:323: "UpateMenuItem":
[
On 2015/12/08 22:24:24, dfalcantara wrote:
> *UpdateMenuItem

Done.

https://codereview.chromium.org/1505913003/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1505913003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:65376: +  <int value="2" label="Not
clicked"/>
On 2015/12/08 22:24:25, dfalcantara wrote:
> nit: Feels like "Not clicked" should be 0.

Done.

[Theresa, 2015-12-10 20:40:47]

ptal - Fixed or responded to all review comments. Still have one TODO note in
ToolbarPhone.java to address.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java
(right):

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:87:
if (getBooleanParam(ENABLE_UPDATE_MENU_ITEM)) {
On 2015/12/10 03:53:18, Theresa Wellington wrote:
> On 2015/12/08 22:24:24, dfalcantara wrote:
> > early exit?
> > 
> > if (!getBooleanParam(ENABLE_UPDATE_MENU_ITEM) || mAlreadyCheckedForUpdates)
> > return;
> 
> Done (mostly). The check for mAlreadyCheckedForUpdates is later.

... is later because we still need to call activity.onCheckForUpdate() to
facilitate more than one DocumentActivity (see response to comment on that
class).

[Theresa, 2015-12-10 21:22:53]

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

https://codereview.chromium.org/1505913003/diff/80001/chrome/android/java/res...
chrome/android/java/res/values-v17/styles.xml:480: <item
name="android:visibility">invisible</item>
I changed this from gone to invisible so that the view gets measured and the tab
switcher animation looks correct the first time it runs. I added another method
to Toolbar to set the visibility to gone that can get called once we know the
badge won't show so that it doesn't take up cycles during layout.

I'm not particularly happy with this solution, so I'll try to think of a better
way of getting the view to measure itself when it needs to.

[gone, 2015-12-10 21:45:12]

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout-sw600dp/toolbar.xml (right):

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout-sw600dp/toolbar.xml:58: <FrameLayout
On 2015/12/10 03:53:17, Theresa Wellington wrote:
> On 2015/12/08 22:24:23, dfalcantara wrote:
> > 1) Does it make sense to pull this FrameLayout + children thing out into its
> own
> > Layout file, then include it from the three locations I see so far?
> > 
> > 2) nit: could you add some newlines to break up this monolithic block of
> stuff? 
> > The attributes are indented at the same level as the children and makes it
> hard
> > to parse.
> 
> 1. All of the menu_buttons have different layout_width and padding so
extracting
> and the FrameLayout for reuse between both toolbar.xml files and
> location_bar.xml will be difficult, but I'm not totally opposed to doing that.
I
> did extract some of the menu_badge style into a styles.xml.
> 
> Another idea, suggested by newt@, is to make a new class that extends
> TintedImageButton, override the onDraw method and draw the badge in the
correct
> spot (if it should be shown).
> 
> I'm most inclined to leave the code how it is since the badge is experimental
> (and less work is better), but can easily be talked into changing it.
> 
> 2. done (are there too many now?)

Looks good.  If this becomes a permanent thing I guess we could revisit.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout-sw600dp/toolbar.xml:77:
android:contentDescription="@string/accessibility_toolbar_update_badge"
On 2015/12/10 03:53:17, Theresa Wellington wrote:
> On 2015/12/08 22:24:23, dfalcantara wrote:
> > I don't know if it makes sense to apply the accessibility description to the
> > tiny badge; does it make sense to apply a common accessibility string to the
> > FrameLayout instead?  One that changes depending on whether an update is
> > available?
> 
> I don't think views inherit contentDescriptions from their parent (if I move
the
> contentDescription from the TintedImageButton to the FrameLayout I get "Button
> 28, unlabeled"). I could add code to set it on the menu_button, but I don't
> think that's necessary.
> 
> I mostly added a content description to the badge because lint complained that
I
> didn't have one. I don't think the accessibility string needs to change when
the
> menu is badged, so I made the badge non-focusable, labeled it as not important
> for accessibility and set the contentDescription to null. A tap on the menu
> button reads "More options button", which is accurate (and concise) whether a
> badge is present or not.

You might be able to apply a @null contentDescription, or define the ImageView
as being unimportant for accessibility:
http://developer.android.com/reference/android/view/View.html#attr_android:im...

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:378: if
(UpdateMenuItemHelper.getInstance().shouldShowMenuItem(activity)
On 2015/12/10 03:53:17, Theresa Wellington wrote:
> On 2015/12/08 22:24:23, dfalcantara wrote:
> > does it make more sense to record whether the UpdateMenuItemHelper thinks
the
> > MenuItem is visible?  that would prevent the logic from drifting apart,
> > especially because UpdateMenuItemHelper doesn't know what overview mode is
> (and
> > I'm not sure it's checked before showing the menu item in the
AppMenuAdapter).
> 
> AppMenuPropertiesDelegate is doing the checking for whether or not the update
> menu item should be visible. It knows about whether the regular menu or the
> overview menu is showing in prepareMenu.
> 
> I only want to log when the menu item is visible (this is for click-through
> rate), so I would prefer not to log whether the UpdateMenuItemHelper things
the
> MenuItem is visible (because that class doesn't know about overview mode and
> doesn't really need to). I changed it to actually check the menu for
visibility.
> Does that work?

Yeah, this is actually what I was hoping you'd do.  I think I suggested it in
some other part of the review and then forgot about this instance...

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java
(right):

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:55:
// Whether OmahaClient has already been checked for an update., and volunteer my
time
On 2015/12/10 03:53:18, Theresa Wellington wrote:
> On 2015/12/08 22:24:24, dfalcantara wrote:
> > /methinks your email window focus was incorrect
> Definitely. How embarrassing - should have done another pass on this not at
> 9:30p, sorry!

I've sent far worse ;)  Yours gives off a benevolent vibe!

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:71:
public static synchronized UpdateMenuItemHelper getInstance() {
On 2015/12/10 03:53:18, Theresa Wellington wrote:
> On 2015/12/08 22:24:24, dfalcantara wrote:
> > On 2015/12/08 17:59:37, Theresa Wellington wrote:
> > > Fix this (findbugs error for making the method synchronized)
> > 
> > Any reason why you're using a singleton?  Can it just be a member of the
> > ChromeApplication?
> 
> Since this class is used in a lot of places that don't have a direct link to
the
> activity (and therefore a way to call getChromeApplication()), I prefer a
> singleton to a bunch of calls to context.getApplicationContext() cast to
> ChromeApplication. This isn't a strong preference, so I'm okay with moving it
to
> ChromeApplication if that makes more sense.
> 
> For background, aurimas@ has a TODO comment in ChromeApplication to replace
> mUpdateInfoBarHelper w/ a static instance, so I figured I'd follow suit. It
got
> added during upstreaming/merging (I think because newt@ said to delete or
inline
> the function (probably because it's only used in ChromeActivity).

Acknowledged.

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

https://codereview.chromium.org/1505913003/diff/80001/chrome/android/java/res...
chrome/android/java/res/values-v17/styles.xml:480: <item
name="android:visibility">invisible</item>
On 2015/12/10 21:22:53, Theresa Wellington wrote:
> I changed this from gone to invisible so that the view gets measured and the
tab
> switcher animation looks correct the first time it runs. I added another
method
> to Toolbar to set the visibility to gone that can get called once we know the
> badge won't show so that it doesn't take up cycles during layout.
> 
> I'm not particularly happy with this solution, so I'll try to think of a
better
> way of getting the view to measure itself when it needs to.

Acknowledged.

https://codereview.chromium.org/1505913003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java
(right):

https://codereview.chromium.org/1505913003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:40:
private static final String TAG = "UpdateMenuItemHelper";
nit: private static finals go above private statics

https://codereview.chromium.org/1505913003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:81:
synchronized (UpdateMenuItemHelper.sCreateInstanceLock) {
you should put the synchronized on the outside of the conditional; this will
create it twice if two threads somehow reach the synchronized block.

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

https://codereview.chromium.org/1505913003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:1995:
mUnbadgedMenuButtonDrawable = mMenuButton.getDrawable();
any chance the drawable will be incorrect after you call
mMenuButton.setImageBitmap?  what happens if the site has a theme color and the
theme switches from a light menu button to a dark one?

https://codereview.chromium.org/1505913003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:2021:
if (shouldShowMenuButton()) {
nit: early exit instead of indenting?

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

https://codereview.chromium.org/1505913003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java:45:
private boolean mInTabSwitcherMode = false;
nit: mIsInTabSwitcherMode?

https://codereview.chromium.org/1505913003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java:430:
UpdateMenuItemHelper.getInstance().getBadgedMenuButtonBitmap(getContext()));
Does it make sense to pull out this duped code, or have setTabSwitcherMode()
call this?

[Theresa, 2015-12-11 19:44:46]

I think everything is handled except my TODO note in ToolbarPhone.java (will ask
someone more knowledgeable about that class about it today or Monday then fix
it).

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout-sw600dp/toolbar.xml (right):

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout-sw600dp/toolbar.xml:58: <FrameLayout
On 2015/12/10 21:45:07, dfalcantara wrote:
> On 2015/12/10 03:53:17, Theresa Wellington wrote:
> > On 2015/12/08 22:24:23, dfalcantara wrote:
> > > 1) Does it make sense to pull this FrameLayout + children thing out into
its
> > own
> > > Layout file, then include it from the three locations I see so far?
> > > 
> > > 2) nit: could you add some newlines to break up this monolithic block of
> > stuff? 
> > > The attributes are indented at the same level as the children and makes it
> > hard
> > > to parse.
> > 
> > 1. All of the menu_buttons have different layout_width and padding so
> extracting
> > and the FrameLayout for reuse between both toolbar.xml files and
> > location_bar.xml will be difficult, but I'm not totally opposed to doing
that.
> I
> > did extract some of the menu_badge style into a styles.xml.
> > 
> > Another idea, suggested by newt@, is to make a new class that extends
> > TintedImageButton, override the onDraw method and draw the badge in the
> correct
> > spot (if it should be shown).
> > 
> > I'm most inclined to leave the code how it is since the badge is
experimental
> > (and less work is better), but can easily be talked into changing it.
> > 
> > 2. done (are there too many now?)
> 
> Looks good.  If this becomes a permanent thing I guess we could revisit.

Acknowledged.

https://codereview.chromium.org/1505913003/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout-sw600dp/toolbar.xml:77:
android:contentDescription="@string/accessibility_toolbar_update_badge"
On 2015/12/10 21:45:07, dfalcantara wrote:
> On 2015/12/10 03:53:17, Theresa Wellington wrote:
> > On 2015/12/08 22:24:23, dfalcantara wrote:
> > > I don't know if it makes sense to apply the accessibility description to
the
> > > tiny badge; does it make sense to apply a common accessibility string to
the
> > > FrameLayout instead?  One that changes depending on whether an update is
> > > available?
> > 
> > I don't think views inherit contentDescriptions from their parent (if I move
> the
> > contentDescription from the TintedImageButton to the FrameLayout I get
"Button
> > 28, unlabeled"). I could add code to set it on the menu_button, but I don't
> > think that's necessary.
> > 
> > I mostly added a content description to the badge because lint complained
that
> I
> > didn't have one. I don't think the accessibility string needs to change when
> the
> > menu is badged, so I made the badge non-focusable, labeled it as not
important
> > for accessibility and set the contentDescription to null. A tap on the menu
> > button reads "More options button", which is accurate (and concise) whether
a
> > badge is present or not.
> 
> You might be able to apply a @null contentDescription, or define the ImageView
> as being unimportant for accessibility:
>
http://developer.android.com/reference/android/view/View.html#attr_android:im...

I set the contentDescription to null and defined it as being unimportant for
accessibility (in styles.xml).

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

https://codereview.chromium.org/1505913003/diff/80001/chrome/android/java/res...
chrome/android/java/res/values-v17/styles.xml:480: <item
name="android:visibility">invisible</item>
On 2015/12/10 21:45:09, dfalcantara wrote:
> On 2015/12/10 21:22:53, Theresa Wellington wrote:
> > I changed this from gone to invisible so that the view gets measured and the
> tab
> > switcher animation looks correct the first time it runs. I added another
> method
> > to Toolbar to set the visibility to gone that can get called once we know
the
> > badge won't show so that it doesn't take up cycles during layout.
> > 
> > I'm not particularly happy with this solution, so I'll try to think of a
> better
> > way of getting the view to measure itself when it needs to.
> 
> Acknowledged.

I discussed this w/ newt@ offline and I think making this invisible is the best
approach. The alternative is manually calling mMenuBadge.onMeasure() or
mMenuBadge.onLayout().

https://codereview.chromium.org/1505913003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java
(right):

https://codereview.chromium.org/1505913003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:40:
private static final String TAG = "UpdateMenuItemHelper";
On 2015/12/10 21:45:09, dfalcantara wrote:
> nit: private static finals go above private statics

Done.

https://codereview.chromium.org/1505913003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java:81:
synchronized (UpdateMenuItemHelper.sCreateInstanceLock) {
On 2015/12/10 21:45:10, dfalcantara wrote:
> you should put the synchronized on the outside of the conditional; this will
> create it twice if two threads somehow reach the synchronized block.

Done.

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

https://codereview.chromium.org/1505913003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:1995:
mUnbadgedMenuButtonDrawable = mMenuButton.getDrawable();
On 2015/12/10 21:45:11, dfalcantara wrote:
> any chance the drawable will be incorrect after you call
> mMenuButton.setImageBitmap?  what happens if the site has a theme color and
the
> theme switches from a light menu button to a dark one?

Yes, it could be incorrect if showappMenuUpdateBadge is called multiple times. I
changed it to ApiCompatibilityUtils.getDrawable(getResources(),
R.drawable.btn_menu);

The tinting still works correctly. In TintedImageButton#drawableStateChanged()
we call updateTintColor() to set a color filter using the button's
ColorStateList.

https://codereview.chromium.org/1505913003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:2021:
if (shouldShowMenuButton()) {
On 2015/12/10 21:45:11, dfalcantara wrote:
> nit: early exit instead of indenting?

Done.

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

https://codereview.chromium.org/1505913003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java:45:
private boolean mInTabSwitcherMode = false;
On 2015/12/10 21:45:11, dfalcantara wrote:
> nit: mIsInTabSwitcherMode?

Done.

https://codereview.chromium.org/1505913003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java:430:
UpdateMenuItemHelper.getInstance().getBadgedMenuButtonBitmap(getContext()));
On 2015/12/10 21:45:11, dfalcantara wrote:
> Does it make sense to pull out this duped code, or have setTabSwitcherMode()
> call this?

Done. Pulled out into a method in ToolbarLayout that ToolbarPhone calls as well.

[gone, 2015-12-11 19:54:59]

lgtm

https://codereview.chromium.org/1505913003/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java
(right):

https://codereview.chromium.org/1505913003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java:420:
displayAppMenuUpdateBadge();
not a fan of having a showAppMenuUpdateBadge and a displayAppMenuUpdateBadge,
but I don't have any better names off the top of my head :/

[Theresa, 2015-12-15 00:35:11]

twellington@chromium.org changed reviewers:
+ anthonyvd@chromium.org, holte@chromium.org

[Theresa, 2015-12-15 00:35:14]

dfalcantara - ptal at minor changes to ToolbarPhone, UpdateMenuItemHelper and
ChromeSwitches (since patch set 8)

+anthonyvd - for profile.cc changes

+holte - for fieldtrial_testing_config_android.json & histograms.xml

https://codereview.chromium.org/1505913003/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java
(right):

https://codereview.chromium.org/1505913003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java:420:
displayAppMenuUpdateBadge();
On 2015/12/11 19:54:59, dfalcantara wrote:
> not a fan of having a showAppMenuUpdateBadge and a displayAppMenuUpdateBadge,
> but I don't have any better names off the top of my head :/

Acknowledged.

[gone, 2015-12-15 01:16:46]

Still lgtm.  Would still like a new name for that function, though.

https://codereview.chromium.org/1505913003/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java
(right):

https://codereview.chromium.org/1505913003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java:420:
displayAppMenuUpdateBadge();
On 2015/12/15 00:35:13, Theresa Wellington wrote:
> On 2015/12/11 19:54:59, dfalcantara wrote:
> > not a fan of having a showAppMenuUpdateBadge and a
displayAppMenuUpdateBadge,
> > but I don't have any better names off the top of my head :/
> 
> Acknowledged.

paintAppMenuUpdateBadge()?  prepareAppMenuUpdateBadge()?

https://codereview.chromium.org/1505913003/diff/220001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java
(right):

https://codereview.chromium.org/1505913003/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:102: *
Forces the update menu bade to show.
nit: badge

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

https://codereview.chromium.org/1505913003/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:986:
// Draw the menu badge if necessary.
does this not require a canvas.save()/canvas.restore() pairing?

[anthonyvd, 2015-12-15 14:43:22]

c/b/profiles/profile.cc lgtm % a little nit.

https://codereview.chromium.org/1505913003/diff/220001/chrome/browser/profile...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/1505913003/diff/220001/chrome/browser/profile...
chrome/browser/profiles/profile.cc:151: registry->RegisterBooleanPref(
nit: I think this fits on a single line.

[Theresa, 2015-12-15 18:10:36]

https://codereview.chromium.org/1505913003/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java
(right):

https://codereview.chromium.org/1505913003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java:420:
displayAppMenuUpdateBadge();
On 2015/12/15 01:16:46, dfalcantara wrote:
> On 2015/12/15 00:35:13, Theresa Wellington wrote:
> > On 2015/12/11 19:54:59, dfalcantara wrote:
> > > not a fan of having a showAppMenuUpdateBadge and a
> displayAppMenuUpdateBadge,
> > > but I don't have any better names off the top of my head :/
> > 
> > Acknowledged.
> 
> paintAppMenuUpdateBadge()?  prepareAppMenuUpdateBadge()?

How about setAppMenuUpdateBadgeToVisible?

https://codereview.chromium.org/1505913003/diff/220001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java
(right):

https://codereview.chromium.org/1505913003/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:102: *
Forces the update menu bade to show.
On 2015/12/15 01:16:46, dfalcantara wrote:
> nit: badge

Done.

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

https://codereview.chromium.org/1505913003/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:986:
// Draw the menu badge if necessary.
On 2015/12/15 01:16:46, dfalcantara wrote:
> does this not require a canvas.save()/canvas.restore() pairing?

No, it does not. Similar to how the mTabSwitcherAnimationMenuDrawable didn't
need it before because it's the last thing being drawn. The save on line 948 &
restore on line 994 ensure the canvas gets back to it's original untranslated
state.

https://codereview.chromium.org/1505913003/diff/220001/chrome/browser/profile...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/1505913003/diff/220001/chrome/browser/profile...
chrome/browser/profiles/profile.cc:151: registry->RegisterBooleanPref(
On 2015/12/15 14:43:22, anthonyvd wrote:
> nit: I think this fits on a single line.

Done.

[gone, 2015-12-15 18:23:22]

https://codereview.chromium.org/1505913003/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java
(right):

https://codereview.chromium.org/1505913003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java:420:
displayAppMenuUpdateBadge();
On 2015/12/15 18:10:36, Theresa Wellington wrote:
> On 2015/12/15 01:16:46, dfalcantara wrote:
> > On 2015/12/15 00:35:13, Theresa Wellington wrote:
> > > On 2015/12/11 19:54:59, dfalcantara wrote:
> > > > not a fan of having a showAppMenuUpdateBadge and a
> > displayAppMenuUpdateBadge,
> > > > but I don't have any better names off the top of my head :/
> > > 
> > > Acknowledged.
> > 
> > paintAppMenuUpdateBadge()?  prepareAppMenuUpdateBadge()?
> 
> How about setAppMenuUpdateBadgeToVisible?

Cool with that... just don't want two different functions that are different
only based on one synonym.  See Tab#loadIfNeeded and Tab#loadIfNecessary.  Ugh.

[Steven Holte, 2015-12-16 20:18:36]

histograms.xml + field_trial_testing_config lgtm

https://codereview.chromium.org/1505913003/diff/260001/testing/variations/fie...
File testing/variations/fieldtrial_testing_config_android.json (right):

https://codereview.chromium.org/1505913003/diff/260001/testing/variations/fie...
testing/variations/fieldtrial_testing_config_android.json:370: }
You might want to add your Control/Default group here, just for completeness. 
Note that right now only the first group listed here will be tested, so pick the
one you most want testing on.  We may start testing other groups in an FYI
build.

[Theresa, 2015-12-16 22:35:22]

https://codereview.chromium.org/1505913003/diff/260001/testing/variations/fie...
File testing/variations/fieldtrial_testing_config_android.json (right):

https://codereview.chromium.org/1505913003/diff/260001/testing/variations/fie...
testing/variations/fieldtrial_testing_config_android.json:370: }
On 2015/12/16 20:18:36, Steven Holte wrote:
> You might want to add your Control/Default group here, just for completeness. 
> Note that right now only the first group listed here will be tested, so pick
the
> one you most want testing on.  We may start testing other groups in an FYI
> build.

Done.

[Theresa, 2015-12-16 22:35:27]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2015-12-16 22:35:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, anthonyvd@chromium.org, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/1505913003/#ps280001
(title: "Rearrange configs in fieldtrial_testing.. and add Default group")

[commit-bot: I haz the power, 2015-12-16 22:37:05]

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

[Theresa, 2015-12-16 22:39:56]

The CQ bit was unchecked by twellington@chromium.org

[Theresa, 2015-12-16 23:04:44]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2015-12-16 23:04:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, holte@chromium.org, anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/1505913003/#ps300001
(title: "Rearrange field_trial.. again to facilitate command line testing")

[commit-bot: I haz the power, 2015-12-16 23:06:11]

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

[commit-bot: I haz the power, 2015-12-17 03:13:06]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-12-17 03:14:02]

Description was changed from

==========
Add update menu item and app menu icon badge

Adds an update menu item and badge, controlled by finch (or command line).

BUG=566085
==========

to

==========
Add update menu item and app menu icon badge

Adds an update menu item and badge, controlled by finch (or command line).

BUG=566085

Committed: https://crrev.com/018d2db49cda296c0932d7935ec9dbf1e3e3e40f
Cr-Commit-Position: refs/heads/master@{#365696}
==========

[commit-bot: I haz the power, 2015-12-17 03:14:05]

Patchset 16 (id:??) landed as
https://crrev.com/018d2db49cda296c0932d7935ec9dbf1e3e3e40f
Cr-Commit-Position: refs/heads/master@{#365696}