Support JS Array.sort() function for sequence wrappers.
authorGlenn Watson <glenn.watson@nokia.com>
Wed, 18 Jul 2012 03:20:00 +0000 (13:20 +1000)
committerQt by Nokia <qt-info@nokia.com>
Tue, 7 Aug 2012 22:34:22 +0000 (00:34 +0200)
The V8 natve sort implementation calls some functions that are
incompatible with the way sequence wrappers work. In particular,
it calls an internal length() function which does not pass through
the length accessor provided by sequence wrappers, so the sort
function always thinks the array is zero length. Instead, clone the
array prototype and override the sort function with one that is
specific to sequence wrappers.

Task-number: QTBUG-25269
Change-Id: Ic83b9ee0bd3a0707e512f28057f0f99b432fded4
Reviewed-by: Matthew Vogt <matthew.vogt@nokia.com>
src/qml/qml/v8/qv8sequencewrapper.cpp
src/qml/qml/v8/qv8sequencewrapper_p.h
src/qml/qml/v8/qv8sequencewrapper_p_p.h
tests/auto/qml/debugger/qqmldebugjs/tst_qqmldebugjs.cpp
tests/auto/qml/qqmlecmascript/data/sequenceSort.qml [new file with mode: 0644]
tests/auto/qml/qqmlecmascript/testtypes.cpp
tests/auto/qml/qqmlecmascript/testtypes.h
tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp

index 883ed1b..6bd7238 100644 (file)
@@ -64,6 +64,22 @@ void QV8SequenceWrapper::init(QV8Engine *engine)
     m_engine = engine;
     m_toString = qPersistentNew<v8::Function>(v8::FunctionTemplate::New(ToString)->GetFunction());
     m_valueOf = qPersistentNew<v8::Function>(v8::FunctionTemplate::New(ValueOf)->GetFunction());
+
+    QString defaultSortString = QLatin1String(
+        "(function compare(x,y) {"
+        "       if (x === y) return 0;"
+        "       x = x.toString();"
+        "       y = y.toString();"
+        "       if (x == y) return 0;"
+        "       else return x < y ? -1 : 1;"
+        "})");
+
+    m_sort = qPersistentNew<v8::Function>(v8::FunctionTemplate::New(Sort)->GetFunction());
+    m_arrayPrototype = qPersistentNew<v8::Object>(v8::Array::New(1)->GetPrototype()->ToObject()->Clone());
+    m_arrayPrototype->Set(v8::String::New("sort"), m_sort);
+    v8::Local<v8::Script> defaultSortCompareScript = v8::Script::Compile(engine->toString(defaultSortString));
+    m_defaultSortComparer = qPersistentNew<v8::Function>(v8::Handle<v8::Function>(v8::Function::Cast(*defaultSortCompareScript->Run())));
+
     v8::Local<v8::FunctionTemplate> ft = v8::FunctionTemplate::New();
     ft->InstanceTemplate()->SetFallbackPropertyHandler(Getter, Setter);
     ft->InstanceTemplate()->SetIndexedPropertyHandler(IndexedGetter, IndexedSetter, 0, IndexedDeleter, IndexedEnumerator);
