Summary: | Linux Chromium sends too many click events | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Evan Stade <estade> | ||||||||
Component: | WebKit Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, evan | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
Attachments: |
|
Description
Evan Stade
2009-11-24 11:10:27 PST
Created attachment 43790 [details]
fix
Comment on attachment 43790 [details] fix > Index: WebKit/chromium/ChangeLog > =================================================================== > --- WebKit/chromium/ChangeLog (revision 51343) > +++ WebKit/chromium/ChangeLog (working copy) > @@ -1,3 +1,18 @@ > +2009-11-24 Evan Stade <estade@chromium.org> > + > + Reviewed by NOBODY (OOPS!). > + > + Linux Chromium sends too many click events > + https://bugs.webkit.org/show_bug.cgi?id=31841 > + > + Manually count number of clicks for double/triple click events. This > + makes us match firefox on http://www.quirksmode.org/js/events_mouse.html Cap Firefox? > + Chromium side of this patch is here: > + http://codereview.chromium.org/431031/show > + > + * src/gtk/WebInputEventFactory.cpp: > + (WebKit::WebInputEventFactory::mouseEvent): > + > 2009-11-24 Adam Barth <abarth@webkit.org> > > Reviewed by Dimitri Glazkov. > Index: WebKit/chromium/src/gtk/WebInputEventFactory.cpp > =================================================================== > --- WebKit/chromium/src/gtk/WebInputEventFactory.cpp (revision 51343) > +++ WebKit/chromium/src/gtk/WebInputEventFactory.cpp (working copy) > @@ -38,10 +38,22 @@ > > #include <gdk/gdk.h> > #include <gdk/gdkkeysyms.h> > +#include <gtk/gtk.h> > #include <gtk/gtkversion.h> > > #include <wtf/Assertions.h> > > +namespace { > + > +gint GetDoubleClickTime() { > + static GtkSettings* settings = gtk_settings_get_default(); > + gint double_click_time = 250; > + g_object_get(G_OBJECT(settings), "gtk-double-click-time", &double_click_time, NULL); > + return double_click_time; > +} Style: getDoubleClickTime, four spaces, variable naming. Why static gobject? Maybe the double click time should be static? Does this work in a sandboxed renderer? I didn't think you could get the real gtk settings. > + > +} // namespace > + > namespace WebKit { > > static double gdkEventTimeToWebEventTime(guint32 time) > @@ -321,24 +333,36 @@ WebMouseEvent WebInputEventFactory::mous > result.clickCount = 0; > > switch (event->type) { > - case GDK_3BUTTON_PRESS: > - ++result.clickCount; > - // fallthrough > - case GDK_2BUTTON_PRESS: > - ++result.clickCount; > - // fallthrough > case GDK_BUTTON_PRESS: > result.type = WebInputEvent::MouseDown; > - ++result.clickCount; > break; > case GDK_BUTTON_RELEASE: > result.type = WebInputEvent::MouseUp; > break; > - > + case GDK_3BUTTON_PRESS: > + // fallthrough I think the comment isn't necessary when there's no code in the case. > + case GDK_2BUTTON_PRESS: > + // fallthrough > default: > ASSERT_NOT_REACHED(); > }; > > + if (GDK_BUTTON_PRESS == event->type) { > + static int numClicks = 0; > + static GdkWindow* eventWindow = NULL; > + static gint lastLeftClickTime = 0; > + > + gint time_diff = event->time - lastLeftClickTime; > + if (eventWindow == event->window && time_diff < GetDoubleClickTime()) > + numClicks++; > + else > + numClicks = 1; > + > + result.clickCount = numClicks; > + eventWindow = event->window; > + lastLeftClickTime = event->time; > + } > + > result.button = WebMouseEvent::ButtonNone; > if (event->button == 1) > result.button = WebMouseEvent::ButtonLeft; Created attachment 43792 [details]
style fixes
style fixed
1. The getDoubleClickTime function is copied from chrome code. The first variable is static to avoid unnecessary get_settings calls, although I admit this optimization might not buy us much. The result is not static because it could potentially change.
2. This code does not execute in the renderer, it executes in the browser process only.
This seems good to me, though I am not a webkit reviewer. The statics make me a bit sad, but not so much. Comment on attachment 43792 [details] style fixes r- for no layout test. You can send clicks from within layout tests, so it seems like it would be possible to add a layout test. As an absolutely last resort, there is always WebKit/WebCore/manual-tests > Index: WebKit/chromium/src/gtk/WebInputEventFactory.cpp > +gint getDoubleClickTime() { The brace needs to go on the next line. > + g_object_get(G_OBJECT(settings), "gtk-double-click-time", &doubleClickTime, NULL); Use 0 instead of NULL. > + if (GDK_BUTTON_PRESS == event->type) { > + static int numClicks = 0; > + static GdkWindow* eventWindow = NULL; Use 0 instead of NULL. Created attachment 43817 [details]
more style
I don't think it's possible to add a layout test because this is system event conversion. Layout tests would bypass this code and create the WebMouseEvent directly. The only way to test this would be with a Chromium UI test.
Comment on attachment 43817 [details] more style Clearing flags on attachment: 43817 Committed r51364: <http://trac.webkit.org/changeset/51364> All reviewed patches have been landed. Closing bug. |