Simplify binding setup code further

Reduce the number of setBinding/removeBinding overloads and
simplify their internal handling.

Change-Id: I87174a3b2dc0ecb8380e8fc28f8969fbf475c728
Reviewed-by: Simon Hausmann <simon.hausmann@theqtcompany.com>

6 files changed, 120 insertions, 193 deletions

diff --git a/src/qml/qml/qqmlabstractbinding_p.h b/src/qml/qml/qqmlabstractbinding_p.h
index b9275dc2c3..0d8766a854 100644
--- a/src/qml/qml/qqmlabstractbinding_p.h
+++ b/src/qml/qml/qqmlabstractbinding_p.h
@@ -88,6 +88,7 @@ public:
     static inline Pointer getPointer(QQmlAbstractBinding *p);
     static void printBindingLoopError(QQmlProperty &prop);
 
+    inline QQmlAbstractBinding *nextBinding() const;
 
 protected:
     QQmlAbstractBinding(BindingType);
@@ -114,7 +115,6 @@ private:
     inline void setAddedToObject(bool v);
     inline bool isAddedToObject() const;
 
-    inline QQmlAbstractBinding *nextBinding() const;
     inline void setNextBinding(QQmlAbstractBinding *);
 
     // Pointer to the next binding in the linked list of bindings.
diff --git a/src/qml/qml/qqmlobjectcreator.cpp b/src/qml/qml/qqmlobjectcreator.cpp
index 5a1dbdfab0..cf438cdad7 100644
--- a/src/qml/qml/qqmlobjectcreator.cpp
+++ b/src/qml/qml/qqmlobjectcreator.cpp
@@ -62,14 +62,6 @@ struct ActiveOCRestorer
 };
 }
 
-static void removeBindingOnProperty(QObject *o, int index)
-{
-    int coreIndex;
-    int valueTypeIndex = QQmlPropertyData::decodeValueTypePropertyIndex(index, &coreIndex);
-    QQmlAbstractBinding *binding = QQmlPropertyPrivate::removeBinding(o, coreIndex, valueTypeIndex);
-    if (binding) binding->destroy();
-}
-
 QQmlObjectCreator::QQmlObjectCreator(QQmlContextData *parentContext, QQmlCompiledData *compiledData, QQmlContextData *creationContext, void *activeVMEDataForRootContext)
     : phase(Startup)
     , compiledData(compiledData)
