Fix history importing by delaying DownloadManager creation.

Fix history importing by delaying DownloadManager creation. Replace GetNextIdThunkType with DownloadIdFactory (RefCountedThreadSafe, created by DownloadService).

DownloadService uses the same DownloadIdFactory for an OTR profile as its original profile.

DownloadService passes the DownloadIdFactory into the DownloadManager so that the DownloadManager can allocate new valid ids for items loaded from the history or downloads started on the ui thread.

Since the DownloadService precedes and outlives its DownloadManager, DownloadManager does not have a scoped_refptr<DownloadIdFactory>. Objects that do have a scoped_refptr<DownloadIdFactory>: DownloadService, ProfileIOData, ShellBrowserContext, ShellResourceContext.

The DownloadIdFactory must be RefCountedThreadSafe because ProfileIOData outlives Profile and because it's used in both the OTR and original profiles.

Longer term, the import process should strictly precede profile initialization, and the next_download_id counter should be loaded from the History db strictly before DownloadService is created and creates a DownloadIdFactory.

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

[benjhayden, 2011-10-27 15:45:02]

Ready for review while I work on making all the tests delete the DIF on the IO
thread.

[Miranda Callahan, 2011-10-27 16:55:04]

On 2011/10/27 15:45:02, b s h wrote:
> Ready for review while I work on making all the tests delete the DIF on the IO
> thread.

The bulk of this review should be done by Randy, who understands the changes
deeply; as for the profile parts, LGTM -- and I patched this into my Windows
build and it fixed the import problem perfectly. Very delighted!

[Miranda Callahan, 2011-10-27 17:00:19]

(some alphabetization nits I noticed while reading through the change.)

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
content/browser/download/download_item.h:29: #include
"content/browser/download/download_id.h"
nit: alphabetization

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
File content/browser/download/download_manager.cc (right):

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
content/browser/download/download_manager.cc:25: #include
"content/browser/download/download_id_factory.h"
nit: alpha

http://codereview.chromium.org/8401001/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/8401001/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/buffered_resource_handler.cc:15: #include
"content/browser/download/download_id_factory.h"
nitalpha

http://codereview.chromium.org/8401001/diff/8001/content/shell/shell_resource...
File content/shell/shell_resource_context.cc (right):

http://codereview.chromium.org/8401001/diff/8001/content/shell/shell_resource...
content/shell/shell_resource_context.cc:9: #include
"content/browser/download/download_id_factory.h"
nit alpha

[Randy Smith (Not in Mondays), 2011-10-27 18:01:44]

I'd appreciate it if you could change the description so that the first line
(preferably the first 80 characters) sums up the change; that makes it easy for
people reading the commit log.  I'd also like the title of the description to
sum up the change--I usually make the two the same, but an argument could be
made that the title of the CL should be more about the purpose and the first
line of the description more about what you did.

http://codereview.chromium.org/8401001/diff/8001/chrome/browser/download/down...
File chrome/browser/download/download_service.cc (right):

http://codereview.chromium.org/8401001/diff/8001/chrome/browser/download/down...
chrome/browser/download/download_service.cc:40: GetDownloadIdFactory(),
nit, suggestion: When they're equally brief with their accessor, I tend to use
member variables inside of classes, just for consistency.  But it's up to you.

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
File content/browser/download/download_create_info.cc (right):

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
content/browser/download/download_create_info.cc:59: download_id.local(),
I'd be inclined to think that printing the domain as well makes sense--if you
want to know the id in a DownloadCreateInfo, you'll want to know the domain.

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
File content/browser/download/download_id_factory.cc (right):

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
content/browser/download/download_id_factory.cc:15: base::AutoLock lock(lock_);
Is there a reason to use AutoLock rather than atomic_sequence_num.h?  I don't
care (I don't think this is performance critical); just making you aware of the
option.

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
content/browser/download/download_id_factory.cc:21: next_id_ += next_id;
The += is strange enough that it deserves a comment (I say this especially
because I don't remember exactly what it's supposed to be gaining us, even after
I went back to the comment in download_manager.cc being deleted :-}).

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
File content/browser/download/download_id_factory.h (right):

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
content/browser/download/download_id_factory.h:23: void SetNextId(int next_id);
Remind me what the constraints are on our use of this function?  It doesn't look
like we're currently using it, and in the future I think we'll be constructing
the DownloadIdFactory with information from the history database, so it'll
belong as part of the constructor.  I have a memory that there are medium-term
constraints on the relationship between db_handle and id, but I don't remember
what they are.

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
content/browser/download/download_item.h:119: const DownloadId& download_id);
Long term, when we construct from the persistent store, we're going to set the
local part of the id from that and the global part from the DownloadManager. 
Short term, we're just going to get the next download id from the
DownloadIdFactory (right?).  So can we just ask the download manager to give us
the next download id now, rather than adding something ot the construction
interface?  We'd just leave the GetNextId() function on the download manager and
have it delegate to the factory.

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
content/browser/download/download_item.h:283: const DownloadId& global_id()
const { return download_id_; }
Hmmm.  I'm actually a litte uncomfortable about this, just because if you get
your lifetimes wrong you'll be holding onto a reference that goes nowhere, and
it's not obvious at the call site that that's what's happening.  This is almost
certainly ok in the large majority of call sites, but it's also not hard to copy
a DownloadId, so I'd leave it as a straight return type.  Just a suggestion.

http://codereview.chromium.org/8401001/diff/13001/chrome/browser/download/dow...
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/8401001/diff/13001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:579: int32 id = 0;
Hmmm.  For consistency, I'd either use "id" as in the previous code, or replace
id with zero throughout this test (it doesn't look to be used).  If you use id,
could you just slap a "const" on the decl so folks know it's being used that
way?

http://codereview.chromium.org/8401001/diff/13001/content/browser/download/sa...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/8401001/diff/13001/content/browser/download/sa...
content/browser/download/save_package.cc:274: id_factory()->GetNextId());
Where is id_factory() defined?  I can't manage to find it.

[benjhayden, 2011-10-27 19:04:40]

http://codereview.chromium.org/8401001/diff/8001/chrome/browser/download/down...
File chrome/browser/download/download_service.cc (right):

http://codereview.chromium.org/8401001/diff/8001/chrome/browser/download/down...
chrome/browser/download/download_service.cc:40: GetDownloadIdFactory(),
On 2011/10/27 18:01:44, rdsmith wrote:
> nit, suggestion: When they're equally brief with their accessor, I tend to use
> member variables inside of classes, just for consistency.  But it's up to you.

Done.

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
File content/browser/download/download_create_info.cc (right):

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
content/browser/download/download_create_info.cc:59: download_id.local(),
On 2011/10/27 18:01:44, rdsmith wrote:
> I'd be inclined to think that printing the domain as well makes sense--if you
> want to know the id in a DownloadCreateInfo, you'll want to know the domain.

Done.

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
File content/browser/download/download_id_factory.cc (right):

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
content/browser/download/download_id_factory.cc:15: base::AutoLock lock(lock_);
On 2011/10/27 18:01:44, rdsmith wrote:
> Is there a reason to use AutoLock rather than atomic_sequence_num.h?  I don't
> care (I don't think this is performance critical); just making you aware of
the
> option.

We're planning on initializing next_id_ to something non-zero in the future, and
I don't see how to do that with AtomicSequenceNumber. We could store an offset_
alongside the AtomicSequenceNumber and always add in the offset_, but that
doesn't feel right. LMK if you would still prefer ASN+offset over lock+counter.

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
content/browser/download/download_id_factory.cc:21: next_id_ += next_id;
On 2011/10/27 18:01:44, rdsmith wrote:
> The += is strange enough that it deserves a comment (I say this especially
> because I don't remember exactly what it's supposed to be gaining us, even
after
> I went back to the comment in download_manager.cc being deleted :-}).

