Refactoring: removing m_reply from GeoReply classes

This patch tries to simplify the code removing the contained m_reply
from all the Geo[Map,Route,Geocode,Places]Reply classes.
The need for m_reply was associated to the "abort" method, but this
can be solved by emitting a signal in the superclass abort() method,
and connecting that to QNetworkReply::abort() in the constructor.

Since QNetworkReplyHttpImpl always sends an OperationCanceledError
it should then be safe to call deleteLater() on the network reply
in the slot connected to QNetworkReply::error

This patch also prevents the series of
"QCoreApplication::postEvent: Unexpected null receiver" warnings
that are generated due to deletingLater already deleted objects
(abort() emits an error, the reply is destroyed inside the
onError slot, but also in the abort() method).

Finally, this patch removes the setFinished() call in
QGeoRouteReply::abort() since the documentation does not mention
this, and all the subclasses do not perform this operation and
emit the corresponding signal.
tst_qgeoroutereply has been adapted accordingly.

Change-Id: I226ee163e7bed784dd7f0da1522e651459543bca
Reviewed-by: Alex Blasche <alexander.blasche@qt.io>
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>

4 files changed, 35 insertions, 83 deletions

diff --git a/src/plugins/geoservices/mapbox/qgeomapreplymapbox.cpp b/src/plugins/geoservices/mapbox/qgeomapreplymapbox.cpp
index 5fe9caa8..4b60231d 100644
--- a/src/plugins/geoservices/mapbox/qgeomapreplymapbox.cpp
+++ b/src/plugins/geoservices/mapbox/qgeomapreplymapbox.cpp
@@ -39,59 +39,42 @@
 #include <QtLocation/private/qgeotilespec_p.h>
 
 QGeoMapReplyMapbox::QGeoMapReplyMapbox(QNetworkReply *reply, const QGeoTileSpec &spec, const QString &format, QObject *parent)
-:   QGeoTiledMapReply(spec, parent), m_reply(reply), m_format (format)
+:   QGeoTiledMapReply(spec, parent), m_format (format)
 {
-    connect(m_reply, SIGNAL(finished()), this, SLOT(networkReplyFinished()));
-    connect(m_reply, SIGNAL(error(QNetworkReply::NetworkError)),
+    if (!reply) {
+        setError(UnknownError, QStringLiteral("Null reply"));
+        return;
+    }
+    connect(reply, SIGNAL(finished()), this, SLOT(networkReplyFinished()));
+    connect(reply, SIGNAL(error(QNetworkReply::NetworkError)),
             this, SLOT(networkReplyError(QNetworkReply::NetworkError)));
+    connect(this, &QGeoTiledMapReply::aborted, reply, &QNetworkReply::abort);
+    connect(this, &QObject::destroyed, reply, &QObject::deleteLater);
 }
 
 QGeoMapReplyMapbox::~QGeoMapReplyMapbox()
 {
-    if (m_reply) {
-        m_reply->deleteLater();
-        m_reply = 0;
-    }
-}
-
-void QGeoMapReplyMapbox::abort()
-{
-    if (!m_reply)
-        return;
-
-    m_reply->abort();
-}
-
-QNetworkReply *QGeoMapReplyMapbox::networkReply() const
-{
-    return m_reply;
 }
 
 void QGeoMapReplyMapbox::networkReplyFinished()
 {
-    if (!m_reply)
-        return;
+    QNetworkReply *reply = static_cast<QNetworkReply *>(sender());
+    reply->deleteLater();
 
-    if (m_reply->error() != QNetworkReply::NoError)
+    if (reply->error() != QNetworkReply::NoError)
         return;
 
-    setMapImageData(m_reply->readAll());
+    setMapImageData(reply->readAll());
     setMapImageFormat(m_format);
     setFinished(true);
-
-    m_reply->deleteLater();
-    m_reply = 0;
 }
 
 void QGeoMapReplyMapbox::networkReplyError(QNetworkReply::NetworkError error)
 {
-    if (!m_reply)
-        return;
-
-    if (error != QNetworkReply::OperationCanceledError)
-        setError(QGeoTiledMapReply::CommunicationError, m_reply->errorString());
-
-    setFinished(true);
-    m_reply->deleteLater();
-    m_reply = 0;
+    QNetworkReply *reply = static_cast<QNetworkReply *>(sender());
+    reply->deleteLater();
+    if (error == QNetworkReply::OperationCanceledError)
+        setFinished(true);
+    else
+        setError(QGeoTiledMapReply::CommunicationError, reply->errorString());
 }
diff --git a/src/plugins/geoservices/mapbox/qgeomapreplymapbox.h b/src/plugins/geoservices/mapbox/qgeomapreplymapbox.h
index 67ad61ad..c4a1dd82 100644
--- a/src/plugins/geoservices/mapbox/qgeomapreplymapbox.h
+++ b/src/plugins/geoservices/mapbox/qgeomapreplymapbox.h
@@ -51,16 +51,11 @@ public:
     explicit QGeoMapReplyMapbox(QNetworkReply *reply, const QGeoTileSpec &spec, const QString &format, QObject *parent = 0);
     ~QGeoMapReplyMapbox();
 
-    void abort();
-
-    QNetworkReply *networkReply() const;
-
 private Q_SLOTS:
     void networkReplyFinished();
     void networkReplyError(QNetworkReply::NetworkError error);
 
 private:
-    QPointer<QNetworkReply> m_reply;
     QString m_format;
 };
 
