Bug 135515

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

Description Peyton Randolph 2014-08-01 11:02:32 PDT
I'd like to have a long press controller in the web process to handle the lifecycle of a long press. Eventually, when a long press begins, this controller will generate information at the point of the long press, such as a snapshot of the long-pressed item, an origin for that snapshot, an associated URL if available, etc. The controller will hold onto this long press data through triggering or canceling the long press, at which point it will release that information and send it up to the UI process.
Comment 1 Peyton Randolph 2014-08-01 11:25:44 PDT
Created attachment 235894 [details]
Patch
Comment 2 Simon Fraser (smfr) 2014-08-01 14:27:10 PDT
Comment on attachment 235894 [details]
Patch

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

> Source/WebCore/page/ChromeClient.h:198
> +    virtual void startLongMousePress(const IntPoint&) const = 0;

This should be didStartLongMousePress, right?

> Source/WebCore/page/ChromeClient.h:199
> +    virtual void triggerLongMousePress() const = 0;

triggered?

> Source/WebCore/page/ChromeClient.h:200
> +    virtual void cancelLongMousePress() const = 0;

didCancel?

> Source/WebKit2/Shared/LongMousePressData.h:48
> +    String urlString;

Can this be a URL?

> Source/WebKit2/WebProcess/WebPage/LongMousePressController.h:50
> +    __unused WebPage* m_webPage;

Weird.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:1176
> +    LongMousePressController m_longMousePressController;

