Cleanup Icon Row custom code.

Cleanup Icon Row custom code.

Remove the hacky icon row and replaced it with using submenus.

BUG=352432
NOTRY=true

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

[aurimas (slooooooooow), 2014-03-19 02:25:41]

dtrainor,kkimlabs: please take a look at this change. It is dependent on
https://chromiumcodereview.appspot.com/199733002/

[aurimas (slooooooooow), 2014-03-19 03:10:24]

On 2014/03/19 02:25:41, aurimas wrote:
> dtrainor,kkimlabs: please take a look at this change. It is dependent on
> https://chromiumcodereview.appspot.com/199733002/

please not that I am yet update AppMenuTest for this patch, so ignore changes in
AppMenuTest.java

[David Trainor- moved to gerrit, 2014-03-19 07:03:38]

Just a few nits.  Waiting on final +2 for AppMenuTests since you said it wasn't
done yet.

https://chromiumcodereview.appspot.com/203173008/diff/40001/chrome/android/ja...
File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java
(right):

https://chromiumcodereview.appspot.com/203173008/diff/40001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:53: *
@param itemRowHeight Desired height for each app menu row.
Update javadoc

https://chromiumcodereview.appspot.com/203173008/diff/40001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:76: *
@param isByHardwareButton Whether or not hardware button triggered it. (oppose
to software
Update javadoc

https://chromiumcodereview.appspot.com/203173008/diff/40001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:163:
void onItemClick(MenuItem menuItem) {
Javadoc

https://chromiumcodereview.appspot.com/203173008/diff/40001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:207:
System.out.println("aurimas getCurrentFocusedPosition " + position);
Remove

https://chromiumcodereview.appspot.com/203173008/diff/40001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:215:
int adjustedPosition = position;
Don't need this int anymore.  Just pass in position?

https://chromiumcodereview.appspot.com/203173008/diff/40001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java
(right):

https://chromiumcodereview.appspot.com/203173008/diff/40001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:96:
} else {
what if convertView isn't a R.layout.menu_item?  Can Android pass in a different
convertView here now that we're inflating different types of views?

https://chromiumcodereview.appspot.com/203173008/diff/40001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuDragHelper.java
(right):

https://chromiumcodereview.appspot.com/203173008/diff/40001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuDragHelper.java:258:
System.out.println("aurimas ACTION UP");
Remove

[aurimas (slooooooooow), 2014-03-19 18:32:52]

https://chromiumcodereview.appspot.com/203173008/diff/40001/chrome/android/ja...
File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java
(right):

https://chromiumcodereview.appspot.com/203173008/diff/40001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:53: *
@param itemRowHeight Desired height for each app menu row.
On 2014/03/19 07:03:38, David Trainor wrote:
> Update javadoc

Done.

https://chromiumcodereview.appspot.com/203173008/diff/40001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:76: *
@param isByHardwareButton Whether or not hardware button triggered it. (oppose
to software
On 2014/03/19 07:03:38, David Trainor wrote:
> Update javadoc

Done.

https://chromiumcodereview.appspot.com/203173008/diff/40001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:163:
void onItemClick(MenuItem menuItem) {
On 2014/03/19 07:03:38, David Trainor wrote:
> Javadoc

Done.

https://chromiumcodereview.appspot.com/203173008/diff/40001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:207:
System.out.println("aurimas getCurrentFocusedPosition " + position);
On 2014/03/19 07:03:38, David Trainor wrote:
> Remove

Done.

https://chromiumcodereview.appspot.com/203173008/diff/40001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:215:
int adjustedPosition = position;
On 2014/03/19 07:03:38, David Trainor wrote:
> Don't need this int anymore.  Just pass in position?

Done.

https://chromiumcodereview.appspot.com/203173008/diff/40001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java
(right):

https://chromiumcodereview.appspot.com/203173008/diff/40001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:96:
} else {
On 2014/03/19 07:03:38, David Trainor wrote:
> what if convertView isn't a R.layout.menu_item?  Can Android pass in a
different
> convertView here now that we're inflating different types of views?

convertView that is being passed in is always based on getItemViewType() so it
will always be correct in this case.

https://chromiumcodereview.appspot.com/203173008/diff/40001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuDragHelper.java
(right):

https://chromiumcodereview.appspot.com/203173008/diff/40001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuDragHelper.java:258:
System.out.println("aurimas ACTION UP");
On 2014/03/19 07:03:38, David Trainor wrote:
> Remove

Done.

[Kibeom Kim (inactive), 2014-03-19 22:04:46]

LGTM. looks much cleaner thanks!

https://chromiumcodereview.appspot.com/203173008/diff/90001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java
(right):

https://chromiumcodereview.appspot.com/203173008/diff/90001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:31:
private static final int VIEW_TYPE_THREE_BUTTON_MENUITEM = 2;
maybe comment what they represent?

https://chromiumcodereview.appspot.com/203173008/diff/90001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:83:
if (getItemViewType(position) == VIEW_TYPE_MENUITEM) {
optinal: In this case, I think switch statement will be more readable.

https://chromiumcodereview.appspot.com/203173008/diff/90001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:118:
} else if (getItemViewType(position) == VIEW_TYPE_THREE_BUTTON_MENUITEM) {
probably not important but just Q: can't we reuse the convertView here and below
else statement?

https://chromiumcodereview.appspot.com/203173008/diff/90001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:129:
} else {
I'd either use switch statement or put "assert getItemViewType(position) ==
VIEW_TYPE_TITLE_BUTTON_MENUITE", for readability.

https://chromiumcodereview.appspot.com/203173008/diff/90001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuDragHelper.java
(left):

https://chromiumcodereview.appspot.com/203173008/diff/90001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuDragHelper.java:174:
}
not sure dragging from hardware menu button would work with this change. My
guess is that, dragging from the bottom could work, but on landscape mode, if
user drags in from hardware menu button to one of these three buttons directly,
might not work.

[Kibeom Kim (inactive), 2014-03-19 22:34:59]

https://codereview.chromium.org/203173008/diff/90001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuDragHelper.java
(left):

https://codereview.chromium.org/203173008/diff/90001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuDragHelper.java:174:
}
On 2014/03/19 22:04:46, Kibeom Kim wrote:
> not sure dragging from hardware menu button would work with this change. My
> guess is that, dragging from the bottom could work, but on landscape mode, if
> user drags in from hardware menu button to one of these three buttons
directly,
> might not work.

-> discussed with Aurimas, it seems the current hardware menu dragging is not
reliable anyways, so let's just land this.

[aurimas (slooooooooow), 2014-03-19 23:31:31]

https://chromiumcodereview.appspot.com/203173008/diff/90001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java
(right):

https://chromiumcodereview.appspot.com/203173008/diff/90001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:31:
private static final int VIEW_TYPE_THREE_BUTTON_MENUITEM = 2;
On 2014/03/19 22:04:46, Kibeom Kim wrote:
> maybe comment what they represent?

Done.

https://chromiumcodereview.appspot.com/203173008/diff/90001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:83:
if (getItemViewType(position) == VIEW_TYPE_MENUITEM) {
On 2014/03/19 22:04:46, Kibeom Kim wrote:
> optinal: In this case, I think switch statement will be more readable.

Done.

https://chromiumcodereview.appspot.com/203173008/diff/90001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:118:
} else if (getItemViewType(position) == VIEW_TYPE_THREE_BUTTON_MENUITEM) {
On 2014/03/19 22:04:46, Kibeom Kim wrote:
> probably not important but just Q: can't we reuse the convertView here and
below
> else statement?

Done.

https://chromiumcodereview.appspot.com/203173008/diff/90001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:129:
} else {
On 2014/03/19 22:04:46, Kibeom Kim wrote:
> I'd either use switch statement or put "assert getItemViewType(position) ==
> VIEW_TYPE_TITLE_BUTTON_MENUITE", for readability.

Done.

[David Trainor- moved to gerrit, 2014-03-20 00:10:00]

lgtm

[aurimas (slooooooooow), 2014-03-20 19:58:30]

The CQ bit was checked by aurimas@chromium.org

[commit-bot: I haz the power, 2014-03-20 20:01:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/203173008/190001

[commit-bot: I haz the power, 2014-03-20 20:05:13]

Change committed as 258387