Bug 85919 - [chromium] Trigger context menu for long press gesture
Summary: [chromium] Trigger context menu for long press gesture
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Varun Jain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-08 14:59 PDT by Varun Jain
Modified: 2012-05-10 11:42 PDT (History)
3 users (show)

See Also:

Attachments
Patch (5.12 KB, patch)
2012-05-08 15:02 PDT, Varun Jain 		no flags Details | Formatted Diff | Diff
Patch (10.81 KB, patch)
2012-05-09 10:45 PDT, Varun Jain 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Varun Jain 2012-05-08 14:59:49 PDT 
[chromium] Trigger context menu for long press gesture
Comment 1 Varun Jain 2012-05-08 15:02:13 PDT 
Created attachment 140792 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-05-08 15:09:21 PDT 
Comment on attachment 140792 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140792&action=review

How do we test this?

> Source/WebCore/page/EventHandler.cpp:2603
> +#if OS(WINDOWS)

Why is the Win difference needed here?
Comment 3 Adam Barth 2012-05-08 16:44:47 PDT 
Comment on attachment 140792 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140792&action=review

>> Source/WebCore/page/EventHandler.cpp:2603
>> +#if OS(WINDOWS)
> 
> Why is the Win difference needed here?

I'm not sure this is the right place to put this code, but I suspect that the difference is that there's a difference in when to show context menus on mac and windows.  I think mac shows the context menu on mouse down and windows shows them on mouse up.  I imagine this is analogous.
Comment 4 Varun Jain 2012-05-09 10:45:12 PDT 
Created attachment 140975 [details]
Patch
Comment 5 Varun Jain 2012-05-09 10:48:20 PDT 
(In reply to comment #2)
> (From update of attachment 140792 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140792&action=review
> 
> How do we test this?

Added layout test

> 
> > Source/WebCore/page/EventHandler.cpp:2603
> > +#if OS(WINDOWS)
> 
> Why is the Win difference needed here?

Adam answered this part. Thanks Adam!
Regarding Adam's concern about the placement of this code, I think this is in line with EventHandler::sendContextMenuEventForKey()
Another motivation to put this in EventHandler is that other platforms can call this to show context menu on different gestures.
Comment 6 Varun Jain 2012-05-10 10:13:46 PDT 
ping... eric, abarth: I have addressed all comments. Please take another look.

(In reply to comment #5)
> (In reply to comment #2)
> > (From update of attachment 140792 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=140792&action=review
> > 
> > How do we test this?
> 
> Added layout test
> 
> > 
> > > Source/WebCore/page/EventHandler.cpp:2603
> > > +#if OS(WINDOWS)
> > 
> > Why is the Win difference needed here?
> 
> Adam answered this part. Thanks Adam!
> Regarding Adam's concern about the placement of this code, I think this is in line with EventHandler::sendContextMenuEventForKey()
> Another motivation to put this in EventHandler is that other platforms can call this to show context menu on different gestures.
Comment 7 Adam Barth 2012-05-10 11:01:54 PDT 
Comment on attachment 140975 [details]
Patch

This looks good.  Thanks.
Comment 8 WebKit Review Bot 2012-05-10 11:42:11 PDT 
Comment on attachment 140975 [details]
Patch

Clearing flags on attachment: 140975

Committed r116671: <http://trac.webkit.org/changeset/116671>
Comment 9 WebKit Review Bot 2012-05-10 11:42:16 PDT 
All reviewed patches have been landed.  Closing bug.