Added menu button to blimp toolbar

Added menu button to blimp toolbar

This is the three dots menu similar to Chrome. Added "Open in Chrome" option to this menu.

BUG=594660
Committed: https://crrev.com/3c3b4e2d3c6444dea2d2e8c238c0e16263813c33
Cr-Commit-Position: refs/heads/master@{#384612}

[shaktisahu, 2016-03-29 19:04:48]

shaktisahu@chromium.org changed reviewers:
+ khushalsagar@chromium.org, nyquist@chromium.org

[Khushal, 2016-03-30 18:44:17]

Would be nice to have a more detailed CL description. :)

https://codereview.chromium.org/1841653005/diff/1/blimp/client/app/android/ja...
File blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java
(right):

https://codereview.chromium.org/1841653005/diff/1/blimp/client/app/android/ja...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java:173:
private void showMenu(View view) {
Would it be better to move this to a seperate class? As you add more options,
this class will start becoming crowded.

https://codereview.chromium.org/1841653005/diff/1/blimp/client/app/android/ja...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java:175:
mPopupMenu = new ListPopupWindow(mContext);
Pull the initialization step into a separate method?
This way it becomes easier to understand the flow of the code as you read it.

https://codereview.chromium.org/1841653005/diff/1/blimp/client/app/android/ja...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java:181:
return super.getView(position, convertView, parent);
Why is it necessary to override this method if we are only calling the base
class method?

https://codereview.chromium.org/1841653005/diff/1/blimp/client/app/android/ja...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java:194:
if (position == 0) {
You could consider defining constants for these, but they would have to be in
sync with how you initialize the adapter.

[shaktisahu, 2016-03-30 22:29:04]

Description was changed from

==========
Added menu button to toolbar

BUG=594660
==========

to

==========
Added menu button to blimp toolbar

This is the three dots menu similar to Chrome. Added "Open in Chrome" option to
this menu.

BUG=594660
==========

[shaktisahu, 2016-03-30 22:29:28]

https://codereview.chromium.org/1841653005/diff/1/blimp/client/app/android/ja...
File blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java
(right):

https://codereview.chromium.org/1841653005/diff/1/blimp/client/app/android/ja...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java:173:
private void showMenu(View view) {
On 2016/03/30 18:44:17, Khushal wrote:
> Would it be better to move this to a seperate class? As you add more options,
> this class will start becoming crowded.

I am also thinking same. But there are some dependency on the menu button,
urlbar and possibly blimp client session etc. due to which I am thinking of
keeping it here as of now. As the methods grow, we will have to revisit this.

https://codereview.chromium.org/1841653005/diff/1/blimp/client/app/android/ja...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java:175:
mPopupMenu = new ListPopupWindow(mContext);
On 2016/03/30 18:44:17, Khushal wrote:
> Pull the initialization step into a separate method?
> This way it becomes easier to understand the flow of the code as you read it.

Done.

https://codereview.chromium.org/1841653005/diff/1/blimp/client/app/android/ja...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java:181:
return super.getView(position, convertView, parent);
On 2016/03/30 18:44:17, Khushal wrote:
> Why is it necessary to override this method if we are only calling the base
> class method?

Done.

https://codereview.chromium.org/1841653005/diff/1/blimp/client/app/android/ja...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java:194:
if (position == 0) {
On 2016/03/30 18:44:17, Khushal wrote:
> You could consider defining constants for these, but they would have to be in
> sync with how you initialize the adapter.

Done.

[Khushal, 2016-03-31 03:15:57]

lgtm.

https://codereview.chromium.org/1841653005/diff/1/blimp/client/app/android/ja...
File blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java
(right):

https://codereview.chromium.org/1841653005/diff/1/blimp/client/app/android/ja...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java:173:
private void showMenu(View view) {
On 2016/03/30 22:29:28, shaktisahu wrote:
> On 2016/03/30 18:44:17, Khushal wrote:
> > Would it be better to move this to a seperate class? As you add more
options,
> > this class will start becoming crowded.
> 
> I am also thinking same. But there are some dependency on the menu button,
> urlbar and possibly blimp client session etc. due to which I am thinking of
> keeping it here as of now. As the methods grow, we will have to revisit this.

Makes sense.

[shaktisahu, 2016-03-31 03:53:09]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2016-03-31 03:53:34]

The CQ bit was unchecked by shaktisahu@chromium.org

[commit-bot: I haz the power, 2016-03-31 03:53:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841653005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841653005/20001

[nyquist, 2016-04-01 01:14:37]

lgtm

https://codereview.chromium.org/1841653005/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java
(right):

https://codereview.chromium.org/1841653005/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java:44:
private static final int ID_OPEN_IN_CHROME = 0;
Nit: Could you move all statics to the top?

Generic order:

- static final
- static
- final
- no modifier

[shaktisahu, 2016-04-01 01:24:54]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2016-04-01 01:24:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, khushalsagar@chromium.org
Link to the patchset: https://codereview.chromium.org/1841653005/#ps40001
(title: "Cleanup")

[commit-bot: I haz the power, 2016-04-01 01:25:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841653005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841653005/40001

[commit-bot: I haz the power, 2016-04-01 07:26:44]

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

[commit-bot: I haz the power, 2016-04-01 07:26:45]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[shaktisahu, 2016-04-01 16:28:14]

The CQ bit was checked by shaktisahu@chromium.org

[commit-bot: I haz the power, 2016-04-01 16:28:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841653005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841653005/40001

[commit-bot: I haz the power, 2016-04-01 16:59:58]

Description was changed from

==========
Added menu button to blimp toolbar

This is the three dots menu similar to Chrome. Added "Open in Chrome" option to
this menu.

BUG=594660
==========

to

==========
Added menu button to blimp toolbar

This is the three dots menu similar to Chrome. Added "Open in Chrome" option to
this menu.

BUG=594660
==========

[commit-bot: I haz the power, 2016-04-01 16:59:59]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-01 17:01:20]

Description was changed from

==========
Added menu button to blimp toolbar

This is the three dots menu similar to Chrome. Added "Open in Chrome" option to
this menu.

BUG=594660
==========

to

==========
Added menu button to blimp toolbar

This is the three dots menu similar to Chrome. Added "Open in Chrome" option to
this menu.

BUG=594660

Committed: https://crrev.com/3c3b4e2d3c6444dea2d2e8c238c0e16263813c33
Cr-Commit-Position: refs/heads/master@{#384612}
==========

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

Patchset 3 (id:??) landed as
https://crrev.com/3c3b4e2d3c6444dea2d2e8c238c0e16263813c33
Cr-Commit-Position: refs/heads/master@{#384612}