Add Android's context menu handler used on the NTP.

Add Android's context menu handler used on the NTP.

BUG=137011
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153151

[Evan Stade, 2012-08-22 00:24:51]

https://chromiumcodereview.appspot.com/10861042/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/ntp/android/context_menu_handler.h (right):

https://chromiumcodereview.appspot.com/10861042/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/ntp/android/context_menu_handler.h:17: // The handler
for Javascript messages related to the context menus.
nit: JavaScript

https://chromiumcodereview.appspot.com/10861042/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/ntp/android/context_menu_handler.h:18: // Its the job of
the actual HTML page to intercept the contextmenu event,
nit: It's

https://chromiumcodereview.appspot.com/10861042/diff/1/chrome/browser/ui/webu...
File
chrome/browser/ui/webui/ntp/android/on_context_menu_item_selected_callback.h
(right):

https://chromiumcodereview.appspot.com/10861042/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/ntp/android/on_context_menu_item_selected_callback.h:15:
// that the ContextMenuHandler will outlive the callback).
any reason not to use a weak pointer?

[Ted C, 2012-08-22 01:02:38]

https://chromiumcodereview.appspot.com/10861042/diff/1/chrome/browser/ui/webu...
File
chrome/browser/ui/webui/ntp/android/on_context_menu_item_selected_callback.h
(right):

https://chromiumcodereview.appspot.com/10861042/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/ntp/android/on_context_menu_item_selected_callback.h:15:
// that the ContextMenuHandler will outlive the callback).
On 2012/08/22 00:24:51, Evan Stade wrote:
> any reason not to use a weak pointer?

Hmm...I didn't add this portion initially, so I'm not really sure.

Are you saying to pass a WeakPtr of the ContextMenuHandler (or some minimal
interface pulled out from that) instead of defining this separate class?  Seems
like other places in webui use that paradigm so I can certainly investigate that
here.

Just want to clarify before I start looking into that.

[Evan Stade, 2012-08-22 01:12:28]

https://chromiumcodereview.appspot.com/10861042/diff/1/chrome/browser/ui/webu...
File
chrome/browser/ui/webui/ntp/android/on_context_menu_item_selected_callback.h
(right):

https://chromiumcodereview.appspot.com/10861042/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/ntp/android/on_context_menu_item_selected_callback.h:15:
// that the ContextMenuHandler will outlive the callback).
On 2012/08/22 01:02:38, Ted C wrote:
> On 2012/08/22 00:24:51, Evan Stade wrote:
> > any reason not to use a weak pointer?
> 
> Hmm...I didn't add this portion initially, so I'm not really sure.
> 
> Are you saying to pass a WeakPtr of the ContextMenuHandler (or some minimal
> interface pulled out from that) instead of defining this separate class? 
Seems
> like other places in webui use that paradigm so I can certainly investigate
that
> here.
> 
> Just want to clarify before I start looking into that.

yes, that's what I meant.

[Ted C, 2012-08-22 20:55:50]

Comments address, PTAL

http://codereview.chromium.org/10861042/diff/1/chrome/browser/ui/webui/ntp/an...
File chrome/browser/ui/webui/ntp/android/context_menu_handler.h (right):

http://codereview.chromium.org/10861042/diff/1/chrome/browser/ui/webui/ntp/an...
chrome/browser/ui/webui/ntp/android/context_menu_handler.h:17: // The handler
for Javascript messages related to the context menus.
On 2012/08/22 00:24:51, Evan Stade wrote:
> nit: JavaScript

Done.

http://codereview.chromium.org/10861042/diff/1/chrome/browser/ui/webui/ntp/an...
chrome/browser/ui/webui/ntp/android/context_menu_handler.h:18: // Its the job of
the actual HTML page to intercept the contextmenu event,
On 2012/08/22 00:24:51, Evan Stade wrote:
> nit: It's

Done.

http://codereview.chromium.org/10861042/diff/1/chrome/browser/ui/webui/ntp/an...
File
chrome/browser/ui/webui/ntp/android/on_context_menu_item_selected_callback.h
(right):

