Merge lp:~mandel/ubuntu-download-manager/remove-network-reply-pimpl into lp:ubuntu-download-manager

[Manuel de la Peña]

Remove pimpl implementation from the NetworkReply object. Moved signals to use new way to connect.

Remove pimpl implementation from the NetworkReply object. Moved signals to use new way to connect.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:173
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/215/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-amd64-ci/104
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-armhf-ci/104/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/ubuntu-download-manager-ci/215/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:174
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/232/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-amd64-ci/121
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-armhf-ci/121/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/ubuntu-download-manager-ci/232/rebuild

review: Needs Fixing (continuous-integration)

[Diego Sarmentero (diegosarmentero)]

+1

review: Approve

[Manuel de la Peña (mandel)]

Approving because it is just a Jenkins error.

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-autolanding/96/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-amd64-autolanding/41
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-armhf-autolanding/41/console

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:176
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/246/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-amd64-ci/135
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-armhf-ci/135/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-download-manager-ci/246/rebuild

review: Needs Fixing (continuous-integration)

[Manuel de la Peña (mandel)]

Looks like an issue with the armhf jenkins instance.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-autolanding/104/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-amd64-autolanding/49
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-armhf-autolanding/49/console

review: Needs Fixing (continuous-integration)

[lp:~mandel/ubuntu-download-manager/remove-network-reply-pimpl]

177. By Manuel de la Peña on 2013-12-02

Remove the need of the tests because the signal connection checks are done at compile time thx to the new connection style.

[Manuel de la Peña (mandel)]

I'm removing the problematic tests because the connection of the signals is now performed with the new connections style and that means that the check is done at compile time.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:177
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/250/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-amd64-ci/139
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-armhf-ci/139

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-download-manager-ci/250/rebuild

review: Approve (continuous-integration)