We were thinking at one point that DM could allocate some ids, then increment
the id counter by the number read from the history, then resume allocating ids,
but this method is not necessary if we aren't reading the number from the
history, and won't be necessary if we can eventually read the number from the
history before creating the DIF.

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
File content/browser/download/download_id_factory.h (right):

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
content/browser/download/download_id_factory.h:23: void SetNextId(int next_id);
On 2011/10/27 18:01:44, rdsmith wrote:
> Remind me what the constraints are on our use of this function?  It doesn't
look
> like we're currently using it, and in the future I think we'll be constructing
> the DownloadIdFactory with information from the history database, so it'll
> belong as part of the constructor.  I have a memory that there are medium-term
> constraints on the relationship between db_handle and id, but I don't remember
> what they are.  

We'd been thinking at one point that, until the id is persisted, we can allocate
some ids then increment the factory counter by the number read from the history,
then resume allocating ids, but I don't think that's necessary if we aren't
reading the number from the history. Is that what you're thinking of?
I'll remove the method.

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
content/browser/download/download_item.h:29: #include
"content/browser/download/download_id.h"
On 2011/10/27 17:00:19, Miranda Callahan wrote:
> nit: alphabetization

Done.

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
content/browser/download/download_item.h:119: const DownloadId& download_id);
On 2011/10/27 18:01:44, rdsmith wrote:
> Long term, when we construct from the persistent store, we're going to set the
> local part of the id from that and the global part from the DownloadManager. 
> Short term, we're just going to get the next download id from the
> DownloadIdFactory (right?).  So can we just ask the download manager to give
us
> the next download id now, rather than adding something ot the construction
> interface?  We'd just leave the GetNextId() function on the download manager
and
> have it delegate to the factory.

Done.

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
content/browser/download/download_item.h:283: const DownloadId& global_id()
const { return download_id_; }
On 2011/10/27 18:01:44, rdsmith wrote:
> Hmmm.  I'm actually a litte uncomfortable about this, just because if you get
> your lifetimes wrong you'll be holding onto a reference that goes nowhere, and
> it's not obvious at the call site that that's what's happening.  This is
almost
> certainly ok in the large majority of call sites, but it's also not hard to
copy
> a DownloadId, so I'd leave it as a straight return type.  Just a suggestion.

Done.

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
File content/browser/download/download_manager.cc (right):

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
content/browser/download/download_manager.cc:25: #include
"content/browser/download/download_id_factory.h"
On 2011/10/27 17:00:19, Miranda Callahan wrote:
> nit: alpha

Done.

http://codereview.chromium.org/8401001/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/8401001/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/buffered_resource_handler.cc:15: #include
"content/browser/download/download_id_factory.h"
On 2011/10/27 17:00:19, Miranda Callahan wrote:
> nitalpha

Done.

http://codereview.chromium.org/8401001/diff/8001/content/shell/shell_resource...
File content/shell/shell_resource_context.cc (right):

http://codereview.chromium.org/8401001/diff/8001/content/shell/shell_resource...
content/shell/shell_resource_context.cc:9: #include
"content/browser/download/download_id_factory.h"
On 2011/10/27 17:00:19, Miranda Callahan wrote:
> nit alpha

Done.

http://codereview.chromium.org/8401001/diff/13001/chrome/browser/download/dow...
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/8401001/diff/13001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:579: int32 id = 0;
On 2011/10/27 18:01:44, rdsmith wrote:
> Hmmm.  For consistency, I'd either use "id" as in the previous code, or
replace
> id with zero throughout this test (it doesn't look to be used).  If you use
id,
> could you just slap a "const" on the decl so folks know it's being used that
> way?

Done.

http://codereview.chromium.org/8401001/diff/13001/content/browser/download/sa...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/8401001/diff/13001/content/browser/download/sa...
content/browser/download/save_package.cc:274: id_factory()->GetNextId());
On 2011/10/27 18:01:44, rdsmith wrote:
> Where is id_factory() defined?  I can't manage to find it.

In download_manager.h... This line is a continuation of the previous. I
re-indented it so PTAL.

