Add ContextMenuSourceType to views::ContextMenuController::ShowContextMenuForView.

Add ContextMenuSourceType to views::ContextMenuController::ShowContextMenuForView.
This will help bring context menu positioning logic to one place and unify the
positioning logic for all views.

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

[varunjain, 2013-06-13 17:48:13]

Hi Scott, This patch was getting out of hand so I wanted to get some feedback
from you on whether this is reasonable addition to ShowContextMenuForView. There
are a few more places where I need to add. I will do those after getting ok from
you.

[sadrul, 2013-06-13 17:53:48]

/cc mohsen@ since we have talked about this.

https://codereview.chromium.org/16979002/diff/1/ui/base/ui_base_types.h
File ui/base/ui_base_types.h (right):

https://codereview.chromium.org/16979002/diff/1/ui/base/ui_base_types.h#newco...
ui/base/ui_base_types.h:46: };
Drive by: can we have a generic EventSourceType instead?

Currently, we have ui::DragDropTypes::DragEventSource,
TabDragController::EventSource, aura::client::WindowMoveSource,
Widget::MoveLoopSource. A generic EventSource[Type] enum would be much cleaner.

[varunjain, 2013-06-13 18:33:01]

https://codereview.chromium.org/16979002/diff/1/ui/base/ui_base_types.h
File ui/base/ui_base_types.h (right):

https://codereview.chromium.org/16979002/diff/1/ui/base/ui_base_types.h#newco...
ui/base/ui_base_types.h:46: };
On 2013/06/13 17:53:48, sadrul wrote:
> Drive by: can we have a generic EventSourceType instead?
> 
> Currently, we have ui::DragDropTypes::DragEventSource,
> TabDragController::EventSource, aura::client::WindowMoveSource,
> Widget::MoveLoopSource. A generic EventSource[Type] enum would be much
cleaner.

Would touch editing make a valid *event* source?

[varunjain, 2013-06-13 18:34:49]

On 2013/06/13 18:33:01, varunjain wrote:
> https://codereview.chromium.org/16979002/diff/1/ui/base/ui_base_types.h
> File ui/base/ui_base_types.h (right):
> 
>
https://codereview.chromium.org/16979002/diff/1/ui/base/ui_base_types.h#newco...
> ui/base/ui_base_types.h:46: };
> On 2013/06/13 17:53:48, sadrul wrote:
> > Drive by: can we have a generic EventSourceType instead?
> > 
> > Currently, we have ui::DragDropTypes::DragEventSource,
> > TabDragController::EventSource, aura::client::WindowMoveSource,
> > Widget::MoveLoopSource. A generic EventSource[Type] enum would be much
> cleaner.
> 
> Would touch editing make a valid *event* source?

I do agree that they need to be combined

[sky, 2013-06-13 19:54:53]

https://codereview.chromium.org/16979002/diff/1/ui/views/controls/menu/menu_r...
File ui/views/controls/menu/menu_runner.h (right):

