Bug 135515 – Create stub long mouse press controller. Part of 135257 - Add long mouse press gesture

• Status: RESOLVED
• Resolution: WONTFIX
• Component: UI Events

[Peyton Randolph]

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.

[Peyton Randolph]

Created attachment 235894 [details]
Patch

[Simon Fraser (smfr)]

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?

[Peyton Randolph]

(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?

[Peyton Randolph]

Created attachment 235912 [details]
Patch

[Peyton Randolph]

Created attachment 235913 [details]
Patch

[Peyton Randolph]

Created attachment 235914 [details]
Patch

[Peyton Randolph]

Created attachment 236039 [details]
Patch

[Peyton Randolph]

Created attachment 236062 [details]
Patch

[Peyton Randolph]

Created attachment 236113 [details]
Patch

[Peyton Randolph]

Created attachment 236248 [details]
Patch

[Peyton Randolph]

Comment on attachment 236248 [details]
Patch

I forgot to alphabetize LongMousePressController.{h,cpp} in the Xcode project. I will do so.

[Tim Horton]

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.

[Peyton Randolph]

(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.

[Peyton Randolph]

Created attachment 236312 [details]
Patch

[WebKit Commit Bot]

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

[Tim Horton]

Needs a rebase.

[Peyton Randolph]

Created attachment 236328 [details]
Patch