Bug 135682

Summary: Runtime switch for long mouse press gesture. Part of 135257 - Add long mouse press gesture
Product: WebKit Reporter: Peyton Randolph <prandolph>
Component: UI EventsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: commit-queue, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 135476    
Bug Blocks: 135257, 135721    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Peyton Randolph 2014-08-06 18:03:06 PDT
Since the long mouse press changes certain interactions with the page, it would be nice to have a runtime switch so clients can opt into the long mouse press.
Comment 1 Peyton Randolph 2014-08-06 18:08:48 PDT
Created attachment 236159 [details]
Patch
Comment 2 Peyton Randolph 2014-08-12 13:57:29 PDT
Created attachment 236466 [details]
Patch
Comment 3 Tim Horton 2014-08-12 14:07:44 PDT
Comment on attachment 236466 [details]
Patch

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

> Source/WebCore/ChangeLog:1
> +2014-08-12  Peyton Randolph  <prandolph@apple.com>

What's up with the duplicate changelog?

> Source/WebCore/page/EventHandler.cpp:1572
> +        clearLongMousePressState();

Seems like you're doing the clear in either case; can this be simplified?

> Source/WebCore/page/EventHandler.cpp:1589
> +        clearLongMousePressState();

Is this all here to make the runtime switch dynamically switchable in the middle of a press? I think it's sufficient if you can't get IN to a long press, you don't need to check in each step once it's started.

> Source/WebCore/page/EventHandler.cpp:1609
> +        clearLongMousePressState();

Another duplicate clear :(
Comment 4 Peyton Randolph 2014-08-12 14:24:43 PDT
(In reply to comment #3)
> (From update of attachment 236466 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236466&action=review
> 
> > Source/WebCore/ChangeLog:1
> > +2014-08-12  Peyton Randolph  <prandolph@apple.com>
> 
> What's up with the duplicate changelog?
> 
> > Source/WebCore/page/EventHandler.cpp:1572
> > +        clearLongMousePressState();
> 
> Seems like you're doing the clear in either case; can this be simplified?
> 
> > Source/WebCore/page/EventHandler.cpp:1589
> > +        clearLongMousePressState();
> 
> Is this all here to make the runtime switch dynamically switchable in the middle of a press? I think it's sufficient if you can't get IN to a long press, you don't need to check in each step once it's started.
> 
> > Source/WebCore/page/EventHandler.cpp:1609
> > +        clearLongMousePressState();
> 
> Another duplicate clear :(

(In reply to comment #3)
> (From update of attachment 236466 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236466&action=review
> 
> > Source/WebCore/ChangeLog:1
> > +2014-08-12  Peyton Randolph  <prandolph@apple.com>
> 
> What's up with the duplicate changelog?

Removed. Had two editors open at the same time.
> 
> > Source/WebCore/page/EventHandler.cpp:1572
> > +        clearLongMousePressState();
> 
> Seems like you're doing the clear in either case; can this be simplified?

Refactored by moving the clear to before the if statement.

> 
> > Source/WebCore/page/EventHandler.cpp:1589
> > +        clearLongMousePressState();
> 
> Is this all here to make the runtime switch dynamically switchable in the middle of a press? I think it's sufficient if you can't get IN to a long press, you don't need to check in each step once it's started.

The check in recognizeLongMousePress isn't strictly necessary, but the check in cancelTrackingPotentialLongMousePress is because it gets called whenever a drag is recognized.

I removed the check in recognizeLongMousePress.

> 
> > Source/WebCore/page/EventHandler.cpp:1609
> > +        clearLongMousePressState();
> 
> Another duplicate clear :(

Refactored.
Comment 5 Peyton Randolph 2014-08-12 14:25:27 PDT
Created attachment 236468 [details]
Patch
Comment 6 Tim Horton 2014-08-12 14:29:54 PDT
Comment on attachment 236468 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:1606
> +    if (!(page && page->settings().longMousePressEnabled()))

You still don't really need this because it's crazy to worry about the case where the timer started and then the preference was turned off and then the cancel comes through, but I don't really care either way.
Comment 7 Peyton Randolph 2014-08-12 15:02:09 PDT
Created attachment 236472 [details]
Patch
Comment 8 WebKit Commit Bot 2014-08-12 16:11:36 PDT
Comment on attachment 236472 [details]
Patch

Clearing flags on attachment: 236472

Committed r172503: <http://trac.webkit.org/changeset/172503>
Comment 9 WebKit Commit Bot 2014-08-12 16:11:39 PDT
All reviewed patches have been landed.  Closing bug.