Bug 30220

Summary:

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

Product:

WebKit

Reporter:

Alejandro G. Castro <alex>

Component:

Tools / Tests

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, vandebo, xan.lopez, zan

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

Linux

Attachments:

Description	Flags
Patch proposed to support the parameter in mouseDown function	none
Patch reviewed	none
Add line to the expected result	none
Added bug URL to the Changelog	none

Alejandro G. Castro

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

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Xan Lopez

Comment 1 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.

Steve VanDeBogart

Comment 2 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.

Alejandro G. Castro

Comment 3 2009-10-08 10:23:05 PDT

Created attachment 40889 [details] Patch reviewed Fixed the style and added the exception handling.

Alejandro G. Castro

Comment 4 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.

Steve VanDeBogart

Comment 5 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.

Alejandro G. Castro

Comment 6 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.

Alejandro G. Castro

Comment 7 2009-10-08 11:05:17 PDT

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

Alejandro G. Castro

Comment 8 2009-10-08 11:08:08 PDT

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

Xan Lopez

Comment 9 2009-10-08 11:12:07 PDT

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

Eric Seidel (no email)

Comment 10 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.

Xan Lopez

Comment 11 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.

Alejandro G. Castro

Comment 12 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.

Alejandro G. Castro

Comment 13 2009-10-08 12:19:23 PDT

Added the bug 30229 regarding the handling of the exceptions.

WebKit Commit Bot

Comment 14 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>

WebKit Commit Bot

Comment 15 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>

WebKit Commit Bot

Comment 16 2009-10-08 14:12:31 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.