Cleanup AppMenu code.

Cleanup AppMenu code.

Split AppMenu code into the following parts
- AppMenu: core AppMenu code
- AppMenuDragHelper: code related to menu dragging
- AppMenuAdapter
- AppMenuItemIcon

Added tests.

NOTRY=true
BUG=352432
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258043

[aurimas (slooooooooow), 2014-03-14 00:16:08]

Hey David,

Please take a look at this change.

Thanks,
Aurimas

[aurimas (slooooooooow), 2014-03-17 20:55:58]

On 2014/03/14 00:16:08, aurimas wrote:
> Hey David,
> 
> Please take a look at this change.
> 
> Thanks,
> Aurimas

ping?

[aurimas (slooooooooow), 2014-03-18 20:51:38]

On 2014/03/17 20:55:58, aurimas wrote:
> On 2014/03/14 00:16:08, aurimas wrote:
> > Hey David,
> > 
> > Please take a look at this change.
> > 
> > Thanks,
> > Aurimas
> 
> ping?

ping?

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

+kkimlabs please take a look at this change

[Kibeom Kim (inactive), 2014-03-19 04:06:45]

Infinite thanks for doing this refactoring Aurimas :). I know refactoring
-dirty- code by others can be easily frustrating, and I think I should be
responsible for cleaning up my own mess, but anyways..

Mostly looks fine to me, with the minor comments addressed. Just one thing is,
pulling out the dragging logic to AppMenuDragHelper is very nice, but on the
other hand, still AppMenuDragHelper has to know too much AppMenu internals to
function. So I felt maybe inheriting AppMenu class makes more sense in this
case, like DraggableAppMenu or AppMenuDraggable. What do you think? (though I'm
fine with as it is)

https://chromiumcodereview.appspot.com/199733002/diff/100001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java
(right):

https://chromiumcodereview.appspot.com/199733002/diff/100001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:61:
private boolean mIsByHardwareButton;
probably no longer needed in this file?

https://chromiumcodereview.appspot.com/199733002/diff/100001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuDragHelper.java
(right):

https://chromiumcodereview.appspot.com/199733002/diff/100001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuDragHelper.java:32:
public class AppMenuDragHelper {
probably we can remove 'public' ?

https://chromiumcodereview.appspot.com/199733002/diff/100001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuDragHelper.java:122:
*
missing comment here?

https://chromiumcodereview.appspot.com/199733002/diff/100001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuDragHelper.java:389:
*         hardware menu button position b/10007237.
I guess we have to remove the last line. :(

https://chromiumcodereview.appspot.com/199733002/diff/100001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuHandler.java
(right):

https://chromiumcodereview.appspot.com/199733002/diff/100001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuHandler.java:106:
}
optional (if you do, I recommend as a follow up cl):

This function is called only from AppMenuButtonHelper. If we remove this
function and add handleDragging(...) function here that calls
AppMenuDragHelper#handleDragging(...), then AppMenuButtonHelper doesn't have to
know about AppMenuDragHelper class.

[aurimas (slooooooooow), 2014-03-19 05:04:04]

I agree that AppMenuDragHelper does need to know a fair bit about the AppMenu
and I would like to look into that as a follow up. I have some of that cleaned
up in https://chromiumcodereview.appspot.com/203173008/ that depends on this CL.

https://chromiumcodereview.appspot.com/199733002/diff/100001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java
(right):

https://chromiumcodereview.appspot.com/199733002/diff/100001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:61:
private boolean mIsByHardwareButton;
On 2014/03/19 04:06:46, Kibeom Kim wrote:
> probably no longer needed in this file?

It is used in setPopupOffset() and setMenuHeight().

https://chromiumcodereview.appspot.com/199733002/diff/100001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuDragHelper.java
(right):

https://chromiumcodereview.appspot.com/199733002/diff/100001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuDragHelper.java:32:
public class AppMenuDragHelper {
On 2014/03/19 04:06:46, Kibeom Kim wrote:
> probably we can remove 'public' ?

Done.

https://chromiumcodereview.appspot.com/199733002/diff/100001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuDragHelper.java:122:
*
On 2014/03/19 04:06:46, Kibeom Kim wrote:
> missing comment here?

Done.

https://chromiumcodereview.appspot.com/199733002/diff/100001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuDragHelper.java:389:
*         hardware menu button position b/10007237.
On 2014/03/19 04:06:46, Kibeom Kim wrote:
> I guess we have to remove the last line. :(

I think it is fine to keep the comment. We have other places the refer to the
internal bug tracker.

https://chromiumcodereview.appspot.com/199733002/diff/100001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuHandler.java
(right):

https://chromiumcodereview.appspot.com/199733002/diff/100001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuHandler.java:106:
}
On 2014/03/19 04:06:46, Kibeom Kim wrote:
> optional (if you do, I recommend as a follow up cl):
> 
> This function is called only from AppMenuButtonHelper. If we remove this
> function and add handleDragging(...) function here that calls
> AppMenuDragHelper#handleDragging(...), then AppMenuButtonHelper doesn't have
to
> know about AppMenuDragHelper class.

I added a comment to the tracking bug to do this.

[Kibeom Kim (inactive), 2014-03-19 16:12:40]

lgtm

[David Trainor- moved to gerrit, 2014-03-19 17:06:01]

lgtm

rubber stamp owner lgtm.

[aurimas (slooooooooow), 2014-03-19 18:20:07]

The CQ bit was checked by aurimas@chromium.org

[commit-bot: I haz the power, 2014-03-19 18:20:21]

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

[commit-bot: I haz the power, 2014-03-19 18:22:25]

Change committed as 258043