Can we store in a unique_pointer and only allocate when we need one?
Comment 3 Peyton Randolph 2014-08-01 15:31:05 PDT
(In reply to comment #2)
> (From update of attachment 235894 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235894&action=review
> 
> > Source/WebCore/page/ChromeClient.h:198
> > +    virtual void startLongMousePress(const IntPoint&) const = 0;
> 
> This should be didStartLongMousePress, right?

I'll rename these methods to be in the past tense.

> 
> > Source/WebCore/page/ChromeClient.h:199
> > +    virtual void triggerLongMousePress() const = 0;
> 
> triggered?

Ditto.
> 
> > Source/WebCore/page/ChromeClient.h:200
> > +    virtual void cancelLongMousePress() const = 0;
> 
> didCancel?

Ditto.

> 
> > Source/WebKit2/Shared/LongMousePressData.h:48
> > +    String urlString;
> 
> Can this be a URL?

I think so. But similar structures like InteractionInformationAtPosition and every WebPageProxy message represent URLs as Strings. I don't know why, so I'll tentatively leave this as String.

> 
> > Source/WebKit2/WebProcess/WebPage/LongMousePressController.h:50
> > +    __unused WebPage* m_webPage;
> 
> Weird.

Removed member variable. I'll add it back when it's used in a future patch.
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.h:1176
> > +    LongMousePressController m_longMousePressController;
> 
> Can we store in a unique_pointer and only allocate when we need one?

Done.

(In reply to comment #2)
> (From update of attachment 235894 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235894&action=review
> 
> > Source/WebCore/page/ChromeClient.h:198
> > +    virtual void startLongMousePress(const IntPoint&) const = 0;
> 
> This should be didStartLongMousePress, right?
> 
> > Source/WebCore/page/ChromeClient.h:199
> > +    virtual void triggerLongMousePress() const = 0;
> 
> triggered?
> 
> > Source/WebCore/page/ChromeClient.h:200
> > +    virtual void cancelLongMousePress() const = 0;
> 
> didCancel?
> 
> > Source/WebKit2/Shared/LongMousePressData.h:48
> > +    String urlString;
> 
> Can this be a URL?
> 
> > Source/WebKit2/WebProcess/WebPage/LongMousePressController.h:50
> > +    __unused WebPage* m_webPage;
> 
> Weird.
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.h:1176
> > +    LongMousePressController m_longMousePressController;
> 
> Can we store in a unique_pointer and only allocate when we need one?
Comment 4 Peyton Randolph 2014-08-01 15:47:15 PDT
Created attachment 235912 [details]
Patch
Comment 5 Peyton Randolph 2014-08-01 15:47:56 PDT
Created attachment 235913 [details]
Patch
Comment 6 Peyton Randolph 2014-08-01 15:48:38 PDT
Created attachment 235914 [details]
Patch
Comment 7 Peyton Randolph 2014-08-05 11:41:24 PDT
Created attachment 236039 [details]
Patch
Comment 8 Peyton Randolph 2014-08-05 16:12:33 PDT
Created attachment 236062 [details]
Patch
Comment 9 Peyton Randolph 2014-08-06 10:52:14 PDT
Created attachment 236113 [details]
Patch
Comment 10 Peyton Randolph 2014-08-07 17:57:08 PDT
Created attachment 236248 [details]
Patch
Comment 11 Peyton Randolph 2014-08-08 13:43:48 PDT
Comment on attachment 236248 [details]
Patch

I forgot to alphabetize LongMousePressController.{h,cpp} in the Xcode project. I will do so.
Comment 12 Tim Horton 2014-08-08 14:13:18 PDT
Comment on attachment 236248 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests.

Either tell us why, or remove the line entirely.

> Source/WebKit/mac/ChangeLog:3
> +        Pass long mouse press event up to the web page. 

It's probably best if all of these changelog titles match the bug, though I'm not s sure that's actually a rule.

> Source/WebCore/page/ChromeClient.h:198
> +    virtual void didBeginLongMousePress(const IntPoint&) const = 0;

Ditto the thing I said in 135515 about this being a *potential* long mouse press, or something.

> Source/WebCore/page/EventHandler.cpp:1577
> -    
> +

Would be nice if the other patch didn't add the bad whitespace in the first place, if that's what's happening here.

> Source/WebCore/page/EventHandler.cpp:1589
> +    m_longMousePressTimer.startOneShot(longMousePressDelay);

Why do you have startOneShot here twice?!

> Source/WebKit2/Shared/LongMousePressDataStore.cpp:39
> +    encoder << snapshotHandle;

I cannot remember if encoding null handles is safe, or if you need to send a boolean first marking whether you have a handle following, and not send the handle if it's null. You should check.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:86
> +#import <WebCore/Element.h>

this should be in the big block up a few lines.

> Source/WebKit2/WebProcess/WebPage/LongMousePressController.h:42
> +    explicit LongMousePressController(WebPage*);

This should take a reference.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:190
> +#if ENABLE(LONG_MOUSE_PRESS)

forward declarations don't need to be ifdeffed.
Comment 13 Peyton Randolph 2014-08-08 14:45:34 PDT
(In reply to comment #12)
> (From update of attachment 236248 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236248&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests.
> 
> Either tell us why, or remove the line entirely.

Removed.
> 
> > Source/WebKit/mac/ChangeLog:3
> > +        Pass long mouse press event up to the web page. 
> 
> It's probably best if all of these changelog titles match the bug, though I'm not s sure that's actually a rule.

Updated all the changelog titles to match the bug.

> 
> > Source/WebCore/page/ChromeClient.h:198
> > +    virtual void didBeginLongMousePress(const IntPoint&) const = 0;
> 
> Ditto the thing I said in 135515 about this being a *potential* long mouse press, or something.

didBeginTrackingPotentialLongMousePress. Verbose but descriptive.
> 
> > Source/WebCore/page/EventHandler.cpp:1577
> > -    
> > +
> 
> Would be nice if the other patch didn't add the bad whitespace in the first place, if that's what's happening here.

Will update the previous patch.

> 
> > Source/WebCore/page/EventHandler.cpp:1589
> > +    m_longMousePressTimer.startOneShot(longMousePressDelay);
> 
> Why do you have startOneShot here twice?!

Removed. Rebase glitch.

> 
> > Source/WebKit2/Shared/LongMousePressDataStore.cpp:39
> > +    encoder << snapshotHandle;
> 
> I cannot remember if encoding null handles is safe, or if you need to send a boolean first marking whether you have a handle following, and not send the handle if it's null. You should check.

The precedent I followed is InteractionInformationAtPosition's image, and that class doesn't send an extra boolean and there are cases where its image is null. I'll do some more digging.

> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:86
> > +#import <WebCore/Element.h>
> 
> this should be in the big block up a few lines.

Done. Also it should be an #include, not an #import.

> 
> > Source/WebKit2/WebProcess/WebPage/LongMousePressController.h:42
> > +    explicit LongMousePressController(WebPage*);
> 
> This should take a reference.

Done.

> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.h:190
> > +#if ENABLE(LONG_MOUSE_PRESS)
> 
> forward declarations don't need to be ifdeffed.

Done.
Comment 14 Peyton Randolph 2014-08-08 15:03:10 PDT
Created attachment 236312 [details]
Patch
Comment 15 WebKit Commit Bot 2014-08-08 17:08:23 PDT
Comment on attachment 236312 [details]
Patch

Rejecting attachment 236312 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 236312, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
pp
patching file Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.h
patching file Source/WebKit2/WebProcess/WebPage/LongMousePressController.cpp
patching file Source/WebKit2/WebProcess/WebPage/LongMousePressController.h
patching file Source/WebKit2/WebProcess/WebPage/WebPage.cpp
patching file Source/WebKit2/WebProcess/WebPage/WebPage.h

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Tim Horton']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/5560314498121728
Comment 16 Tim Horton 2014-08-08 17:18:59 PDT
Needs a rebase.
Comment 17 Peyton Randolph 2014-08-08 17:58:32 PDT
Created attachment 236328 [details]
Patch