Bug 30914 - [Qt] Javascript Prompt API default return values check failure on QtLauncher
Summary: [Qt] Javascript Prompt API default return values check failure on QtLauncher
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Hardware S60 3rd edition
: P3 Normal
Assignee: Holger Freyther
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-29 10:32 PDT by Manish Gupta
Modified: 2010-01-26 02:44 PST (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Manish Gupta 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>
Comment 1 Manish Gupta 2009-10-29 11:09:01 PDT
Created attachment 42115 [details]
Fix for Bug 30914 for QtLauncher
Comment 2 Holger Freyther 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.
Comment 3 Holger Freyther 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();"
Comment 4 Manish Gupta 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.
Comment 5 Manish Gupta 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.
Comment 6 Holger Freyther 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.
Comment 7 Holger Freyther 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.
Comment 8 Holger Freyther 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.
Comment 9 Holger Freyther 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...
Comment 10 WebKit Review Bot 2009-12-14 22:19:53 PST
style-queue ran check-webkit-style on attachment 44845 [details] without any errors.
Comment 11 Kenneth Rohde Christiansen 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.
Comment 12 Holger Freyther 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.
Comment 13 Holger Freyther 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.
Comment 14 Kenneth Rohde Christiansen 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
Comment 15 Laszlo Gombos 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.
Comment 16 Yael 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?
Comment 17 Holger Freyther 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?
Comment 18 Holger Freyther 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.
Comment 19 Holger Freyther 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.
Comment 20 Holger Freyther 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+.
Comment 21 Simon Hausmann 2010-01-26 02:43:29 PST
Committed r53847: <http://trac.webkit.org/changeset/53847>
Comment 22 Simon Hausmann 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