https://codereview.chromium.org/16979002/diff/1/ui/views/controls/menu/menu_r...
ui/views/controls/menu/menu_runner.h:100: RunResult RunContextMenuAt(Widget*
parent,
We already have RunTypes::CONTEXT_MENU, so we don't need another method for
context menus. How about adding MenuSourceType as a parameter to RunMenuAt?
MenuSourceType could either be defined here, rename ContextMenuSourceType to
MenuSourceType (I haven't investigated the later to make sure that makes sense.

[varunjain, 2013-06-13 20:26:43]

https://codereview.chromium.org/16979002/diff/1/ui/views/controls/menu/menu_r...
File ui/views/controls/menu/menu_runner.h (right):

https://codereview.chromium.org/16979002/diff/1/ui/views/controls/menu/menu_r...
ui/views/controls/menu/menu_runner.h:100: RunResult RunContextMenuAt(Widget*
parent,
On 2013/06/13 19:54:53, sky wrote:
> We already have RunTypes::CONTEXT_MENU, so we don't need another method for
> context menus. How about adding MenuSourceType as a parameter to RunMenuAt?
> MenuSourceType could either be defined here, rename ContextMenuSourceType to
> MenuSourceType (I haven't investigated the later to make sure that makes
sense.

The requirement here is to identify, 1) if its a context menu or not, 2) if its
a context menu, whats the menu source.
How does this look as the definition of MenuSourceType:
MenuSourceType {
  MENU_SOURCE_TYPE_NONE, // non-context menu source type
  MENU_SOURCE_TYPE_CONTEXT_MOUSE,
  MENU_SOURCE_TYPE_CONTEXT_TOUCH,
  MENU_SOURCE_TYPE_CONTEXT_KEYBOARD,
  MENU_SOURCE_TYPE_CONTEXT_TOUCH_EDITING,
}
Also, what are your thoughts on sadrul's suggestion of combining various
*SourceType enums? Having a source type enum specific to menus will make that
harder.

[sky, 2013-06-13 20:59:43]

On Thu, Jun 13, 2013 at 1:26 PM,  <varunjain@chromium.org> wrote:
>
>
https://codereview.chromium.org/16979002/diff/1/ui/views/controls/menu/menu_r...
> File ui/views/controls/menu/menu_runner.h (right):
>
>
https://codereview.chromium.org/16979002/diff/1/ui/views/controls/menu/menu_r...
> ui/views/controls/menu/menu_runner.h:100: RunResult
> RunContextMenuAt(Widget* parent,
> On 2013/06/13 19:54:53, sky wrote:
>>
>> We already have RunTypes::CONTEXT_MENU, so we don't need another
>
> method for
>>
>> context menus. How about adding MenuSourceType as a parameter to
>
> RunMenuAt?
>>
>> MenuSourceType could either be defined here, rename
>
> ContextMenuSourceType to
>>
>> MenuSourceType (I haven't investigated the later to make sure that
>
> makes sense.
>
> The requirement here is to identify, 1) if its a context menu or not,

You already have that by way of RunTypes::CONTEXT_MENU, right?

> 2)
> if its a context menu, whats the menu source.

We should make this work regardless of whether it is a context menu.
That's why I favored MenuSourceMouse.

> How does this look as the definition of MenuSourceType:
> MenuSourceType {
>   MENU_SOURCE_TYPE_NONE, // non-context menu source type
>   MENU_SOURCE_TYPE_CONTEXT_MOUSE,
>   MENU_SOURCE_TYPE_CONTEXT_TOUCH,
>   MENU_SOURCE_TYPE_CONTEXT_KEYBOARD,
>   MENU_SOURCE_TYPE_CONTEXT_TOUCH_EDITING,
> }
> Also, what are your thoughts on sadrul's suggestion of combining various
> *SourceType enums? Having a source type enum specific to menus will make
> that harder.

I'm ok with a generic event source as long all the values apply across all
uses.

  -Scott

>
> https://codereview.chromium.org/16979002/

[varunjain, 2013-06-17 03:35:56]

Hi Scott, Changed to MenuSourceType and added parameter to
MenuRunner::RunMenuAt() as per our discussion. PTAL.

On 2013/06/13 20:59:43, sky wrote:
> On Thu, Jun 13, 2013 at 1:26 PM,  <mailto:varunjain@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/16979002/diff/1/ui/views/controls/menu/menu_r...
> > File ui/views/controls/menu/menu_runner.h (right):
> >
> >
>
https://codereview.chromium.org/16979002/diff/1/ui/views/controls/menu/menu_r...
> > ui/views/controls/menu/menu_runner.h:100: RunResult
> > RunContextMenuAt(Widget* parent,
> > On 2013/06/13 19:54:53, sky wrote:
> >>
> >> We already have RunTypes::CONTEXT_MENU, so we don't need another
> >
> > method for
> >>
> >> context menus. How about adding MenuSourceType as a parameter to
> >
> > RunMenuAt?
> >>
> >> MenuSourceType could either be defined here, rename
> >
> > ContextMenuSourceType to
> >>
> >> MenuSourceType (I haven't investigated the later to make sure that
> >
> > makes sense.
> >
> > The requirement here is to identify, 1) if its a context menu or not,
> 
> You already have that by way of RunTypes::CONTEXT_MENU, right?
> 
> > 2)
> > if its a context menu, whats the menu source.
> 
> We should make this work regardless of whether it is a context menu.
> That's why I favored MenuSourceMouse.
> 
> > How does this look as the definition of MenuSourceType:
> > MenuSourceType {
> >   MENU_SOURCE_TYPE_NONE, // non-context menu source type
> >   MENU_SOURCE_TYPE_CONTEXT_MOUSE,
> >   MENU_SOURCE_TYPE_CONTEXT_TOUCH,
> >   MENU_SOURCE_TYPE_CONTEXT_KEYBOARD,
> >   MENU_SOURCE_TYPE_CONTEXT_TOUCH_EDITING,
> > }
> > Also, what are your thoughts on sadrul's suggestion of combining various
> > *SourceType enums? Having a source type enum specific to menus will make
> > that harder.
> 
> I'm ok with a generic event source as long all the values apply across all
> uses.
> 
>   -Scott
> 
> >
> > https://codereview.chromium.org/16979002/

[sky, 2013-06-17 16:08:19]

https://codereview.chromium.org/16979002/diff/29001/ash/launcher/launcher.cc
File ash/launcher/launcher.cc (right):

https://codereview.chromium.org/16979002/diff/29001/ash/launcher/launcher.cc#...
ash/launcher/launcher.cc:133: void Launcher::ShowContextMenu(const gfx::Point&
location) {
I don't think this is used, can you nuke it? Otherwise it needs a
MenuSourceType.

https://codereview.chromium.org/16979002/diff/29001/ash/launcher/launcher_vie...
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/16979002/diff/29001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1520: ui::MENU_SOURCE_NONE);
This can be invoked from touch or keyboard.

https://codereview.chromium.org/16979002/diff/29001/ash/system/web_notificati...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/16979002/diff/29001/ash/system/web_notificati...
ash/system/web_notification/web_notification_tray.cc:317: ui::MENU_SOURCE_NONE,
Why the none here? Looks like the caller has an event.

https://codereview.chromium.org/16979002/diff/29001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/language_switch_menu.cc (right):

https://codereview.chromium.org/16979002/diff/29001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/language_switch_menu.cc:187: ui::MENU_SOURCE_NONE,
views::MenuRunner::HAS_MNEMONICS) ==
Why the none here?

https://codereview.chromium.org/16979002/diff/29001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/16979002/diff/29001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:550:
ui::MENU_SOURCE_NONE,
Two things, I suspect this should really be context menu (ask estade), and
secondly even if it isn't please pass in the right thing. This comment (about
right source) applies to all similar places in this patch.

