Bug 34850 - [Qt] QScriptValue::toString() returns incorrect values
Summary: [Qt] QScriptValue::toString() returns incorrect values
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Jędrzej Nowacki
URL:
Keywords: Qt
Depends on: 34843
Blocks: 31863 35387 35577
  Show dependency treegraph
 
Reported: 2010-02-11 09:51 PST by Jędrzej Nowacki
Modified: 2010-03-02 13:39 PST (History)
3 users (show)

See Also:


Attachments
test cases (3.69 KB, text/plain)
2010-02-11 09:51 PST, Jędrzej Nowacki
no flags Details
Fix v1 (14.29 KB, patch)
2010-02-12 04:35 PST, Jędrzej Nowacki
hausmann: review-
hausmann: commit-queue-
Details | Formatted Diff | Diff
Fix v2 (13.49 KB, patch)
2010-02-16 05:11 PST, Jędrzej Nowacki
no flags Details | Formatted Diff | Diff
Fix v3 (14.20 KB, patch)
2010-02-19 03:31 PST, Jędrzej Nowacki
no flags Details | Formatted Diff | Diff
Fix v4 (14.20 KB, patch)
2010-02-22 06:09 PST, Jędrzej Nowacki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jędrzej Nowacki 2010-02-11 09:51:18 PST
Created attachment 48576 [details]
test cases

QScriptValue::toString() is not perfect, for example in place of "Infinity" and "NaN" it returns "inf", "nan". More issues are in the attachment.
Comment 1 Jędrzej Nowacki 2010-02-12 04:35:00 PST
Created attachment 48635 [details]
Fix v1
Comment 2 Simon Hausmann 2010-02-15 04:10:29 PST
Comment on attachment 48635 [details]
Fix v1


> +        // FIXME this should be easier. The QString API had to be developed, it should contain
> +        // a converting function, a bit more flexible then the number(). The ideal fix is to create
> +        // a new function in JSC C API which could cover the functionality.

I suggest to pursue that route by adding the functionality to the C API.

The old set of unit tests that this patch removes also include a test of converting a QScriptValue containing a non-latin string back to a QString. The new test set does removes that from the input values. Is that intentional?

I'm okay with having the workaround in QScriptConverter for now, but I'm concerned about the removal of the utf8 test.
Comment 3 Jędrzej Nowacki 2010-02-16 05:11:53 PST
Created attachment 48807 [details]
Fix v2

(In reply to comment #2)
> (From update of attachment 48635 [details])
> 
> > +        // FIXME this should be easier. The QString API had to be developed, it should contain
> > +        // a converting function, a bit more flexible then the number(). The ideal fix is to create
> > +        // a new function in JSC C API which could cover the functionality.
> 
> I suggest to pursue that route by adding the functionality to the C API.
> 
> The old set of unit tests that this patch removes also include a test of
> converting a QScriptValue containing a non-latin string back to a QString. The
> new test set does removes that from the input values. Is that intentional?
> 
> I'm okay with having the workaround in QScriptConverter for now, but I'm
> concerned about the removal of the utf8 test.
Agree, I renamed old test instead of replacing it.
Comment 4 Jędrzej Nowacki 2010-02-19 03:31:40 PST
Created attachment 49063 [details]
Fix v3

The algorithm was changed.
Comment 5 Ariya Hidayat 2010-02-19 07:58:56 PST
Comment on attachment 49063 [details]
Fix v3

Looks sane to me, except...

> +        QByteArray buf;
> +        buf.reserve(80);

..where does this value come from? I can't recall you can print a double value into 80-char long string.

Maybe add some comment or use shorter buffer length for that?
Comment 6 Jędrzej Nowacki 2010-02-22 06:09:43 PST
Created attachment 49210 [details]
Fix v4

(In reply to comment #5)
> (From update of attachment 49063 [details])
> Looks sane to me, except...
> 
> > +        QByteArray buf;
> > +        buf.reserve(80);
> 
> ..where does this value come from? I can't recall you can print a double value
> into 80-char long string.
> 
> Maybe add some comment or use shorter buffer length for that?

2^64 => 20 digits + 1 sign
min double (-1.7976931348623158e+308) => 20 digits + 4 others

It seems that 24 is enough, but I'm not really sure :-).
Comment 7 Kent Hansen 2010-02-23 02:02:05 PST
(In reply to comment #5)
> (From update of attachment 49063 [details])
> Looks sane to me, except...
> 
> > +        QByteArray buf;
> > +        buf.reserve(80);
> 
> ..where does this value come from? I can't recall you can print a double value
> into 80-char long string.
> 
> Maybe add some comment or use shorter buffer length for that?

This code comes straight from the old QtScript back-end.
Like Jedrzej comments in the patch, ideally we would be able to use the JSC C API for converting a plain number (not JSValue) to a string. If we were to use the C API right now we would have to create a temporary context + value, convert the value to string, and then destroy the context again.
It's probably going to be slow, but the code will be smaller and we have the guarantee that the result will be the same as for normal JSValues. (Even though the implementation in this patch has good test coverage in the old back-end, I can't guarantee it always gives the same result as JSC.)
Comment 8 Simon Hausmann 2010-03-02 03:16:45 PST
Comment on attachment 49210 [details]
Fix v4

Suggestions for follow-up patch:

* Use QVarLengthArray
* Use Q_CORE_EXPORT with the qdtoa declaration.
Comment 9 WebKit Commit Bot 2010-03-02 13:39:08 PST
Comment on attachment 49210 [details]
Fix v4

Clearing flags on attachment: 49210

Committed r55427: <http://trac.webkit.org/changeset/55427>
Comment 10 WebKit Commit Bot 2010-03-02 13:39:13 PST
All reviewed patches have been landed.  Closing bug.