Merge lp:~mandel/ubuntu-download-manager/network-errors-management into lp:ubuntu-download-manager

[Manuel de la Peña]

Ensure that when there is a network error the state of the GroupDownload is correctly set.

Ensure that when there is a network error the state of the GroupDownload is correctly set.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:131
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/59/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-saucy-amd64-ci/59
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-saucy-armhf-ci/50

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

review: Approve (continuous-integration)

[lp:~mandel/ubuntu-download-manager/network-errors-management]

132. By Manuel de la Peña on 2013-09-20

Do no use the DownloadQueue object for GroupDownloads so that segfault is fixes and downloads are done in parallel.

[Diego Sarmentero (diegosarmentero)]

looks good

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:132
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/60/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-saucy-amd64-ci/60
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-saucy-armhf-ci/51

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

review: Approve (continuous-integration)

[Manuel de la Peña (mandel)]

Please allow me to do the global approve, when this branch lands on trunk is very important due to deployment details.

[Mike McCracken (mikemc)]

This MP and its description is confusing me:

I see the change from 'emit q->error(errorMsg)' to 'q->emitError(errorMsg)', which will correctly set the state to ERROR in onError, but there are a bunch of other changes that you haven't described and I can't tell from the linked bugs what's going on:

1. What was causing the segfault bug, and which changes in here fixed it?
1a. Is it possible to add a test that triggers that bug? The tests added seem to just be about the ERROR state.

2. Is the change in cancelDownload() and pauseDownload() from a reverse loop to a regular foreach significant?

On an API style note - It took a while to understand why you had pairs of methods like cancel() and cancelDownload() - I guess it was because the DownloadQueue listens for the state change caused by cancel(), and then calls cancelDownload() itself. That seems like cancel() should be called something like setState(CANCELLED) or something like that, because 'download->cancelDownload()' is awkward - that should be 'download->cancel()', because it's actually doing the cancelling.

It was also strange to me that the object can be in the "cancelled" (or "paused" or whatever) state, while not actually having been cancelled yet. So for each of these states, there's two states - "about to be <state>" and "really <state>". This seems like it might be a source of problems, and was definitely a source of confusion while reading. Anyway, probably not worth messing with here, but I thought I'd mention it.

review: Needs Information

[Mike McCracken (mikemc)]

Also, my usual grumpy complaint that even though it's short, this still seems like at least two independent branches worth of changes. The error state fix is trivially reviewable, but the other part is delaying it.

[Manuel de la Peña (mandel)]

> This MP and its description is confusing me:
>
> I see the change from 'emit q->error(errorMsg)' to 'q->emitError(errorMsg)',
> which will correctly set the state to ERROR in onError, but there are a bunch
> of other changes that you haven't described and I can't tell from the linked
> bugs what's going on:
>
> 1. What was causing the segfault bug, and which changes in here fixed it?
> 1a. Is it possible to add a test that triggers that bug? The tests added seem
> to just be about the ERROR state.
>

The segafault was caused because the DownloadQueue calls deleteLater when a Download is completed or canceled, once that is done the pointer in the _downloads array in the GroupDownload is pointer to unknown memory space and we go banannas due to that. I really cannot think of a good way to test that error specially now that the q is not longer present.

> 2. Is the change in cancelDownload() and pauseDownload() from a reverse loop
> to a regular foreach significant?
>

At the very beginning (with the use of the DownloadQueue) it was a good idea to do it in reverse order because it minimized the number of operations to be performed in the q, mainlye because the downloads being paused/canceled are not the 'current download'. With the q implementation if you paused from first to last the download q will attempt to recalculate the next download to be performed something that really is not required in a group. Once the Download Queue was out there was no need to be careful with the order.

>
> On an API style note - It took a while to understand why you had pairs of
> methods like cancel() and cancelDownload() - I guess it was because the
> DownloadQueue listens for the state change caused by cancel(), and then calls
> cancelDownload() itself. That seems like cancel() should be called something
> like setState(CANCELLED) or something like that, because
> 'download->cancelDownload()' is awkward - that should be 'download->cancel()',
> because it's actually doing the cancelling.

Yes, I agree that it is a funny way to do it, the main issue is that the cancel, start, etc.. methods are exposed by the DownloadAdapetor that does something of the following:

QMetaObject::invokeMethod(parent(), "cancel");

As you can see, if you use the adaptors generated by the qt tool you need to call the methods the same way as those found in the interface... I might move away from the xml-generated classes but I need to consider if that is a source of problems later in the future if we change the xml.

>
> It was also strange to me that the object can be in the "cancelled" (or
> "paused" or whatever) state, while not actually having been cancelled yet. So
> for each of these states, there's two states - "about to be <state>" and
> "really <state>". This seems like it might be a source of problems, and was
> definitely a source of confusion while reading. Anyway, probably not worth
> messing with here, but I thought I'd mention it.