[benjhayden, 2011-10-27 21:02:39]

Sorry, I forgot to change the issue title.

[Randy Smith (Not in Mondays), 2011-10-28 13:35:43]

LGTM; none of the below comments are gating.  Thanks for throwing this together
so quickly!

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
File content/browser/download/download_id_factory.h (right):

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
content/browser/download/download_id_factory.h:23: void SetNextId(int next_id);
On 2011/10/27 19:04:41, b s h wrote:
> On 2011/10/27 18:01:44, rdsmith wrote:
> > Remind me what the constraints are on our use of this function?  It doesn't
> look
> > like we're currently using it, and in the future I think we'll be
constructing
> > the DownloadIdFactory with information from the history database, so it'll
> > belong as part of the constructor.  I have a memory that there are
medium-term
> > constraints on the relationship between db_handle and id, but I don't
remember
> > what they are.  
> 
> We'd been thinking at one point that, until the id is persisted, we can
allocate
> some ids then increment the factory counter by the number read from the
history,
> then resume allocating ids, but I don't think that's necessary if we aren't
> reading the number from the history. Is that what you're thinking of?
> I'll remove the method.

That's what I was thinking of--I wish I remembered what the motivation for the
+= was (what it would give us).  Ah, well, if it was important it'll call back
:-}.

http://codereview.chromium.org/8401001/diff/13001/content/browser/download/sa...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/8401001/diff/13001/content/browser/download/sa...
content/browser/download/save_package.cc:274: id_factory()->GetNextId());
On 2011/10/27 19:04:41, b s h wrote:
> On 2011/10/27 18:01:44, rdsmith wrote:
> > Where is id_factory() defined?  I can't manage to find it.
> 
> In download_manager.h... This line is a continuation of the previous. I
> re-indented it so PTAL.

Whoops--sorry.  Thanks.

http://codereview.chromium.org/8401001/diff/18001/content/browser/download/do...
File content/browser/download/download_id_factory.cc (right):

http://codereview.chromium.org/8401001/diff/18001/content/browser/download/do...
content/browser/download/download_id_factory.cc:16: return DownloadId(domain_,
next_id_++);
Nit's nit: For complex data types (i.e. most iterators) defaulting to
pre-increment is better because it doesn't involve a copy.  So I think it's
useful to be in the habit of using pre-increment when it doesn't matter.  This
is just FYI; you're welcome to leave this code however you'd like.

http://codereview.chromium.org/8401001/diff/18001/content/browser/download/sa...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/8401001/diff/18001/content/browser/download/sa...
content/browser/download/save_package.cc:273:
browser_context->GetDownloadManager()->GetNextId());
nit: Why the shift to browser_context?

[benjhayden, 2011-10-28 14:37:54]

Waiting for try-bots to catch up.

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
File content/browser/download/download_id_factory.h (right):

http://codereview.chromium.org/8401001/diff/8001/content/browser/download/dow...
content/browser/download/download_id_factory.h:23: void SetNextId(int next_id);
On 2011/10/28 13:35:43, rdsmith wrote:
> On 2011/10/27 19:04:41, b s h wrote:
> > On 2011/10/27 18:01:44, rdsmith wrote:
> > > Remind me what the constraints are on our use of this function?  It
doesn't
> > look
> > > like we're currently using it, and in the future I think we'll be
> constructing
> > > the DownloadIdFactory with information from the history database, so it'll
> > > belong as part of the constructor.  I have a memory that there are
> medium-term
> > > constraints on the relationship between db_handle and id, but I don't
> remember
> > > what they are.  
> > 
> > We'd been thinking at one point that, until the id is persisted, we can
> allocate
> > some ids then increment the factory counter by the number read from the
> history,
> > then resume allocating ids, but I don't think that's necessary if we aren't
> > reading the number from the history. Is that what you're thinking of?
> > I'll remove the method.
> 
> That's what I was thinking of--I wish I remembered what the motivation for the
> += was (what it would give us).  Ah, well, if it was important it'll call back
> :-}.

