[Remoting Android] Navigation Menu Cleanups

[Remoting Android] Navigation Menu Cleanups

This CL:

* Removes the Credits item from the menu since it will be merged into Help's
  overflow menu.
* Refactors the item layout so that we only needs one XML and specifying icon
  and text in the code.

BUG=613604
Committed: https://crrev.com/84f1fd3a7766e37932e6e86b3f17e9260dee1344
Cr-Commit-Position: refs/heads/master@{#422590}

[Yuwei, 2016-09-30 20:47:09]

yuweih@chromium.org changed reviewers:
+ joedow@chromium.org, lambroslambrou@chromium.org

[Yuwei, 2016-09-30 20:47:10]

PTAL. Thanks!

https://codereview.chromium.org/2383283002/diff/1/remoting/android/java/res/m...
File remoting/android/java/res/menu/navigation_menu_item.xml (right):

https://codereview.chromium.org/2383283002/diff/1/remoting/android/java/res/m...
remoting/android/java/res/menu/navigation_menu_item.xml:1: <?xml version="1.0"
encoding="utf-8"?>
There is also a "navigation_list_item.xml" in java/res/layout and looks like it
is not being used and is suppressed for lint. Maybe we can reuse that file name?

[Lambros, 2016-10-03 21:10:04]

lgtm when comments are addressed.

https://codereview.chromium.org/2383283002/diff/1/remoting/android/java/res/m...
File remoting/android/java/res/menu/navigation_menu_item.xml (right):

https://codereview.chromium.org/2383283002/diff/1/remoting/android/java/res/m...
remoting/android/java/res/menu/navigation_menu_item.xml:1: <?xml version="1.0"
encoding="utf-8"?>
On 2016/09/30 20:47:10, Yuwei wrote:
> There is also a "navigation_list_item.xml" in java/res/layout and looks like
it
> is not being used and is suppressed for lint. Maybe we can reuse that file
name?

SGTM. Can do that here or in a separate CL.

https://codereview.chromium.org/2383283002/diff/1/remoting/android/java/src/o...
File
remoting/android/java/src/org/chromium/chromoting/NavigationMenuAdapter.java
(right):

https://codereview.chromium.org/2383283002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/NavigationMenuAdapter.java:78:
Drawable drawable = ContextCompat.getDrawable(activity, drawableId);
ContextCompat is fine, I think. If you run into any build issues with it, you
can use instead:
org.chromium.base.ApiCompatibilityUtils.getDrawable()

I'm not sure why Chrome has this, given that ContextCompat already does the same
job?

https://codereview.chromium.org/2383283002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/NavigationMenuAdapter.java:94:
TextView textView = (TextView) inflater.inflate(R.menu.navigation_menu_item,
null);
s/null/parent/ ?
I think we are meant to use the ViewGroup parent for layout inflation?
See https://possiblemobile.com/2013/05/layout-inflation-as-intended/

https://codereview.chromium.org/2383283002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/NavigationMenuAdapter.java:95:
textView.setCompoundDrawables(item.mIcon, null, null, null);
I don't know if this is legitimate?
I think this line, and the next line, need to be after the 'if' block? I think
Views can be recycled between items.
So a non-null |convertView| might originate from a different navigation item, so
you need to replace the icon and text that might be previously set to different
values?

[Yuwei, 2016-10-03 22:06:21]

Thanks!

https://codereview.chromium.org/2383283002/diff/1/remoting/android/java/res/m...
File remoting/android/java/res/menu/navigation_menu_item.xml (right):

https://codereview.chromium.org/2383283002/diff/1/remoting/android/java/res/m...
remoting/android/java/res/menu/navigation_menu_item.xml:1: <?xml version="1.0"
encoding="utf-8"?>
On 2016/10/03 21:10:03, Lambros wrote:
> On 2016/09/30 20:47:10, Yuwei wrote:
> > There is also a "navigation_list_item.xml" in java/res/layout and looks like
> it
> > is not being used and is suppressed for lint. Maybe we can reuse that file
> name?
> 
> SGTM. Can do that here or in a separate CL.

Done here.

https://codereview.chromium.org/2383283002/diff/1/remoting/android/java/src/o...
File
remoting/android/java/src/org/chromium/chromoting/NavigationMenuAdapter.java
(right):

https://codereview.chromium.org/2383283002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/NavigationMenuAdapter.java:78:
Drawable drawable = ContextCompat.getDrawable(activity, drawableId);
On 2016/10/03 21:10:03, Lambros wrote:
> ContextCompat is fine, I think. If you run into any build issues with it, you
> can use instead:
> org.chromium.base.ApiCompatibilityUtils.getDrawable()
> 
> I'm not sure why Chrome has this, given that ContextCompat already does the
same
> job?

No idea... Done anyway. Maybe it's API level independent?

https://codereview.chromium.org/2383283002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/NavigationMenuAdapter.java:94:
TextView textView = (TextView) inflater.inflate(R.menu.navigation_menu_item,
null);
On 2016/10/03 21:10:03, Lambros wrote:
> s/null/parent/ ?
> I think we are meant to use the ViewGroup parent for layout inflation?
> See https://possiblemobile.com/2013/05/layout-inflation-as-intended/

Done.

https://codereview.chromium.org/2383283002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/NavigationMenuAdapter.java:95:
textView.setCompoundDrawables(item.mIcon, null, null, null);
On 2016/10/03 21:10:03, Lambros wrote:
> I don't know if this is legitimate?
> I think this line, and the next line, need to be after the 'if' block? I think
> Views can be recycled between items.
> So a non-null |convertView| might originate from a different navigation item,
so
> you need to replace the icon and text that might be previously set to
different
> values?

Done. Didn't know that Android will reuse views for different item. Weird design
O_o

[Yuwei, 2016-10-03 22:25:43]

The CQ bit was checked by yuweih@chromium.org

[Yuwei, 2016-10-03 22:25:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
lambroslambrou@chromium.org
Link to the patchset: https://codereview.chromium.org/2383283002/#ps20001
(title: "Reviewer's Feedback")

[commit-bot: I haz the power, 2016-10-03 22:26:52]

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, 2016-10-03 22:30:08]

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

[commit-bot: I haz the power, 2016-10-03 22:30:09]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yuwei, 2016-10-03 22:39:09]

