WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
69411
[WK2] [GTK] Implement a MouseDown/MouseUp/MouseMoveTo/MouseScrollBy/LeapForward functions for WebKit2 EventSender
https://bugs.webkit.org/show_bug.cgi?id=69411
Summary
[WK2] [GTK] Implement a MouseDown/MouseUp/MouseMoveTo/MouseScrollBy/LeapForwa...
Alejandro G. Castro
Reported
2011-10-05 04:30:32 PDT
Explained in the summary, check bug: -
https://bugs.webkit.org/show_bug.cgi?id=68108
Attachments
Patch
(17.27 KB, patch)
2011-10-23 12:04 PDT
,
Kaustubh Atrawalkar
mrobinson
: review-
Details
Formatted Diff
Diff
Updated Patch
(36.95 KB, patch)
2011-10-31 04:45 PDT
,
Kaustubh Atrawalkar
no flags
Details
Formatted Diff
Diff
Updated Patch
(40.25 KB, patch)
2011-11-02 00:19 PDT
,
Kaustubh Atrawalkar
kaustubh.ra
: review-
kaustubh.ra
: commit-queue-
Details
Formatted Diff
Diff
Updated Patch
(40.65 KB, patch)
2011-11-02 00:51 PDT
,
Kaustubh Atrawalkar
no flags
Details
Formatted Diff
Diff
Updated Patch
(50.71 KB, patch)
2011-11-04 04:47 PDT
,
Kaustubh Atrawalkar
gustavo
: commit-queue-
Details
Formatted Diff
Diff
Updated Patch
(50.67 KB, patch)
2011-11-04 05:14 PDT
,
Kaustubh Atrawalkar
no flags
Details
Formatted Diff
Diff
Patch
(46.71 KB, patch)
2011-11-07 08:08 PST
,
Sergio Villar Senin
mrobinson
: review-
Details
Formatted Diff
Diff
Patch
(49.00 KB, patch)
2011-11-08 09:54 PST
,
Sergio Villar Senin
no flags
Details
Formatted Diff
Diff
Updated Patch
(51.25 KB, patch)
2011-11-09 01:08 PST
,
Kaustubh Atrawalkar
no flags
Details
Formatted Diff
Diff
Patch
(51.26 KB, patch)
2011-11-09 01:27 PST
,
Kaustubh Atrawalkar
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Kaustubh Atrawalkar
Comment 1
2011-10-22 03:01:04 PDT
Working on this. Will upload patch soon.
Kaustubh Atrawalkar
Comment 2
2011-10-23 12:04:43 PDT
Created
attachment 112122
[details]
Patch
Martin Robinson
Comment 3
2011-10-23 12:19:16 PDT
Comment on
attachment 112122
[details]
Patch The fact that this patch only unskips a few tests means that there's still work to do here. At the very least you should take a stab at figuring out why the rest are failing.
Kaustubh Atrawalkar
Comment 4
2011-10-23 12:33:29 PDT
(In reply to
comment #3
)
> (From update of
attachment 112122
[details]
) > The fact that this patch only unskips a few tests means that there's still work to do here. At the very least you should take a stab at figuring out why the rest are failing.
Yes, the main reason is all the Skipped tests are scattered and need to reorganize based on each functionality as Alejandro said in the
bug 69410
. I am working on that also but this patch was already under progress. I will file a separate bug for that and will cleanup the Skipped tests file.
Martin Robinson
Comment 5
2011-10-23 18:09:33 PDT
(In reply to
comment #4
)
> Yes, the main reason is all the Skipped tests are scattered and need to reorganize based on each functionality as Alejandro said in the
bug 69410
. I am working on that also but this patch was already under progress. I will file a separate bug for that and will cleanup the Skipped tests file.
I think it would be good to do that work in this patch or at least before this patch lands. It's important to know if this implementation is correct or just a little bit correct.
Alejandro G. Castro
Comment 6
2011-10-24 02:57:02 PDT
(In reply to
comment #4
)
> (In reply to
comment #3
) > > (From update of
attachment 112122
[details]
[details]) > > The fact that this patch only unskips a few tests means that there's still work to do here. At the very least you should take a stab at figuring out why the rest are failing. > > Yes, the main reason is all the Skipped tests are scattered and need to reorganize based on each functionality as Alejandro said in the
bug 69410
. I am working on that also but this patch was already under progress. I will file a separate bug for that and will cleanup the Skipped tests file.
In theory they are not scattered, you can check there are sections inside the Skipped file, with bugs assigned in some sections. That is why I said in the previous bug that they idea would be to reorganize when we close a section with a bug like this one because now we have in the Skipped file a section that has the bug already closed with tests failing inside it. I hope this explains better.
Kaustubh Atrawalkar
Comment 7
2011-10-24 10:27:17 PDT
(In reply to
comment #6
)
> (In reply to
comment #4
) > > (In reply to
comment #3
) > > > (From update of
attachment 112122
[details]
[details] [details]) > > > The fact that this patch only unskips a few tests means that there's still work to do here. At the very least you should take a stab at figuring out why the rest are failing. > > > > Yes, the main reason is all the Skipped tests are scattered and need to reorganize based on each functionality as Alejandro said in the
bug 69410
. I am working on that also but this patch was already under progress. I will file a separate bug for that and will cleanup the Skipped tests file. > > In theory they are not scattered, you can check there are sections inside the Skipped file, with bugs assigned in some sections. That is why I said in the previous bug that they idea would be to reorganize when we close a section with a bug like this one because now we have in the Skipped file a section that has the bug already closed with tests failing inside it. I hope this explains better.
Yes, I got the idea. I will try to reorganize the tests in this patch only. I have just found there are many more test cases that are succeeding and still residing in Skipped file. Is it fine to remove them just?
Martin Robinson
Comment 8
2011-10-24 10:43:41 PDT
(In reply to
comment #7
)
> Yes, I got the idea. I will try to reorganize the tests in this patch only. I have just found there are many more test cases that are succeeding and still residing in Skipped file. Is it fine to remove them just?
Definitely!
Kaustubh Atrawalkar
Comment 9
2011-10-31 04:45:12 PDT
Created
attachment 113040
[details]
Updated Patch
Kaustubh Atrawalkar
Comment 10
2011-10-31 04:46:24 PDT
Sorry for delayed updated patch. I have removed all the passing tests from Layout tests now. Lots of tests are passing now. Changed few lines of code. Please review. Thanks.
Kaustubh Atrawalkar
Comment 11
2011-11-02 00:19:27 PDT
Created
attachment 113281
[details]
Updated Patch Re-organized Skipped test.
WebKit Review Bot
Comment 12
2011-11-02 00:25:47 PDT
Attachment 113281
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Tools/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] LayoutTests/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 2 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Martin Robinson
Comment 13
2011-11-02 00:45:29 PDT
Comment on
attachment 113281
[details]
Updated Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=113281&action=review
> LayoutTests/platform/gtk-wk2/Skipped:69 > fullscreen/full-screen-keyboard-disabled.html > fullscreen/full-screen-keyboard-enabled.html > -fullscreen/full-screen-remove-ancestor-during-transition.html > fullscreen/full-screen-request.html > fullscreen/full-screen-cancel.html
Lots of these tests requiring KeyDown still seem to be failing. :/
> LayoutTests/platform/gtk-wk2/Skipped:207 > +# Support for Mouse Double-clicking > editing/selection/doubleclick-beside-cr-span.html > editing/selection/doubleclick-whitespace.html > editing/selection/extend-selection-after-double-click.html > +fast/events/dblclick-addEventListener.html > +fast/events/zoom-dblclick.html > +
Your patch adds support for double-clicking, doesn' t it?
> LayoutTests/platform/gtk-wk2/Skipped:209 > svg/text/select-text-svgfont.html
It seems odd that this test would fail since SVG code is shared between WebKit1 and WebKit2. Can you elaborate on why it's failing?
> LayoutTests/platform/gtk-wk2/Skipped:212 > media/controls-drag-timebar.html
If the test is passing in WebKit1 the results should be the same for WebKIt2.
> LayoutTests/platform/gtk-wk2/Skipped:214 > +# Plugin related bugs.
This doesn't really add anything...
> LayoutTests/platform/gtk-wk2/Skipped:311 > +# Make regions repaint properly > fast/repaint/repaint-across-writing-mode-boundary.html > -fast/repaint/slider-thumb-float.html > fast/repaint/japanese-rl-selection-repaint-in-regions.html > fast/repaint/no-caret-repaint-in-non-content-editable-element.html > fast/repaint/japanese-rl-selection-clear.html > fast/repaint/selection-rl.html > fast/repaint/japanese-rl-selection-repaint.html
Repaint tests shouldn't cause failures unless you are running pixel tests. These tests must be failing for some other reason...
> LayoutTests/platform/gtk-wk2/Skipped:-524 > platform/win/accessibility/scroll-to-anchor.html > platform/win/fast/events/panScroll-event-fired.html > -platform/win/fast/events/panScroll-preventDefault.html > -platform/win/fast/events/panScroll-no-iframe-jump.html
You can probably just remove platform/foo tests from the skipped list altogether. They are not run on GTK+.
> LayoutTests/platform/gtk-wk2/Skipped:331 > platform/gtk/fast/forms/menulist-typeahead-find.html > platform/gtk/editing/pasteboard/middle-button-paste.html
How could these GTK+ tests be failing for platform specific reasons?
Kaustubh Atrawalkar
Comment 14
2011-11-02 00:51:34 PDT
Created
attachment 113286
[details]
Updated Patch
Alejandro G. Castro
Comment 15
2011-11-02 06:32:07 PDT
Comment on
attachment 113286
[details]
Updated Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=113286&action=review
> Tools/WebKitTestRunner/EventSenderProxy.h:56 > + void replaySavedEvents();
Are you adding this to all the platforms? If I understand correctly it is originally just for Qt.
> Tools/WebKitTestRunner/TestController.cpp:-568 > -#if PLATFORM(MAC) || PLATFORM(QT)
Shouln't we add GTK instead of removing this? At least EFL is also adding WK2 support AFAIK.
> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:79 > +void EventSenderProxy::createMouseButtonEvent(GdkEvent* mouseEvent, guint modifiers)
Did you check the prepareMouseButtonEvent function from DRT? There are some cases that maybe we have to check also here? Why are you discarding them?
> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:108 > +void EventSenderProxy::updateClickCountForButton(int button)
Same here with updateClickCount. I would not like to have very different code in both testers for no good reason. Isn't the GtkClickCounter doing the job?
> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:134 > + // First send all the events that are ready to be sent
Add point to the end of the comment.
> LayoutTests/platform/gtk-wk2/Skipped:68 > fullscreen/full-screen-frameset.html > fullscreen/full-screen-keyboard-disabled.html > fullscreen/full-screen-keyboard-enabled.html > -fullscreen/full-screen-remove-ancestor-during-transition.html > fullscreen/full-screen-request.html
We have to reclassify all the keyDown section tets because the bug was already closed.
> LayoutTests/platform/gtk-wk2/Skipped:211 > +# Results for this test is needlessly platform specific
Not sure what this comment means, I think in this file we have to set a way to fix it or just move it to the unchecked failures.
> LayoutTests/platform/gtk-wk2/Skipped:313 > +# Platform specific test cases.
I don't think this comment describes the issue with the tests. In my opinion in this comments we say what we say what we need to implement or the bug with the issue, or we move it to unchecked.
Kaustubh Atrawalkar
Comment 16
2011-11-02 06:41:42 PDT
(In reply to
comment #15
) + void replaySavedEvents();
> > Are you adding this to all the platforms? If I understand correctly it is originally just for Qt.
> Oh, I saw that in DRT Gtk port as well. So implemented it.
> > Tools/WebKitTestRunner/TestController.cpp:-568 > > -#if PLATFORM(MAC) || PLATFORM(QT) > > Shouln't we add GTK instead of removing this? At least EFL is also adding WK2 support AFAIK. >
Sure, I will add GTK there.
> > Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:79 > > +void EventSenderProxy::createMouseButtonEvent(GdkEvent* mouseEvent, guint modifiers) > > Did you check the prepareMouseButtonEvent function from DRT? There are some cases that maybe we have to check also here? Why are you discarding them? >
Yes, I have implemented it taking reference of DRT only. I m just extracting gdkMouseButton in small helper function.
> > Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:108 > > +void EventSenderProxy::updateClickCountForButton(int button) > > Same here with updateClickCount. I would not like to have very different code in both testers for no good reason. Isn't the GtkClickCounter doing the job?
> I have updated function with reference to DRT. Also I m refactoring the code again to fix few test cases. And will again reorganize the leftover Skipped test cases.
Kaustubh Atrawalkar
Comment 17
2011-11-04 04:47:35 PDT
Created
attachment 113645
[details]
Updated Patch Updated patch
Kaustubh Atrawalkar
Comment 18
2011-11-04 04:49:37 PDT
After lot of efforts, I finally have re-organized the huge list of Skipped test cases. Also, few modifications to the code as suggested. Please review. Thanks.
Gustavo Noronha (kov)
Comment 19
2011-11-04 04:56:54 PDT
Comment on
attachment 113645
[details]
Updated Patch
Attachment 113645
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/10318116
Kaustubh Atrawalkar
Comment 20
2011-11-04 05:14:40 PDT
Created
attachment 113647
[details]
Updated Patch Opps.. Sorry for misplaced patch. Updated.
Kaustubh Atrawalkar
Comment 21
2011-11-05 21:44:22 PDT
Hi Martin/Alejandro, Can you review the patch once. There are some test cases which I could not yet categorize. But surely with next implementation of eventSender.drag support I would clear them may be. Thanks.
Sergio Villar Senin
Comment 22
2011-11-07 08:08:44 PST
Created
attachment 113870
[details]
Patch I've been working on this since the first patch and I came up with a very similar patch to the one posted here by Kaustubh Atrawalkar. There are a couple of things that are better addressed in my patch IMHO. - we should use the private attributes of the EventSender instead of defining a set of static variables - the PLATFORM() stuff should not be removed but PLATFORM(GTK) should be added - the Skipped file should have links to the existent bugs. Also moved tests with no results to their proper section. - some function names should be more explanatory - There was no ChangeLog entry for Tools Of course I kept the authorship from Kaustubh Atrawalkar in my patch.
Martin Robinson
Comment 23
2011-11-07 08:53:45 PST
Comment on
attachment 113870
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=113870&action=review
Thanks for cleanup Sergio. Couple nits here, but I think this is a big improvement.
> LayoutTests/platform/gtk-wk2/Skipped:69 > +# FAIL: Timed out waiting for notifyDone to be called
There's already a section of the Skipped file for tests that time out. You should probably just move this section there.
> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:56 > +struct WTREventQueue { > + GdkEvent* event; > + gulong delay; > +}; > + > +static WTREventQueue msgQueue[1024]; > +static unsigned endOfQueue; > +static unsigned startOfQueue;
This should be a member of EventSenderProxy as well. Instead of using an array you should use a Vector. It should probably be named eventQueue, but at the very least messageQueue.
> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:69 > + , m_position()
I'm pretty sure you can omit this. The default constructor is called by default.
> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:73 > + , m_clickPosition()
Ditto.
> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:111 > + // Map EventSender button to actual GDK Mouse button.
You can omit this comment entirely. The function name epresses the same idea.
> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:144 > + // First send all the events that are ready to be sent
This comment is missing a period.
> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:168 > static guint getModifiers(WKEventModifiers modifiersRef)
Please rename this to webkitModifiersToGKDModifiers to match the other conversion functions. I know it's unrelated, but this is a good opportunity for it. Also modifiersRef isn't actually a reference, so it should be renamed as well. wkModifiers is probably fine.
> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:326 > + // Depending on click count change the type of button event. > + if (m_clickCount == 2) > + event->type = GDK_2BUTTON_PRESS; > + else if (m_clickCount == 3) > + event->type = GDK_3BUTTON_PRESS; > + > + createMouseButtonEvent(event, getModifiers(modifiers)); > + sendOrQueueEvent(event); > +} > + > +void EventSenderProxy::mouseUp(unsigned button, WKEventModifiers modifiers) > +{ > + // Create initially Single click button event. > + GdkEvent* event = gdk_event_new(GDK_BUTTON_RELEASE); > + event->button.button = eventSenderButtonToGDKButton(button); > + > + createMouseButtonEvent(event, getModifiers(modifiers)); > + sendOrQueueEvent(event);
It seems that createMouseButtonEvent should take care of all of this. I think it should work such that you call it like this: GdkEvent* event = createMouseButtonEvent(GDK_BUTTON_RELEASE, button, modifiers); sendOrQueueEvent(event); and then: GdkEvent* event = createMouseButtonEvent(GDK_BUTTON_PRESS, button, modifiers); sendOrQueueEvent(event); functions that have "create" in the name are assumed to be factories.
Kaustubh Atrawalkar
Comment 24
2011-11-07 22:33:35 PST
Thanks Sergio, I really needed an extra hand to make this huge patch work. :)
Sergio Villar Senin
Comment 25
2011-11-08 09:54:43 PST
Created
attachment 114100
[details]
Patch
Sergio Villar Senin
Comment 26
2011-11-08 09:55:36 PST
(In reply to
comment #24
)
> Thanks Sergio, I really needed an extra hand to make this huge patch work. :)
You're welcome, thanks for starting to work on this.
Martin Robinson
Comment 27
2011-11-08 10:06:54 PST
Comment on
attachment 114100
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=114100&action=review
Looks good, but please resolve the issues below before landing.
> LayoutTests/platform/gtk-wk2/Skipped:43 > +editing/pasteboard/smart-drag-drop.html > +editing/selection/5195166-1.html > +editing/selection/resources/select-all-iframe-src.html > +fast/events/resources/drag-outside-window-frame.html > +fast/frames/resources/frame-dead-region-left.html > +fast/loader/resources/click-fragment-link.html > +fast/loader/resources/early-load-cancel-inner.html > +fast/loader/resources/document-with-fragment-url-test.html > +plugins/resources/resize-from-plugin-frame.html > +plugins/plugin-initiate-popup-window.html
This seems fishy. The results are already generated for WebKit1 aren't they. If these simply differ from WebKit1 results, then either the tests are failing here or in WebKit1.
> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:61 > + WTREventQueueItem(): event(0), delay(0) { }
The style for this needs to be: WTREventQeueuItem() : event(0), , delay(0) { }
> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:62 > + WTREventQueueItem(GdkEvent* e, gulong d) : event(e), delay(d) { }
Ditto and e should be named event and d should be named delay.
> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:296 > +void EventSenderProxy::mouseDown(unsigned button, WKEventModifiers wkModifiers)
Shouldn't this method check whether the mouse down event has already happened like the WK1 event sender? See EventSender line 319. We need to be careful to not throw away all the knowledge we've gained from the first EventSender!
> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:308 > + updateClickCountForButton(button); > + > + if (m_clickCount == 2) > + eventType = GDK_2BUTTON_PRESS; > + else if (m_clickCount == 3) > + eventType = GDK_3BUTTON_PRESS; > + else > + eventType = GDK_BUTTON_PRESS; > +
There was a huge comment in the WebKit1 EventSender here. I think we should preserve it. // Normally GDK will send both GDK_BUTTON_PRESS and GDK_2BUTTON_PRESS for // the second button press during double-clicks. WebKit GTK+ selectively // ignores the first GDK_BUTTON_PRESS of that pair using gdk_event_peek. // Since our events aren't ever going onto the GDK event queue, WebKit won't // be able to filter out the first GDK_BUTTON_PRESS, so we just don't send // it here. Eventually this code should probably figure out a way to get all // appropriate events onto the event queue and this work-around should be // removed.
Sergio Villar Senin
Comment 28
2011-11-08 13:43:44 PST
(In reply to
comment #27
)
> (From update of
attachment 114100
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=114100&action=review
> > Looks good, but please resolve the issues below before landing. > > > LayoutTests/platform/gtk-wk2/Skipped:43 > > +editing/pasteboard/smart-drag-drop.html > > +editing/selection/5195166-1.html > > +editing/selection/resources/select-all-iframe-src.html > > +fast/events/resources/drag-outside-window-frame.html > > +fast/frames/resources/frame-dead-region-left.html > > +fast/loader/resources/click-fragment-link.html > > +fast/loader/resources/early-load-cancel-inner.html > > +fast/loader/resources/document-with-fragment-url-test.html > > +plugins/resources/resize-from-plugin-frame.html > > +plugins/plugin-initiate-popup-window.html > > This seems fishy. The results are already generated for WebKit1 aren't they. If these simply differ from WebKit1 results, then either the tests are failing here or in WebKit1.
Yeah I removed them, most of them were not even tests
> > Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:296 > > +void EventSenderProxy::mouseDown(unsigned button, WKEventModifiers wkModifiers) > > Shouldn't this method check whether the mouse down event has already happened like the WK1 event sender? See EventSender line 319. We need to be careful to not throw away all the knowledge we've gained from the first EventSender!
Well I'd say that if we get 2 consecutive mouseDown without any mouseUp then whether the test is incorrect or it is trying to test a non-real life sample. We'd need another attribute in the eventSender if we want to support this.
Martin Robinson
Comment 29
2011-11-08 13:56:23 PST
(In reply to
comment #28
)
> Well I'd say that if we get 2 consecutive mouseDown without any mouseUp then whether the test is incorrect or it is trying to test a non-real life sample. We'd need another attribute in the eventSender if we want to support this.
Here's the test: LayoutTests/editing/selection/select-out-of-editable.html. I can't say whether or not the test should be fixed, but it seems this was added to support weird behavior on some other platforms. Nevertheless, we should add the work-around now so that the test passes.
Sergio Villar Senin
Comment 30
2011-11-09 00:31:45 PST
(In reply to
comment #29
)
> (In reply to
comment #28
) > > > Well I'd say that if we get 2 consecutive mouseDown without any mouseUp then whether the test is incorrect or it is trying to test a non-real life sample. We'd need another attribute in the eventSender if we want to support this. > > Here's the test: LayoutTests/editing/selection/select-out-of-editable.html. I can't say whether or not the test should be fixed, but it seems this was added to support weird behavior on some other platforms. Nevertheless, we should add the work-around now so that the test passes.
Thing is that test does not pass because we do not properly generate drag events in the WK2 event sender. The workaround will no fix that test. IMO we should land this patch first and then address the drag'drop issue.
Martin Robinson
Comment 31
2011-11-09 00:35:59 PST
(In reply to
comment #30
)
> Thing is that test does not pass because we do not properly generate drag events in the WK2 event sender. The workaround will no fix that test. IMO we should land this patch first and then address the drag'drop issue.
That test also relies on drag-and-drop support to work, as well as this work-around that deal only with mouse button down events. Please add the work-around to the patch. It is only two lines.
Martin Robinson
Comment 32
2011-11-09 00:36:27 PST
(In reply to
comment #31
)
> That test also relies on drag-and-drop support to work, as well as this work-around that deal only with mouse button down events. Please add the work-around to the patch. It is only two lines.
But if you prefer, I think it's okay to also open another bug for this to track it.
Kaustubh Atrawalkar
Comment 33
2011-11-09 01:03:22 PST
I second Martin about adding support for weird behavior. Adding an extra check would make testing more rigid. And also we can fix the test & remove workaround in another bug may be.
Kaustubh Atrawalkar
Comment 34
2011-11-09 01:08:54 PST
Created
attachment 114221
[details]
Updated Patch 1) Added workaround for checking 2 consecutive mouseDown 2) Removed some more passing test cases.
Alejandro G. Castro
Comment 35
2011-11-09 01:17:53 PST
Comment on
attachment 114100
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=114100&action=review
> Tools/WebKitTestRunner/EventSenderProxy.h:89 > +#if PLATFORM(QT) || PLATFORM(GTK) > + void replaySavedEvents(); > + > #if PLATFORM(QT) > #if ENABLE(TOUCH_EVENTS) > void sendTouchEvent(QEvent::Type); > #endif > void sendOrQueueEvent(QEvent*); > - void replaySavedEvents(); > +#else > + void sendOrQueueEvent(GdkEvent*); > + GdkEvent* createMouseButtonEvent(GdkEventType, unsigned button, WKEventModifiers); > +#endif > #endif
Wouldn't it be clearer if we leave the first #if alone and create and elif? Like this: #if PLATFORM(QT) || PLATFORM(GTK) void replaySavedEvents(); #endif #if PLATFORM(QT) #if ENABLE(TOUCH_EVENTS) void sendTouchEvent(QEvent::Type); #endif void sendOrQueueEvent(QEvent*); #elif PLATFORM(GTK) void sendOrQueueEvent(GdkEvent*); GdkEvent* createMouseButtonEvent(GdkEventType, unsigned button, WKEventModifiers); #endif
Kaustubh Atrawalkar
Comment 36
2011-11-09 01:23:44 PST
(In reply to
comment #35
)
> Wouldn't it be clearer if we leave the first #if alone and create and elif?
Agree, that should give more clear way to understand it. Will fix that.
Kaustubh Atrawalkar
Comment 37
2011-11-09 01:27:52 PST
Created
attachment 114224
[details]
Patch
Sergio Villar Senin
Comment 38
2011-11-09 01:35:16 PST
(In reply to
comment #32
)
> (In reply to
comment #31
) > > > That test also relies on drag-and-drop support to work, as well as this work-around that deal only with mouse button down events. Please add the work-around to the patch. It is only two lines. > > But if you prefer, I think it's okay to also open another bug for this to track it.
I don't mind adding the workaround, the thing is that test will not be fixed by it.
Sergio Villar Senin
Comment 39
2011-11-09 01:59:05 PST
(In reply to
comment #34
)
> Created an attachment (id=114221) [details] > Updated Patch > > 1) Added workaround for checking 2 consecutive mouseDown > 2) Removed some more passing test cases.
Could you please specify which new tests are you removing? Because I saw you're unskipping several http/tests/appcache tests that are failing for me.
Kaustubh Atrawalkar
Comment 40
2011-11-09 02:09:00 PST
(In reply to
comment #39
)
> (In reply to
comment #34
) > > Created an attachment (id=114221) [details] [details] > > Updated Patch > > > > 1) Added workaround for checking 2 consecutive mouseDown > > 2) Removed some more passing test cases. > > Could you please specify which new tests are you removing? Because I saw you're unskipping several http/tests/appcache tests that are failing for me.
Yes, those are passing for me. Also viewport and HiDPI test cases which are passing earlier as well but not removed from Skipped.
Kaustubh Atrawalkar
Comment 41
2011-11-09 02:14:09 PST
(In reply to
comment #40
)
> (In reply to
comment #39
) > > (In reply to
comment #34
) > > > Created an attachment (id=114221) [details] [details] [details] > > > Updated Patch > > > > > > 1) Added workaround for checking 2 consecutive mouseDown > > > 2) Removed some more passing test cases. > > > > Could you please specify which new tests are you removing? Because I saw you're unskipping several http/tests/appcache tests that are failing for me. > > Yes, those are passing for me. Also viewport and HiDPI test cases which are passing earlier as well but not removed from Skipped.
Earlier I mean without this patch. For viewport -
https://bugs.webkit.org/show_bug.cgi?id=69365
For HiDPI -
https://bugs.webkit.org/show_bug.cgi?id=70050
Sergio Villar Senin
Comment 42
2011-11-09 03:01:21 PST
(In reply to
comment #40
)
> (In reply to
comment #39
) > > (In reply to
comment #34
) > > > Created an attachment (id=114221) [details] [details] [details] > > > Updated Patch > > > > > > 1) Added workaround for checking 2 consecutive mouseDown > > > 2) Removed some more passing test cases. > > > > Could you please specify which new tests are you removing? Because I saw you're unskipping several http/tests/appcache tests that are failing for me. > > Yes, those are passing for me. Also viewport and HiDPI test cases which are passing earlier as well but not removed from Skipped.
hidpi tests should be removed from the WK1 Skipped file. Anyway I tried also with your patch and none of the http appcache tests from the Skipped file work for me, all of them time out. Are you running them with "run-webkit-tests --gtk -2" ?
Kaustubh Atrawalkar
Comment 43
2011-11-09 03:22:32 PST
(In reply to
comment #42
)
> hidpi tests should be removed from the WK1 Skipped file. Anyway I tried also with your patch and none of the http appcache tests from the Skipped file work for me, all of them time out. Are you running them with > "run-webkit-tests --gtk -2" ?
Yes, I have debug build. Also there is support added in LayoutTestController for AppCache. So they should work for you too.
Sergio Villar Senin
Comment 44
2011-11-10 04:38:58 PST
Committed
r99839
: <
http://trac.webkit.org/changeset/99839
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug