Improve QJSValueIterator implementation.

The old implementation was a hack, it had some memory leak (in case of
deleted engine) and performance problems (for example all names were
copied to separate QList instance instead of reusing v8::Array).

Change-Id: Ic70ad511127a8c05df3c627e4496083004c6452a
Reviewed-on: http://codereview.qt.nokia.com/2512
Reviewed-by: Qt Sanity Bot <qt_sanity_bot@ovi.com>
Reviewed-by: Kent Hansen <kent.hansen@nokia.com>

8 files changed, 221 insertions, 138 deletions

diff --git a/src/declarative/qml/v8/qjsvalue_p.h b/src/declarative/qml/v8/qjsvalue_p.h
index 7b2a001f97..2e24ad0ae3 100644
--- a/src/declarative/qml/v8/qjsvalue_p.h
+++ b/src/declarative/qml/v8/qjsvalue_p.h
@@ -49,6 +49,8 @@
 
 QT_BEGIN_NAMESPACE
 
+class QV8Engine;
+
 /*!
   \internal
   \class QJSValuePrivate
diff --git a/src/declarative/qml/v8/qjsvalueiterator.cpp b/src/declarative/qml/v8/qjsvalueiterator.cpp
index ca9123f0c0..42bfd3408c 100644
--- a/src/declarative/qml/v8/qjsvalueiterator.cpp
+++ b/src/declarative/qml/v8/qjsvalueiterator.cpp
@@ -22,6 +22,7 @@
 ****************************************************************************/
 
 #include "qjsvalueiterator.h"
+#include "qjsvalueiterator_p.h"
 
 #include "qscriptisolate_p.h"
 #include "qjsvalue_p.h"
@@ -69,143 +70,6 @@ QT_BEGIN_NAMESPACE
     \sa QJSValue::property()
 */
 
