Bug 69411

Working on this. Will upload patch soon.

Created attachment 112122 [details] Patch

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.

(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.

(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.

(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.

(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?

(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!

Created attachment 113040 [details] Updated Patch

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.

Created attachment 113281 [details] Updated Patch Re-organized Skipped test.

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.

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?

Created attachment 113286 [details] Updated Patch

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.

(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.

Created attachment 113645 [details] Updated Patch Updated patch

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.

Comment on attachment 113645 [details] Updated Patch Attachment 113645 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10318116

Created attachment 113647 [details] Updated Patch Opps.. Sorry for misplaced patch. Updated.

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.

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.

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.

Thanks Sergio, I really needed an extra hand to make this huge patch work. :)

Created attachment 114100 [details] Patch

(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.

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.

(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.

(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.

(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.

(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.

(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 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.

Created attachment 114221 [details] Updated Patch 1) Added workaround for checking 2 consecutive mouseDown 2) Removed some more passing test cases.

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

(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.

Created attachment 114224 [details] Patch

(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.

(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.

(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.

(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

(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" ?

(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.

Committed r99839: <http://trac.webkit.org/changeset/99839>