Update menu look and feel on Android

Update menu look and feel on Android

- Change the overflow menu to have an updated layout.

BUG=375379
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282563

[David Trainor- moved to gerrit, 2014-07-08 22:55:15]

ptal

[aurimas (slooooooooow), 2014-07-08 23:49:45]

Please update the CL description to remove duplicate "BUG=..." lines

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/res/layo...
File chrome/android/java/res/layout/four_button_menu_item.xml (right):

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/res/layo...
chrome/android/java/res/layout/four_button_menu_item.xml:15:
android:layout_width="48dp"
Why are you hardcoding the width of the button? This will create gaps between
the buttons?

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/res/layo...
chrome/android/java/res/layout/four_button_menu_item.xml:16:
android:layout_height="?android:attr/listPreferredItemHeightSmall"
Why is match parent the problem? Just set the parent height to be
listPreferredItemHeightSmall

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/res/layo...
File chrome/android/java/res/layout/three_button_menu_item.xml (right):

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/res/layo...
chrome/android/java/res/layout/three_button_menu_item.xml:21:
android:layout_width="48dp"
Same question as four_button_menu_item.

Why don't we use the full width for buttons instead of hardcoding them to 48dp?

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

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/res/valu...
chrome/android/java/res/values-v17/styles.xml:20: <item
name="android:popupBackground">@drawable/menu_bg</item>
Are we are setting the popupBackground twice: once from Java and once here?
Should we set the background here to be @null then?

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/res/valu...
chrome/android/java/res/values-v17/styles.xml:21: <item
name="android:dropDownWidth">460dp</item>
What happens for devices with screen width <460dp? Galaxy start for example is
320dp.

The mock seems to say 260dp?

How is this different from menu_width in dimens.xml?

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/res/valu...
chrome/android/java/res/values-v17/styles.xml:27: <item
name="android:fontFamily">Roboto</item>
s/Roboto/sans-serif

We do not explicitly specify the font family anywhere else in Chrome for
Android.

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

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:103: //
an incorrectly drawn background.
Can you file a bug about this to Android?

[David Trainor- moved to gerrit, 2014-07-09 00:01:56]

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/res/layo...
File chrome/android/java/res/layout/four_button_menu_item.xml (right):

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/res/layo...
chrome/android/java/res/layout/four_button_menu_item.xml:15:
android:layout_width="48dp"
On 2014/07/08 23:49:44, aurimas wrote:
> Why are you hardcoding the width of the button? This will create gaps between
> the buttons?

Yes it will.  Talked to Roma and that's the desired behavior.  16dp in between
the buttons at 48 dp each button.  The spacers are 0dp with weight 1 to properly
fill the gaps (it ends up being 16 dp with the current measurements though).

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/res/layo...
chrome/android/java/res/layout/four_button_menu_item.xml:16:
android:layout_height="?android:attr/listPreferredItemHeightSmall"
On 2014/07/08 23:49:44, aurimas wrote:
> Why is match parent the problem? Just set the parent height to be
> listPreferredItemHeightSmall 

Good point!  I did try this earlier and IIRC for some reason it didn't work. 
But it should so I'll try again (I could have been testing something else and
messed it up).  The only reason the current menu works is that the dividers
happened to have the height set which bumps this view's height to the proper
size.

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/res/layo...
File chrome/android/java/res/layout/three_button_menu_item.xml (right):

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/res/layo...
chrome/android/java/res/layout/three_button_menu_item.xml:21:
android:layout_width="48dp"
On 2014/07/08 23:49:44, aurimas wrote:
> Same question as four_button_menu_item.
> 
> Why don't we use the full width for buttons instead of hardcoding them to
48dp?

Not hardcoding to 48dp also will make it difficult to line up the menu buttons
properly.  Also talked to Roma and this seemed like what they want.   Thinking
about it though, I'm not sure it'd be good to have shrunken hit targets.  Worst
case I can hard code the padding of the views to make it fit for 260dp...

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

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/res/valu...
chrome/android/java/res/values-v17/styles.xml:20: <item
name="android:popupBackground">@drawable/menu_bg</item>
On 2014/07/08 23:49:44, aurimas wrote:
> Are we are setting the popupBackground twice: once from Java and once here?
> Should we set the background here to be @null then?

Ah yeah I was going to make this null.  Was still playing with it a bit to see
if I could fix it.  My bad forgot to move it over.

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/res/valu...
chrome/android/java/res/values-v17/styles.xml:21: <item
name="android:dropDownWidth">460dp</item>
On 2014/07/08 23:49:44, aurimas wrote:
> What happens for devices with screen width <460dp? Galaxy start for example is
> 320dp.
> 
> The mock seems to say 260dp?
> 
> How is this different from menu_width in dimens.xml?

