[core] Incremental offline downloads #6446
Conversation
|
@jfirebaugh, thanks for your PR! By analyzing this pull request, we identified @mourner, @1ec5 and @tmpsantos to be potential reviewers. |
jfirebaugh
force-pushed
the
incremental-offline-download
branch
from
67894fc to
81bb29f
Compare
|
I was thinking of picking up this |
| if (status.complete()) { | ||
| setState(OfflineRegionDownloadState::Inactive); | ||
| } | ||
| continueDownload(); |
There was a problem hiding this comment.
Shouldn't we call continueDownload() also on error?
There was a problem hiding this comment.
A request that errors will be retried after some delay. So in that sense it's still "active" and consuming resources, notably the request object, its timer, and network resources when the timer fires.
We could try to squeeze in subsequent requests while we wait for the errored request to retry. But that risks overloading the upstream request queue -- defeating our own metering here -- if there are a lot of errored requests that all come up for retry at the same time.
Many times, the cause of a request error will apply to many requests of the same type. For instance if a server is unreachable, all the requests to that host are going to error. In that case, continuing to try subsequent resources after the first few errors is fruitless anyway.
I'll add the above rationale as a comment.
| result.requiredResourceCount += spriteResources(parser).size(); | ||
| result.requiredResourceCount += glyphResources(parser).size(); | ||
| if (!parser.glyphURL.empty()) { | ||
| result.requiredResourceCount += parser.fontStacks().size() * 256; |
There was a problem hiding this comment.
Factor 256 (used multiple times here) into a global constant?
Don't allow OfflineDownload to flood the request queue. Fixes #4414.