@@ -84,6 +100,9 @@ void QV8SequenceWrapper::init(QV8Engine *engine)
 
 void QV8SequenceWrapper::destroy()
 {
+    qPersistentDispose(m_defaultSortComparer);
+    qPersistentDispose(m_sort);
+    qPersistentDispose(m_arrayPrototype);
     qPersistentDispose(m_toString);
     qPersistentDispose(m_valueOf);
     qPersistentDispose(m_constructor);
@@ -122,7 +141,7 @@ v8::Local<v8::Object> QV8SequenceWrapper::newSequence(int sequenceType, QObject
 
     v8::Local<v8::Object> rv = m_constructor->NewInstance();
     rv->SetExternalResource(r);
-    rv->SetPrototype(v8::Array::New(1)->GetPrototype());
+    rv->SetPrototype(m_arrayPrototype);
     return rv;
 }
 #undef NEW_REFERENCE_SEQUENCE
@@ -145,7 +164,7 @@ v8::Local<v8::Object> QV8SequenceWrapper::fromVariant(const QVariant& v, bool *s
 
     v8::Local<v8::Object> rv = m_constructor->NewInstance();
     rv->SetExternalResource(r);
-    rv->SetPrototype(v8::Array::New(1)->GetPrototype());
+    rv->SetPrototype(m_arrayPrototype);
     return rv;
 }
 #undef NEW_COPY_SEQUENCE
@@ -227,6 +246,27 @@ v8::Handle<v8::Value> QV8SequenceWrapper::ValueOfGetter(v8::Local<v8::String> pr
     return info.Data();
 }
 
+v8::Handle<v8::Value> QV8SequenceWrapper::Sort(const v8::Arguments &args)
+{
+    int argCount = args.Length();
+
+    if (argCount < 2) {
+        QV8SequenceResource *sr = v8_resource_cast<QV8SequenceResource>(args.This());
+        Q_ASSERT(sr);
+
+        qint32 length = sr->lengthGetter();
+        if (length > 1) {
+            v8::Handle<v8::Function> jsCompareFn = sr->engine->sequenceWrapper()->m_defaultSortComparer;
+            if (argCount == 1 && args[0]->IsFunction())
+                jsCompareFn = v8::Handle<v8::Function>(v8::Function::Cast(*args[0]));
+
+            sr->sort(jsCompareFn);
+        }
+    }
+
+    return args.This();
+}
+
 v8::Handle<v8::Value> QV8SequenceWrapper::ToString(const v8::Arguments &args)
 {
     QV8SequenceResource *sr = v8_resource_cast<QV8SequenceResource>(args.This());
index 104135f..08bc614 100644 (file)
@@ -61,6 +61,7 @@ QT_BEGIN_NAMESPACE
 
 class QV8Engine;
 class QV8ObjectResource;
+
 class QV8SequenceWrapper
 {
 public:
@@ -85,6 +86,9 @@ private:
     v8::Persistent<v8::Function> m_constructor;
     v8::Persistent<v8::Function> m_toString;
     v8::Persistent<v8::Function> m_valueOf;
+    v8::Persistent<v8::Function> m_sort;
+    v8::Persistent<v8::Object> m_arrayPrototype;
+    v8::Persistent<v8::Function> m_defaultSortComparer;
 
     static v8::Handle<v8::Value> IndexedGetter(quint32 index, const v8::AccessorInfo &info);
     static v8::Handle<v8::Value> IndexedSetter(quint32 index, v8::Local<v8::Value> value, const v8::AccessorInfo &info);
@@ -98,6 +102,7 @@ private:
     static v8::Handle<v8::Value> ValueOf(const v8::Arguments &args);
     static v8::Handle<v8::Value> Getter(v8::Local<v8::String> property, const v8::AccessorInfo &info);
     static v8::Handle<v8::Value> Setter(v8::Local<v8::String> property, v8::Local<v8::Value> value, const v8::AccessorInfo &info);
+    static v8::Handle<v8::Value> Sort(const v8::Arguments &args);
 };
 
 
index cf20aa3..e74a584 100644 (file)
@@ -88,6 +88,7 @@ public:
     virtual v8::Handle<v8::Boolean> indexedDeleter(quint32 index) = 0;
     virtual v8::Handle<v8::Array> indexedEnumerator() = 0;
     virtual v8::Handle<v8::Value> toString() = 0;
+    virtual void sort(v8::Handle<v8::Function> comparer) = 0;
 
     ObjectType objectType;
     QByteArray typeName;
@@ -474,6 +475,25 @@ static QString convertUrlToString(QV8Engine *, const QUrl &v)
                 void *a[] = { &c, 0, &status, &flags }; \
                 QMetaObject::metacall(object, QMetaObject::WriteProperty, propertyIndex, a); \
             } \
+            class CompareFunctor \
+            { \
+            public: \
+                CompareFunctor(QV8Engine *engine, v8::Handle<v8::Function> f) : jsFn(f), eng(engine) {} \
+                bool operator()(SequenceElementType e0, SequenceElementType e1) \
+                { \
+                    v8::Handle<v8::Value> argv[2] = { eng->fromVariant(e0), eng->fromVariant(e1) }; \
+                    v8::Handle<v8::Value> compareValue = jsFn->Call(eng->global(), 2, argv); \
+                    return compareValue->NumberValue() < 0; \
+                } \
+            private: \
+                v8::Handle<v8::Function> jsFn; \
+                QV8Engine *eng; \
+            }; \
+            void sort(v8::Handle<v8::Function> jsCompareFunction) \
+            { \
+                CompareFunctor cf(engine, jsCompareFunction); \
+                qSort(c.begin(), c.end(), cf); \
+            } \
         private: \
             QQmlGuard<QObject> object; \
             int propertyIndex; \
index 5f74f86..803c2d8 100644 (file)
@@ -1772,7 +1772,7 @@ void tst_QQmlDebugJS::getScripts()
 
     QList<QVariant> scripts = value.value("body").toList();
 
-    QCOMPARE(scripts.count(), 2);
+    QCOMPARE(scripts.count(), 3);
 }
 
 void tst_QQmlDebugJS::getSource()
