Attachment 55577[details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/DumpRenderTree/gtk/EventSender.cpp:130: Missing space after , [whitespace/comma] [3]
Total errors found: 1 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Could the patch be divided like:
- Refactoring, these changes look nice: prepareMouseButtonEvent, sendOrQueueEvent,
- Fix click problems
- leapForward support, why is this required? Could you just leave the main loop iterate as usual?
+ while (!doneLeapingForward)
+ gtk_main_iteration();
Does this change in the condition inside makeEventSender solves the clicks problem or a problem with the replaying queue?
- if (!replayingSavedEvents) {
- // This function can be called in the middle of a test, even
- // while replaying saved events. Resetting these while doing that
- // can break things.
+ if (isTopFrame) {
(In reply to comment #4)
> Could the patch be divided like:
> - Refactoring, these changes look nice: prepareMouseButtonEvent, sendOrQueueEvent,
> - Fix click problems
Thanks for the review!
I've split the patch into two:
1. One to add rudimentary leapForward support which includes the initialization fix.
2. Another which includes the style fixes and add prepareMouseButtonEvent.
> - leapForward support, why is this required? Could you just leave the main loop iterate as usual?
>
> + while (!doneLeapingForward)
> + gtk_main_iteration();
>
I think you're right here. I've reworked the logic a bit. It's simpler now, because we don't need a lot of the drag-and-drop stuff yet (perhaps until we add dropping support to the DRT and perhaps never).
Created attachment 55917[details]
Take into account the timeOffset when synthesizing events in the DRT
Added an additional change which adjust event time relative to the timeOffset controlled via leapForward. This will ensure that clicks going into WebKit will not be mistaken for double, quadruple, quintuple, sextuple, etc clicks.
Created attachment 55918[details]
Take into account the timeOffset when synthesizing events in the DRT
Added a patch with a small addition to the previous one.
Created attachment 55919[details]
Take into account the timeOffset when synthesizing events in the DRT
My sincere apologies for the patch spam. This should be correct version. It's been a long day.
Created attachment 55922[details]
Send the proper mouse events during clicking
I've added a patch that fixes the test. Click counting is now very similar to that of Win32, which also does not support native click counts above 2.
Created attachment 56092[details]
Combine two related patches
I've combined the leapForward support patch and the timeOffset update. These changes
are very closely related and should probably be in the same patch.
Comment on attachment 56092[details]
Combine two related patches
Xan and Alex expressed some concern that sending events with the time component as GDK_CURRENT_TIME + timeOffset may have unintended consequences. I've attached a new patch which confines it to only mouseUp and mouseDown event simulation. These events are sent directly to the widget and should not affect GDK or GTK+ internal state.
(In reply to comment #19)
> Created an attachment (id=57235) [details]
> Rudimentary leapForward support
Patch looks good to me, it has the leapForward feature and a lot of really nice refactoring that makes sense to me (refactor events sending in functions, remove variables, fix makeSenderEvent function). Nitpicking, there is a blank line added in the first line of the method mouseDownCallback.
Comment on attachment 57235[details]
Rudimentary leapForward support
I've attached a new patch which doesn't remove the missing line and alos fixes the issue where the timeOffset was used during mouse motion events, but not mouse down events.
Comment on attachment 56076[details]
Send the proper mouse events during clicking (update)
>+ static gint doubleClickDistance = 0;
>+ static gint doubleClickTime = 0;
>+ if (!doubleClickDistance) {
>+ GtkSettings* settings = gtk_settings_get_for_screen(gdk_drawable_get_screen(widget->window));
>+ g_object_get(settings, "gtk-double-click-distance", &doubleClickDistance, NULL);
>+ g_object_get(settings, "gtk-double-click-time", &doubleClickTime, NULL);
>+ }
You should connect to the notify signal in case the values are updated. You can do this in a follow-up.
>+
>+ PlatformMouseEvent platformEvent(event);
>+
>+ if ((event->type == GDK_2BUTTON_PRESS || event->type == GDK_3BUTTON_PRESS)
>+ || ((abs(event->x - previousPoint.x()) < doubleClickDistance)
>+ && (abs(event->y - previousPoint.y()) < doubleClickDistance)
>+ && (event->time - previousTime < static_cast<guint>(doubleClickTime))
>+ && (event->button == previousButton)))
>+ currentClickCount++;
>+ else
>+ currentClickCount = 1;
We should have a comment explaining why do we have to do the manual check for increased click count (since GTK+ only checks up to triple click).
LGTM otherwise.
(In reply to comment #25)
Thanks for the review!
> You should connect to the notify signal in case the values are updated. You can do this in a follow-up.
Xan and I decided that it's better in this case to simply not cache these values. If we run into performance disadvantages later, we can fix them then.
2010-05-10 11:17 PDT, Martin Robinson
2010-05-10 11:31 PDT, Martin Robinson
2010-05-10 15:10 PDT, Martin Robinson
2010-05-10 19:09 PDT, Martin Robinson
2010-05-10 19:32 PDT, Martin Robinson
2010-05-10 20:50 PDT, Martin Robinson
2010-05-12 17:53 PDT, Martin Robinson
2010-05-12 17:59 PDT, Martin Robinson
2010-05-12 18:07 PDT, Martin Robinson
2010-05-12 18:22 PDT, Martin Robinson
2010-05-12 20:35 PDT, Martin Robinson
2010-05-14 09:07 PDT, Martin Robinson
2010-05-14 11:57 PDT, Martin Robinson
2010-05-27 06:32 PDT, Martin Robinson
2010-05-28 04:50 PDT, Martin Robinson