@@ -664,8 +656,7 @@ void QQmlObjectCreator::setupBindings(const QBitArray &bindingsToSkip)
             QQmlPropertyPrivate::binding(_bindingTarget, _valueTypeProperty->coreIndex, -1);
 
         if (binding && binding->bindingType() != QQmlAbstractBinding::ValueTypeProxy) {
-            QQmlPropertyPrivate::removeBinding(_bindingTarget, _valueTypeProperty->coreIndex, -1);
-            binding->destroy();
+            QQmlPropertyPrivate::removeBinding(_bindingTarget, _valueTypeProperty->coreIndex, QQmlPropertyPrivate::DestroyOldBinding);
         } else if (binding) {
             QQmlValueTypeProxyBinding *proxy =
                 static_cast<QQmlValueTypeProxyBinding *>(binding);
@@ -800,7 +791,7 @@ bool QQmlObjectCreator::setPropertyBinding(const QQmlPropertyData *property, con
     if (_ddata->hasBindingBit(property->coreIndex) && !(binding->flags & QV4::CompiledData::Binding::IsSignalHandlerExpression)
         && !(binding->flags & QV4::CompiledData::Binding::IsOnAssignment)
         && !_valueTypeProperty)
-        removeBindingOnProperty(_bindingTarget, property->coreIndex);
+        QQmlPropertyPrivate::removeBinding(_bindingTarget, property->coreIndex, QQmlPropertyPrivate::DestroyOldBinding);
 
     if (binding->type == QV4::CompiledData::Binding::Type_Script) {
         QV4::Function *runtimeFunction = compiledData->compilationUnit->runtimeFunctions[binding->value.compiledScriptIndex];
@@ -834,12 +825,7 @@ bool QQmlObjectCreator::setPropertyBinding(const QQmlPropertyData *property, con
             qmlBinding->setTarget(_bindingTarget, targetCorePropertyData);
 
             if (targetCorePropertyData.isAlias()) {
-                QQmlAbstractBinding *old =
-                    QQmlPropertyPrivate::setBindingNoEnable(_bindingTarget,
-                                                            targetCorePropertyData.coreIndex,
-                                                            targetCorePropertyData.getValueTypeCoreIndex(),
-                                                            qmlBinding);
-                if (old) { old->destroy(); }
+                QQmlPropertyPrivate::setBinding(qmlBinding, QQmlPropertyPrivate::DontEnable|QQmlPropertyPrivate::DestroyOldBinding);
             } else {
                 qmlBinding->addToObject();
 
diff --git a/src/qml/qml/qqmlproperty.cpp b/src/qml/qml/qqmlproperty.cpp
index f003bf8c3f..cd59c2dd41 100644
--- a/src/qml/qml/qqmlproperty.cpp
+++ b/src/qml/qml/qqmlproperty.cpp
@@ -717,37 +717,73 @@ QQmlPropertyPrivate::binding(const QQmlProperty &that)
     \a flags is passed through to the binding and is used for the initial update (when
     the binding sets the initial value, it will use these flags for the write).
 */
-QQmlAbstractBinding *
-QQmlPropertyPrivate::setBinding(const QQmlProperty &that,
-                                QQmlAbstractBinding *newBinding,
-                                WriteFlags flags)
+void
+QQmlPropertyPrivate::setBinding(const QQmlProperty &that, QQmlAbstractBinding *newBinding)
 {
-    if (!newBinding)
+    if (!newBinding) {
         removeBinding(that);
+        return;
+    }
 
     if (!that.d || !that.isProperty() || !that.d->object) {
         newBinding->destroy();
+        return;
+    }
+    setBinding(newBinding);
+}
+
+static QQmlAbstractBinding *removeOldBinding(QObject *object, int index, QQmlPropertyPrivate::BindingFlags flags)
+{
+    int coreIndex;
+    int valueTypeIndex = QQmlPropertyData::decodeValueTypePropertyIndex(index, &coreIndex);
+
+    QQmlData *data = QQmlData::get(object, false);
+
+    if (!data || !data->hasBindingBit(coreIndex))
+        return 0;
+
+    QQmlAbstractBinding *oldBinding = data->bindings;
+
+    while (oldBinding && oldBinding->targetPropertyIndex() != coreIndex)
+        oldBinding = oldBinding->nextBinding();
+
+    if (!oldBinding)
+        return 0;
+
+    if (valueTypeIndex != -1 && oldBinding->bindingType() == QQmlAbstractBinding::ValueTypeProxy)
+        oldBinding = static_cast<QQmlValueTypeProxyBinding *>(oldBinding)->binding(index);
+
+    if (!oldBinding)
+        return 0;
+
+    oldBinding->removeFromObject();
+    if (!(flags & QQmlPropertyPrivate::DontEnable))
+        oldBinding->setEnabled(false, 0);
+
+    if (flags & QQmlPropertyPrivate::DestroyOldBinding) {
+        oldBinding->destroy();
         return 0;
     }
 
-    // In the case that the new binding is provided, we must target the property it
-    // is associated with.  If we don't do this, retargetBinding() can fail.
-    QObject *object = newBinding->targetObject();
-    int pi = newBinding->targetPropertyIndex();
+    return oldBinding;
+}
 
-    int core;
-    int vt = QQmlPropertyData::decodeValueTypePropertyIndex(pi, &core);
+QQmlAbstractBinding *QQmlPropertyPrivate::removeBinding(QObject *o, int index, QQmlPropertyPrivate::BindingFlag flags)
+{
+    Q_ASSERT(o);
 
-    return setBinding(object, core, vt, newBinding, flags);
+    QObject *target;
+    int targetIndex;
+    findAliasTarget(o, index, &target, &targetIndex);
+    return removeOldBinding(target, targetIndex, flags);
 }
 
-QQmlAbstractBinding *QQmlPropertyPrivate::removeBinding(const QQmlProperty &that)
+QQmlAbstractBinding *QQmlPropertyPrivate::removeBinding(const QQmlProperty &that, BindingFlag flags)
 {
     if (!that.d || !that.isProperty() || !that.d->object)
         return 0;
 
-    return removeBinding(that.d->object, that.d->core.coreIndex,
-                         that.d->core.getValueTypeCoreIndex());
+    return removeBinding(that.d->object, that.d->core.encodedIndex(), flags);
 }
 
 QQmlAbstractBinding *
@@ -792,11 +828,11 @@ QQmlPropertyPrivate::binding(QObject *object, int coreIndex, int valueTypeIndex)
 void QQmlPropertyPrivate::findAliasTarget(QObject *object, int bindingIndex,
                                                   QObject **targetObject, int *targetBindingIndex)
 {
-    int coreIndex;
-    int valueTypeIndex = QQmlPropertyData::decodeValueTypePropertyIndex(bindingIndex, &coreIndex);
-
     QQmlData *data = QQmlData::get(object, false);
     if (data) {
+        int coreIndex;
+        int valueTypeIndex = QQmlPropertyData::decodeValueTypePropertyIndex(bindingIndex, &coreIndex);
+
         QQmlPropertyData *propertyData =
             data->propertyCache?data->propertyCache->property(coreIndex):0;
         if (propertyData && propertyData->isAlias()) {
@@ -823,118 +859,30 @@ void QQmlPropertyPrivate::findAliasTarget(QObject *object, int bindingIndex,
     *targetBindingIndex = bindingIndex;
 }
 
-QQmlAbstractBinding *
-QQmlPropertyPrivate::setBinding(QObject *object, int coreIndex, int valueTypeIndex,
-                                        QQmlAbstractBinding *newBinding, WriteFlags flags)
-{
-    QQmlData *data = QQmlData::get(object, 0 != newBinding);
-    QQmlAbstractBinding *binding = 0;
-
-    if (data) {
-        QQmlPropertyData *propertyData =
-            data->propertyCache?data->propertyCache->property(coreIndex):0;
-        if (propertyData && propertyData->isAlias()) {
-            QQmlVMEMetaObject *vme = QQmlVMEMetaObject::getForProperty(object, coreIndex);
-
-            QObject *aObject = 0; int aCoreIndex = -1; int aValueTypeIndex = -1;
-            if (!vme->aliasTarget(coreIndex, &aObject, &aCoreIndex, &aValueTypeIndex)) {
-                if (newBinding) newBinding->destroy();
-                return 0;
-            }
-
-            // This will either be a value type sub-reference or an alias to a value-type sub-reference not both
-            Q_ASSERT(valueTypeIndex == -1 || aValueTypeIndex == -1);
-            aValueTypeIndex = (valueTypeIndex == -1)?aValueTypeIndex:valueTypeIndex;
-            return setBinding(aObject, aCoreIndex, aValueTypeIndex, newBinding, flags);
-        }
-    }
-
-    if (data && data->hasBindingBit(coreIndex)) {
-        binding = data->bindings;
-
-        while (binding && binding->targetPropertyIndex() != coreIndex)
-            binding = binding->nextBinding();
-    }
-
-    int index = coreIndex;
-    if (valueTypeIndex != -1)
-        index = QQmlPropertyData::encodeValueTypePropertyIndex(index, valueTypeIndex);
-
-    if (binding && valueTypeIndex != -1 && binding->bindingType() == QQmlAbstractBinding::ValueTypeProxy)
-        binding = static_cast<QQmlValueTypeProxyBinding *>(binding)->binding(index);
-
-    if (binding) {
-        binding->removeFromObject();
-        binding->setEnabled(false, 0);
-    }
-
-    if (newBinding) {
-        Q_ASSERT(newBinding->targetPropertyIndex() == index);
-        Q_ASSERT(newBinding->targetObject() == object);
-
-        newBinding->addToObject();
-        newBinding->setEnabled(true, flags);
-    }
-
-    return binding;
-}
-
-QQmlAbstractBinding *QQmlPropertyPrivate::removeBinding(QObject *object, int coreIndex, int valueTypeIndex)
-{
-    return setBinding(object, coreIndex, valueTypeIndex, 0);
-}
 
-QQmlAbstractBinding *
-QQmlPropertyPrivate::setBindingNoEnable(QObject *object, int coreIndex, int valueTypeIndex,
-                                                QQmlAbstractBinding *newBinding)
+void QQmlPropertyPrivate::setBinding(QQmlAbstractBinding *binding, BindingFlags flags, WriteFlags writeFlags)
 {
-    QQmlData *data = QQmlData::get(object, 0 != newBinding);
-    QQmlAbstractBinding *binding = 0;
+    Q_ASSERT(binding);
 
-    if (data) {
-        QQmlPropertyData *propertyData =
-            data->propertyCache?data->propertyCache->property(coreIndex):0;
-        if (propertyData && propertyData->isAlias()) {
-            QQmlVMEMetaObject *vme = QQmlVMEMetaObject::getForProperty(object, coreIndex);
-
-            QObject *aObject = 0; int aCoreIndex = -1; int aValueTypeIndex = -1;
-            if (!vme->aliasTarget(coreIndex, &aObject, &aCoreIndex, &aValueTypeIndex)) {
-                if (newBinding) newBinding->destroy();
-                return 0;
-            }
-
-            // This will either be a value type sub-reference or an alias to a value-type sub-reference not both
-            Q_ASSERT(valueTypeIndex == -1 || aValueTypeIndex == -1);
-            aValueTypeIndex = (valueTypeIndex == -1)?aValueTypeIndex:valueTypeIndex;
-            return setBindingNoEnable(aObject, aCoreIndex, aValueTypeIndex, newBinding);
-        }
-    }
+    QObject *object = binding->targetObject();
+    int index = binding->targetPropertyIndex();
 
-    if (data && data->hasBindingBit(coreIndex)) {
-        binding = data->bindings;
-
-        while (binding && binding->targetPropertyIndex() != coreIndex)
-            binding = binding->nextBinding();
+#ifndef QT_NO_DEBUG
+    int coreIndex;
+    QQmlPropertyData::decodeValueTypePropertyIndex(index, &coreIndex);
+    QQmlData *data = QQmlData::get(object, true);
+    if (data->propertyCache) {
+        QQmlPropertyData *propertyData = data->propertyCache->property(coreIndex);
+        Q_ASSERT(propertyData && !propertyData->isAlias());
     }
+#endif
 
-    int index = coreIndex;
-    if (valueTypeIndex != -1)
-        index = QQmlPropertyData::encodeValueTypePropertyIndex(index, valueTypeIndex);
-
-    if (binding && valueTypeIndex != -1 && binding->bindingType() == QQmlAbstractBinding::ValueTypeProxy)
-        binding = static_cast<QQmlValueTypeProxyBinding *>(binding)->binding(index);
-
-    if (binding)
-        binding->removeFromObject();
-
-    if (newBinding) {
-        Q_ASSERT(newBinding->targetPropertyIndex() == index);
-        Q_ASSERT(newBinding->targetObject() == object);
+    removeOldBinding(object, index, flags);
 
-        newBinding->addToObject();
-    }
+    binding->addToObject();
+    if (!(flags & DontEnable))
+        binding->setEnabled(true, writeFlags);
 
-    return binding;
 }
 
 /*!
@@ -1215,11 +1163,8 @@ QQmlPropertyPrivate::writeValueProperty(QObject *object,
                                         QQmlContextData *context, WriteFlags flags)
 {
     // Remove any existing bindings on this property
-    if (!(flags & DontRemoveBinding) && object) {
-        QQmlAbstractBinding *binding = removeBinding(object, core.coreIndex,
-                                                          core.getValueTypeCoreIndex());
-        if (binding) binding->destroy();
-    }
+    if (!(flags & DontRemoveBinding) && object)
+        removeBinding(object, core.encodedIndex(), DestroyOldBinding);
 
     bool rv = false;
     if (core.isValueTypeVirtual()) {
diff --git a/src/qml/qml/qqmlproperty_p.h b/src/qml/qml/qqmlproperty_p.h
index 4b51128fef..b6b5421999 100644
--- a/src/qml/qml/qqmlproperty_p.h
+++ b/src/qml/qml/qqmlproperty_p.h
@@ -104,14 +104,17 @@ public:
                       QQmlContextData *, WriteFlags flags = 0);
     static void findAliasTarget(QObject *, int, QObject **, int *);
 
-    static QQmlAbstractBinding *setBinding(QObject *, int coreIndex,
-                                                   int valueTypeIndex /* -1 */,
-                                                   QQmlAbstractBinding *,
-                                                   WriteFlags flags = DontRemoveBinding);
-    static QQmlAbstractBinding *removeBinding(QObject *object, int coreIndex, int valueTypeIndex /* -1 */);
-    static QQmlAbstractBinding *setBindingNoEnable(QObject *, int coreIndex,
-                                                           int valueTypeIndex /* -1 */,
-                                                           QQmlAbstractBinding *);
+    enum BindingFlag {
+        None = 0,
+        DestroyOldBinding = 0x1,
+        DontEnable = 0x2
+    };
+    Q_DECLARE_FLAGS(BindingFlags, BindingFlag)
+
+    static void setBinding(QQmlAbstractBinding *binding, BindingFlags flags = None, WriteFlags writeFlags = DontRemoveBinding);
+
+    static QQmlAbstractBinding *removeBinding(const QQmlProperty &that, BindingFlag flag = None);
+    static QQmlAbstractBinding *removeBinding(QObject *o, int index, BindingFlag flag = None);
     static QQmlAbstractBinding *binding(QObject *, int coreIndex,
                                                 int valueTypeIndex /* -1 */);
 
@@ -130,10 +133,7 @@ public:
 
     // "Public" (to QML) methods
     static QQmlAbstractBinding *binding(const QQmlProperty &that);
-    static QQmlAbstractBinding *setBinding(const QQmlProperty &that,
-                                                   QQmlAbstractBinding *,
-                                                   WriteFlags flags = DontRemoveBinding);
-    static QQmlAbstractBinding *removeBinding(const QQmlProperty &that);
+    static void setBinding(const QQmlProperty &that, QQmlAbstractBinding *);
     static QQmlBoundSignalExpression *signalExpression(const QQmlProperty &that);
     static QQmlBoundSignalExpressionPointer setSignalExpression(const QQmlProperty &that,
                                                                 QQmlBoundSignalExpression *);
@@ -156,6 +156,7 @@ public:
 };
 
 Q_DECLARE_OPERATORS_FOR_FLAGS(QQmlPropertyPrivate::WriteFlags)
+Q_DECLARE_OPERATORS_FOR_FLAGS(QQmlPropertyPrivate::BindingFlags)
 
 QT_END_NAMESPACE
 
diff --git a/src/qml/qml/qqmlvaluetypewrapper.cpp b/src/qml/qml/qqmlvaluetypewrapper.cpp
index 93df3a6d89..3d54833481 100644
--- a/src/qml/qml/qqmlvaluetypewrapper.cpp
+++ b/src/qml/qml/qqmlvaluetypewrapper.cpp
@@ -407,45 +407,42 @@ void QQmlValueTypeWrapper::put(Managed *m, String *name, const Value &value)
     QMetaProperty property = metaObject->property(pd->coreIndex);
     Q_ASSERT(property.isValid());
 
-    QQmlBinding *newBinding = 0;
-
-    QV4::ScopedFunctionObject f(scope, value);
-    if (reference && f) {
-        if (!f->isBinding()) {
-            // assigning a JS function to a non-var-property is not allowed.
-            QString error = QStringLiteral("Cannot assign JavaScript function to value-type property");
-            ScopedString e(scope, v4->newString(error));
-            v4->throwError(e);
-            return;
-        }
+    if (reference) {
+        QV4::ScopedFunctionObject f(scope, value);
+        if (f) {
+            if (!f->isBinding()) {
+                // assigning a JS function to a non-var-property is not allowed.
+                QString error = QStringLiteral("Cannot assign JavaScript function to value-type property");
+                ScopedString e(scope, v4->newString(error));
+                v4->throwError(e);
+                return;
+            }
 
-        QQmlContextData *context = QmlContextWrapper::callingContext(v4);
+            QQmlContextData *context = QmlContextWrapper::callingContext(v4);
 
-        QQmlPropertyData cacheData;
-        cacheData.setFlags(QQmlPropertyData::IsWritable |
-                           QQmlPropertyData::IsValueTypeVirtual);
-        cacheData.propType = writeBackPropertyType;
-        cacheData.coreIndex = reference->d()->property;
-        cacheData.valueTypeFlags = 0;
-        cacheData.valueTypeCoreIndex = pd->coreIndex;
-        cacheData.valueTypePropType = property.userType();
+            QQmlPropertyData cacheData;
+            cacheData.setFlags(QQmlPropertyData::IsWritable |
+                               QQmlPropertyData::IsValueTypeVirtual);
+            cacheData.propType = writeBackPropertyType;
+            cacheData.coreIndex = reference->d()->property;
+            cacheData.valueTypeFlags = 0;
+            cacheData.valueTypeCoreIndex = pd->coreIndex;
+            cacheData.valueTypePropType = property.userType();
 
-        QV4::Scoped<QQmlBindingFunction> bindingFunction(scope, (const Value &)f);
-        bindingFunction->initBindingLocation();
+            QV4::Scoped<QQmlBindingFunction> bindingFunction(scope, (const Value &)f);
+            bindingFunction->initBindingLocation();
 
-        newBinding = new QQmlBinding(value, reference->d()->object, context);
-        newBinding->setTarget(reference->d()->object, cacheData);
-    }
+            QQmlBinding *newBinding = new QQmlBinding(value, reference->d()->object, context);
+            newBinding->setTarget(reference->d()->object, cacheData);
+            QQmlPropertyPrivate::setBinding(newBinding, QQmlPropertyPrivate::DestroyOldBinding);
+            return;
+        } else {
+            QQmlPropertyPrivate::removeBinding(reference->d()->object, QQmlPropertyData::encodeValueTypePropertyIndex(reference->d()->property, pd->coreIndex),
+                                               QQmlPropertyPrivate::DestroyOldBinding);
 
-    if (reference) {
-        QQmlAbstractBinding *oldBinding =
-                QQmlPropertyPrivate::setBinding(reference->d()->object, reference->d()->property, pd->coreIndex, newBinding);
-        if (oldBinding)
-            oldBinding->destroy();
+        }
     }
 
-    if (newBinding)
-        return;
 
     QVariant v = v4->toVariant(value, property.userType());
 
diff --git a/src/qml/qml/qqmlvmemetaobject.cpp b/src/qml/qml/qqmlvmemetaobject.cpp
index 12600229a9..586d4aa0b1 100644
--- a/src/qml/qml/qqmlvmemetaobject.cpp
+++ b/src/qml/qml/qqmlvmemetaobject.cpp
@@ -874,10 +874,8 @@ int QQmlVMEMetaObject::metaCall(QMetaObject::Call c, int _id, void **a)
                     int flags = *reinterpret_cast<int*>(a[3]);
                     if (flags & QQmlPropertyPrivate::RemoveBindingOnAliasWrite) {
                         QQmlData *targetData = QQmlData::get(target);
-                        if (targetData && targetData->hasBindingBit(d->propertyIndex())) {
-                            QQmlAbstractBinding *binding = QQmlPropertyPrivate::removeBinding(target, d->propertyIndex(), d->isValueTypeAlias() ? d->valueTypeIndex() : -1);
-                            if (binding) binding->destroy();
-                        }
+                        if (targetData && targetData->hasBindingBit(d->propertyIndex()))
+                            QQmlPropertyPrivate::removeBinding(target, d->propertyIdx, QQmlPropertyPrivate::DestroyOldBinding);
                     }
                 }