http://codereview.chromium.org/10861042/diff/1/chrome/browser/ui/webui/ntp/an...
chrome/browser/ui/webui/ntp/android/on_context_menu_item_selected_callback.h:15:
// that the ContextMenuHandler will outlive the callback).
On 2012/08/22 01:12:28, Evan Stade wrote:
> On 2012/08/22 01:02:38, Ted C wrote:
> > On 2012/08/22 00:24:51, Evan Stade wrote:
> > > any reason not to use a weak pointer?
> > 
> > Hmm...I didn't add this portion initially, so I'm not really sure.
> > 
> > Are you saying to pass a WeakPtr of the ContextMenuHandler (or some minimal
> > interface pulled out from that) instead of defining this separate class? 
> Seems
> > like other places in webui use that paradigm so I can certainly investigate
> that
> > here.
> > 
> > Just want to clarify before I start looking into that.
> 
> yes, that's what I meant.

Gave it a shot and seems to work just fine.

[Evan Stade, 2012-08-22 23:50:04]

http://codereview.chromium.org/10861042/diff/7001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/android/context_menu_handler.cc (right):

http://codereview.chromium.org/10861042/diff/7001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/android/context_menu_handler.cc:88:
tab->ShowCustomContextMenu(menu, weak_ptr_factory_.GetWeakPtr());
the second param here needs to be something like

base::Bind(&ContextMenuHandler::OnItemSelected, weak_ptr_factory_.GetWeakPtr())

while the param type is

const base::Callback<void(int)>&

[Ted C, 2012-08-23 20:50:52]

PTAL

http://codereview.chromium.org/10861042/diff/7001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/android/context_menu_handler.cc (right):

http://codereview.chromium.org/10861042/diff/7001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/android/context_menu_handler.cc:88:
tab->ShowCustomContextMenu(menu, weak_ptr_factory_.GetWeakPtr());
On 2012/08/22 23:50:05, Evan Stade wrote:
> the second param here needs to be something like
> 
> base::Bind(&ContextMenuHandler::OnItemSelected,
weak_ptr_factory_.GetWeakPtr())
> 
> while the param type is
> 
> const base::Callback<void(int)>&

Done.

[Evan Stade, 2012-08-23 21:31:48]

http://codereview.chromium.org/10861042/diff/9001/chrome/browser/android/tab_...
File chrome/browser/android/tab_android.h (right):

http://codereview.chromium.org/10861042/diff/9001/chrome/browser/android/tab_...
chrome/browser/android/tab_android.h:15: class ContextMenuHandler;
used?

http://codereview.chromium.org/10861042/diff/9001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/android/context_menu_handler.cc (right):

http://codereview.chromium.org/10861042/diff/9001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/android/context_menu_handler.cc:109:
content::PAGE_TRANSITION_AUTO_BOOKMARK, false));
these two functions could definitely be refactored to share code.

[Ted C, 2012-08-23 21:51:58]

http://codereview.chromium.org/10861042/diff/9001/chrome/browser/android/tab_...
File chrome/browser/android/tab_android.h (right):

http://codereview.chromium.org/10861042/diff/9001/chrome/browser/android/tab_...
chrome/browser/android/tab_android.h:15: class ContextMenuHandler;
On 2012/08/23 21:31:48, Evan Stade wrote:
> used?

D'oh...nope

http://codereview.chromium.org/10861042/diff/9001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/android/context_menu_handler.cc (right):

http://codereview.chromium.org/10861042/diff/9001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/android/context_menu_handler.cc:109:
content::PAGE_TRANSITION_AUTO_BOOKMARK, false));
On 2012/08/23 21:31:48, Evan Stade wrote:
> these two functions could definitely be refactored to share code.

Done.

[Evan Stade, 2012-08-23 21:53:18]

lgtm

[commit-bot: I haz the power, 2012-08-23 22:08:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/10861042/9002

[commit-bot: I haz the power, 2012-08-23 22:08:30]

Presubmit check for 10861042-9002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.7s to calculate.

[Ted C, 2012-08-23 23:04:56]

+thakis for OWNERS of chrome_browser.gypi

[Nico, 2012-08-23 23:43:10]

lgtm

[commit-bot: I haz the power, 2012-08-23 23:52:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/10861042/9002

[commit-bot: I haz the power, 2012-08-24 04:20:37]

Change committed as 153151