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.
Created attachment 235894 [details] Patch
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?
(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?
Created attachment 235912 [details] Patch
Created attachment 235913 [details] Patch
Created attachment 235914 [details] Patch
Created attachment 236039 [details] Patch
Created attachment 236062 [details] Patch
Created attachment 236113 [details] Patch
Created attachment 236248 [details] Patch
Comment on attachment 236248 [details] Patch I forgot to alphabetize LongMousePressController.{h,cpp} in the Xcode project. I will do so.
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.
(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.
Created attachment 236312 [details] Patch
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
Needs a rebase.
Created attachment 236328 [details] Patch