Bug 69411 - [WK2] [GTK] Implement a MouseDown/MouseUp/MouseMoveTo/MouseScrollBy/LeapForward functions for WebKit2 EventSender
Summary: [WK2] [GTK] Implement a MouseDown/MouseUp/MouseMoveTo/MouseScrollBy/LeapForwa...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Kaustubh Atrawalkar
URL:
Keywords:
Depends on:
Blocks: 69523
  Show dependency treegraph
 
Reported: 2011-10-05 04:30 PDT by Alejandro G. Castro
Modified: 2011-11-10 09:20 PST (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Alejandro G. Castro 2011-10-05 04:30:32 PDT
Explained in the summary, check bug:

  - https://bugs.webkit.org/show_bug.cgi?id=68108
Comment 1 Kaustubh Atrawalkar 2011-10-22 03:01:04 PDT
Working on this. Will upload patch soon.
Comment 2 Kaustubh Atrawalkar 2011-10-23 12:04:43 PDT
Created attachment 112122 [details]
Patch
Comment 3 Martin Robinson 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.
Comment 4 Kaustubh Atrawalkar 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.
Comment 5 Martin Robinson 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.
Comment 6 Alejandro G. Castro 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.
Comment 7 Kaustubh Atrawalkar 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?
Comment 8 Martin Robinson 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!
Comment 9 Kaustubh Atrawalkar 2011-10-31 04:45:12 PDT
Created attachment 113040 [details]
Updated Patch
Comment 10 Kaustubh Atrawalkar 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.
Comment 11 Kaustubh Atrawalkar 2011-11-02 00:19:27 PDT
Created attachment 113281 [details]
Updated Patch

Re-organized Skipped test.
Comment 12 WebKit Review Bot 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.
Comment 13 Martin Robinson 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?
Comment 14 Kaustubh Atrawalkar 2011-11-02 00:51:34 PDT
Created attachment 113286 [details]
Updated Patch
Comment 15 Alejandro G. Castro 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.
Comment 16 Kaustubh Atrawalkar 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.
Comment 17 Kaustubh Atrawalkar 2011-11-04 04:47:35 PDT
Created attachment 113645 [details]
Updated Patch

Updated patch
Comment 18 Kaustubh Atrawalkar 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.
Comment 19 Gustavo Noronha (kov) 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
Comment 20 Kaustubh Atrawalkar 2011-11-04 05:14:40 PDT
Created attachment 113647 [details]
Updated Patch

Opps.. Sorry for misplaced patch. Updated.
Comment 21 Kaustubh Atrawalkar 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.
Comment 22 Sergio Villar Senin 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.
Comment 23 Martin Robinson 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.
Comment 24 Kaustubh Atrawalkar 2011-11-07 22:33:35 PST
Thanks Sergio, I really needed an extra hand to make this huge patch work. :)
Comment 25 Sergio Villar Senin 2011-11-08 09:54:43 PST
Created attachment 114100 [details]
Patch
Comment 26 Sergio Villar Senin 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.
Comment 27 Martin Robinson 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.
Comment 28 Sergio Villar Senin 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.
Comment 29 Martin Robinson 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.
Comment 30 Sergio Villar Senin 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.
Comment 31 Martin Robinson 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.
Comment 32 Martin Robinson 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.
Comment 33 Kaustubh Atrawalkar 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.
Comment 34 Kaustubh Atrawalkar 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.
Comment 35 Alejandro G. Castro 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
Comment 36 Kaustubh Atrawalkar 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.
Comment 37 Kaustubh Atrawalkar 2011-11-09 01:27:52 PST
Created attachment 114224 [details]
Patch
Comment 38 Sergio Villar Senin 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.
Comment 39 Sergio Villar Senin 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.
Comment 40 Kaustubh Atrawalkar 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.
Comment 41 Kaustubh Atrawalkar 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
Comment 42 Sergio Villar Senin 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" ?
Comment 43 Kaustubh Atrawalkar 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.
Comment 44 Sergio Villar Senin 2011-11-10 04:38:58 PST
Committed r99839: <http://trac.webkit.org/changeset/99839>