A non-GTK version of menus for touchui.

A non-GTK version of menus for touchui.

For the menus to work correctly, the X messge pump also needs updating to allow
nested event dispatching.

BUG=None
TEST=On a touchui build, non-GTK context menus in web-pages should work correctly.

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

[sky, 2010-11-22 17:06:15]

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/menu_contro...
File views/controls/menu/menu_controller.cc (right):

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/menu_contro...
views/controls/menu/menu_controller.cc:870:
MessageLoopForUI::current()->QuitNow();
How come you need the QuitNow instead of returning false?

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/native_menu...
File views/controls/menu/native_menu_x.cc (right):

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/native_menu...
views/controls/menu/native_menu_x.cc:22: delete root_;
Use scoped_ptr

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/native_menu...
views/controls/menu/native_menu_x.cc:116: menus::MenuModel::ItemType menu_type =
model->GetTypeAt(i);
Offset by GetFirstItemIndex.

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/native_menu...
views/controls/menu/native_menu_x.cc:121: case menus::MenuModel::TYPE_COMMAND:
Move the code that converts from menus::MenuModel::ItemType to
MenuItemView::Type to a common place that both this and wrech_menu use.

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/native_menu...
views/controls/menu/native_menu_x.cc:156: menus::MenuModel* model) {
spacing is off.

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/native_menu...
views/controls/menu/native_menu_x.cc:158: if (model->GetTypeAt(i) ==
menus::MenuModel::TYPE_SEPARATOR) {
Don't you need to offset by model->GetFirstItemIndex()?

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/native_menu...
File views/controls/menu/native_menu_x.h (right):

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/native_menu...
views/controls/menu/native_menu_x.h:19: class NativeMenuX : public MenuWrapper,
Description?

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/native_menu...
views/controls/menu/native_menu_x.h:20: public MenuDelegate {
indentation is off.

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/native_menu...
views/controls/menu/native_menu_x.h:46: menus::MenuModel* model_;
Document ownership.

[sky, 2010-11-22 17:08:25]

Also, can you get someone else to review the message pump changes.

  -Scott

[sadrul, 2010-11-22 18:48:22]

I have added Rob for the message-pump change. Would you recommend an additional
reviewer?

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/menu_contro...
File views/controls/menu/menu_controller.cc (right):

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/menu_contro...
views/controls/menu/menu_controller.cc:870:
MessageLoopForUI::current()->QuitNow();
On 2010/11/22 17:06:15, sky wrote:
> How come you need the QuitNow instead of returning false?

Dispatch(XEvent*) returns a value indicating whether the event was processed or
not. If the event was processed, then the message-pump proceeds as usual. If it
wasn't, then the message-pump lets the GDK dispatcher process it (through
Dispatch(GdkEvent*)). i.e., the message-pump doesn't terminate based on the
return value. So we need to explicitly terminate it here.

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/native_menu...
File views/controls/menu/native_menu_x.cc (right):

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/native_menu...
views/controls/menu/native_menu_x.cc:22: delete root_;
On 2010/11/22 17:06:15, sky wrote:
> Use scoped_ptr

Done.

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/native_menu...
views/controls/menu/native_menu_x.cc:116: menus::MenuModel::ItemType menu_type =
model->GetTypeAt(i);
On 2010/11/22 17:06:15, sky wrote:
> Offset by GetFirstItemIndex.

Done.

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/native_menu...
views/controls/menu/native_menu_x.cc:121: case menus::MenuModel::TYPE_COMMAND:
On 2010/11/22 17:06:15, sky wrote:
> Move the code that converts from menus::MenuModel::ItemType to
> MenuItemView::Type to a common place that both this and wrech_menu use.

Added MenuItemView::AppendMenuItemFromModel to do the work, and use it from
wrench_menu and here.

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/native_menu...
views/controls/menu/native_menu_x.cc:156: menus::MenuModel* model) {
On 2010/11/22 17:06:15, sky wrote:
> spacing is off.

Done.

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/native_menu...
views/controls/menu/native_menu_x.cc:158: if (model->GetTypeAt(i) ==
menus::MenuModel::TYPE_SEPARATOR) {
On 2010/11/22 17:06:15, sky wrote:
> Don't you need to offset by model->GetFirstItemIndex()?

Done.

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/native_menu...
File views/controls/menu/native_menu_x.h (right):

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/native_menu...
views/controls/menu/native_menu_x.h:20: public MenuDelegate {
On 2010/11/22 17:06:15, sky wrote:
> indentation is off.

Fixed, and added a description.

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/native_menu...
views/controls/menu/native_menu_x.h:46: menus::MenuModel* model_;
On 2010/11/22 17:06:15, sky wrote:
> Document ownership.

Done.

[sky, 2010-11-22 19:23:01]

As to reviewing the message-pump change I would ask whoever reviewed your last
review to the file. Was it Evan Martin?

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/menu_contro...
File views/controls/menu/menu_controller.cc (right):

http://codereview.chromium.org/5110011/diff/1/views/controls/menu/menu_contro...
views/controls/menu/menu_controller.cc:870:
MessageLoopForUI::current()->QuitNow();
On 2010/11/22 18:48:22, sadrul wrote:
> On 2010/11/22 17:06:15, sky wrote:
> > How come you need the QuitNow instead of returning false?
> 
> Dispatch(XEvent*) returns a value indicating whether the event was processed
or
> not. If the event was processed, then the message-pump proceeds as usual. If
it
> wasn't, then the message-pump lets the GDK dispatcher process it (through
> Dispatch(GdkEvent*)). i.e., the message-pump doesn't terminate based on the
> return value. So we need to explicitly terminate it here.

The docs don't indicate that. Doesn't that make for an inconsistent
implementation? One Dispatcher continues, the other exits? Wouldn't it be better
to have the X variant return an enum?

http://codereview.chromium.org/5110011/diff/8001/views/controls/menu/menu_ite...
File views/controls/menu/menu_item_view.cc (right):

http://codereview.chromium.org/5110011/diff/8001/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.cc:242: int index, int id) {
id should be on its own line.

http://codereview.chromium.org/5110011/diff/8001/views/controls/menu/menu_ite...
File views/controls/menu/menu_item_view.h (right):

http://codereview.chromium.org/5110011/diff/8001/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.h:185: int index,
index->model_index. Document that the index has already been offset.

[sadrul, 2010-11-22 19:42:37]

Regarding the behaviour of Dispatch(XEvent*):

> The docs don't indicate that. Doesn't that make for an
> inconsistent implementation? One Dispatcher continues, the
> other exits? Wouldn't it be better to have the X variant
> return an enum?

Indeed. The docs are incorrect, and it should be fixed. I like the enum
proposal, with EVENT_PROCESSED, EVENT_NOT_PROCESSED and EVENT_QUIT. Evan?

http://codereview.chromium.org/5110011/diff/8001/views/controls/menu/menu_ite...
File views/controls/menu/menu_item_view.cc (right):

http://codereview.chromium.org/5110011/diff/8001/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.cc:242: int index, int id) {
On 2010/11/22 19:23:01, sky wrote:
> id should be on its own line.

Done.

http://codereview.chromium.org/5110011/diff/8001/views/controls/menu/menu_ite...
File views/controls/menu/menu_item_view.h (right):

http://codereview.chromium.org/5110011/diff/8001/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.h:185: int index,
On 2010/11/22 19:23:01, sky wrote:
> index->model_index. Document that the index has already been offset.

Done.

[sky, 2010-11-22 20:38:09]

LGTM (everything except the message_pump_glib_x.cc change)

[Evan Martin, 2010-11-22 20:43:26]

http://codereview.chromium.org/5110011/diff/14001/base/message_pump_glib_x.cc
File base/message_pump_glib_x.cc (right):

http://codereview.chromium.org/5110011/diff/14001/base/message_pump_glib_x.cc...
base/message_pump_glib_x.cc:88: static gboolean (*gdkdispatcher)(GSource*,
GSourceFunc, void*) = NULL;
Is this really the best place for this state?  Why not a member variable?  Is it
shared across all instances of this class?

http://codereview.chromium.org/5110011/diff/14001/views/focus/accelerator_han...
File views/focus/accelerator_handler_touch.cc (right):

http://codereview.chromium.org/5110011/diff/14001/views/focus/accelerator_han...
views/focus/accelerator_handler_touch.cc:106: xwindow = xiev->event;
This seems unrelated to the change.  (?)

[sadrul, 2010-11-22 21:23:06]

Also, do you agree with the change to have Dispatch(XEvent*) return an enum? (in
a different CL?)

http://codereview.chromium.org/5110011/diff/14001/base/message_pump_glib_x.cc
File base/message_pump_glib_x.cc (right):

http://codereview.chromium.org/5110011/diff/14001/base/message_pump_glib_x.cc...
base/message_pump_glib_x.cc:88: static gboolean (*gdkdispatcher)(GSource*,
GSourceFunc, void*) = NULL;
On 2010/11/22 20:43:26, Evan Martin wrote:
> Is this really the best place for this state?  Why not a member variable?  Is
it
> shared across all instances of this class?

The default GDK event dispatcher should not change during the lifetime of
chrome, unless it is replaced by something other than the message-pump itself. A
member variable should do just fine. Sounds like that's preferrable. So made
that change.

http://codereview.chromium.org/5110011/diff/14001/views/focus/accelerator_han...
File views/focus/accelerator_handler_touch.cc (right):

http://codereview.chromium.org/5110011/diff/14001/views/focus/accelerator_han...
views/focus/accelerator_handler_touch.cc:106: xwindow = xiev->event;
On 2010/11/22 20:43:26, Evan Martin wrote:
> This seems unrelated to the change.  (?)

This is necessary for the change in message_pump_glib_x.cc. The call to
XGetEventData is removed from here and added to the message-pump, and making
this call on the same event twice results in an error.

[sadrul, 2010-11-23 20:29:22]

I have made the change to the dispatcher. So now, the
MessagePumpGlibXDispatcher::Dispatch(XEvent*) returns an enum indicating whether
the event was processed, ignored (i.e. not processed), or whether the event
triggered termination of the loop. To do this, I had to bump the |RunState*
state_| in the glib message-pump from private to protected, moved the definition
of RunState from the .cc file into the .h file. I also cleaned up the
nested-dispatcher for GTK menus, since it's not used for touchui.

(also, sent to trybots)

[sadrul, 2010-11-23 21:58:40]

On 2010/11/23 20:29:22, sadrul wrote:
[snip]
> (also, sent to trybots)

The failure on chromeos looks unrelated.

[rjkroege, 2010-11-23 23:38:54]

http://codereview.chromium.org/5110011/diff/37001/base/message_pump_glib.h
File base/message_pump_glib.h (right):

http://codereview.chromium.org/5110011/diff/37001/base/message_pump_glib.h#ne...
base/message_pump_glib.h:97: struct RunState {
why did you have to move this into the include? It could be a separate class?

http://codereview.chromium.org/5110011/diff/37001/base/message_pump_glib_x.h
File base/message_pump_glib_x.h (right):

http://codereview.chromium.org/5110011/diff/37001/base/message_pump_glib_x.h#...
base/message_pump_glib_x.h:65: gboolean (*gdkdispatcher_)(GSource*, GSourceFunc,
void*);
Per the discussion with beng, this code will need to be adjusted.

Also, what happens to xinput2 handling when inside this case?

http://codereview.chromium.org/5110011/diff/37001/views/focus/accelerator_han...
File views/focus/accelerator_handler.h (right):

http://codereview.chromium.org/5110011/diff/37001/views/focus/accelerator_han...
views/focus/accelerator_handler.h:39: virtual
MessagePumpGlibXDispatcher::DispatchStatus Dispatch(XEvent* xev);
how does this get this here definition-wise?

[sadrul, 2010-11-24 00:55:21]

http://codereview.chromium.org/5110011/diff/37001/base/message_pump_glib.h
File base/message_pump_glib.h (right):

http://codereview.chromium.org/5110011/diff/37001/base/message_pump_glib.h#ne...
base/message_pump_glib.h:97: struct RunState {
On 2010/11/23 23:38:54, rjkroege wrote:
> why did you have to move this into the include? It could be a separate class?

the |should_quit| needs to be set from inside MessagePumpGlibX, i.e. it needs to
be able to see |should_quit|. So the definition of the struct (which was already
declared inside MessagePumpGlib) needed to move into here.

http://codereview.chromium.org/5110011/diff/37001/base/message_pump_glib_x.h
File base/message_pump_glib_x.h (right):

http://codereview.chromium.org/5110011/diff/37001/base/message_pump_glib_x.h#...
base/message_pump_glib_x.h:65: gboolean (*gdkdispatcher_)(GSource*, GSourceFunc,
void*);
On 2010/11/23 23:38:54, rjkroege wrote:
> Per the discussion with beng, this code will need to be adjusted.

I am unsure how it would need to change.

> Also, what happens to xinput2 handling when inside this case?

The menu themselves receive do not receive XInput2 events. This is because the
Xwindows for the views-menus aren't created by GTK+, and so we do not set them
up to receive XInput2 events. However, they do still receive XInput events, i.e.
they can still be focused, navigated and clicked on.

http://codereview.chromium.org/5110011/diff/37001/views/focus/accelerator_han...
File views/focus/accelerator_handler.h (right):

http://codereview.chromium.org/5110011/diff/37001/views/focus/accelerator_han...
views/focus/accelerator_handler.h:39: virtual
MessagePumpGlibXDispatcher::DispatchStatus Dispatch(XEvent* xev);
On 2010/11/23 23:38:54, rjkroege wrote:
> how does this get this here definition-wise?

From base/message_loop.h, which includes base/message_pump_glib_x_dispatcher.h
with TOUCH_UI.

[rjkroege, 2010-11-24 16:59:12]

On 2010/11/24 00:55:21, sadrul wrote:
> http://codereview.chromium.org/5110011/diff/37001/base/message_pump_glib.h
> File base/message_pump_glib.h (right):
> 
>
http://codereview.chromium.org/5110011/diff/37001/base/message_pump_glib.h#ne...
> base/message_pump_glib.h:97: struct RunState {
> On 2010/11/23 23:38:54, rjkroege wrote:
> > why did you have to move this into the include? It could be a separate
class?
> 
> the |should_quit| needs to be set from inside MessagePumpGlibX, i.e. it needs
to
> be able to see |should_quit|. So the definition of the struct (which was
already
> declared inside MessagePumpGlib) needed to move into here.

ok

> 
> http://codereview.chromium.org/5110011/diff/37001/base/message_pump_glib_x.h
> File base/message_pump_glib_x.h (right):
> 
>
http://codereview.chromium.org/5110011/diff/37001/base/message_pump_glib_x.h#...
> base/message_pump_glib_x.h:65: gboolean (*gdkdispatcher_)(GSource*,
GSourceFunc,
> void*);
> On 2010/11/23 23:38:54, rjkroege wrote:
> > Per the discussion with beng, this code will need to be adjusted.
> 
> I am unsure how it would need to change.

So am I. This comment was advisory -- no action needed yet.

> 
> > Also, what happens to xinput2 handling when inside this case?
> 
> The menu themselves receive do not receive XInput2 events. This is because the
> Xwindows for the views-menus aren't created by GTK+, and so we do not set them
> up to receive XInput2 events. However, they do still receive XInput events,
i.e.
> they can still be focused, navigated and clicked on.

This means that the menus cannot receive touch events correct? If this is so,
please add a TODO to rectify this (and fix this) in a subsequent CL.

> 
>
http://codereview.chromium.org/5110011/diff/37001/views/focus/accelerator_han...
> File views/focus/accelerator_handler.h (right):
> 
>
http://codereview.chromium.org/5110011/diff/37001/views/focus/accelerator_han...
> views/focus/accelerator_handler.h:39: virtual
> MessagePumpGlibXDispatcher::DispatchStatus Dispatch(XEvent* xev);
> On 2010/11/23 23:38:54, rjkroege wrote:
> > how does this get this here definition-wise?
> 
> From base/message_loop.h, which includes base/message_pump_glib_x_dispatcher.h
> with TOUCH_UI.

OK.

LGTM (with nits)

[sadrul, 2010-11-24 17:35:49]

[snip]
> This means that the menus cannot receive touch events correct? If this is so,
> please add a TODO to rectify this (and fix this) in a subsequent CL.

Done.

> LGTM (with nits)

Thanks!

[sadrul, 2010-11-29 15:56:56]

http://codereview.chromium.org/5110011/diff/38020/base/message_pump_glib_x.cc
File base/message_pump_glib_x.cc (right):

http://codereview.chromium.org/5110011/diff/38020/base/message_pump_glib_x.cc...
base/message_pump_glib_x.cc:112: state_->should_quit = true;
Instead of setting |should_quit| here, we can simply do 'Quit();' instead. In
that case, the changes made in message_pump_glib.cc|h will not be necessary. Is
that preferrable?

[rjkroege, 2010-11-29 16:50:12]

On 2010/11/29 15:56:56, sadrul wrote:
> http://codereview.chromium.org/5110011/diff/38020/base/message_pump_glib_x.cc
> File base/message_pump_glib_x.cc (right):
> 
>
http://codereview.chromium.org/5110011/diff/38020/base/message_pump_glib_x.cc...
> base/message_pump_glib_x.cc:112: state_->should_quit = true;
> Instead of setting |should_quit| here, we can simply do 'Quit();' instead. In
> that case, the changes made in message_pump_glib.cc|h will not be necessary.
Is
> that preferrable?

I think so.

[rjkroege, 2010-11-29 17:09:42]

LGTM.