[Marble-devel] Review Request: Download policies part 1, basic infrastructure

[Torsten Rahn]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1745/#review2512
-----------------------------------------------------------

Ship it!


Wow, this looks nice to me. Much nicer to read. 
Only nitpicks: License headers seem to be missing at least for the DownloadPolicy classes
And in the nice HttpJob lifecycle documentation I guess it should be m_activeJobs (instead of m_activatedJobs).

- Torsten


On 2009-09-30 16:21:56, jmho wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1745/
> -----------------------------------------------------------
> 
> (Updated 2009-09-30 16:21:56)
> 
> 
> Review request for marble.
> 
> 
> Summary
> -------
> 
> This patch adds basic infrastructure for host and usage specific download policies.
> It does not add features, only adds infrastructure and changes HttpDownloadManager
> to use the new infrastructure.
> 
> Motivation:
> ===========
> OpenStreetMap allows for bulk downloads (planned "Download region" feature)
> only two concurrent connections. For browsing it is not a problem to have 10+
> connections (like web browsers do).
> 
> So we need a way to distinguish between bulk downloading and browsing the map
> per host/set of hosts.
> 
> What this patch contains:
> =========================
> - basic classes (DownloadQueueSet, DownloadPolicy)
> - changes to HttpDownloadManager to use DownloadQueueSet internally
> - one default download policy, which allows 20 concurrent connections
> - change acceptJob to canAcceptJob and only create HttpJob if the job can be
>   accepted
> 
> What has to be done:
> ====================
> - agree on additional DGML elements/attributes for defining policies
> - implement those DGML extensions
> - make HttpJobs aware of their usage
> ...
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdeedu/marble/src/lib/CMakeLists.txt 1029721 
>   trunk/KDE/kdeedu/marble/src/lib/DownloadPolicy.h PRE-CREATION 
>   trunk/KDE/kdeedu/marble/src/lib/DownloadPolicy.cpp PRE-CREATION 
>   trunk/KDE/kdeedu/marble/src/lib/DownloadQueueSet.h PRE-CREATION 
>   trunk/KDE/kdeedu/marble/src/lib/DownloadQueueSet.cpp PRE-CREATION 
>   trunk/KDE/kdeedu/marble/src/lib/HttpDownloadManager.h 1029721 
>   trunk/KDE/kdeedu/marble/src/lib/HttpDownloadManager.cpp 1029721 
> 
> Diff: http://reviewboard.kde.org/r/1745/diff
> 
> 
> Testing
> -------
> 
> I'm using this patch for some time now and it works for me.
> 
> 
> Thanks,
> 
> jmho
> 
>