-using v8::Persistent;
-using v8::Local;
-using v8::Array;
-using v8::String;
-using v8::Handle;
-using v8::Object;
-using v8::Value;
-
-// FIXME (Qt5) This class should be refactored. It should use the common Iterator interface.
-// FIXME it could be faster!
-class QJSValueIteratorPrivate {
-public:
-    inline QJSValueIteratorPrivate(const QJSValuePrivate* value);
-    inline ~QJSValueIteratorPrivate();
-
-    inline bool hasNext() const;
-    inline bool next();
-
-    inline QString name() const;
-
-    inline QScriptPassPointer<QJSValuePrivate> value() const;
-
-    inline bool isValid() const;
-    inline QV8Engine* engine() const;
-private:
-    Q_DISABLE_COPY(QJSValueIteratorPrivate)
-    //void dump(QString) const;
-
-    QScriptSharedDataPointer<QJSValuePrivate> m_object;
-    QList<v8::Persistent<v8::String> > m_names;
-    QMutableListIterator<v8::Persistent<v8::String> > m_iterator;
-};
-
-inline QJSValueIteratorPrivate::QJSValueIteratorPrivate(const QJSValuePrivate* value)
-    : m_object(const_cast<QJSValuePrivate*>(value))
-    , m_iterator(m_names)
-{
-    Q_ASSERT(value);
-    QV8Engine *engine = m_object->engine();
-    QScriptIsolate api(engine);
-    if (!m_object->isObject())
-        m_object = 0;
-    else {
-        v8::HandleScope scope;
-        Handle<Value> tmp = *value;
-        Handle<Object> obj = Handle<Object>::Cast(tmp);
-        Local<Array> names;
-
-        // FIXME we need newer V8!
-        //names = obj->GetOwnPropertyNames();
-        names = engine->getOwnPropertyNames(obj);
-
-        // it is suboptimal, it would be better to write iterator instead
-        uint32_t count = names->Length();
-        Local<String> name;
-        m_names.reserve(count); // The count is the maximal count of values.
-        for (uint32_t i = count - 1; i < count; --i) {
-            name = names->Get(i)->ToString();
-            m_names.append(v8::Persistent<v8::String>::New(name));
-        }
-
-        // Reinitialize the iterator.
-        m_iterator = m_names;
-    }
-}
-
-inline QJSValueIteratorPrivate::~QJSValueIteratorPrivate()
-{
-    QMutableListIterator<v8::Persistent<v8::String> > it = m_names;
-    //FIXME: we need register this QJSVAlueIterator
-    if (engine()) {
-        while (it.hasNext()) {
-            it.next().Dispose();
-        }
-    } else {
-        // FIXME leak ?
-    }
-}
-
-inline bool QJSValueIteratorPrivate::hasNext() const
-{
-    //dump("hasNext()");
-    return isValid()
-            ? m_iterator.hasNext() : false;
-}
-
-inline bool QJSValueIteratorPrivate::next()
-{
-    //dump("next();");
-    if (m_iterator.hasNext()) {
-        m_iterator.next();
-        return true;
-    }
-    return false;
-}
-
-inline QString QJSValueIteratorPrivate::name() const
-{
-    //dump("name");
-    if (!isValid())
-        return QString();
-
-    return QJSConverter::toString(m_iterator.value());
-}
-
-inline QScriptPassPointer<QJSValuePrivate> QJSValueIteratorPrivate::value() const
-{
-    //dump("value()");
-    if (!isValid())
-        return InvalidValue();
-
-    return m_object->property(m_iterator.value());
-}
-
-inline bool QJSValueIteratorPrivate::isValid() const
-{
-    bool result = m_object ? m_object->isValid() : false;
-    // We know that if this object is still valid then it is an object
-    // if this assumption is not correct then some other logic in this class
-    // have to be changed too.
-    Q_ASSERT(!result || m_object->isObject());
-    return result;
-}
-
-inline QV8Engine* QJSValueIteratorPrivate::engine() const
-{
-    return m_object ? m_object->engine() : 0;
-}
-
-//void QJSValueIteratorPrivate::dump(QString fname) const
-//{
-//    qDebug() << "    *** " << fname << " ***";
-//    foreach (Persistent<String> name, m_names) {
-//        qDebug() << "        - " << QJSConverter::toString(name);
-//    }
-//}
-
 /*!
     Constructs an iterator for traversing \a object. The iterator is
     set to be at the front of the sequence of properties (before the
diff --git a/src/declarative/qml/v8/qjsvalueiterator_impl_p.h b/src/declarative/qml/v8/qjsvalueiterator_impl_p.h
new file mode 100644
index 0000000000..68075e55f3
--- /dev/null
+++ b/src/declarative/qml/v8/qjsvalueiterator_impl_p.h
@@ -0,0 +1,121 @@
+/****************************************************************************
+**
+** Copyright (C) 2011 Nokia Corporation and/or its subsidiary(-ies).
+** All rights reserved.
+** Contact: Nokia Corporation (qt-info@nokia.com)
+**
+** This file is part of the QtDeclarative module of the Qt Toolkit.
+**
+** $QT_BEGIN_LICENSE:LGPL-ONLY$
+** GNU Lesser General Public License Usage
+** This file may be used under the terms of the GNU Lesser
+** General Public License version 2.1 as published by the Free Software
+** Foundation and appearing in the file LICENSE.LGPL included in the
+** packaging of this file.  Please review the following information to
+** ensure the GNU Lesser General Public License version 2.1 requirements
+** will be met: http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html.
+**
+** If you have questions regarding the use of this file, please contact
+** Nokia at qt-info@nokia.com.
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+
+#ifndef QJSVALUEITERATOR_IMPL_P_H
+#define QJSVALUEITERATOR_IMPL_P_H
+
+#include <qjsvalueiterator_p.h>
+#include <qv8engine_p.h>
+#include <qjsconverter_p.h>
+
+inline QJSValueIteratorPrivate::QJSValueIteratorPrivate(const QJSValuePrivate* value)
+    : m_object(const_cast<QJSValuePrivate*>(value))
+    , m_index(0)
+    , m_count(0)
+{
+    Q_ASSERT(value);
+    QV8Engine *engine = m_object->engine();
+    if (!m_object->isObject())
+        m_object = 0;
+    else {
+        QScriptIsolate api(engine, QScriptIsolate::NotNullEngine);
+        v8::HandleScope scope;
+
+        v8::Handle<v8::Value> tmp = *value;
+        v8::Handle<v8::Object> obj = v8::Handle<v8::Object>::Cast(tmp);
+        v8::Local<v8::Array> names;
+
+        // FIXME we need newer V8!
+        //names = obj->GetOwnPropertyNames();
+        names = engine->getOwnPropertyNames(obj);
+        m_names = v8::Persistent<v8::Array>::New(names);
+        m_count = names->Length();
+
+        engine->registerValueIterator(this);
+    }
+}
+
+inline QJSValueIteratorPrivate::~QJSValueIteratorPrivate()
+{
+    if (isValid()) {
+        engine()->unregisterValueIterator(this);
+        m_names.Dispose();
+    }
+}
+
+inline void QJSValueIteratorPrivate::invalidate()
+{
+    m_names.Dispose();
+    m_object.reset();
+    m_index = 0;
+    m_count = 0;
+}
+
+inline bool QJSValueIteratorPrivate::hasNext() const
+{
+    return isValid() ? m_index < m_count : false;
+}
+
+inline bool QJSValueIteratorPrivate::next()
+{
+    if (hasNext()) {
+        ++m_index;
+        return true;
+    }
+    return false;
+}
+
+inline QString QJSValueIteratorPrivate::name() const
+{
+    if (!isValid())
+        return QString();
+
+    v8::HandleScope handleScope;
+    return QJSConverter::toString(m_names->Get(m_index - 1)->ToString());
+}
+
+inline QScriptPassPointer<QJSValuePrivate> QJSValueIteratorPrivate::value() const
+{
+    if (!isValid())
+        return InvalidValue();
+
+    v8::HandleScope handleScope;
+    return m_object->property(m_names->Get(m_index - 1)->ToString());
+}
+
+inline bool QJSValueIteratorPrivate::isValid() const
+{
+    bool result = m_object ? m_object->isValid() : false;
+    // We know that if this object is still valid then it is an object
+    // if this assumption is not correct then some other logic in this class
+    // have to be changed too.
+    Q_ASSERT(!result || m_object->isObject());
+    return result;
+}
+
+inline QV8Engine* QJSValueIteratorPrivate::engine() const
+{
+    return m_object ? m_object->engine() : 0;
+}
+
+#endif // QJSVALUEITERATOR_IMPL_P_H
diff --git a/src/declarative/qml/v8/qjsvalueiterator_p.h b/src/declarative/qml/v8/qjsvalueiterator_p.h
new file mode 100644
index 0000000000..d3033d496b
--- /dev/null
+++ b/src/declarative/qml/v8/qjsvalueiterator_p.h
@@ -0,0 +1,64 @@
+/****************************************************************************
+**
+** Copyright (C) 2011 Nokia Corporation and/or its subsidiary(-ies).
+** All rights reserved.
+** Contact: Nokia Corporation (qt-info@nokia.com)
+**
+** This file is part of the QtDeclarative module of the Qt Toolkit.
+**
+** $QT_BEGIN_LICENSE:LGPL-ONLY$
+** GNU Lesser General Public License Usage
+** This file may be used under the terms of the GNU Lesser
+** General Public License version 2.1 as published by the Free Software
+** Foundation and appearing in the file LICENSE.LGPL included in the
+** packaging of this file.  Please review the following information to
+** ensure the GNU Lesser General Public License version 2.1 requirements
+** will be met: http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html.
+**
+** If you have questions regarding the use of this file, please contact
+** Nokia at qt-info@nokia.com.
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+
+#ifndef QJSVALUEITERATOR_P_H
+#define QJSVALUEITERATOR_P_H
+
+#include <qscripttools_p.h>
+#include <qjsvalue_p.h>
+
+#include <qv8_p.h>
+
+QT_BEGIN_NAMESPACE
+
+class QV8Engine;
+
+class QJSValueIteratorPrivate : public QScriptLinkedNode {
+public:
+    inline QJSValueIteratorPrivate(const QJSValuePrivate* value);
+    inline ~QJSValueIteratorPrivate();
+
+    inline bool hasNext() const;
+    inline bool next();
+
+    inline QString name() const;
+
+    inline QScriptPassPointer<QJSValuePrivate> value() const;
+
+    inline bool isValid() const;
+    inline QV8Engine* engine() const;
+
+    inline void invalidate();
+private:
+    Q_DISABLE_COPY(QJSValueIteratorPrivate)
+
+    QScriptSharedDataPointer<QJSValuePrivate> m_object;
+    v8::Persistent<v8::Array> m_names;
+    uint32_t m_index;
+    uint32_t m_count;
+};
+
+
+QT_END_NAMESPACE
+
+#endif // QJSVALUEITERATOR_P_H
diff --git a/src/declarative/qml/v8/qscript_impl_p.h b/src/declarative/qml/v8/qscript_impl_p.h
index e66b561efe..d197d9fd6b 100644
--- a/src/declarative/qml/v8/qscript_impl_p.h
+++ b/src/declarative/qml/v8/qscript_impl_p.h
@@ -37,5 +37,6 @@
 
 #include "qv8engine_impl_p.h"
 #include "qjsvalue_impl_p.h"
+#include "qjsvalueiterator_impl_p.h"
 
 #endif //QSCRIPT_IMPL_P_H
diff --git a/src/declarative/qml/v8/qv8engine_impl_p.h b/src/declarative/qml/v8/qv8engine_impl_p.h
index 5c56efdf39..382e221f5a 100644
--- a/src/declarative/qml/v8/qv8engine_impl_p.h
+++ b/src/declarative/qml/v8/qv8engine_impl_p.h
@@ -38,6 +38,7 @@
 #include "qv8engine_p.h"
 #include "qjsvalue_p.h"
 #include "qjsconverter_p.h"
+#include "qjsvalueiterator_p.h"
 
 QT_BEGIN_NAMESPACE
 
@@ -80,6 +81,10 @@ public:
     {
         value->reinitialize();
     }
+    void operator () (QJSValueIteratorPrivate *iterator) const
+    {
+        iterator->invalidate();
+    }
 };
 
 inline void QV8Engine::registerValue(QJSValuePrivate *data)
@@ -99,6 +104,23 @@ inline void QV8Engine::invalidateAllValues()
     m_values.clear();
 }
 
+inline void QV8Engine::registerValueIterator(QJSValueIteratorPrivate *data)
+{
+    m_valueIterators.insert(data);
+}
+
+inline void QV8Engine::unregisterValueIterator(QJSValueIteratorPrivate *data)
+{
+    m_valueIterators.remove(data);
+}
+
+inline void QV8Engine::invalidateAllIterators()
+{
+    QtScriptBagCleaner invalidator;
+    m_valueIterators.forEach(invalidator);
+    m_valueIterators.clear();
+}
+
 /*!
   \internal
   \note property can be index (v8::Integer) or a property (v8::String) name, according to ECMA script
diff --git a/src/declarative/qml/v8/qv8engine_p.h b/src/declarative/qml/v8/qv8engine_p.h
index 938454c901..9258a80b48 100644
--- a/src/declarative/qml/v8/qv8engine_p.h
+++ b/src/declarative/qml/v8/qv8engine_p.h
@@ -214,6 +214,8 @@ class QDeclarativeEngine;
 class QDeclarativeValueType;
 class QNetworkAccessManager;
 class QDeclarativeContextData;
+class QJSValueIteratorPrivate;
+
 class Q_DECLARATIVE_EXPORT QV8Engine
 {
 public:
@@ -260,6 +262,10 @@ public:
     inline void unregisterValue(QJSValuePrivate *data);
     inline void invalidateAllValues();
 
+    inline void registerValueIterator(QJSValueIteratorPrivate *data);
+    inline void unregisterValueIterator(QJSValueIteratorPrivate *data);
+    inline void invalidateAllIterators();
+
     QV8ContextWrapper *contextWrapper() { return &m_contextWrapper; }
     QV8QObjectWrapper *qobjectWrapper() { return &m_qobjectWrapper; }
     QV8TypeWrapper *typeWrapper() { return &m_typeWrapper; }
@@ -455,6 +461,7 @@ protected:
     QDateTime qtDateTimeFromJsDate(double jsDate);
 private:
     QScriptBagContainer<QJSValuePrivate> m_values;
+    QScriptBagContainer<QJSValueIteratorPrivate> m_valueIterators;
 
     Q_DISABLE_COPY(QV8Engine)
 };
diff --git a/src/declarative/qml/v8/script.pri b/src/declarative/qml/v8/script.pri
index 04a23d1f2b..7454d2004c 100644
--- a/src/declarative/qml/v8/script.pri
+++ b/src/declarative/qml/v8/script.pri
@@ -17,4 +17,6 @@ HEADERS += \
     $$PWD/qscriptshareddata_p.h \
     $$PWD/qscripttools_p.h \
     $$PWD/qscript_impl_p.h \
-    $$PWD/qscriptoriginalglobalobject_p.h
+    $$PWD/qscriptoriginalglobalobject_p.h \
+    $$PWD/qjsvalueiterator_p.h \
+    $$PWD/qjsvalueiterator_impl_p.h