Tooltip for home page menu buttons

Tooltip for home page menu buttons

Before the change, tooltip is only available on 'open tab' button. 
Now a tooltip will show as a toast for every ImageButton in main menu 
on a user long click.

BUG=685459
R=mdjones@chromium.org

Review-Url: https://codereview.chromium.org/2891493004
Cr-Commit-Position: refs/heads/master@{#472618}
Committed: https://chromium.googlesource.com/chromium/src/+/47137024bf6d15127330336310406d3f27a3391a

[huayinz1, 2017-05-16 20:48:35]

huayinz@google.com changed reviewers:
+ huayinz@google.com

[huayinz1, 2017-05-16 20:52:16]

Description was changed from

==========
tooltip for home page menu buttons

BUG=685459
R=mdjones@chromium.org
==========

to

==========
tooltip for home page menu buttons

BUG=685459
R=mdjones@chromium.org
==========

[huayinz1, 2017-05-16 20:52:16]

huayinz@google.com changed reviewers:
- huayinz@google.com

[huayinz, 2017-05-16 21:18:27]

Patchset #1 (id:1) has been deleted

[huayinz, 2017-05-16 21:24:17]

Patchset #1 (id:20001) has been deleted

[mdjones, 2017-05-16 21:53:57]

Looks good for your first patch!

Please update the description to be more detailed. For example, include
something about changed behavior and how it is triggered.

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java
(right):

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:384: *
@param view The anchor view of the menu item
nit: punctuation.

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:387: if
(menuItem.isEnabled()) {
You can un-nest this by using an early return (in general this is considered
easier to read):

if (!menuItem.isEnabled()) return false;
...

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:388:
CharSequence description = null;
nit: "String" would work just as well here (same for the other uses of
CharSequence in this patch).

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:392:
description = context.getResources().getString(R.string.menu_forward);
Making a local var for resources might make this block easier to read (similarly
for the other blocks in this patch).

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

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:305:
return true;
Should this instead return the value provided by onItemLongClick?

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java
(left):

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:243:

nit: Re-add this new-line since there are no other changes in this file.

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/util/AccessibilityUtil.java
(right):

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/util/AccessibilityUtil.java:124:
* @param context the context to use for the toast
nit: "the" -> "The" and punctuation.

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

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:2309: <message
name="IDS_BUTTON_NEW_INCOGNITO_TAB" desc="Button for opening a new incognito
tab.">
These descriptions should reflect what the string is being used for. For
example: "The text inside the tooltip that shows when a user long-presses on the
'new incognito tab' button".

[huayinz, 2017-05-16 22:59:42]

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java
(right):

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:384: *
@param view The anchor view of the menu item
On 2017/05/16 21:53:57, mdjones wrote:
> nit: punctuation.

Done.

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:387: if
(menuItem.isEnabled()) {
On 2017/05/16 21:53:56, mdjones wrote:
> You can un-nest this by using an early return (in general this is considered
> easier to read):
> 
> if (!menuItem.isEnabled()) return false;
> ...

Done.

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:388:
CharSequence description = null;
On 2017/05/16 21:53:56, mdjones wrote:
> nit: "String" would work just as well here (same for the other uses of
> CharSequence in this patch).

Done.

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:392:
description = context.getResources().getString(R.string.menu_forward);
On 2017/05/16 21:53:56, mdjones wrote:
> Making a local var for resources might make this block easier to read
(similarly
> for the other blocks in this patch).

Done.

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

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:305:
return true;
On 2017/05/16 21:53:57, mdjones wrote:
> Should this instead return the value provided by onItemLongClick?

Done.

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java
(left):

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:243:

On 2017/05/16 21:53:57, mdjones wrote:
> nit: Re-add this new-line since there are no other changes in this file.

Done.

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/util/AccessibilityUtil.java
(right):

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/util/AccessibilityUtil.java:124:
* @param context the context to use for the toast
On 2017/05/16 21:53:57, mdjones wrote:
> nit: "the" -> "The" and punctuation.

Done.

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

https://codereview.chromium.org/2891493004/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:2309: <message
name="IDS_BUTTON_NEW_INCOGNITO_TAB" desc="Button for opening a new incognito
tab.">
On 2017/05/16 21:53:57, mdjones wrote:
> These descriptions should reflect what the string is being used for. For
> example: "The text inside the tooltip that shows when a user long-presses on
the
> 'new incognito tab' button".

Done.

[mdjones, 2017-05-16 23:26:31]

Don't forget to update the description of the CL.

https://codereview.chromium.org/2891493004/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java
(right):

https://codereview.chromium.org/2891493004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:392:
final String tooltip_forward =
context.getResources().getString(R.string.menu_forward);
I should have been more clear; I meant to suggest you simply do something like:

Resources resources = context.getResources();

Then you could do the following in your conditions:

resources.getString(R.string....);

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

https://codereview.chromium.org/2891493004/diff/60001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:2326: <message
name="IDS_MENU_FORWARD" desc="Tooltip on Android for the button to advance to
the next web page. Only enabled if the user has gone back to a previously opened
page. Verb. [CHAR-LIMIT=27]">
I hate to nit-pick on this, but now there is a little too much information. You
don't necessarily need to explain how it works and the fact that it is on
android since this file is called "android_*". Something like this is probably
fine for all of them:

"The text for the tooltip generated by long-pressing the x button [in the
overflow menu]."

[huayinz, 2017-05-17 00:21:42]

https://codereview.chromium.org/2891493004/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java
(right):

https://codereview.chromium.org/2891493004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:392:
final String tooltip_forward =
context.getResources().getString(R.string.menu_forward);
On 2017/05/16 23:26:31, mdjones wrote:
> I should have been more clear; I meant to suggest you simply do something
like:
> 
> Resources resources = context.getResources();
> 
> Then you could do the following in your conditions:
> 
> resources.getString(R.string....);

Haha okay.

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

https://codereview.chromium.org/2891493004/diff/60001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:2326: <message
name="IDS_MENU_FORWARD" desc="Tooltip on Android for the button to advance to
the next web page. Only enabled if the user has gone back to a previously opened
page. Verb. [CHAR-LIMIT=27]">
On 2017/05/16 23:26:31, mdjones wrote:
> I hate to nit-pick on this, but now there is a little too much information.
You
> don't necessarily need to explain how it works and the fact that it is on
> android since this file is called "android_*". Something like this is probably
> fine for all of them:
> 
> "The text for the tooltip generated by long-pressing the x button [in the
> overflow menu]."

The tooltip descriptions are written by UX designer srahim@. I believe she has
concerns on translating them correctly for localization but I'm not sure...

[mdjones, 2017-05-17 15:53:10]

See earlier comments about CL description before I approve.

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

https://codereview.chromium.org/2891493004/diff/60001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:2326: <message
name="IDS_MENU_FORWARD" desc="Tooltip on Android for the button to advance to
the next web page. Only enabled if the user has gone back to a previously opened
page. Verb. [CHAR-LIMIT=27]">
On 2017/05/17 00:21:42, huayinz wrote:
> On 2017/05/16 23:26:31, mdjones wrote:
> > I hate to nit-pick on this, but now there is a little too much information.
> You
> > don't necessarily need to explain how it works and the fact that it is on
> > android since this file is called "android_*". Something like this is
probably
> > fine for all of them:
> > 
> > "The text for the tooltip generated by long-pressing the x button [in the
> > overflow menu]."
> 
> The tooltip descriptions are written by UX designer srahim@. I believe she has
> concerns on translating them correctly for localization but I'm not sure...

