Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(26)

Issue 1602903003: Display the context menu on Windows on long press release (Closed)

Created:
4 years, 11 months ago by ananta
Modified:
4 years, 11 months ago
Reviewers:
jam, tdresser
CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, James Su
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Patch Set 2 : Rebased to tip #

Patch Set 3 : Remove space #

Total comments: 6

Patch Set 4 : Address review comments and add unittest #

Patch Set 5 : Update comment and disable the test for android #

Patch Set 6 : Fix build error #

Patch Set 7 : Fix build error #

Patch Set 8 : Fix build error #

Total comments: 6

Patch Set 9 : Address review comments and fix ASAN build redness #

Total comments: 6

Patch Set 10 : Address review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+254 lines, -18 lines) Patch
M content/browser/renderer_host/render_view_host_factory.h View 1 2 3 4 5 6 7 8 9 2 chunks +16 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_view_host_factory.cc View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.h View 1 5 chunks +15 lines, -1 line 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.cc View 1 2 3 2 chunks +53 lines, -2 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura_unittest.cc View 1 2 3 4 5 6 7 8 9 3 chunks +141 lines, -0 lines 0 comments Download
M content/browser/web_contents/web_contents_view_aura.h View 1 2 3 4 5 6 7 8 9 2 chunks +5 lines, -2 lines 0 comments Download
M content/browser/web_contents/web_contents_view_aura.cc View 1 2 3 4 5 6 7 8 9 4 chunks +21 lines, -13 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 21 (8 generated)
ananta
This works well on Windows in my testing. Please take a peek and let me ...
4 years, 11 months ago (2016-01-20 03:54:32 UTC) #2
tdresser
https://codereview.chromium.org/1602903003/diff/40001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1602903003/diff/40001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode2726 content/browser/renderer_host/render_widget_host_view_aura.cc:2726: showing_context_menu_ = true; Should showing_context_menu_ be true in the ...
4 years, 11 months ago (2016-01-20 15:26:12 UTC) #3
tdresser
On 2016/01/20 15:26:12, tdresser wrote: > https://codereview.chromium.org/1602903003/diff/40001/content/browser/renderer_host/render_widget_host_view_aura.cc > File content/browser/renderer_host/render_widget_host_view_aura.cc (right): > > https://codereview.chromium.org/1602903003/diff/40001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode2726 > ...
4 years, 11 months ago (2016-01-20 15:26:35 UTC) #4
tdresser
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/renderer_host/render_widget_host_view_aura.cc ...
4 years, 11 months ago (2016-01-20 15:27:24 UTC) #5
ananta
+jam for content owners. jam, I made some changes to the RenderViewHostFactory to allow a ...
4 years, 11 months ago (2016-01-21 02:08:53 UTC) #9
tdresser
LGTM https://codereview.chromium.org/1602903003/diff/140001/content/browser/renderer_host/render_view_host_factory.h File content/browser/renderer_host/render_view_host_factory.h (right): https://codereview.chromium.org/1602903003/diff/140001/content/browser/renderer_host/render_view_host_factory.h#newcode83 content/browser/renderer_host/render_view_host_factory.h:83: // Set to true if we a RenderWidgetHostView ...
4 years, 11 months ago (2016-01-21 15:40:28 UTC) #10
ananta
https://codereview.chromium.org/1602903003/diff/140001/content/browser/renderer_host/render_view_host_factory.h File content/browser/renderer_host/render_view_host_factory.h (right): https://codereview.chromium.org/1602903003/diff/140001/content/browser/renderer_host/render_view_host_factory.h#newcode83 content/browser/renderer_host/render_view_host_factory.h:83: // Set to true if we a RenderWidgetHostView pointer ...
4 years, 11 months ago (2016-01-21 21:11:18 UTC) #11
jam
https://codereview.chromium.org/1602903003/diff/160001/content/browser/renderer_host/render_view_host_factory.h File content/browser/renderer_host/render_view_host_factory.h (right): https://codereview.chromium.org/1602903003/diff/160001/content/browser/renderer_host/render_view_host_factory.h#newcode84 content/browser/renderer_host/render_view_host_factory.h:84: // RenderWidgetHostViewAura pointer in a test. Defaults to false. ...
4 years, 11 months ago (2016-01-21 21:31:10 UTC) #12
ananta
https://codereview.chromium.org/1602903003/diff/160001/content/browser/renderer_host/render_view_host_factory.h File content/browser/renderer_host/render_view_host_factory.h (right): https://codereview.chromium.org/1602903003/diff/160001/content/browser/renderer_host/render_view_host_factory.h#newcode84 content/browser/renderer_host/render_view_host_factory.h:84: // RenderWidgetHostViewAura pointer in a test. Defaults to false. ...
4 years, 11 months ago (2016-01-21 22:17:48 UTC) #13
jam
lgtm
4 years, 11 months ago (2016-01-22 21:30:30 UTC) #14
commit-bot: I haz the power
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
4 years, 11 months ago (2016-01-22 23:21:14 UTC) #17
commit-bot: I haz the power
Committed patchset #10 (id:180001)
4 years, 11 months ago (2016-01-22 23:31:16 UTC) #19
commit-bot: I haz the power
4 years, 11 months ago (2016-01-22 23:32:55 UTC) #21
Message was sent while issue was closed.
Patchset 10 (id:??) landed as
https://crrev.com/4376e70737ba5dccedcfc07c48d0bad5edc00e58
Cr-Commit-Position: refs/heads/master@{#371078}

Powered by Google App Engine
This is Rietveld 408576698