Code review comment for lp:~mandel/ubuntu-download-manager/network-errors-management

Revision history for this message
Mike McCracken (mikemc) wrote :

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

« Back to merge proposal