Avoid LazyInstance in DownloadPathReservationTracker.

Avoid LazyInstance in DownloadPathReservationTracker.

BUG=78085
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150410

[Randy Smith (Not in Mondays), 2012-08-03 19:17:14]

http://codereview.chromium.org/10824132/diff/4001/chrome/browser/download/dow...
File chrome/browser/download/download_path_reservation_tracker.cc (right):

http://codereview.chromium.org/10824132/diff/4001/chrome/browser/download/dow...
chrome/browser/download/download_path_reservation_tracker.cc:35: // that is
supposed to overwrite an existing file or reservation.
I don't understand this comment, and I think it doesn't match the code.  As I
understand it, a "std::map" is a 1->1 mapping; you need a multimap to have a
1->many mapping.  Did you mean multiple DownloadIds assigned to the same path?

http://codereview.chromium.org/10824132/diff/4001/chrome/browser/download/dow...
chrome/browser/download/download_path_reservation_tracker.cc:72: if
(iter->second ==  path)
nit: Extra space.

http://codereview.chromium.org/10824132/diff/4001/chrome/browser/download/dow...
chrome/browser/download/download_path_reservation_tracker.cc:92: // there is a
.crdownload left around.
Hmmm.  Say a bit more about this use case?  My immediate reaction was "but we
want to cleanup those .crdownload files; isn't overwriting a good way to do
that?" but that's probably wrong when we do download resumption.  What are your
thoughts here?

[asanka, 2012-08-03 19:50:47]

http://codereview.chromium.org/10824132/diff/4001/chrome/browser/download/dow...
File chrome/browser/download/download_path_reservation_tracker.cc (right):

http://codereview.chromium.org/10824132/diff/4001/chrome/browser/download/dow...
chrome/browser/download/download_path_reservation_tracker.cc:35: // that is
supposed to overwrite an existing file or reservation.
On 2012/08/03 19:17:14, rdsmith wrote:
> I don't understand this comment, and I think it doesn't match the code.  As I
> understand it, a "std::map" is a 1->1 mapping; you need a multimap to have a
> 1->many mapping.  Did you mean multiple DownloadIds assigned to the same path?

Written backwards, the comment I have. enoD.

http://codereview.chromium.org/10824132/diff/4001/chrome/browser/download/dow...
chrome/browser/download/download_path_reservation_tracker.cc:72: if
(iter->second ==  path)
On 2012/08/03 19:17:14, rdsmith wrote:
> nit: Extra space.

Done.

http://codereview.chromium.org/10824132/diff/4001/chrome/browser/download/dow...
chrome/browser/download/download_path_reservation_tracker.cc:92: // there is a
.crdownload left around.
On 2012/08/03 19:17:14, rdsmith wrote:
> Hmmm.  Say a bit more about this use case?  My immediate reaction was "but we
> want to cleanup those .crdownload files; isn't overwriting a good way to do
> that?" but that's probably wrong when we do download resumption.  What are
your
> thoughts here?

Interrupted downloads are what I was thinking about. We release the reservation
if the download is interrupted because the DRT no longer has a good signal for
whether the reservation should be considered valid. After that if the
.crdownload file is still around, we assume that the target name is "in use" and
skip over.

[Randy Smith (Not in Mondays), 2012-08-03 20:45:23]

LGTM with below requested change.

http://codereview.chromium.org/10824132/diff/4001/chrome/browser/download/dow...
File chrome/browser/download/download_path_reservation_tracker.cc (right):

http://codereview.chromium.org/10824132/diff/4001/chrome/browser/download/dow...
chrome/browser/download/download_path_reservation_tracker.cc:92: // there is a
.crdownload left around.
On 2012/08/03 19:50:48, asanka wrote:
> On 2012/08/03 19:17:14, rdsmith wrote:
> > Hmmm.  Say a bit more about this use case?  My immediate reaction was "but
we
> > want to cleanup those .crdownload files; isn't overwriting a good way to do
> > that?" but that's probably wrong when we do download resumption.  What are
> your
> > thoughts here?
> 
> Interrupted downloads are what I was thinking about. We release the
reservation
> if the download is interrupted because the DRT no longer has a good signal for
> whether the reservation should be considered valid. After that if the
> .crdownload file is still around, we assume that the target name is "in use"
and
> skip over.

[As discussed offline]  I've looked at the current code, and gone back to before
my refactoring, and I think we've always deleted the download file if it fails
in error.  I'm somewhat worried that that's not the behavior being seen by
users, but it is what the code looks like.  Given this, I think we should
consider .crdownload files that we don't know about turds, and ignore them for
reservation purposes (though we'll obviously have to change this behavior in
some fashion for download resumption).

[commit-bot: I haz the power, 2012-08-07 19:11:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/10824132/9005

[commit-bot: I haz the power, 2012-08-07 21:16:26]

Change committed as 150410