The CQ bit was checked by yuweih@chromium.org

[Yuwei, 2016-10-03 22:39:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
lambroslambrou@chromium.org
Link to the patchset: https://codereview.chromium.org/2383283002/#ps60001
(title: "Remove blank line")

[commit-bot: I haz the power, 2016-10-03 22:39:59]

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, 2016-10-03 23:01:16]

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

[commit-bot: I haz the power, 2016-10-03 23:01:17]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Yuwei, 2016-10-03 23:04:04]

The CQ bit was checked by yuweih@chromium.org

[Yuwei, 2016-10-03 23:04:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
lambroslambrou@chromium.org
Link to the patchset: https://codereview.chromium.org/2383283002/#ps80001
(title: "Fix Findbug")

[commit-bot: I haz the power, 2016-10-03 23:05:07]

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, 2016-10-03 23:19:38]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-03 23:22:08]

Description was changed from

==========
[Remoting Android] Navigation Menu Cleanups

This CL:

* Removes the Credits item from the menu since it will be merged into Help's
  overflow menu.
* Refactors the item layout so that we only needs one XML and specifying icon
  and text in the code.

BUG=613604
==========

to

==========
[Remoting Android] Navigation Menu Cleanups

This CL:

* Removes the Credits item from the menu since it will be merged into Help's
  overflow menu.
* Refactors the item layout so that we only needs one XML and specifying icon
  and text in the code.

BUG=613604

Committed: https://crrev.com/84f1fd3a7766e37932e6e86b3f17e9260dee1344
Cr-Commit-Position: refs/heads/master@{#422590}
==========

[commit-bot: I haz the power, 2016-10-03 23:22:09]

Patchset 5 (id:??) landed as
https://crrev.com/84f1fd3a7766e37932e6e86b3f17e9260dee1344
Cr-Commit-Position: refs/heads/master@{#422590}