30914 – [Qt] Javascript Prompt API default return values check failure on QtLauncher

RESOLVED FIXED 30914

[Qt] Javascript Prompt API default return values check failure on QtLauncher

https://bugs.webkit.org/show_bug.cgi?id=30914

Summary [Qt] Javascript Prompt API default return values check failure on QtLauncher

Manish Gupta

Reported 2009-10-29 10:32:21 PDT

The following test fails with QtLauncher and the variable RESULTS is ERROR, if the user presses the OK button on the prompt popup window. <html> <head> <title>Prompt Test</title> <script type="text/ecmascript"> var RESULTS = "ERROR"; var msg = "Accept this message without changing the default value:"; var retVal = prompt(msg); if((retVal=="undefined")||(retVal=="")) RESULTS="SUCCESS" ; </script> </head>

Attachments
Fix for Bug 30914 for QtLauncher (1.43 KB, patch) 2009-10-29 11:09 PDT, Manish Gupta	zecke: review-	Details Formatted Diff Diff
Fix for Bug #30914 (6.27 KB, patch) 2009-12-14 22:16 PST, Holger Freyther	no flags	Details Formatted Diff Diff
Possible fix with updated manual test (3.87 KB, patch) 2010-01-25 21:04 PST, Holger Freyther	no flags	Details Formatted Diff Diff
Attempt to fix the bug (8.16 KB, patch) 2010-01-26 00:21 PST, Holger Freyther	hausmann: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Manish Gupta

Comment 1 2009-10-29 11:09:01 PDT

Created attachment 42115 [details] Fix for Bug 30914 for QtLauncher

Holger Freyther

Comment 2 2009-11-07 00:16:28 PST

Comment on attachment 42115 [details] Fix for Bug 30914 for QtLauncher Could you point us to the spec that list that requirement? and this bug needs a test case too.

Holger Freyther

Comment 3 2009-11-09 16:49:42 PST

Taking a small look at the other ports... could you try changing the return to "return !x.isNull();"

Manish Gupta

Comment 4 2009-11-13 06:40:27 PST

Tried the fix from comment #3, does not work. The reason is that the return value is always fine(i.e. true, when the user presses ok), it is the result (passed back in the parameter) which has an issue. The ESMP spec version 1.0 lists the above issue as a requirement in section 9.1.2.1.

Manish Gupta

Comment 5 2009-12-09 11:49:26 PST

Hi Zecke, any comments from your side on this fix?, we would like to have a fix in the QtWebKit soon.

Holger Freyther

Comment 6 2009-12-10 00:09:51 PST

(In reply to comment #5) > Hi Zecke, any comments from your side on this fix? Nothing from my statement has changed. If you want this change to be included you will have to show that your code is right either by citing the specification, or in terms of other ports/browsers. E.g. you should change your test case to print the result on the webpage... so other people can also check what happens when the prompt is canceled.

Holger Freyther

Comment 7 2009-12-10 00:16:58 PST

To be more precise: You should add a document.write(RESULTS); after the prompt and then you should add a description on how to run the test. Something like: 1.) Open the page and a prompt will come up 2.) Do not enter any text, simpy press OK 3.) The expected result is that "SUCCESS" is printed on the webpage. And then you should report on your tests and draw a conclusion: With the above test setup WebKit/GTK+, Firefox print "SUCCESS" but QtLauncher is printing ERROR. Do you see how this is different to what you did? In your case you say your JS is not working and change the code, but it is hard to judge who is at fault. If you start describing the test case, and are able to test it in different browsers you have a strong indication that your test case is right and the Qt implementation is wrong.

Holger Freyther

Comment 8 2009-12-10 02:47:13 PST

