[Home] Show the app menu icons as a footer

[Home] Show the app menu icons as a footer

Move the icon row at the top of the app menu to a fixed footer when
Chrome Home is enabled.

BUG=707094
Review-Url: https://codereview.chromium.org/2821223004
Cr-Commit-Position: refs/heads/master@{#465448}
Committed: https://chromium.googlesource.com/chromium/src/+/7601678d95b575aab8d337d26559fc321409ee17

[Theresa, 2017-04-18 16:38:51]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 16:39:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2017-04-18 16:50:39]

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

[Theresa, 2017-04-18 16:50:39]

ptal

[commit-bot: I haz the power, 2017-04-18 18:12:55]

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

[commit-bot: I haz the power, 2017-04-18 18:12:56]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-04-18 22:58:46]

https://codereview.chromium.org/2821223004/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/icon_row_menu_footer.xml (right):

https://codereview.chromium.org/2821223004/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/icon_row_menu_footer.xml:16: <LinearLayout
I was thinking it would be nice to share icon_row_menu_item.xml here.  Even in
the "medium" term I think we'll end up needing both since custom tabs will
likely have a top menu in the chrome home world.

I'm curious why we need the tint to handle disabled here, but I didn't spot
anything like that in the menu (although that code is not exactly clear).

It would be good to find a way to converge (even if we ended up just using this
layout for the menu and got rid of the overly flexible icon_row_menu_item)...I
think that can be a TODO for now to find a path to merge these to unblock this
though.

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

https://codereview.chromium.org/2821223004/diff/1/chrome/android/java/res/val...
chrome/android/java/res/values-v17/styles.xml:589: <style
name="AppMenuIconRowIcon">
Any reason OverflowMenuButton doesn't work here?

https://codereview.chromium.org/2821223004/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/appmenu/ChromeHomeAppMenuTest.java
(right):

https://codereview.chromium.org/2821223004/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/appmenu/ChromeHomeAppMenuTest.java:31:
getInstrumentation().waitForIdleSync();
why do we need this?

Actually...why do we need showAppMenuAndAssertMenuShown here in setup?  We do it
again in testTabSwitcherMenu and this seems like doing this in setup will slow
down that test.

https://codereview.chromium.org/2821223004/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/appmenu/ChromeHomeAppMenuTest.java:81:
CriteriaHelper.pollInstrumentationThread(new Criteria("AppMenu did not show") {
why the instrumentation thread?

[Theresa, 2017-04-18 23:30:27]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[Theresa, 2017-04-18 23:30:59]

https://codereview.chromium.org/2821223004/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/icon_row_menu_footer.xml (right):

https://codereview.chromium.org/2821223004/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/icon_row_menu_footer.xml:16: <LinearLayout
On 2017/04/18 22:58:46, Ted C wrote:
> I was thinking it would be nice to share icon_row_menu_item.xml here.  Even in
> the "medium" term I think we'll end up needing both since custom tabs will
> likely have a top menu in the chrome home world.
> 
> I'm curious why we need the tint to handle disabled here, but I didn't spot
> anything like that in the menu (although that code is not exactly clear).
> 
> It would be good to find a way to converge (even if we ended up just using
this
> layout for the menu and got rid of the overly flexible icon_row_menu_item)...I
> think that can be a TODO for now to find a path to merge these to unblock this
> though.

I agree. Added a TODO.

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

https://codereview.chromium.org/2821223004/diff/1/chrome/android/java/res/val...
chrome/android/java/res/values-v17/styles.xml:589: <style
name="AppMenuIconRowIcon">
On 2017/04/18 22:58:46, Ted C wrote:
> Any reason OverflowMenuButton doesn't work here?

None at all. I removed this style.

https://codereview.chromium.org/2821223004/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/appmenu/ChromeHomeAppMenuTest.java
(right):

https://codereview.chromium.org/2821223004/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/appmenu/ChromeHomeAppMenuTest.java:31:
getInstrumentation().waitForIdleSync();
On 2017/04/18 22:58:46, Ted C wrote:
> why do we need this?
> 
> Actually...why do we need showAppMenuAndAssertMenuShown here in setup?  We do
it
> again in testTabSwitcherMenu and this seems like doing this in setup will slow
> down that test.

Done.

https://codereview.chromium.org/2821223004/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/appmenu/ChromeHomeAppMenuTest.java:81:
CriteriaHelper.pollInstrumentationThread(new Criteria("AppMenu did not show") {
On 2017/04/18 22:58:46, Ted C wrote:
> why the instrumentation thread?

Changed it to poll the UI thread.

[commit-bot: I haz the power, 2017-04-18 23:31:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ted C, 2017-04-18 23:36:28]

lgtm

[Theresa, 2017-04-18 23:46:07]

The CQ bit was unchecked by twellington@chromium.org

[Theresa, 2017-04-18 23:46:07]

The CQ bit was checked by twellington@chromium.org

[commit-bot: I haz the power, 2017-04-18 23:46:41]

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-04-19 00:47:34]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492559167736900,
"parent_rev": "7865cc51cb1ebe2c072f62f7de98723e0f326c59", "commit_rev":
"ff9aff5b29d6388b71e48e4d3ee5f907ce0eb109"}

[commit-bot: I haz the power, 2017-04-19 00:47:47]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492559167736900,
"parent_rev": "d428a71dde28a84fa0cdc0fa5fc958c65131c173", "commit_rev":
"11e9e0d2484c6bded016f73e6c04c0708db6755f"}

[commit-bot: I haz the power, 2017-04-19 00:48:05]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492559167736900,
"parent_rev": "fea1c8c2dc19be6d66287b68d6ffd2d86b8c939e", "commit_rev":
"7601678d95b575aab8d337d26559fc321409ee17"}

[commit-bot: I haz the power, 2017-04-19 00:48:16]

Description was changed from

==========
[Home] Show the app menu icons as a footer

Move the icon row at the top of the app menu to a fixed footer when
Chrome Home is enabled.

BUG=707094
==========

to

==========
[Home] Show the app menu icons as a footer

Move the icon row at the top of the app menu to a fixed footer when
Chrome Home is enabled.

BUG=707094

Review-Url: https://codereview.chromium.org/2821223004
Cr-Commit-Position: refs/heads/master@{#465448}
Committed:
https://chromium.googlesource.com/chromium/src/+/7601678d95b575aab8d337d26559...
==========

[commit-bot: I haz the power, 2017-04-19 00:48:20]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/7601678d95b575aab8d337d26559...