Major refactoring of DownloadThread.run().
authorSteve Howard <showard@google.com>
Fri, 16 Jul 2010 21:28:35 +0000 (14:28 -0700)
committerSteve Howard <showard@google.com>
Tue, 20 Jul 2010 22:47:43 +0000 (15:47 -0700)
commitb5629da794cb3c1ca1970d206343743b165b9644
tree53703ac7a0ef36f9c3eaa2fc44bb6a43754a3b7f
parent5180de23e77139dd6971dfd48269242e3e3562d9
Major refactoring of DownloadThread.run().

Motivation: I need to fix the handling of 302s, so that after a
disconnect, subsequent retries will use the original URI, not the
redirected one.  Rather than store extra information in the DB, I'd
like to just keep the redirected URI in memory and make the redirected
request within the same DownloadThread.  This involves working with
the large-scale structure of DownloadThread.run().  Since run() was a
~700 line method, I didn't feel comfortable making such changes.

So this change refactors run() into a ~80 line method which calls into
a collection of ~20 other short methods.  The state previously kept in
local variables has been pulled into a couple of state-only inner
classes.  The error-handling control flow, formerly handled by "break
http_request_loop" statements, is now handled by throwing a
"StopRequest" exception.  The remaining structure of run() has been
simplified -- the outermost for loop, for example, could never
actually repeat and has been removed for now.  Some other bits of code
have been cleaned up a bit, but the functionality has not been
modified.

There are many good next steps to this refactoring.  Besides various
other cleanup bits, a major improvement would be to consolidate the
State/InnerState classes, move some functionality to this new class
(there are many functions of the form "void foo(State)" which would be
good candidates), and promote it to a top-level class.  But I want to
take things one step at a time, and I think what I've got here is a
major improvement and should be enough to allow me to safely implement
the changes to redirection handling.

In the process of doing this refactoring I added many test cases to
PublicApiFunctionalTest to exercise some of the pieces of code I was
moving around.  I also moved some test cases from
DownloadManagerFunctionalTest.  Over time I'd like to move everything
over to use the PublicApiFunctionalTest approach, and then I may break
that into some smaller suites.

Other minor changes:
* use longs instead of ints to track file sizes, as these may be
  getting quite large in the future
* provide a default DB value of -1 for COLUMN_TOTAL_BYTES, as this
  simplifies some logic in DownloadThread
* small extensions to MockResponse to faciliate new test cases

Change-Id: If7862349296ad79ff6cdc97e554ad14c01ce1f49
src/com/android/providers/downloads/DownloadInfo.java
src/com/android/providers/downloads/DownloadProvider.java
src/com/android/providers/downloads/DownloadThread.java
src/com/android/providers/downloads/Helpers.java
tests/src/com/android/providers/downloads/AbstractDownloadManagerFunctionalTest.java
tests/src/com/android/providers/downloads/DownloadManagerFunctionalTest.java
tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java
tests/src/tests/http/MockResponse.java