Added clearing of downloads_ set on RemoveDownloadsBetween pathway.

Added clearing of downloads_ set on RemoveDownloadsBetween pathway.

BUG=66676
TEST=66676 does not repro three times running.

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

[Randy Smith (Not in Mondays), 2010-12-15 21:11:22]

Could you folks take a look at this?  Given the priority of 66676, I'm going to
commit on the first LGTM, and incorporate comments from the other reviews in a
follow-on basis.  So if you review this, please act as if you're the only one
reviewing it--you might be :-}.

This is a classic PIBKAC, except in this case the P used the K to commit code
:-{.  I scanned all of download_manager.cc to make sure I hadn't missed any
other cases where stuff was removed from the history_downloads_ map where I also
needed to remove it from the new owning downloads_ set, but I invite you to do
your own scan for that same thing.

I've repro'd both crash and fix on linux (preferred dev env) and am relying on
trybots for testing other platforms; I'll hold commit on successful trybot runs.

[ahendrickson, 2010-12-15 21:26:05]

LGTM.

On 2010/12/15 21:11:22, rdsmith wrote:
> Could you folks take a look at this?  Given the priority of 66676, I'm going
to
> commit on the first LGTM, and incorporate comments from the other reviews in a
> follow-on basis.  So if you review this, please act as if you're the only one
> reviewing it--you might be :-}.
> 
> This is a classic PIBKAC, except in this case the P used the K to commit code
> :-{.  I scanned all of download_manager.cc to make sure I hadn't missed any
> other cases where stuff was removed from the history_downloads_ map where I
also
> needed to remove it from the new owning downloads_ set, but I invite you to do
> your own scan for that same thing.
> 
> I've repro'd both crash and fix on linux (preferred dev env) and am relying on
> trybots for testing other platforms; I'll hold commit on successful trybot
runs.

[cbentzel, 2010-12-15 21:34:19]

Thanks for speedy fix.

http://codereview.chromium.org/5877003/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/5877003/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_manager.cc:744: 
Would this be clearer if you did

if (pending_deletes.empty())
  return 0;

int num_deleted = static_cast<int>

[Randy Smith (Not in Mondays), 2010-12-15 23:30:26]

On 2010/12/15 21:34:19, cbentzel wrote:
> Thanks for speedy fix.
> 
>
http://codereview.chromium.org/5877003/diff/1/chrome/browser/download/downloa...
> File chrome/browser/download/download_manager.cc (right):
> 
>
http://codereview.chromium.org/5877003/diff/1/chrome/browser/download/downloa...
> chrome/browser/download/download_manager.cc:744: 
> Would this be clearer if you did
> 
> if (pending_deletes.empty())
>   return 0;
> 
> int num_deleted = static_cast<int>

Yes, but I think I'm going to give in to my paranoia and say that I have green
try bots with the current code, and commit as written.  But I'll almost
certainly take your suggestion in a later CL.

[Paweł Hajdan Jr., 2010-12-16 08:51:54]

LGTM