Display the context menu on Windows on long press release

The context menu on the web page on Windows should be displayed when the long press gesture is released rather than while it is pressed.

This is inconsistent with the way the long press context menu gesture works on Windows. Additionally the long press gesture in Chrome displays the
context menu on the tabstrip and the caption when the touch is released which makes it super confusing.

Changes in this patch are as below:-

1. The RenderWidgetHostViewAura::OnShowContextMenu function now returns a bool indicating whether the context menu should be displayed or not. It currently
   returns false on Windows if the menu is being displayed in response to a touch gesture. If yes then it sets a state indicating that the context menu
   needs to display when the long press gesture is released via a touch up.
   On other platforms it returns true.

2, The WebContentsViewAura::ShowContextMenu function which gets invoked when the context menu is requested by the web page calls the
   RenderWidgetHostViewAura::OnShowContextMenu function and if it returns false does not display the context menu.

3. The RenderWidgetHostViewAura::HandleGesutureForTouchSelection handles the ET_GESTURE_LONG_TAP on Windows and if we have a pending context menu request, it
   calls the WebContentsViewAura::ShowContextMenu function to display the context menu with the menu source now set to mouse.

BUG=562739
.
TEST=RenderWidgetHostViewAuraWithViewHarnessTest.ContextMenuTest