=== modified file 'libubuntudownloadmanager/system/network_reply.cpp'
--- libubuntudownloadmanager/system/network_reply.cpp	2013-10-29 13:42:44 +0000
+++ libubuntudownloadmanager/system/network_reply.cpp	2013-12-02 11:34:50 +0000
@@ -25,126 +25,80 @@
 
 namespace System {
 
-/*
- * PRIVATE IMPLEMENTATION
- */
-
-class NetworkReplyPrivate {
-    Q_DECLARE_PUBLIC(NetworkReply)
-
- public:
-    NetworkReplyPrivate(QNetworkReply* reply, NetworkReply* parent)
-        : _reply(reply),
-          q_ptr(parent) {
-        // connect to all the signals so that we foward them
-        Q_Q(NetworkReply);
-        if (_reply != NULL) {
-            q->connect(_reply, SIGNAL(downloadProgress(qint64, qint64)),
-                q, SIGNAL(downloadProgress(qint64, qint64)));
-            q->connect(_reply, SIGNAL(error(QNetworkReply::NetworkError)),
-                q, SIGNAL(error(QNetworkReply::NetworkError)));
-            q->connect(_reply, SIGNAL(finished()),
-                q, SIGNAL(finished()));
-            q->connect(_reply, SIGNAL(sslErrors(const QList<QSslError>&)),
-                q, SIGNAL(sslErrors(const QList<QSslError>&)));
-        }
-    }
-
-    // public methods used by other parts of the code
-    QByteArray readAll() {
-        return _reply->readAll();
-    }
-
-    void abort() {
-        _reply->abort();
-    }
-
-    void setReadBufferSize(uint size) {
-        _reply->setReadBufferSize(size);
-    }
-
-    void setAcceptedCertificates(const QList<QSslCertificate>& certs) {
-        _certs = certs;
-        // build possible errors
-        foreach(const QSslCertificate& certificate, _certs) {
-            QSslError error(QSslError::SelfSignedCertificate, certificate);
-            _sslErrors.append(error);
-        }
-    }
-
-    bool canIgnoreSslErrors(const QList<QSslError>& errors) {
-        if (_sslErrors.count() > 0 && errors.count() > 0) {
-            foreach(QSslError error, errors) {
-                QSslError::SslError type = error.error();
-                if (type != QSslError::NoError &&
-                    type != QSslError::SelfSignedCertificate) {
-                    // we only support self signed certificates all errors
-                    // will not be ignored
-                    qDebug() << "SSL error type not ignored";
-                    return false;
-                } else if (type == QSslError::SelfSignedCertificate) {
-                    // just ignore those errors of the added errors
-                    if (!_certs.contains(error.certificate())) {
-                        qDebug() << "SSL certificate not ignored";
-                        return false;
-                    }
-                }
-            }
-
-            if (_reply != NULL) {
-                _reply->ignoreSslErrors(_sslErrors);
-            }
-
-            return true;
-        }
-        return false;
-    }
-
- private:
-    QList<QSslCertificate> _certs;
-    QList<QSslError> _sslErrors;
-    QNetworkReply* _reply;
-    NetworkReply* q_ptr;
-};
-
-/*
- * PUBLIC IMPLEMENTATION
- */
-
-NetworkReply::NetworkReply(QNetworkReply* reply, QObject *parent)
+NetworkReply::NetworkReply(QNetworkReply* reply, QObject* parent)
     : QObject(parent),
-      d_ptr(new NetworkReplyPrivate(reply, this)) {
+      _reply(reply) {
+    // connect to all the signals so that we foward them
+    if (_reply != NULL) {
+        connect(_reply, &QNetworkReply::downloadProgress,
+            this, &NetworkReply::downloadProgress);
+        connect(_reply, &QNetworkReply::finished,
+            this, &NetworkReply::finished);
+        connect(_reply, &QNetworkReply::sslErrors,
+            this, &NetworkReply::sslErrors);
+	// becuase error is overloaded we need to help the compiler
+	connect(_reply, static_cast<void(QNetworkReply::*)
+	    (QNetworkReply::NetworkError)>(&QNetworkReply::error),
+		this, &NetworkReply::error);
+    }
 }
 
+NetworkReply::~NetworkReply() {
+    delete _reply;
+}
 
 QByteArray
 NetworkReply::readAll() {
-    Q_D(NetworkReply);
-    return d->readAll();
+    return _reply->readAll();
 }
 
 void
 NetworkReply::abort() {
-    Q_D(NetworkReply);
-    d->abort();
+    _reply->abort();
 }
 
 void
 NetworkReply::setReadBufferSize(uint size) {
-    Q_D(NetworkReply);
-    d->setReadBufferSize(size);
+    _reply->setReadBufferSize(size);
 }
 
 void
 NetworkReply::setAcceptedCertificates(const QList<QSslCertificate>& certs) {
-    Q_D(NetworkReply);
-    d->setAcceptedCertificates(certs);
+    _certs = certs;
+    // build possible errors
+    foreach(const QSslCertificate& certificate, _certs) {
+        QSslError error(QSslError::SelfSignedCertificate, certificate);
+        _sslErrors.append(error);
+    }
 }
 
 bool
 NetworkReply::canIgnoreSslErrors(const QList<QSslError>& errors) {
-    Q_D(NetworkReply);
-    return d->canIgnoreSslErrors(errors);
+    if (_sslErrors.count() > 0 && errors.count() > 0) {
+        foreach(QSslError error, errors) {
+            QSslError::SslError type = error.error();
+            if (type != QSslError::NoError &&
+                type != QSslError::SelfSignedCertificate) {
+                // we only support self signed certificates all errors
+                // will not be ignored
+                qDebug() << "SSL error type not ignored";
+                return false;
+            } else if (type == QSslError::SelfSignedCertificate) {
+                // just ignore those errors of the added errors
+                if (!_certs.contains(error.certificate())) {
+                    qDebug() << "SSL certificate not ignored";
+                    return false;
+                }
+            }
+        }
+
+        if (_reply != NULL) {
+            _reply->ignoreSslErrors(_sslErrors);
+        }
+
+        return true;
+    }
+    return false;
 }
 
 }  // System

=== modified file 'libubuntudownloadmanager/system/network_reply.h'
--- libubuntudownloadmanager/system/network_reply.h	2013-10-29 13:42:44 +0000
+++ libubuntudownloadmanager/system/network_reply.h	2013-12-02 11:34:50 +0000
@@ -30,13 +30,12 @@
 
 namespace System {
 
-class NetworkReplyPrivate;
 class NetworkReply : public QObject {
     Q_OBJECT
-    Q_DECLARE_PRIVATE(NetworkReply)
 
  public:
-    explicit NetworkReply(QNetworkReply* reply, QObject *parent = 0);
+    NetworkReply(QNetworkReply* reply, QObject *parent = 0);
+    virtual ~NetworkReply();
 
     virtual QByteArray readAll();
     virtual void abort();
@@ -52,8 +51,9 @@
     void sslErrors(const QList<QSslError>& errors);
 
  private:
-    // use pimpl so that we can mantains ABI compatibility
-    NetworkReplyPrivate* d_ptr;
+    QList<QSslCertificate> _certs;
+    QList<QSslError> _sslErrors;
+    QNetworkReply* _reply;
 };
 
 }  // System

=== removed directory 'ubuntu-download-manager-tests/system'
=== removed file 'ubuntu-download-manager-tests/system/test_network_reply.cpp'
--- ubuntu-download-manager-tests/system/test_network_reply.cpp	2013-10-30 11:04:37 +0000
+++ ubuntu-download-manager-tests/system/test_network_reply.cpp	1970-01-01 00:00:00 +0000
@@ -1,93 +0,0 @@
-/*
- * Copyright 2013 Canonical Ltd.
- *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of version 3 of the GNU Lesser General Public
- * License as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with this library; if not, write to the
- * Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
- * Boston, MA 02110-1301, USA.
- */
-
-#include <QSslError>
-#include <QSignalSpy>
-#include "fakes/fake_qnetwork_reply.h"
-#include "test_network_reply.h"
-
-TestNetworkReply::TestNetworkReply(QObject *parent)
-    : BaseTestCase("TestNetworkReply", parent) {
-}
-
-void
-TestNetworkReply::init() {
-    BaseTestCase::init();
-    _qReply = new FakeQNetworkReply();
-    _reply = new NetworkReply(_qReply);
-}
-
-void
-TestNetworkReply::cleanup() {
-    BaseTestCase::cleanup();
-
-    if (_reply != NULL)
-        _reply->deleteLater();
-
-    if (_qReply != NULL)
-        _qReply->deleteLater();
-}
-
-void
-TestNetworkReply::testDownloadProgressForwarded_data() {
-    QTest::addColumn<uint>("received");
-    QTest::addColumn<uint>("total");
-
-    QTest::newRow("First row") << 67u << 200u;
-    QTest::newRow("Second row") << 45u << 12000u;
-    QTest::newRow("Third row") << 2u << 2345u;
-    QTest::newRow("Last row") << 3434u << 2323u;
-}
-
-void
-TestNetworkReply::testDownloadProgressForwarded() {
-    QFETCH(uint, received);
-    QFETCH(uint, total);
-
-    QSignalSpy spy(_reply, SIGNAL(downloadProgress(qint64, qint64)));
-    emit _qReply->downloadProgress(received, total);
-
-    QCOMPARE(spy.count(), 1);
-    QList<QVariant> arguments = spy.takeFirst();
-    QCOMPARE(arguments.at(0).toUInt(), received);
-    QCOMPARE(arguments.at(1).toUInt(), total);
-}
-
-void
-TestNetworkReply::testErrorForwarded() {
-    QSignalSpy spy(_reply, SIGNAL(error(QNetworkReply::NetworkError)));
-    emit _qReply->error(QNetworkReply::NoError);
-
-    QCOMPARE(spy.count(), 1);
-}
-
-void
-TestNetworkReply::testFinishedForwarded() {
-    QSignalSpy spy(_reply, SIGNAL(finished()));
-    emit _qReply->finished();
-    QCOMPARE(spy.count(), 1);
-}
-
-void
-TestNetworkReply::testSslErrorsForwarded() {
-    QList<QSslError> errors;
-    QSignalSpy spy(_reply, SIGNAL(sslErrors(const QList<QSslError>&)));
-    emit _qReply->sslErrors(errors);
-
-    QCOMPARE(spy.count(), 1);
-}

=== removed file 'ubuntu-download-manager-tests/system/test_network_reply.h'
--- ubuntu-download-manager-tests/system/test_network_reply.h	2013-10-30 11:04:37 +0000
+++ ubuntu-download-manager-tests/system/test_network_reply.h	1970-01-01 00:00:00 +0000
@@ -1,54 +0,0 @@
-/*
- * Copyright 2013 Canonical Ltd.
- *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of version 3 of the GNU Lesser General Public
- * License as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with this library; if not, write to the
- * Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
- * Boston, MA 02110-1301, USA.
- */
-
-#ifndef TEST_NETWORK_REPLY_H
-#define TEST_NETWORK_REPLY_H
-
-#include <QObject>
-#include <system/network_reply.h>
-#include "base_testcase.h"
-#include "test_runner.h"
-
-using namespace Ubuntu::DownloadManager::System;
-
-class TestNetworkReply : public BaseTestCase {
-    Q_OBJECT
-
-  public:
-    explicit TestNetworkReply(QObject *parent = 0);
-
- private slots:  // NOLINT(whitespace/indent)
-
-    void init() override;
-    void cleanup() override;
-
-    // data functions used in the tests
-    void testDownloadProgressForwarded_data();
-
-    void testDownloadProgressForwarded();
-    void testErrorForwarded();
-    void testFinishedForwarded();
-    void testSslErrorsForwarded();
- private:
-    QNetworkReply* _qReply;
-    NetworkReply* _reply;
-};
-
-DECLARE_TEST(TestNetworkReply)
-
-#endif  // TEST_NETWORK_REPLY_H

=== modified file 'ubuntu-download-manager-tests/ubuntu-download-manager-tests.pro'
--- ubuntu-download-manager-tests/ubuntu-download-manager-tests.pro	2013-11-11 16:30:03 +0000
+++ ubuntu-download-manager-tests/ubuntu-download-manager-tests.pro	2013-12-02 11:34:50 +0000
@@ -38,7 +38,6 @@
     fakes/fake_download_factory.cpp \
     fakes/fake_file_manager.cpp \
     fakes/fake_apparmor.cpp \
-    system/test_network_reply.cpp \
     main.cpp \
     base_testcase.cpp \
     downloads/state_machines/test_network_error_transition.cpp \
@@ -76,7 +75,6 @@
     fakes/fake_download_factory.h \
     fakes/fake_file_manager.h \
     fakes/fake_apparmor.h \
-    system/test_network_reply.h \
     test_runner.h \
     base_testcase.h \
     downloads/state_machines/test_network_error_transition.h \