Bug 30220 – [GTK] middle-click-onpaste.html test does not work

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Alejandro G. Castro]

Created attachment 40885 [details]
Patch proposed to support the parameter in mouseDown function

We have two problems with this tests:
   - DRT EventSend mouseDown does not support the parameter setting the button that was pressed.
   - The expected result has a line in the bottom that we are not generating.

I have a patch for the first point (Zan Dobersek could you review it) and for the second point I added Steve VanDeBogart (the developer that added this example), Steve is it a problem if we add that line to the end of the file?

[Xan Lopez]

Also, adding tests to platform/gtk that fail in the WebKitGTK+ port shouldn't be done. If the tests can be shared between Chrome/GTK+ and WebKitGTK+ then that's great, but otherwise they should go elsewhere. In this case it seems this test was depending on some DRT functionality not present in our port, so it couldn't have possibly worked.

[Steve VanDeBogart]

I don't see what line you are proposing to add.

Xan: This was part of my first change in webkit, so I certainly could have added it in the wrong place.  The test works in the chromium test shell, but I didn't try it in DRT.

[Alejandro G. Castro]

Created attachment 40889 [details]
Patch reviewed

Fixed the style and added the exception handling.

[Alejandro G. Castro]

(In reply to comment #2)
> I don't see what line you are proposing to add.
> 

I meant adding one line here in the bottom of this file:

WebKit$ cat platform/gtk/editing/pasteboard/middle-click-onpaste-expected.txt 
Test that middle click triggers the onpaste event

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".


PASS pasteCount is 1
PASS successfullyParsed is true

TEST COMPLETE
------> One new line here <---------


We need to add it to pass the test so if you could add that line and your test is still ok it will help us.

[Steve VanDeBogart]

(In reply to comment #4)
> (In reply to comment #2)
> > I don't see what line you are proposing to add.
> 
> I meant adding one line here in the bottom of this file:
> 
> TEST COMPLETE
> ------> One new line here <---------
> 
> We need to add it to pass the test so if you could add that line and your test
> is still ok it will help us.

That white space doesn't affect my test run. Go ahead and add it.

[Alejandro G. Castro]

(In reply to comment #5)
> That white space doesn't affect my test run. Go ahead and add it.

Ok, thanks for the information, I'm going to upload a patch for this.

[Alejandro G. Castro]

Created attachment 40892 [details]
Add line to the expected result

[Alejandro G. Castro]

Created attachment 40893 [details]
Added bug URL to the Changelog

[Xan Lopez]

Comment on attachment 40892 [details]
Add line to the expected result

r=me

[Eric Seidel (no email)]

Comment on attachment 40893 [details]
Added bug URL to the Changelog

Personally I don't find g_return_val_if_fail very readable.

if (!exception || !*exception)
    return JSValueMakeUndefined(context);
is the same number of characters, split over 2 lines, and much more readable to those of us who don't hack on gtk normally. :)

Also, I think that this should just assert(!exception) instead.  The testing tool is allowed to fail hard in cases like this.

[Xan Lopez]

Comment on attachment 40893 [details]
Added bug URL to the Changelog

I disagree with Eric.

The code already uses g_return_val_if_fail everywhere, so if we were to change it we should do it all at once. Besides, I don't think it makes any sense to let DRT do stupid things (or crash) when you already know the input is wrong. This is what g_return_* is for.

In any case the logic in these checks is wrong, so we are going to do another patch to change them all at once.

I'm going to r+ this now so we can pass the tests.

[Alejandro G. Castro]

(In reply to comment #10)
> (From update of attachment 40893 [details])
> Personally I don't find g_return_val_if_fail very readable.
> 
> if (!exception || !*exception)
>     return JSValueMakeUndefined(context);
> is the same number of characters, split over 2 lines, and much more readable to
> those of us who don't hack on gtk normally. :)
> 

Well, I'm not the one deciding about using or not the return if fail, they are all over the code, maybe you should open a bug to discuss this.

> Also, I think that this should just assert(!exception) instead.  The testing
> tool is allowed to fail hard in cases like this.

I agree with you here :), I copied the tests that were done in all this code for the exceptions and they do not look right. I'll upload a new bug to change all the checks for once.

[Alejandro G. Castro]

Added the bug 30229 regarding the handling of the exceptions.

[WebKit Commit Bot]

Comment on attachment 40892 [details]
Add line to the expected result

Clearing flags on attachment: 40892

Committed r49316: <http://trac.webkit.org/changeset/49316>

[WebKit Commit Bot]

Comment on attachment 40893 [details]
Added bug URL to the Changelog

Clearing flags on attachment: 40893

Committed r49318: <http://trac.webkit.org/changeset/49318>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.