Committed: https://crrev.com/4376e70737ba5dccedcfc07c48d0bad5edc00e58
Cr-Commit-Position: refs/heads/master@{#371078}

[ananta, 2016-01-20 03:54:32]

ananta@chromium.org changed reviewers:
+ tdresser@chromium.org

[ananta, 2016-01-20 03:54:32]

This works well on Windows in my testing. Please take a peek and let me know
your thoughts.

[tdresser, 2016-01-20 15:26:12]

https://codereview.chromium.org/1602903003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1602903003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2726:
showing_context_menu_ = true;
Should showing_context_menu_ be true in the case where we return false?

https://codereview.chromium.org/1602903003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2953:
ContextMenuParams params = *last_context_menu_params_;
You shouldn't need to copy the ContextMenuParams here.
I think you can make params a scoped_ptr, and then std::move to it?

https://codereview.chromium.org/1602903003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2955:
params.source_type = ui::MENU_SOURCE_MOUSE;
It might be worth adding a comment here - settings the source_type to mouse is a
bit unintuitive.

[tdresser, 2016-01-20 15:26:35]

On 2016/01/20 15:26:12, tdresser wrote:
>
https://codereview.chromium.org/1602903003/diff/40001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/1602903003/diff/40001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_aura.cc:2726:
> showing_context_menu_ = true;
> Should showing_context_menu_ be true in the case where we return false?
> 
>
https://codereview.chromium.org/1602903003/diff/40001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_aura.cc:2953:
> ContextMenuParams params = *last_context_menu_params_;
> You shouldn't need to copy the ContextMenuParams here.
> I think you can make params a scoped_ptr, and then std::move to it?
> 
>
https://codereview.chromium.org/1602903003/diff/40001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_aura.cc:2955:
> params.source_type = ui::MENU_SOURCE_MOUSE;
> It might be worth adding a comment here - settings the source_type to mouse is
a
> bit unintuitive.

Can you shorten the CL title?

[tdresser, 2016-01-20 15:27:24]

On 2016/01/20 15:26:35, tdresser wrote:
> On 2016/01/20 15:26:12, tdresser wrote:
> >
>
https://codereview.chromium.org/1602903003/diff/40001/content/browser/rendere...
> > File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> > 
> >
>
https://codereview.chromium.org/1602903003/diff/40001/content/browser/rendere...
> > content/browser/renderer_host/render_widget_host_view_aura.cc:2726:
> > showing_context_menu_ = true;
> > Should showing_context_menu_ be true in the case where we return false?
> > 
> >
>
https://codereview.chromium.org/1602903003/diff/40001/content/browser/rendere...
> > content/browser/renderer_host/render_widget_host_view_aura.cc:2953:
> > ContextMenuParams params = *last_context_menu_params_;
> > You shouldn't need to copy the ContextMenuParams here.
> > I think you can make params a scoped_ptr, and then std::move to it?
> > 
> >
>
https://codereview.chromium.org/1602903003/diff/40001/content/browser/rendere...
> > content/browser/renderer_host/render_widget_host_view_aura.cc:2955:
> > params.source_type = ui::MENU_SOURCE_MOUSE;
> > It might be worth adding a comment here - settings the source_type to mouse
is
> a
> > bit unintuitive.
> 
> Can you shorten the CL title?

Could we add a test to render_widget_host_view_aura_unittest.cc?

[ananta, 2016-01-21 01:33:12]

Description was changed from

==========
The context menu on the web page on Windows should be displayed when the long
press gesture is released rather than while it is pressed.

This is inconsistent with the way the long press context menu gesture works on
Windows. Additionally the long press gesture in Chrome displays the
context menu on the tabstrip and the caption when the touch is released which
makes it super confusing.

Changes in this patch are as below:-

1. The RenderWidgetHostViewAura::OnShowContextMenu function now returns a bool
indicating whether the context menu should be displayed or not. It currently
   returns false on Windows if the menu is being displayed in response to a
touch gesture. If yes then it sets a state indicating that the context menu
   needs to display when the long press gesture is released via a touch up.
   On other platforms it returns true.

2, The WebContentsViewAura::ShowContextMenu function which gets invoked when the
context menu is requested by the web page calls the
   RenderWidgetHostViewAura::OnShowContextMenu function and if it returns false
does not display the context menu.

3. The RenderWidgetHostViewAura::HandleGesutureForTouchSelection handles the
ET_GESTURE_LONG_TAP on Windows and if we have a pending context menu request, it
   calls the WebContentsViewAura::ShowContextMenu function to display the
context menu with the menu source now set to mouse.

BUG=562739.
==========

to

==========
The context menu on the web page on Windows should be displayed when the long
press gesture is released rather than while it is pressed.

This is inconsistent with the way the long press context menu gesture works on
Windows. Additionally the long press gesture in Chrome displays the
context menu on the tabstrip and the caption when the touch is released which
makes it super confusing.

Changes in this patch are as below:-

1. The RenderWidgetHostViewAura::OnShowContextMenu function now returns a bool
indicating whether the context menu should be displayed or not. It currently
   returns false on Windows if the menu is being displayed in response to a
touch gesture. If yes then it sets a state indicating that the context menu
   needs to display when the long press gesture is released via a touch up.
   On other platforms it returns true.

2, The WebContentsViewAura::ShowContextMenu function which gets invoked when the
context menu is requested by the web page calls the
   RenderWidgetHostViewAura::OnShowContextMenu function and if it returns false
does not display the context menu.

3. The RenderWidgetHostViewAura::HandleGesutureForTouchSelection handles the
ET_GESTURE_LONG_TAP on Windows and if we have a pending context menu request, it
   calls the WebContentsViewAura::ShowContextMenu function to display the
context menu with the menu source now set to mouse.

BUG=562739.
==========

[ananta, 2016-01-21 01:37:20]

Description was changed from

==========
The context menu on the web page on Windows should be displayed when the long
press gesture is released rather than while it is pressed.

This is inconsistent with the way the long press context menu gesture works on
Windows. Additionally the long press gesture in Chrome displays the
context menu on the tabstrip and the caption when the touch is released which
makes it super confusing.

Changes in this patch are as below:-

1. The RenderWidgetHostViewAura::OnShowContextMenu function now returns a bool
indicating whether the context menu should be displayed or not. It currently
   returns false on Windows if the menu is being displayed in response to a
touch gesture. If yes then it sets a state indicating that the context menu
   needs to display when the long press gesture is released via a touch up.
   On other platforms it returns true.

2, The WebContentsViewAura::ShowContextMenu function which gets invoked when the
context menu is requested by the web page calls the
   RenderWidgetHostViewAura::OnShowContextMenu function and if it returns false
does not display the context menu.

3. The RenderWidgetHostViewAura::HandleGesutureForTouchSelection handles the
ET_GESTURE_LONG_TAP on Windows and if we have a pending context menu request, it
   calls the WebContentsViewAura::ShowContextMenu function to display the
context menu with the menu source now set to mouse.

BUG=562739.
==========

to

==========
The context menu on the web page on Windows should be displayed when the long
press gesture is released rather than while it is pressed.

This is inconsistent with the way the long press context menu gesture works on
Windows. Additionally the long press gesture in Chrome displays the
context menu on the tabstrip and the caption when the touch is released which
makes it super confusing.

Changes in this patch are as below:-

1. The RenderWidgetHostViewAura::OnShowContextMenu function now returns a bool
indicating whether the context menu should be displayed or not. It currently
   returns false on Windows if the menu is being displayed in response to a
touch gesture. If yes then it sets a state indicating that the context menu
   needs to display when the long press gesture is released via a touch up.
   On other platforms it returns true.

2, The WebContentsViewAura::ShowContextMenu function which gets invoked when the
context menu is requested by the web page calls the
   RenderWidgetHostViewAura::OnShowContextMenu function and if it returns false
does not display the context menu.

3. The RenderWidgetHostViewAura::HandleGesutureForTouchSelection handles the
ET_GESTURE_LONG_TAP on Windows and if we have a pending context menu request, it
   calls the WebContentsViewAura::ShowContextMenu function to display the
context menu with the menu source now set to mouse.

BUG=562739.
TEST=RenderWidgetHostViewAuraWithViewHarnessTest.ContextMenuTest
==========

[ananta, 2016-01-21 02:08:53]

ananta@chromium.org changed reviewers:
+ jam@chromium.org

[ananta, 2016-01-21 02:08:53]

+jam for content owners. jam, I made some changes to the
RenderViewHostFactory to allow a cast to RenderWidgetHostViewAura for the
duration of the test. The flag defaults to false. Some other changes include
providing a method on the WebContentsViewAura to set the delegate explicitly.
This is only used by the test at the moment.

https://codereview.chromium.org/1602903003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1602903003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2726:
showing_context_menu_ = true;
On 2016/01/20 15:26:12, tdresser wrote:
> Should showing_context_menu_ be true in the case where we return false?

Yeah. Good point. Moved it to after the if check

https://codereview.chromium.org/1602903003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2953:
ContextMenuParams params = *last_context_menu_params_;
On 2016/01/20 15:26:12, tdresser wrote:
> You shouldn't need to copy the ContextMenuParams here.
> I think you can make params a scoped_ptr, and then std::move to it?

Done.

https://codereview.chromium.org/1602903003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2955:
params.source_type = ui::MENU_SOURCE_MOUSE;
On 2016/01/20 15:26:12, tdresser wrote:
> It might be worth adding a comment here - settings the source_type to mouse is
a
> bit unintuitive.

Done.

[tdresser, 2016-01-21 15:40:28]

LGTM

https://codereview.chromium.org/1602903003/diff/140001/content/browser/render...
File content/browser/renderer_host/render_view_host_factory.h (right):

https://codereview.chromium.org/1602903003/diff/140001/content/browser/render...
content/browser/renderer_host/render_view_host_factory.h:83: // Set to true if
we a RenderWidgetHostView pointer can be casted to
Fix "if we a"

https://codereview.chromium.org/1602903003/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1602903003/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2963:
context_menu_params->source_type = ui::MENU_SOURCE_MOUSE;
I think it would be slightly preferable to have a MENU_SOURCE_LONG_PRESS and
MENU_SOURCE_LONG_TAP, so we didn't have to use MENU_SOURCE_MOUSE here, but this
is fine with me as is.

https://codereview.chromium.org/1602903003/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/1602903003/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3654: #if
!defined(OS_ANDROID)
Is this define necessary?

[ananta, 2016-01-21 21:11:18]

https://codereview.chromium.org/1602903003/diff/140001/content/browser/render...
File content/browser/renderer_host/render_view_host_factory.h (right):

https://codereview.chromium.org/1602903003/diff/140001/content/browser/render...
content/browser/renderer_host/render_view_host_factory.h:83: // Set to true if
we a RenderWidgetHostView pointer can be casted to
On 2016/01/21 15:40:27, tdresser wrote:
> Fix "if we a"

Done.

https://codereview.chromium.org/1602903003/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1602903003/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2963:
context_menu_params->source_type = ui::MENU_SOURCE_MOUSE;
On 2016/01/21 15:40:28, tdresser wrote:
> I think it would be slightly preferable to have a MENU_SOURCE_LONG_PRESS and
> MENU_SOURCE_LONG_TAP, so we didn't have to use MENU_SOURCE_MOUSE here, but
this
> is fine with me as is.

Yeah. Once this stabilizes we can add the new enum

https://codereview.chromium.org/1602903003/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/1602903003/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3654: #if
!defined(OS_ANDROID)
On 2016/01/21 15:40:28, tdresser wrote:
> Is this define necessary?

Yeah. Removed. Android does not have Aura.

[jam, 2016-01-21 21:31:10]

https://codereview.chromium.org/1602903003/diff/160001/content/browser/render...
File content/browser/renderer_host/render_view_host_factory.h (right):

https://codereview.chromium.org/1602903003/diff/160001/content/browser/render...
content/browser/renderer_host/render_view_host_factory.h:84: //
RenderWidgetHostViewAura pointer in a test. Defaults to false.
you're mentioning aura, but this can also be called on mac if someone set it to
true...

perhaps take out the aura bit, and just say that if it's true, the
renderviewhost object isn't a test one?

https://codereview.chromium.org/1602903003/diff/160001/content/browser/web_co...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/1602903003/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_view_aura.cc:96: &&
this indentation seems odd? why isn't this on the previous line?

https://codereview.chromium.org/1602903003/diff/160001/content/browser/web_co...
File content/browser/web_contents/web_contents_view_aura.h (right):

https://codereview.chromium.org/1602903003/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_view_aura.h:54: void
SetDelegate(WebContentsViewDelegate* delegate);
nit: add ForTesting suffix

[ananta, 2016-01-21 22:17:48]

https://codereview.chromium.org/1602903003/diff/160001/content/browser/render...
File content/browser/renderer_host/render_view_host_factory.h (right):

https://codereview.chromium.org/1602903003/diff/160001/content/browser/render...
content/browser/renderer_host/render_view_host_factory.h:84: //
RenderWidgetHostViewAura pointer in a test. Defaults to false.
On 2016/01/21 21:31:09, jam wrote:
> you're mentioning aura, but this can also be called on mac if someone set it
to
> true...
> 
> perhaps take out the aura bit, and just say that if it's true, the
> renderviewhost object isn't a test one?

Done. I renamed the flag to is_real_render_view_host_

https://codereview.chromium.org/1602903003/diff/160001/content/browser/web_co...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/1602903003/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_view_aura.cc:96: &&
On 2016/01/21 21:31:09, jam wrote:
> this indentation seems odd? why isn't this on the previous line?

Fixed

https://codereview.chromium.org/1602903003/diff/160001/content/browser/web_co...
File content/browser/web_contents/web_contents_view_aura.h (right):

https://codereview.chromium.org/1602903003/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_view_aura.h:54: void
SetDelegate(WebContentsViewDelegate* delegate);
On 2016/01/21 21:31:09, jam wrote:
> nit: add ForTesting suffix

Done.

[jam, 2016-01-22 21:30:30]

lgtm

[ananta, 2016-01-22 23:20:33]

The CQ bit was checked by ananta@chromium.org

[ananta, 2016-01-22 23:20:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/1602903003/#ps180001
(title: "Address review comments")

[commit-bot: I haz the power, 2016-01-22 23:21:14]

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

[commit-bot: I haz the power, 2016-01-22 23:31:15]

Description was changed from

==========
The context menu on the web page on Windows should be displayed when the long
press gesture is released rather than while it is pressed.

This is inconsistent with the way the long press context menu gesture works on
Windows. Additionally the long press gesture in Chrome displays the
context menu on the tabstrip and the caption when the touch is released which
makes it super confusing.

Changes in this patch are as below:-

1. The RenderWidgetHostViewAura::OnShowContextMenu function now returns a bool
indicating whether the context menu should be displayed or not. It currently
   returns false on Windows if the menu is being displayed in response to a
touch gesture. If yes then it sets a state indicating that the context menu
   needs to display when the long press gesture is released via a touch up.
   On other platforms it returns true.

2, The WebContentsViewAura::ShowContextMenu function which gets invoked when the
context menu is requested by the web page calls the
   RenderWidgetHostViewAura::OnShowContextMenu function and if it returns false
does not display the context menu.

3. The RenderWidgetHostViewAura::HandleGesutureForTouchSelection handles the
ET_GESTURE_LONG_TAP on Windows and if we have a pending context menu request, it
   calls the WebContentsViewAura::ShowContextMenu function to display the
context menu with the menu source now set to mouse.

BUG=562739.
TEST=RenderWidgetHostViewAuraWithViewHarnessTest.ContextMenuTest
==========

to

==========
The context menu on the web page on Windows should be displayed when the long
press gesture is released rather than while it is pressed.

This is inconsistent with the way the long press context menu gesture works on
Windows. Additionally the long press gesture in Chrome displays the
context menu on the tabstrip and the caption when the touch is released which
makes it super confusing.

Changes in this patch are as below:-

1. The RenderWidgetHostViewAura::OnShowContextMenu function now returns a bool
indicating whether the context menu should be displayed or not. It currently
   returns false on Windows if the menu is being displayed in response to a
touch gesture. If yes then it sets a state indicating that the context menu
   needs to display when the long press gesture is released via a touch up.
   On other platforms it returns true.

2, The WebContentsViewAura::ShowContextMenu function which gets invoked when the
context menu is requested by the web page calls the
   RenderWidgetHostViewAura::OnShowContextMenu function and if it returns false
does not display the context menu.

3. The RenderWidgetHostViewAura::HandleGesutureForTouchSelection handles the
ET_GESTURE_LONG_TAP on Windows and if we have a pending context menu request, it
   calls the WebContentsViewAura::ShowContextMenu function to display the
context menu with the menu source now set to mouse.

BUG=562739.
TEST=RenderWidgetHostViewAuraWithViewHarnessTest.ContextMenuTest
==========

[commit-bot: I haz the power, 2016-01-22 23:31:16]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-01-22 23:32:54]

Description was changed from

==========
The context menu on the web page on Windows should be displayed when the long
press gesture is released rather than while it is pressed.

This is inconsistent with the way the long press context menu gesture works on
Windows. Additionally the long press gesture in Chrome displays the
context menu on the tabstrip and the caption when the touch is released which
makes it super confusing.

Changes in this patch are as below:-

1. The RenderWidgetHostViewAura::OnShowContextMenu function now returns a bool
indicating whether the context menu should be displayed or not. It currently
   returns false on Windows if the menu is being displayed in response to a
touch gesture. If yes then it sets a state indicating that the context menu
   needs to display when the long press gesture is released via a touch up.
   On other platforms it returns true.

2, The WebContentsViewAura::ShowContextMenu function which gets invoked when the
context menu is requested by the web page calls the
   RenderWidgetHostViewAura::OnShowContextMenu function and if it returns false
does not display the context menu.

3. The RenderWidgetHostViewAura::HandleGesutureForTouchSelection handles the
ET_GESTURE_LONG_TAP on Windows and if we have a pending context menu request, it
   calls the WebContentsViewAura::ShowContextMenu function to display the
context menu with the menu source now set to mouse.

BUG=562739.
TEST=RenderWidgetHostViewAuraWithViewHarnessTest.ContextMenuTest
==========

to

==========
The context menu on the web page on Windows should be displayed when the long
press gesture is released rather than while it is pressed.

This is inconsistent with the way the long press context menu gesture works on
Windows. Additionally the long press gesture in Chrome displays the
context menu on the tabstrip and the caption when the touch is released which
makes it super confusing.

Changes in this patch are as below:-

1. The RenderWidgetHostViewAura::OnShowContextMenu function now returns a bool
indicating whether the context menu should be displayed or not. It currently
   returns false on Windows if the menu is being displayed in response to a
touch gesture. If yes then it sets a state indicating that the context menu
   needs to display when the long press gesture is released via a touch up.
   On other platforms it returns true.

2, The WebContentsViewAura::ShowContextMenu function which gets invoked when the
context menu is requested by the web page calls the
   RenderWidgetHostViewAura::OnShowContextMenu function and if it returns false
does not display the context menu.

3. The RenderWidgetHostViewAura::HandleGesutureForTouchSelection handles the
ET_GESTURE_LONG_TAP on Windows and if we have a pending context menu request, it
   calls the WebContentsViewAura::ShowContextMenu function to display the
context menu with the menu source now set to mouse.

BUG=562739.
TEST=RenderWidgetHostViewAuraWithViewHarnessTest.ContextMenuTest

Committed: https://crrev.com/4376e70737ba5dccedcfc07c48d0bad5edc00e58
Cr-Commit-Position: refs/heads/master@{#371078}
==========

[commit-bot: I haz the power, 2016-01-22 23:32:55]

Patchset 10 (id:??) landed as
https://crrev.com/4376e70737ba5dccedcfc07c48d0bad5edc00e58
Cr-Commit-Position: refs/heads/master@{#371078}