[Webkit-unassigned] [Bug 135515] Create stub long mouse press controller. Part of 135257 - Add long mouse press gesture

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Aug 8 14:45:23 PDT 2014


https://bugs.webkit.org/show_bug.cgi?id=135515





--- Comment #13 from Peyton Randolph <prandolph at apple.com>  2014-08-08 14:45:34 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list