diff --git a/tests/auto/qml/qqmlecmascript/data/sequenceSort.qml b/tests/auto/qml/qqmlecmascript/data/sequenceSort.qml
new file mode 100644 (file)
index 0000000..5e2892a
--- /dev/null
@@ -0,0 +1,95 @@
+import QtQuick 2.0
+import Qt.test 1.0
+
+Item {
+
+    MyStringClass {
+        id: msc
+    }
+
+    function compare(a0, a1) {
+        var compareOk = a0.length === a1.length;
+
+        if (compareOk === true) {
+            for (var i=0 ; i < a0.length ; ++i) {
+                if (a0[i] != a1[i]) {
+                    compareOk = false;
+                    break;
+                }
+            }
+        }
+
+        return compareOk;
+    }
+
+    function compareStrings(a, b) {
+        return (a < b) ? 1 : -1;
+    }
+
+    function compareNumbers(a, b) {
+        return a - b;
+    }
+
+    function createExpected(list, sortFn) {
+        var expected = []
+        for (var i=0 ; i < list.length ; ++i)
+            expected.push(list[i]);
+        if (sortFn === null)
+            expected.sort();
+        else
+            expected.sort(sortFn);
+        return expected;
+    }
+
+    function checkResults(expected, actual, sortFn) {
+        if (sortFn === null)
+            actual.sort();
+        else
+            actual.sort(sortFn);
+        return compare(expected, actual);
+    }
+
+    function doStringTest(stringList, fn) {
+        var expected = createExpected(stringList, fn);
+        var actual = msc.strings(stringList);
+        return checkResults(expected, actual, fn);
+    }
+    function doIntTest(intList, fn) {
+        var expected = createExpected(intList, fn);
+        var actual = msc.integers(intList);
+        return checkResults(expected, actual, fn);
+    }
+    function doRealTest(realList, fn) {
+        var expected = createExpected(realList, fn);
+        var actual = msc.reals(realList);
+        return checkResults(expected, actual, fn);
+    }
+
+    function test_qtbug_25269(useCustomCompare) {
+        return doStringTest( [ "one", "two", "three" ], null );
+    }
+    function test_alphabet_insertionSort(useCustomCompare) {
+        var fn = useCustomCompare ? compareStrings : null;
+        return doStringTest( [ "z", "y", "M", "a", "c", "B" ], fn );
+    }
+    function test_alphabet_quickSort(useCustomCompare) {
+        var fn = useCustomCompare ? compareStrings : null;
+        return doStringTest( [ "z", "y", "m", "a", "c", "B", "gG", "u", "bh", "lk", "GW", "Z", "n", "nm", "oi", "njk", "f", "dd", "ooo", "3des", "num123", "ojh", "lkj", "a6^^", "bl!!", "!o" ], fn );
+    }
+    function test_numbers_insertionSort(useCustomCompare) {
+        var fn = useCustomCompare ? compareNumbers : null;
+        return doIntTest( [ 7, 3, 9, 1, 0, -1, 20, -11 ], fn );
+    }
+    function test_numbers_quickSort(useCustomCompare) {
+        var fn = useCustomCompare ? compareNumbers : null;
+        return doIntTest( [ 7, 3, 37, 9, 1, 0, -1, 20, -11, -300, -87, 1, 3, -2, 100, 108, 96, 9, 99999, 12, 11, 11, 12, 11, 13, -13, 10, 10, 10, 8, 12 ], fn );
+    }
+    function test_reals_insertionSort(useCustomCompare) {
+        var fn = useCustomCompare ? compareNumbers : null;
+        return doRealTest( [ -3.4, 1, 10, 4.23, -30.1, 4.24, 4.21, -1, -1 ], fn );
+    }
+    function test_reals_quickSort(useCustomCompare) {
+        var fn = useCustomCompare ? compareNumbers : null;
+        return doRealTest( [ -3.4, 1, 10, 4.23, -30.1, 4.24, 4.21, -1, -1, 12, -100, 87.4, 101.3, -8.88888, 7.76, 10.10, 1.1, -1.1, -0, 11, 12.8, 0.001, -11, -0.75, 99999.99, 11.12, 32.3, 3.333333, 9.876 ], fn );
+    }
+}
index b02e996..77670f7 100644 (file)
@@ -190,6 +190,46 @@ void MyWorkerObject::doIt()
     new MyWorkerObjectThread(this);
 }
 