I see; I didn't realize they were specified in the bug. The just seem really
weirdly worded.

[huayinz, 2017-05-17 19:31:42]

Patchset #4 (id:100001) has been deleted

[huayinz, 2017-05-17 19:43:06]

Description was changed from

==========
tooltip for home page menu buttons

BUG=685459
R=mdjones@chromium.org
==========

to

==========
tooltip for home page menu buttons

Added tooltips for ImageButtons in main menu items and 'New tab' button in tab
switcher. The tooltip will show up as a toast when user long clicks the button.


BUG=685459
R=mdjones@chromium.org
==========

[huayinz, 2017-05-17 19:48:46]

I did a sync to the branch and the updates from the sync show up in the cl
changes. I only edited the the string descriptions for the menu items. Sorry for
the inconvenience.

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

https://codereview.chromium.org/2891493004/diff/60001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:2326: <message
name="IDS_MENU_FORWARD" desc="Tooltip on Android for the button to advance to
the next web page. Only enabled if the user has gone back to a previously opened
page. Verb. [CHAR-LIMIT=27]">
On 2017/05/17 15:53:09, mdjones wrote:
> On 2017/05/17 00:21:42, huayinz wrote:
> > On 2017/05/16 23:26:31, mdjones wrote:
> > > I hate to nit-pick on this, but now there is a little too much
information.
> > You
> > > don't necessarily need to explain how it works and the fact that it is on
> > > android since this file is called "android_*". Something like this is
> probably
> > > fine for all of them:
> > > 
> > > "The text for the tooltip generated by long-pressing the x button [in the
> > > overflow menu]."
> > 
> > The tooltip descriptions are written by UX designer srahim@. I believe she
has
> > concerns on translating them correctly for localization but I'm not sure...
> 
> I see; I didn't realize they were specified in the bug. The just seem really
> weirdly worded.

