Merge lp:~aacid/oxide/qt54_variantjs into lp:~oxide-developers/oxide/oxide.trunk

[Albert Astals Cid]

Adapt to behaviour change in handling of QVariants from QML to C++ in Qt 5.4

[QTBUG-40431] When a JavaScript object/array is passed to C++ through
  a QVariant, the engine no longer immediately converts the object
  recursively into a QVariantMap or QVariantList but instead stores a
  QJSValue in the QVariant. This prevents a loss of data when the JS
  object contains non-primitive types such as function objects for
  example. Code that expects the variant type to be exactly
  QVariant::Map or QVariant::List may need to be adapted. Registered
  conversion functions however ensure that code that merely calls
  toMap() or toList() continues to work. Fixes: https://bugs.launchpad.net/bugs/1417963

[Olivier Tilloy (osomon)]

Looks good to me, and I’ve verified that the unit tests still pass with Qt 5.3.
Haven’t tested with Qt 5.4 yet.

[Timo Jyrinki (timo-jyrinki)]

I'll build it against Qt 5.4 once there's a version of the patch that applies to either 1.4 or 1.5. Olivier is looking at it.

[Timo Jyrinki (timo-jyrinki)]

Ok I've it rebased for 1.4 (just some fuzz) and it'd apply against 1.5 fine. I'll report once the armhf 1.4 build patched with this branch has finished building.

[Chris Coulson (chrisccoulson)]

This Qt change kind of sucks - it's effectively an API break in what should be a stable API. It also means that if existing objects without a QtQml dependency have QVariant properties, they need to add a QtQml dependency for the object to be usable in Qml when they really only needed a QtCore dependency before.

If developers wanted to avoid a loss of data in the case where the JS object contains non-primitive types, surely they would just expose a QJSValue rather than using QVariant and making existing users of QVariant jump through an extra hoop (where we just discard the non-primitive types anyway, resulting in the same loss of data as before)?

review: Needs Information

[Albert Astals Cid (aacid)]

> This Qt change kind of sucks
Agreed

> It also means that if existing objects without a QtQml dependency have QVariant properties,
> they need to add a QtQml dependency for the object to be usable in Qml when they really only
> needed a QtCore dependency before.
Or they need to stop checking for the type of things, checking for the type of a QVariant is not a great idea when there's canConvert that will do the right thing.

> If developers wanted to avoid a loss of data in the case where the JS object contains
> non-primitive types, surely they would just expose a QJSValue rather than using QVariant and
> making existing users of QVariant jump through an extra hoop (where we just discard the
> non-primitive types anyway, resulting in the same loss of data as before)?
I didn't do that change, so i can't provide any more insight than the changelog entry.

[Chris Coulson (chrisccoulson)]

I'll approve this as I guess we'll need to do it anyway. It's a shame they couldn't have fixed that bug another way though

review: Approve

[Chris Coulson (chrisccoulson)]

And thanks for fixing it

=== modified file 'qt/quick/api/oxideqquickwebcontextdelegateworker.cc'
--- qt/quick/api/oxideqquickwebcontextdelegateworker.cc	2014-10-16 19:14:06 +0000
+++ qt/quick/api/oxideqquickwebcontextdelegateworker.cc	2015-02-05 14:08:26 +0000
@@ -116,13 +116,17 @@
       controller_(controller) {}
    
 void Api::sendMessage(const QVariant& message) {
-  if (message.type() != QVariant::Map &&
-      message.type() != QVariant::List &&
-      message.type() != QVariant::StringList) {
+  QVariant aux = message;
+  if (aux.userType() == qMetaTypeId<QJSValue>()) {
+    aux = aux.value<QJSValue>().toVariant();
+  }
+  if (aux.type() != QVariant::Map &&
+      aux.type() != QVariant::List &&
+      aux.type() != QVariant::StringList) {
     return;
   }
 
-  Q_EMIT controller_->sendMessage(message);
+  Q_EMIT controller_->sendMessage(aux);
 }
 
 void IOThreadControllerImpl::CallEntryPointInWorker(
@@ -300,16 +304,21 @@
 void OxideQQuickWebContextDelegateWorker::sendMessage(const QVariant& message) {
   Q_D(OxideQQuickWebContextDelegateWorker);
 
-  if (message.type() != QVariant::Map &&
-      message.type() != QVariant::List &&
-      message.type() != QVariant::StringList) {
-    qWarning() << "Called WebContextDelegateWorker.sendMessage with an invalid argument";
+  QVariant aux = message;
+  if (aux.userType() == qMetaTypeId<QJSValue>()) {
+    aux = aux.value<QJSValue>().toVariant();
+  }
+
+  if (aux.type() != QVariant::Map &&
+      aux.type() != QVariant::List &&
+      aux.type() != QVariant::StringList) {
+    qWarning() << "Called WebContextDelegateWorker.sendMessage with an invalid argument" << aux;
     return;
   }
 
   QMetaObject::invokeMethod(d->io_thread_controller_.data(),
                             "receiveMessage",
-                            Q_ARG(QVariant, message));
+                            Q_ARG(QVariant, aux));
 }
 
 #include "oxideqquickwebcontextdelegateworker.moc"

=== modified file 'qt/quick/api/oxideqquickwebframe.cc'
--- qt/quick/api/oxideqquickwebframe.cc	2015-01-16 22:46:17 +0000
+++ qt/quick/api/oxideqquickwebframe.cc	2015-02-05 14:08:26 +0000
@@ -193,7 +193,12 @@
   OxideQQuickScriptMessageRequest* request =
       new OxideQQuickScriptMessageRequest();
 
-  if (!d->sendMessage(context, msg_id, args,
+  QVariant aux = args;
+  if (aux.userType() == qMetaTypeId<QJSValue>()) {
+    aux = aux.value<QJSValue>().toVariant();
+  }
+
+  if (!d->sendMessage(context, msg_id, aux,
                       OxideQQuickScriptMessageRequestPrivate::get(request))) {
     delete request;
     return nullptr;
@@ -207,5 +212,10 @@
                                              const QVariant& args) {
   Q_D(OxideQQuickWebFrame);
 
-  d->sendMessageNoReply(context, msg_id, args);
+  QVariant aux = args;
+  if (aux.userType() == qMetaTypeId<QJSValue>()) {
+    aux = aux.value<QJSValue>().toVariant();
+  }
+
+  d->sendMessageNoReply(context, msg_id, aux);
 }