Gah this needs to be removed.  Nobody reads this parameter (even in
ListPopupWindow).

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/res/valu...
chrome/android/java/res/values-v17/styles.xml:27: <item
name="android:fontFamily">Roboto</item>
On 2014/07/08 23:49:44, aurimas wrote:
> s/Roboto/sans-serif
> 
> We do not explicitly specify the font family anywhere else in Chrome for
> Android.

Ah yeah I'll use sans-serif.  We do specify sans-serif throughout the code, but
not Roboto.

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

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:103: //
an incorrectly drawn background.
On 2014/07/08 23:49:44, aurimas wrote:
> Can you file a bug about this to Android?

Will write up a small test app and confirm it and file a bug.

[aurimas (slooooooooow), 2014-07-09 00:14:52]

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/res/layo...
File chrome/android/java/res/layout/four_button_menu_item.xml (right):

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/res/layo...
chrome/android/java/res/layout/four_button_menu_item.xml:15:
android:layout_width="48dp"
On 2014/07/09 00:01:56, David Trainor wrote:
> On 2014/07/08 23:49:44, aurimas wrote:
> > Why are you hardcoding the width of the button? This will create gaps
between
> > the buttons?
> 
> Yes it will.  Talked to Roma and that's the desired behavior.  16dp in between
> the buttons at 48 dp each button.  The spacers are 0dp with weight 1 to
properly
> fill the gaps (it ends up being 16 dp with the current measurements though).

THAT. IS. SO. UGLY.

[David Trainor- moved to gerrit, 2014-07-09 00:15:17]

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/res/layo...
File chrome/android/java/res/layout/four_button_menu_item.xml (right):

https://codereview.chromium.org/377083002/diff/1/chrome/android/java/res/layo...
chrome/android/java/res/layout/four_button_menu_item.xml:16:
android:layout_height="?android:attr/listPreferredItemHeightSmall"
On 2014/07/09 00:01:56, David Trainor wrote:
> On 2014/07/08 23:49:44, aurimas wrote:
> > Why is match parent the problem? Just set the parent height to be
> > listPreferredItemHeightSmall 
> 
> Good point!  I did try this earlier and IIRC for some reason it didn't work. 
> But it should so I'll try again (I could have been testing something else and
> messed it up).  The only reason the current menu works is that the dividers
> happened to have the height set which bumps this view's height to the proper
> size.

Just tried this again.  Didn't work.  I'm guessing the width/height are ignored
when the ListView inflates and adds the view.  I'll figure out why/if there is a
cleaner way around it tomorrow.

[David Trainor- moved to gerrit, 2014-07-09 20:40:38]

Updated CL based on your comments.  ptal.

[aurimas (slooooooooow), 2014-07-09 21:12:52]

LGTM. Please update CL description to remove duplicate "BUG=???"

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

https://codereview.chromium.org/377083002/diff/20001/chrome/android/java/res/...
chrome/android/java/res/values-v17/styles.xml:26: <item
name="android:fontFamily">sans-serif</item>
I was under impression that this is the default fontFamily of
Theme.AppCompat.Light, so I dont think we need to specify it.

[David Trainor- moved to gerrit, 2014-07-10 16:43:26]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2014-07-10 16:44:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtrainor@chromium.org/377083002/40001

[commit-bot: I haz the power, 2014-07-10 18:49:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-07-10 18:53:25]

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

[commit-bot: I haz the power, 2014-07-10 18:53:26]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[David Trainor- moved to gerrit, 2014-07-10 19:06:06]

The CQ bit was unchecked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2014-07-10 23:12:13]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2014-07-10 23:13:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtrainor@chromium.org/377083002/60001

[commit-bot: I haz the power, 2014-07-11 02:10:26]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-07-11 02:13:26]

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

[commit-bot: I haz the power, 2014-07-11 02:13:27]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[aurimas (slooooooooow), 2014-07-11 02:14:00]

On 2014/07/11 02:13:27, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

Missing LGTM from an OWNER for these files:
    chrome/chrome.gyp

[aurimas (slooooooooow), 2014-07-11 02:14:49]

thakis@chromium.org: Please review 1 line change in chrome/chrome.gyp

[Nico, 2014-07-11 03:04:43]

That line lgtm.

[aurimas (slooooooooow), 2014-07-11 05:11:21]

The CQ bit was checked by aurimas@chromium.org

[commit-bot: I haz the power, 2014-07-11 05:12:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtrainor@chromium.org/377083002/60001

[commit-bot: I haz the power, 2014-07-11 07:01:54]

Change committed as 282563