We want to set next_id_ = next_id_from_history, but we were worried that an id
could be allocated before the history callback was called, which might cause us
to allocate the same id twice. I think that we could have also said next_id_ =
max(next_id_, next_id_from_history), but I think that we're safe now either way.

http://codereview.chromium.org/8401001/diff/13001/content/browser/download/sa...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/8401001/diff/13001/content/browser/download/sa...
content/browser/download/save_package.cc:274: id_factory()->GetNextId());
On 2011/10/28 13:35:43, rdsmith wrote:
> On 2011/10/27 19:04:41, b s h wrote:
> > On 2011/10/27 18:01:44, rdsmith wrote:
> > > Where is id_factory() defined?  I can't manage to find it.
> > 
> > In download_manager.h... This line is a continuation of the previous. I
> > re-indented it so PTAL.
> 
> Whoops--sorry.  Thanks.

Done.

http://codereview.chromium.org/8401001/diff/18001/content/browser/download/do...
File content/browser/download/download_id_factory.cc (right):

http://codereview.chromium.org/8401001/diff/18001/content/browser/download/do...
content/browser/download/download_id_factory.cc:16: return DownloadId(domain_,
next_id_++);
On 2011/10/28 13:35:44, rdsmith wrote:
> Nit's nit: For complex data types (i.e. most iterators) defaulting to
> pre-increment is better because it doesn't involve a copy.  So I think it's
> useful to be in the habit of using pre-increment when it doesn't matter.  This
> is just FYI; you're welcome to leave this code however you'd like.

Yeah, but I'd rather initialize the counter to the first value that will be
returned for clarity. If this changes to an ASN then I'll change it to
pre-increment and document the -1.

http://codereview.chromium.org/8401001/diff/18001/content/browser/download/sa...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/8401001/diff/18001/content/browser/download/sa...
content/browser/download/save_package.cc:273:
browser_context->GetDownloadManager()->GetNextId());
On 2011/10/28 13:35:44, rdsmith wrote:
> nit: Why the shift to browser_context?

Done.

[benjhayden, 2011-10-28 19:16:56]

PS5 is at revision 107766.

[commit-bot: I haz the power, 2011-10-28 21:55:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/8401001/18108

[commit-bot: I haz the power, 2011-10-28 21:56:05]

Presubmit check for 8401001-18108 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/resource_context.cc,content/browser/resource_context.h,content/browser/renderer_host/resource_dispatcher_host_unittest.cc,content/browser/renderer_host/buffered_resource_handler.cc,content/browser/renderer_host/resource_dispatcher_host.cc

Presubmit checks took 4.3s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[jam, 2011-10-28 22:24:55]

lgtm

http://codereview.chromium.org/8401001/diff/18108/content/browser/download/do...
File content/browser/download/download_id_factory.h (right):

http://codereview.chromium.org/8401001/diff/18108/content/browser/download/do...
content/browser/download/download_id_factory.h:5: #ifndef
CHROME_BROWSER_DOWNLOAD_DOWNLOAD_ID_FACTORY_H_
nit: CONTENT

[commit-bot: I haz the power, 2011-10-28 22:37:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/8401001/8039

[benjhayden, 2011-10-28 22:39:08]

Thank you!

http://codereview.chromium.org/8401001/diff/18108/content/browser/download/do...
File content/browser/download/download_id_factory.h (right):

http://codereview.chromium.org/8401001/diff/18108/content/browser/download/do...
content/browser/download/download_id_factory.h:5: #ifndef
CHROME_BROWSER_DOWNLOAD_DOWNLOAD_ID_FACTORY_H_
On 2011/10/28 22:24:56, John Abd-El-Malek wrote:
> nit: CONTENT

Done.

[benjhayden, 2011-10-29 00:21:24]

Looks like the tree is closed? And the CQ skipped mac_rel? But the CQ hasn't
updated this issue yet?
I'll check back and try the CQ again before I go to sleep if the tree opens back
up.

[commit-bot: I haz the power, 2011-10-29 00:46:55]

Change committed as 107836