diff --git a/src/plugins/geoservices/mapbox/qgeoroutereplymapbox.cpp b/src/plugins/geoservices/mapbox/qgeoroutereplymapbox.cpp
index 4c98412b..8fc3386a 100644
--- a/src/plugins/geoservices/mapbox/qgeoroutereplymapbox.cpp
+++ b/src/plugins/geoservices/mapbox/qgeoroutereplymapbox.cpp
@@ -84,28 +84,21 @@ static QList<QGeoCoordinate> parseGeometry(const QJsonValue &geometry)
 
 QGeoRouteReplyMapbox::QGeoRouteReplyMapbox(QNetworkReply *reply, const QGeoRouteRequest &request,
                                      QObject *parent)
-:   QGeoRouteReply(request, parent), m_reply(reply)
+:   QGeoRouteReply(request, parent)
 {
-    connect(m_reply, SIGNAL(finished()), this, SLOT(networkReplyFinished()));
-    connect(m_reply, SIGNAL(error(QNetworkReply::NetworkError)),
+    if (!reply) {
+        setError(UnknownError, QStringLiteral("Null reply"));
+        return;
+    }
+    connect(reply, SIGNAL(finished()), this, SLOT(networkReplyFinished()));
+    connect(reply, SIGNAL(error(QNetworkReply::NetworkError)),
             this, SLOT(networkReplyError(QNetworkReply::NetworkError)));
+    connect(this, &QGeoRouteReply::aborted, reply, &QNetworkReply::abort);
+    connect(this, &QObject::destroyed, reply, &QObject::deleteLater);
 }
 
 QGeoRouteReplyMapbox::~QGeoRouteReplyMapbox()
 {
-    if (m_reply)
-        m_reply->deleteLater();
-}
-
-void QGeoRouteReplyMapbox::abort()
-{
-    if (!m_reply)
-        return;
-
-    m_reply->abort();
-
-    m_reply->deleteLater();
-    m_reply = 0;
 }
 
 static QGeoRoute constructRoute(const QJsonObject &obj)
@@ -177,25 +170,19 @@ static QGeoRoute constructRoute(const QJsonObject &obj)
 
 void QGeoRouteReplyMapbox::networkReplyFinished()
 {
-    if (!m_reply)
-        return;
+    QNetworkReply *reply = static_cast<QNetworkReply *>(sender());
+    reply->deleteLater();
 
-    if (m_reply->error() != QNetworkReply::NoError) {
-        setError(QGeoRouteReply::CommunicationError, m_reply->errorString());
-        m_reply->deleteLater();
-        m_reply = 0;
+    if (reply->error() != QNetworkReply::NoError)
         return;
-    }
 
-    QJsonDocument document = QJsonDocument::fromJson(m_reply->readAll());
+    QJsonDocument document = QJsonDocument::fromJson(reply->readAll());
     if (document.isObject()) {
         QJsonObject object = document.object();
 
         QString status = object.value(QStringLiteral("code")).toString();
         if (status != QStringLiteral("Ok")) {
             setError(QGeoRouteReply::UnknownError, object.value(QStringLiteral("message")).toString());
-            m_reply->deleteLater();
-            m_reply = 0;
             return;
         }
 
@@ -210,22 +197,14 @@ void QGeoRouteReplyMapbox::networkReplyFinished()
     } else {
         setError(QGeoRouteReply::ParseError, QStringLiteral("Couldn't parse json."));
     }
-
-    m_reply->deleteLater();
-    m_reply = 0;
 }
 
 void QGeoRouteReplyMapbox::networkReplyError(QNetworkReply::NetworkError error)
 {
     Q_UNUSED(error)
-
-    if (!m_reply)
-        return;
-
-    setError(QGeoRouteReply::CommunicationError, m_reply->errorString());
-
-    m_reply->deleteLater();
-    m_reply = 0;
+    QNetworkReply *reply = static_cast<QNetworkReply *>(sender());
+    reply->deleteLater();
+    setError(QGeoRouteReply::CommunicationError, reply->errorString());
 }
 
 QT_END_NAMESPACE
diff --git a/src/plugins/geoservices/mapbox/qgeoroutereplymapbox.h b/src/plugins/geoservices/mapbox/qgeoroutereplymapbox.h
index 9df45ac4..f19faee7 100644
--- a/src/plugins/geoservices/mapbox/qgeoroutereplymapbox.h
+++ b/src/plugins/geoservices/mapbox/qgeoroutereplymapbox.h
@@ -55,14 +55,9 @@ public:
     QGeoRouteReplyMapbox(QNetworkReply *reply, const QGeoRouteRequest &request, QObject *parent = 0);
     ~QGeoRouteReplyMapbox();
 
-    void abort() Q_DECL_OVERRIDE;
-
 private Q_SLOTS:
     void networkReplyFinished();
     void networkReplyError(QNetworkReply::NetworkError error);
-
-private:
-    QNetworkReply *m_reply;
 };
 
 QT_END_NAMESPACE