https://codereview.chromium.org/16979002/diff/29001/ui/views/controls/menu/me...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/16979002/diff/29001/ui/views/controls/menu/me...
ui/views/controls/menu/menu_controller.cc:962: screen_loc,
ui::MENU_SOURCE_MOUSE);
I think this can be keyboard too. See description of WM_CONTEXTMENU.

[varunjain, 2013-06-17 20:13:55]

Hi Scott, as per our conversation, I tried to remove the usage of
MENU_SOURCE_NONE for all menus. I did get a few of them. But some (16) still
remain. All 16 are implementations of
views::MenuButtonListener::OnMenuButtonClicked() which does not provide the
event that triggered the menu.

https://codereview.chromium.org/16979002/diff/29001/ash/launcher/launcher.cc
File ash/launcher/launcher.cc (right):

https://codereview.chromium.org/16979002/diff/29001/ash/launcher/launcher.cc#...
ash/launcher/launcher.cc:133: void Launcher::ShowContextMenu(const gfx::Point&
location) {
On 2013/06/17 16:08:19, sky wrote:
> I don't think this is used, can you nuke it? Otherwise it needs a
> MenuSourceType.

Done.

https://codereview.chromium.org/16979002/diff/29001/ash/launcher/launcher_vie...
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/16979002/diff/29001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1520: ui::MENU_SOURCE_NONE);
On 2013/06/17 16:08:19, sky wrote:
> This can be invoked from touch or keyboard.

Done.

https://codereview.chromium.org/16979002/diff/29001/ash/system/web_notificati...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/16979002/diff/29001/ash/system/web_notificati...
ash/system/web_notification/web_notification_tray.cc:317: ui::MENU_SOURCE_NONE,
On 2013/06/17 16:08:19, sky wrote:
> Why the none here? Looks like the caller has an event.

Done.

https://codereview.chromium.org/16979002/diff/29001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/language_switch_menu.cc (right):

https://codereview.chromium.org/16979002/diff/29001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/language_switch_menu.cc:187: ui::MENU_SOURCE_NONE,
views::MenuRunner::HAS_MNEMONICS) ==
On 2013/06/17 16:08:19, sky wrote:
> Why the none here?

This is one of the instances that cannot be fixed without change to
views::MenuButtonListener.

https://codereview.chromium.org/16979002/diff/29001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/16979002/diff/29001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:550:
ui::MENU_SOURCE_NONE,
On 2013/06/17 16:08:19, sky wrote:
> Two things, I suspect this should really be context menu (ask estade), and
> secondly even if it isn't please pass in the right thing. This comment (about
> right source) applies to all similar places in this patch.