(In reply to comment #7) > To be more precise: You should add a document.write(RESULTS); after the prompt > and then you should add a description on how to run the test. Something like: > > 1.) Open the page and a prompt will come up > 2.) Do not enter any text, simpy press OK > 3.) The expected result is that "SUCCESS" is printed on the webpage. > > And then you should report on your tests and draw a conclusion: > With the above test setup WebKit/GTK+, Firefox print "SUCCESS" but QtLauncher > is printing ERROR. The interesting bit is that if you modify 2.) to 2.) Enter a charachter and backspace, then press OK.. in this case Qt is returning a QString that isEmpty() but not isNull() and then we see SUCCESS.

Holger Freyther

Comment 9 2009-12-14 22:16:49 PST

Created attachment 44845 [details] Fix for Bug #30914 - Provide a manual test for this... so we can verify it in other browsers - Document what the API user should do.. - Same fixup but only assign result once...

WebKit Review Bot

Comment 10 2009-12-14 22:19:53 PST

style-queue ran check-webkit-style on attachment 44845 [details] without any errors.

Kenneth Rohde Christiansen

Comment 11 2009-12-15 04:54:27 PST

Comment on attachment 44845 [details] Fix for Bug #30914 LGTM, but there are a few things you might consider changing. > +++ b/WebCore/manual-tests/qt/java-script-prompt.html The manual tests doesn't look Qt specific. Is it? If it is not, we should move it. > - result should be written to \a result and true should be returned. > + result should be written to \a result and true should be returned. If the prompt was not cacelled by the cancelled with an n. Please change this before landing.

Holger Freyther

Comment 12 2009-12-15 05:02:57 PST

(In reply to comment #11) > (From update of attachment 44845 [details]) > LGTM, but there are a few things you might consider changing. > > > +++ b/WebCore/manual-tests/qt/java-script-prompt.html > > The manual tests doesn't look Qt specific. Is it? If it is not, we should move > it. I would prefer to have it in Qt as it is more related to the advantage/shortcomings of QInputDialog and not using pointers in the API.

Holger Freyther

Comment 13 2009-12-15 08:19:43 PST

I did land this with the spelling fix, an extra spelling fix, but I did not move the file. If you insist I can move it... landed in r52152.

Kenneth Rohde Christiansen

Comment 14 2009-12-15 09:47:29 PST

(In reply to comment #13) > I did land this with the spelling fix, an extra spelling fix, but I did not > move the file. If you insist I can move it... landed in r52152. No need, you explained your reasons for it being in the qt/ sub dir

Laszlo Gombos

Comment 15 2009-12-22 15:23:12 PST

Comment on attachment 44845 [details] Fix for Bug #30914 Clear review flag as this has been already landed - http://trac.webkit.org/changeset/52152.

Yael

Comment 16 2010-01-25 19:00:06 PST

After this fix (r53441), JavaScript can not get the text anymore. if (rc && result.isNull()) should be replaced with if (rc && x.isNull()) Should this bug be reopened, or create a new one?

Holger Freyther

Comment 17 2010-01-25 20:50:20 PST

(In reply to comment #16) > After this fix (r53441), JavaScript can not get the text anymore. > > if (rc && result.isNull()) > should be replaced with > if (rc && x.isNull()) > > Should this bug be reopened, or create a new one? Sure, did you try the manual test? Is it failing?

Holger Freyther

Comment 18 2010-01-25 20:54:41 PST

(In reply to comment #17) > Sure, did you try the manual test? Is it failing? The test case should be modified to print the retVal or at least the size of it.

Holger Freyther

Comment 19 2010-01-25 21:04:44 PST

Created attachment 47385 [details] Possible fix with updated manual test Yael, you are totally correct. It probably should be 'x' and not result. I have updated the test case and included your fix. Could you please give it a try? I did not have time to compile it. I will do it in a second.

Holger Freyther

Comment 20 2010-01-26 00:21:23 PST

Created attachment 47390 [details] Attempt to fix the bug Address Yael's problem, replace the manual test with an automated one that is covering empty and non empty input in both the Accept and Cancel case. The behavior was verified with WebKit/GTK+.

Simon Hausmann

Comment 21 2010-01-26 02:43:29 PST

Committed r53847: <http://trac.webkit.org/changeset/53847>

Simon Hausmann

Comment 22 2010-01-26 02:44:09 PST

(In reply to comment #20) > Created an attachment (id=47390) [details] > Attempt to fix the bug > > Address Yael's problem, replace the manual test with an automated one that is > covering empty and non empty input in both the Accept and Cancel case. The > behavior was verified with WebKit/GTK+. Cherry-picked into qtwebkit-4.6 with commit a1402f70ea5ea4626c627076ba3d283b0efdf702

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

Status RESOLVED

Resolution FIXED

Priority P3

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware S60 Hardware

OS S60 3rd edition

Product WebKit

Component WebKit Qt

Assignee

Holger Freyther

Reported

2009-10-29 10:32 PDT

Modified

2010-01-26 02:44 PST History

CC List

7 users Show

URL

Keywords

Depends on

Blocks