+class MyStringClass : public QObject
+{
+    Q_OBJECT
+public:
+    Q_INVOKABLE QStringList strings(QStringList stringList) const
+    {
+        return stringList;
+    }
+    Q_INVOKABLE QList<int> integers(QVariant v) const
+    {
+        QList<int> intList;
+        QList<QVariant> vList = v.toList();
+        for (int i=0 ; i < vList.size() ; ++i) {
+            int iv = vList[i].toInt();
+            intList.append(iv);
+        }
+        return intList;
+    }
+    Q_INVOKABLE QList<qreal> reals(QVariant v) const
+    {
+        QList<qreal> realList;
+        QList<QVariant> vList = v.toList();
+        for (int i=0 ; i < vList.size() ; ++i) {
+            qreal fv = vList[i].toReal();
+            realList.append(fv);
+        }
+        return realList;
+    }
+    Q_INVOKABLE QList<bool> bools(QVariant v) const
+    {
+        QList<bool> boolList;
+        QList<QVariant> vList = v.toList();
+        for (int i=0 ; i < vList.size() ; ++i) {
+            bool bv = vList[i].toBool();
+            boolList.append(bv);
+        }
+        return boolList;
+    }
+};
+
 void registerTypes()
 {
     qmlRegisterType<MyQmlObject>("Qt.test", 1,0, "MyQmlObjectAlias");
@@ -253,6 +293,8 @@ void registerTypes()
 
     qmlRegisterType<FallbackBindingsTypeObject>("Qt.test.fallbackBindingsObject", 1, 0, "FallbackBindingsType");
     qmlRegisterType<FallbackBindingsTypeDerived>("Qt.test.fallbackBindingsDerived", 1, 0, "FallbackBindingsType");
+
+    qmlRegisterType<MyStringClass>("Qt.test", 1, 0, "MyStringClass");
 }
 
 #include "testtypes.moc"
index d857b64..eaecf71 100644 (file)
@@ -755,7 +755,7 @@ public:
     Q_INVOKABLE void method_overload(const QJsonArray &a) { invoke(26); m_actuals << QVariant::fromValue(a); }
     Q_INVOKABLE void method_overload(const QJsonValue &a) { invoke(27); m_actuals << QVariant::fromValue(a); }
 
-    Q_INVOKABLE void method_unknown(MyInvokableObject *o) { invoke(28); }
+    Q_INVOKABLE void method_unknown(MyInvokableObject *) { invoke(28); }
 
 private:
     friend class MyInvokableBaseObject;
index 4e1638f..f6ec475 100644 (file)
@@ -186,6 +186,8 @@ private slots:
     void sequenceConversionBindings();
     void sequenceConversionCopy();
     void assignSequenceTypes();
+    void sequenceSort_data();
+    void sequenceSort();
     void qtbug_22464();
     void qtbug_21580();
     void singleV8BindingDestroyedDuringEvaluation();
@@ -7121,6 +7123,48 @@ void tst_qqmlecmascript::fallbackBindings()
     QCOMPARE(object->property("success").toBool(), true);
 }
 
+void tst_qqmlecmascript::sequenceSort_data()
+{
+    QTest::addColumn<QString>("function");
+    QTest::addColumn<bool>("useComparer");
+
+    QTest::newRow("qtbug_25269") << "test_qtbug_25269" << false;
+
+    const char *types[] = { "alphabet", "numbers", "reals" };
+    const char *sort[] = { "insertionSort", "quickSort" };
+
+    for (size_t t=0 ; t < sizeof(types)/sizeof(types[0]) ; ++t) {
+        for (size_t s=0 ; s < sizeof(sort)/sizeof(sort[0]) ; ++s) {
+            for (int c=0 ; c < 2 ; ++c) {
+                QString testName = QLatin1String(types[t]) + QLatin1String("_") + QLatin1String(sort[s]);
+                QString fnName = QLatin1String("test_") + testName;
+                bool useComparer = c != 0;
+                testName += useComparer ? QLatin1String("[custom]") : QLatin1String("[default]");
+                QTest::newRow(testName.toAscii().constData()) << fnName << useComparer;
+            }
+        }
+    }
+}
+
+void tst_qqmlecmascript::sequenceSort()
+{
+    QFETCH(QString, function);
+    QFETCH(bool, useComparer);
+
+    QQmlComponent component(&engine, testFileUrl("sequenceSort.qml"));
+
+    QObject *object = component.create();
+    if (object == 0)
+        qDebug() << component.errorString();
+    QVERIFY(object != 0);
+
+    QVariant q;
+    QMetaObject::invokeMethod(object, function.toAscii().constData(), Q_RETURN_ARG(QVariant, q), Q_ARG(QVariant, useComparer));
+    QVERIFY(q.toBool() == true);
+
+    delete object;
+}
+
 QTEST_MAIN(tst_qqmlecmascript)
 
 #include "tst_qqmlecmascript.moc"