I simplified them a little.

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

https://codereview.chromium.org/2891493004/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:522:
if (!isInTabSwitcherMode()) updateUrlExpansionAnimation();
This change shows up in cl after I sync the branch. Same for other changes in
this file.

[huayinz, 2017-05-17 21:39:12]

Description was changed from

==========
tooltip for home page menu buttons

Added tooltips for ImageButtons in main menu items and 'New tab' button in tab
switcher. The tooltip will show up as a toast when user long clicks the button.


BUG=685459
R=mdjones@chromium.org
==========

to

==========
tooltip for home page menu buttons

Before the change, tooltip is only available on 'open tab' button. 
Now a tooltip will show as a toast for every ImageButton in main menu 
on a user long click.

BUG=685459
R=mdjones@chromium.org
==========

[huayinz, 2017-05-17 22:10:17]

Description was changed from

==========
tooltip for home page menu buttons

Before the change, tooltip is only available on 'open tab' button. 
Now a tooltip will show as a toast for every ImageButton in main menu 
on a user long click.

BUG=685459
R=mdjones@chromium.org
==========

to

==========
Tooltip for home page menu buttons

Before the change, tooltip is only available on 'open tab' button. 
Now a tooltip will show as a toast for every ImageButton in main menu 
on a user long click.

BUG=685459
R=mdjones@chromium.org
==========

[mdjones, 2017-05-17 22:17:19]

lgtm!

[huayinz, 2017-05-17 22:30:28]

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

[huayinz, 2017-05-17 22:30:28]

tedchoc@chromium.org: Hi Ted, 

Could you please review the changes since you are one of the owners of these
files?

Thanks,
Becky

[Ted C, 2017-05-17 23:26:25]

lgtm

[huayinz, 2017-05-17 23:40:44]

The CQ bit was checked by huayinz@chromium.org

[commit-bot: I haz the power, 2017-05-17 23:41:56]

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-05-18 01:15:36]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495064444569260,
"parent_rev": "02582585906bf67195c50330ffef797fb2f7ce59", "commit_rev":
"47137024bf6d15127330336310406d3f27a3391a"}

[commit-bot: I haz the power, 2017-05-18 01:15:45]

Description was changed from

==========
Tooltip for home page menu buttons

Before the change, tooltip is only available on 'open tab' button. 
Now a tooltip will show as a toast for every ImageButton in main menu 
on a user long click.

BUG=685459
R=mdjones@chromium.org
==========

to

==========
Tooltip for home page menu buttons

Before the change, tooltip is only available on 'open tab' button. 
Now a tooltip will show as a toast for every ImageButton in main menu 
on a user long click.

BUG=685459
R=mdjones@chromium.org

Review-Url: https://codereview.chromium.org/2891493004
Cr-Commit-Position: refs/heads/master@{#472618}
Committed:
https://chromium.googlesource.com/chromium/src/+/47137024bf6d1512733033631040...
==========

[commit-bot: I haz the power, 2017-05-18 01:15:46]

Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/47137024bf6d1512733033631040...