That is not perse true atm, the Download state is correct, if you set it to be canceled is not about to be, is canceled, the main reason is that the DownloadQueue atm ensures that there is a singled download at a time (being a SingleDo...

[Manuel de la Peña (mandel)]

> Also, my usual grumpy complaint that even though it's short, this still seems
> like at least two independent branches worth of changes. The error state fix
> is trivially reviewable, but the other part is delaying it.

Sorry I pushed in the wrong branch and though it would not be a huge problem.

[lp:~mandel/ubuntu-download-manager/network-errors-management]

133. By Manuel de la Peña on 2013-09-23

Merged smart-ignore-sslerrors into network-errors-management.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:133
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/64/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-saucy-amd64-ci/64/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-saucy-armhf-ci/55/console

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

review: Needs Fixing (continuous-integration)

[lp:~mandel/ubuntu-download-manager/network-errors-management]

134. By Manuel de la Peña on 2013-09-23

Merged smart-ignore-sslerrors into network-errors-management.

135. By Manuel de la Peña on 2013-09-23

Merged smart-ignore-sslerrors into network-errors-management.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:135
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/66/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-saucy-amd64-ci/66
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-saucy-armhf-ci/57

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

review: Approve (continuous-integration)

[Mike McCracken (mikemc)]

review: Approve

=== modified file 'libubuntudownloadmanager/download_queue.cpp'
--- libubuntudownloadmanager/download_queue.cpp	2013-09-09 16:05:49 +0000
+++ libubuntudownloadmanager/download_queue.cpp	2013-09-23 11:39:12 +0000
@@ -145,7 +145,7 @@
             Download::State state = currentDownload->state();
             if (state == Download::CANCEL || state == Download::FINISH
                 || state == Download::ERROR) {
-                qDebug() << "States is CANCEL || FINISH";
+                qDebug() << "State is CANCEL || FINISH || ERROR";
                 remove(_current);
                 _current = "";
             } else if (!currentDownload->canDownload()

=== modified file 'libubuntudownloadmanager/group_download.cpp'
--- libubuntudownloadmanager/group_download.cpp	2013-09-18 11:25:47 +0000
+++ libubuntudownloadmanager/group_download.cpp	2013-09-23 11:39:12 +0000
@@ -18,7 +18,6 @@
 
 #include <QDebug>
 #include "./download_adaptor.h"
-#include "./download_queue.h"
 #include "./single_download.h"
 #include "./uuid_factory.h"
 #include "./group_download.h"
@@ -68,7 +67,6 @@
                   QCryptographicHash::Algorithm algo,
                   bool isGSMDownloadAllowed) {
         Q_Q(GroupDownload);
-        _q = QSharedPointer<DownloadQueue>(new DownloadQueue(_networkInfo));
         QVariantMap metadata = q->metadata();
         QMap<QString, QString> headers = q->headers();
 
@@ -110,23 +108,19 @@
             q->connect(singleDownload, SIGNAL(finished(const QString&)),
                 q, SLOT(onFinished(const QString&)));
             qDebug() << "Signals connected.";
-            _q->add(singleDownload);
         }
     }
 
     void cancelDownload() {
         qDebug() << __PRETTY_FUNCTION__;
         Q_Q(GroupDownload);
-        // loop over the events in reverse order and if the downloads and
-        // if the are no cancelled do it
-        for (int index = _downloads.count() - 1; index >= 0; index--) {
-            qDebug() << "Index is" << index;
-            SingleDownload* download = _downloads[index];
+        foreach(SingleDownload* download, _downloads) {
             Download::State state = download->state();
             if (state != Download::FINISH && state != Download::ERROR
                     && state != Download::CANCEL) {
                 qDebug() << "Canceling download of " << download->url();
                 download->cancel();
+                download->cancelDownload();
             }
         }
 
@@ -142,14 +136,12 @@
 
     void pauseDownload() {
         Q_Q(GroupDownload);
-        // loop over the events in reverse order and if the downloads and
-        // if the are no paused or canceled do it
-        for (int index = _downloads.count() - 1; index >= 0; index--) {
-            SingleDownload* download = _downloads[index];
+        foreach(SingleDownload* download, _downloads) {
             Download::State state = download->state();
             if (state == Download::START || state == Download::RESUME) {
                 qDebug() << "Pausing download of " << download->url();
                 download->pause();
+                download->pauseDownload();
             }
         }
         emit q->paused(true);
@@ -161,6 +153,7 @@
             Download::State state = download->state();
             if (state == Download::PAUSE) {
                 download->resume();
+                download->resumeDownload();
             }
         }
         emit q->resumed(true);
@@ -172,6 +165,7 @@
             Download::State state = download->state();
             if (state == Download::IDLE) {
                 download->start();
+                download->startDownload();
             }
         }
         emit q->started(true);
@@ -223,7 +217,7 @@
             _fileManager->remove(file);
         }
         QString errorMsg = sender->url().toString() + ":" + error;
-        emit q->error(errorMsg);
+        q->emitError(errorMsg);
     }
 
     void onProgress(qulonglong received, qulonglong total) {
@@ -288,7 +282,6 @@
     QStringList _finishedDownloads;
     QMap<QUrl, QPair<qulonglong, qulonglong> > _downloadsProgress;
     QSharedPointer<SystemNetworkInfo> _networkInfo;
-    QSharedPointer<DownloadQueue> _q;
     QSharedPointer<DownloadFactory> _downFactory;
     QSharedPointer<FileManager> _fileManager;
     GroupDownload* q_ptr;

=== modified file 'ubuntu-download-manager-tests/test_group_download.cpp'
--- ubuntu-download-manager-tests/test_group_download.cpp	2013-09-16 15:57:06 +0000
+++ ubuntu-download-manager-tests/test_group_download.cpp	2013-09-23 11:39:12 +0000
@@ -656,6 +656,7 @@
     QList<MethodData> calledMethods = _fileManager->calledMethods();
     QCOMPARE(0, calledMethods.count());
     QCOMPARE(spy.count(), 1);
+    QCOMPARE(Download::ERROR, group->state());
 }
 
 void
@@ -688,6 +689,7 @@
     QList<MethodData> calledMethods = _fileManager->calledMethods();
     QCOMPARE(2, calledMethods.count());
     QCOMPARE(spy.count(), 1);
+    QCOMPARE(Download::ERROR, group->state());
 }
 
 void