Tooltip for 'Stop refreshing' menu item

Tooltip for 'Stop refreshing' menu item

Before, 'Refreshing' shows on long click 'Stop refreshing' menu item.
Now added tooltip 'Stop refreshing' on the menu item.

BUG=725140
Review-Url: https://codereview.chromium.org/2921463002
Cr-Commit-Position: refs/heads/master@{#476380}
Committed: https://chromium.googlesource.com/chromium/src/+/d4b5948ed8ff5d9b99b876998a5835ffc6ff42c8

[huayinz, 2017-05-31 19:37:36]

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

[huayinz, 2017-05-31 19:37:36]

Hi Matt,

Could you take a look please?

Thanks so much,
Becky

[mdjones, 2017-05-31 20:51:30]

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

https://codereview.chromium.org/2921463002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:405: }
else if (itemId == R.id.reload_menu_id) {
I'd rather assume the the clicked view with the provided id is an ImageButton.
In this case we can access the "level" of the button which changes based on
whether the "refresh" or "stop" image is showing. So this could be something
like:

if (((ImageButton) view).getLevel() == 0) {
  // show reload text.
} else {
  // show stop text.
}

see:
https://cs.chromium.org/chromium/src/chrome/android/java/res/drawable/btn_rel...

This way you don't have to do any plumbing with the activity or tab and you
don't have to worry about any logic discrepancies.

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

https://codereview.chromium.org/2921463002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuIconRowFooter.java:108:
} else if (itemId == R.id.reload_menu_id) {
In this case you already have access to mReloadButton.

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

https://codereview.chromium.org/2921463002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java:333:
if (v == mReloadButton) {
Same as above, mReloadButton is available to query.

[huayinz, 2017-05-31 22:54:38]

Patchset #2 (id:20001) has been deleted

[huayinz, 2017-05-31 23:03:56]

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

https://codereview.chromium.org/2921463002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:405: }
else if (itemId == R.id.reload_menu_id) {
On 2017/05/31 20:51:30, mdjones wrote:
> I'd rather assume the the clicked view with the provided id is an ImageButton.
> In this case we can access the "level" of the button which changes based on
> whether the "refresh" or "stop" image is showing. So this could be something
> like:
> 
> if (((ImageButton) view).getLevel() == 0) {
>   // show reload text.
> } else {
>   // show stop text.
> }
> 
> see:
>
https://cs.chromium.org/chromium/src/chrome/android/java/res/drawable/btn_rel...
> 
> This way you don't have to do any plumbing with the activity or tab and you
> don't have to worry about any logic discrepancies. 

Done.

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

https://codereview.chromium.org/2921463002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuIconRowFooter.java:108:
} else if (itemId == R.id.reload_menu_id) {
On 2017/05/31 20:51:30, mdjones wrote:
> In this case you already have access to mReloadButton.

Done.

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

https://codereview.chromium.org/2921463002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java:333:
if (v == mReloadButton) {
On 2017/05/31 20:51:30, mdjones wrote:
> Same as above, mReloadButton is available to query.

Done.

[mdjones, 2017-05-31 23:59:56]

lgtm % suggestion

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

https://codereview.chromium.org/2921463002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java:45:
private static final int RELOAD_BUTTON_LEVEL_STOP_RELOADING = 1;
I think we can expose the constants in AppMenuPropertiesDelegate. Though it
feels a bit weird referencing that class from here. Maybe we can move them to
somewhere like ToolbarLayout.java. Thoughts?

[huayinz, 2017-06-01 00:39:31]

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

[huayinz, 2017-06-01 00:39:31]

tedchoc@chromium.org:

Hi Ted,

Could you take a look?

Thanks,
Becky

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

https://codereview.chromium.org/2921463002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java:45:
private static final int RELOAD_BUTTON_LEVEL_STOP_RELOADING = 1;
On 2017/05/31 23:59:56, mdjones wrote:
> I think we can expose the constants in AppMenuPropertiesDelegate. Though it
> feels a bit weird referencing that class from here. Maybe we can move them to
> somewhere like ToolbarLayout.java. Thoughts?

Hm... I'm still pretty new to the structure of the code, but here is what I
think. I also feel it's weird referencing AppMenuPropertiesDelegate's constants
here, since the image buttons are not in app menu on tablet version. I'm okay
with moving them to ToolbarLayout. But it seems to me exposing const from
ToolbarLayout to AppMenu is not very straight forward either. If we are not
sharing these const b/w app menu in phone and toolbar in tablet, I guess they
are fine right now?

[Ted C, 2017-06-01 14:12:33]

lgtm

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

https://codereview.chromium.org/2921463002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java:45:
private static final int RELOAD_BUTTON_LEVEL_STOP_RELOADING = 1;
On 2017/06/01 00:39:31, huayinz wrote:
> On 2017/05/31 23:59:56, mdjones wrote:
> > I think we can expose the constants in AppMenuPropertiesDelegate. Though it
> > feels a bit weird referencing that class from here. Maybe we can move them
to
> > somewhere like ToolbarLayout.java. Thoughts?
> 
> Hm... I'm still pretty new to the structure of the code, but here is what I
> think. I also feel it's weird referencing AppMenuPropertiesDelegate's
constants
> here, since the image buttons are not in app menu on tablet version. I'm okay
> with moving them to ToolbarLayout. But it seems to me exposing const from
> ToolbarLayout to AppMenu is not very straight forward either. If we are not
> sharing these const b/w app menu in phone and toolbar in tablet, I guess they
> are fine right now?

Another option would be to change btn_reload_stop.xml.  Right now it hardcodes
0, 1 in the xml file, but we could introduce <integer> entries in values.xml and
then it would be something like maxLevel="@integer/reload_button_reload_level".

Then in these files it could get getLevel() ==
getResources().getInteger(R.integer.reload_button_reload_level);

I think that is probably the cleanest, but whether it is worth it is another
story
that i'll leave up to you.

[huayinz, 2017-06-01 17:59:44]

Hi,

I moved the reload button constants to values.xml. Could you take a quick look
if the changes are fine?

Thanks,
Becky

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

https://codereview.chromium.org/2921463002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java:45:
private static final int RELOAD_BUTTON_LEVEL_STOP_RELOADING = 1;
On 2017/06/01 14:12:33, Ted C wrote:
> On 2017/06/01 00:39:31, huayinz wrote:
> > On 2017/05/31 23:59:56, mdjones wrote:
> > > I think we can expose the constants in AppMenuPropertiesDelegate. Though
it
> > > feels a bit weird referencing that class from here. Maybe we can move them
> to
> > > somewhere like ToolbarLayout.java. Thoughts?
> > 
> > Hm... I'm still pretty new to the structure of the code, but here is what I
> > think. I also feel it's weird referencing AppMenuPropertiesDelegate's
> constants
> > here, since the image buttons are not in app menu on tablet version. I'm
okay
> > with moving them to ToolbarLayout. But it seems to me exposing const from
> > ToolbarLayout to AppMenu is not very straight forward either. If we are not
> > sharing these const b/w app menu in phone and toolbar in tablet, I guess
they
> > are fine right now?
> 
> Another option would be to change btn_reload_stop.xml.  Right now it hardcodes
> 0, 1 in the xml file, but we could introduce <integer> entries in values.xml
and
> then it would be something like
maxLevel="@integer/reload_button_reload_level".
> 
> Then in these files it could get getLevel() ==
> getResources().getInteger(R.integer.reload_button_reload_level);
> 
> I think that is probably the cleanest, but whether it is worth it is another
> story
> that i'll leave up to you.

Ahh, haven't thought of that. Thanks for the advice. This is pretty clean. I
feel it is worth to clean up the constants to values.xml for maintainability.

[Ted C, 2017-06-01 18:03:37]

lgtm

https://codereview.chromium.org/2921463002/diff/60001/chrome/android/java/res...
File chrome/android/java/res/drawable/btn_reload_stop.xml (right):

https://codereview.chromium.org/2921463002/diff/60001/chrome/android/java/res...
chrome/android/java/res/drawable/btn_reload_stop.xml:11:
android:maxLevel="@integer/reload_button_level_stop_reload"
nit, I'd drop the trailing _reload here

[huayinz, 2017-06-01 18:18:32]

The CQ bit was checked by huayinz@chromium.org

[huayinz, 2017-06-01 18:18:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2921463002/#ps80001
(title: "naming issue")

[commit-bot: I haz the power, 2017-06-01 18:19:15]

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-06-01 19:10:55]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496341112519870,
"parent_rev": "ece963bf6bf190b57620261673c6bc3d70f5ec97", "commit_rev":
"d4b5948ed8ff5d9b99b876998a5835ffc6ff42c8"}

[commit-bot: I haz the power, 2017-06-01 19:11:11]

Description was changed from

==========
Tooltip for 'Stop refreshing' menu item

Before, 'Refreshing' shows on long click 'Stop refreshing' menu item.
Now added tooltip 'Stop refreshing' on the menu item.

BUG=725140
==========

to

==========
Tooltip for 'Stop refreshing' menu item

Before, 'Refreshing' shows on long click 'Stop refreshing' menu item.
Now added tooltip 'Stop refreshing' on the menu item.

BUG=725140

Review-Url: https://codereview.chromium.org/2921463002
Cr-Commit-Position: refs/heads/master@{#476380}
Committed:
https://chromium.googlesource.com/chromium/src/+/d4b5948ed8ff5d9b99b876998a58...
==========

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

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/d4b5948ed8ff5d9b99b876998a58...