Done.

https://codereview.chromium.org/16979002/diff/29001/ui/views/controls/menu/me...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/16979002/diff/29001/ui/views/controls/menu/me...
ui/views/controls/menu/menu_controller.cc:962: screen_loc,
ui::MENU_SOURCE_MOUSE);
On 2013/06/17 16:08:19, sky wrote:
> I think this can be keyboard too. See description of WM_CONTEXTMENU.

Done.

[sky, 2013-06-17 20:34:13]

On Mon, Jun 17, 2013 at 1:13 PM,  <varunjain@chromium.org> wrote:
> Hi Scott, as per our conversation, I tried to remove the usage of
> MENU_SOURCE_NONE for all menus. I did get a few of them. But some (16) still
> remain. All 16 are implementations of
> views::MenuButtonListener::OnMenuButtonClicked() which does not provide the
> event that triggered the menu.

If you don't intend to make all uses consistent, which is ok with me,
document it.

  -Scott

>
>
>
> https://codereview.chromium.org/16979002/diff/29001/ash/launcher/launcher.cc
> File ash/launcher/launcher.cc (right):
>
>
https://codereview.chromium.org/16979002/diff/29001/ash/launcher/launcher.cc#...
> ash/launcher/launcher.cc:133: void Launcher::ShowContextMenu(const
> gfx::Point& location) {
> On 2013/06/17 16:08:19, sky wrote:
>>
>> I don't think this is used, can you nuke it? Otherwise it needs a
>> MenuSourceType.
>
>
> Done.
>
>
>
https://codereview.chromium.org/16979002/diff/29001/ash/launcher/launcher_vie...
> File ash/launcher/launcher_view.cc (right):
>
>
https://codereview.chromium.org/16979002/diff/29001/ash/launcher/launcher_vie...
> ash/launcher/launcher_view.cc:1520: ui::MENU_SOURCE_NONE);
> On 2013/06/17 16:08:19, sky wrote:
>>
>> This can be invoked from touch or keyboard.
>
>
> Done.
>
>
>
https://codereview.chromium.org/16979002/diff/29001/ash/system/web_notificati...
> File ash/system/web_notification/web_notification_tray.cc (right):
>
>
https://codereview.chromium.org/16979002/diff/29001/ash/system/web_notificati...
> ash/system/web_notification/web_notification_tray.cc:317:
> ui::MENU_SOURCE_NONE,
> On 2013/06/17 16:08:19, sky wrote:
>>
>> Why the none here? Looks like the caller has an event.
>
>
> Done.
>
>
>
https://codereview.chromium.org/16979002/diff/29001/chrome/browser/chromeos/l...
> File chrome/browser/chromeos/login/language_switch_menu.cc (right):
>
>
https://codereview.chromium.org/16979002/diff/29001/chrome/browser/chromeos/l...
> chrome/browser/chromeos/login/language_switch_menu.cc:187:
> ui::MENU_SOURCE_NONE, views::MenuRunner::HAS_MNEMONICS) ==
> On 2013/06/17 16:08:19, sky wrote:
>>
>> Why the none here?
>
>
> This is one of the instances that cannot be fixed without change to
> views::MenuButtonListener.
>
>
>
https://codereview.chromium.org/16979002/diff/29001/chrome/browser/ui/views/a...
> File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):
>
>
https://codereview.chromium.org/16979002/diff/29001/chrome/browser/ui/views/a...
> chrome/browser/ui/views/autofill/autofill_dialog_views.cc:550:
> ui::MENU_SOURCE_NONE,
> On 2013/06/17 16:08:19, sky wrote:
>>
>> Two things, I suspect this should really be context menu (ask estade),
>
> and
>>
>> secondly even if it isn't please pass in the right thing. This comment
>
> (about
>>
>> right source) applies to all similar places in this patch.
>
>
> Done.
>
>
>
https://codereview.chromium.org/16979002/diff/29001/ui/views/controls/menu/me...
> File ui/views/controls/menu/menu_controller.cc (right):
>
>
https://codereview.chromium.org/16979002/diff/29001/ui/views/controls/menu/me...
> ui/views/controls/menu/menu_controller.cc:962: screen_loc,
> ui::MENU_SOURCE_MOUSE);
> On 2013/06/17 16:08:19, sky wrote:
>>
>> I think this can be keyboard too. See description of WM_CONTEXTMENU.
>
>
> Done.
>
> https://codereview.chromium.org/16979002/

