Bug 30220 - [GTK] middle-click-onpaste.html test does not work
Summary: [GTK] middle-click-onpaste.html test does not work
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-08 10:01 PDT by Alejandro G. Castro
Modified: 2009-10-08 14:12 PDT (History)
4 users (show)

See Also:


Attachments
Patch proposed to support the parameter in mouseDown function (1.69 KB, patch)
2009-10-08 10:01 PDT, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch reviewed (1.89 KB, patch)
2009-10-08 10:23 PDT, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Add line to the expected result (1.04 KB, patch)
2009-10-08 11:05 PDT, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Added bug URL to the Changelog (1.95 KB, patch)
2009-10-08 11:08 PDT, Alejandro G. Castro
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 2009-10-08 10:01:11 PDT
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?
Comment 1 Xan Lopez 2009-10-08 10:09:23 PDT
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.
Comment 2 Steve VanDeBogart 2009-10-08 10:16:50 PDT
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.
Comment 3 Alejandro G. Castro 2009-10-08 10:23:05 PDT
Created attachment 40889 [details]
Patch reviewed

Fixed the style and added the exception handling.
Comment 4 Alejandro G. Castro 2009-10-08 10:30:08 PDT
(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.
Comment 5 Steve VanDeBogart 2009-10-08 10:43:47 PDT
(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.
Comment 6 Alejandro G. Castro 2009-10-08 11:03:21 PDT
(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.
Comment 7 Alejandro G. Castro 2009-10-08 11:05:17 PDT
Created attachment 40892 [details]
Add line to the expected result
Comment 8 Alejandro G. Castro 2009-10-08 11:08:08 PDT
Created attachment 40893 [details]
Added bug URL to the Changelog
Comment 9 Xan Lopez 2009-10-08 11:12:07 PDT
Comment on attachment 40892 [details]
Add line to the expected result

r=me
Comment 10 Eric Seidel (no email) 2009-10-08 11:14:47 PDT
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.
Comment 11 Xan Lopez 2009-10-08 11:23:37 PDT
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.
Comment 12 Alejandro G. Castro 2009-10-08 11:27:20 PDT
(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.
Comment 13 Alejandro G. Castro 2009-10-08 12:19:23 PDT
Added the bug 30229 regarding the handling of the exceptions.
Comment 14 WebKit Commit Bot 2009-10-08 14:05:12 PDT
Comment on attachment 40892 [details]
Add line to the expected result

Clearing flags on attachment: 40892

Committed r49316: <http://trac.webkit.org/changeset/49316>
Comment 15 WebKit Commit Bot 2009-10-08 14:12:28 PDT
Comment on attachment 40893 [details]
Added bug URL to the Changelog

Clearing flags on attachment: 40893

Committed r49318: <http://trac.webkit.org/changeset/49318>
Comment 16 WebKit Commit Bot 2009-10-08 14:12:31 PDT
All reviewed patches have been landed.  Closing bug.