Created attachment 48635 [details] Fix v1

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.

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.

Created attachment 49063 [details] Fix v3 The algorithm was changed.

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?

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 :-).

(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 on attachment 49210 [details] Fix v4 Suggestions for follow-up patch: * Use QVarLengthArray * Use Q_CORE_EXPORT with the qdtoa declaration.

Comment on attachment 49210 [details] Fix v4 Clearing flags on attachment: 49210 Committed r55427: <http://trac.webkit.org/changeset/55427>

All reviewed patches have been landed. Closing bug.