[sky, 2013-06-17 20:44:07]

LGTM

[varunjain, 2013-06-17 20:57:35]

On 2013/06/17 20:34:13, sky wrote:
> On Mon, Jun 17, 2013 at 1:13 PM,  <mailto:varunjain@chromium.org> wrote:
> > Hi Scott, as per our conversation, I tried to remove the usage of
> > MENU_SOURCE_NONE for all menus. I did get a few of them. But some (16) still
> > remain. All 16 are implementations of
> > views::MenuButtonListener::OnMenuButtonClicked() which does not provide the
> > event that triggered the menu.
> 
> If you don't intend to make all uses consistent, which is ok with me,
> document it.

Added TODO to fix the remaining cases.

> 
>   -Scott
> 
> >
> >
> >
> > https://codereview.chromium.org/16979002/diff/29001/ash/launcher/launcher.cc
> > File ash/launcher/launcher.cc (right):
> >
> >
>
https://codereview.chromium.org/16979002/diff/29001/ash/launcher/launcher.cc#...
> > ash/launcher/launcher.cc:133: void Launcher::ShowContextMenu(const
> > gfx::Point& location) {
> > On 2013/06/17 16:08:19, sky wrote:
> >>
> >> I don't think this is used, can you nuke it? Otherwise it needs a
> >> MenuSourceType.
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/16979002/diff/29001/ash/launcher/launcher_vie...
> > File ash/launcher/launcher_view.cc (right):
> >
> >
>
https://codereview.chromium.org/16979002/diff/29001/ash/launcher/launcher_vie...
> > ash/launcher/launcher_view.cc:1520: ui::MENU_SOURCE_NONE);
> > On 2013/06/17 16:08:19, sky wrote:
> >>
> >> This can be invoked from touch or keyboard.
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/16979002/diff/29001/ash/system/web_notificati...
> > File ash/system/web_notification/web_notification_tray.cc (right):
> >
> >
>
https://codereview.chromium.org/16979002/diff/29001/ash/system/web_notificati...
> > ash/system/web_notification/web_notification_tray.cc:317:
> > ui::MENU_SOURCE_NONE,
> > On 2013/06/17 16:08:19, sky wrote:
> >>
> >> Why the none here? Looks like the caller has an event.
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/16979002/diff/29001/chrome/browser/chromeos/l...
> > File chrome/browser/chromeos/login/language_switch_menu.cc (right):
> >
> >
>
https://codereview.chromium.org/16979002/diff/29001/chrome/browser/chromeos/l...
> > chrome/browser/chromeos/login/language_switch_menu.cc:187:
> > ui::MENU_SOURCE_NONE, views::MenuRunner::HAS_MNEMONICS) ==
> > On 2013/06/17 16:08:19, sky wrote:
> >>
> >> Why the none here?
> >
> >
> > This is one of the instances that cannot be fixed without change to
> > views::MenuButtonListener.
> >
> >
> >
>
https://codereview.chromium.org/16979002/diff/29001/chrome/browser/ui/views/a...
> > File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):
> >
> >
>
https://codereview.chromium.org/16979002/diff/29001/chrome/browser/ui/views/a...
> > chrome/browser/ui/views/autofill/autofill_dialog_views.cc:550:
> > ui::MENU_SOURCE_NONE,
> > On 2013/06/17 16:08:19, sky wrote:
> >>
> >> Two things, I suspect this should really be context menu (ask estade),
> >
> > and
> >>
> >> secondly even if it isn't please pass in the right thing. This comment
> >
> > (about
> >>
> >> right source) applies to all similar places in this patch.
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/16979002/diff/29001/ui/views/controls/menu/me...
> > File ui/views/controls/menu/menu_controller.cc (right):
> >
> >
>
https://codereview.chromium.org/16979002/diff/29001/ui/views/controls/menu/me...
> > ui/views/controls/menu/menu_controller.cc:962: screen_loc,
> > ui::MENU_SOURCE_MOUSE);
> > On 2013/06/17 16:08:19, sky wrote:
> >>
> >> I think this can be keyboard too. See description of WM_CONTEXTMENU.
> >
> >
> > Done.
> >
> > https://codereview.chromium.org/16979002/

[commit-bot: I haz the power, 2013-06-18 13:50:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/16979002/69001

[commit-bot: I haz the power, 2013-06-18 16:54:32]

Change committed as 207019