[webkit-reviews] review granted: [Bug 135515] Create stub long mouse press controller. Part of 135257 - Add long mouse press gesture : [Attachment 236248] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Aug 8 14:13:06 PDT 2014
Tim Horton <thorton at apple.com> has granted Peyton Randolph
<prandolph at apple.com>'s request for review:
Bug 135515: Create stub long mouse press controller. Part of 135257 - Add long
mouse press gesture
https://bugs.webkit.org/show_bug.cgi?id=135515
Attachment 236248: Patch
https://bugs.webkit.org/attachment.cgi?id=236248&action=review
------- Additional Comments from Tim Horton <thorton at apple.com>
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.
More information about the webkit-reviews
mailing list