DownloadHistory::Load use HistoryServiceFactory::GetForProfile not IfExists

DownloadHistory::Load use HistoryServiceFactory::GetForProfile not IfExists
BUG=141779
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152146

[benjhayden, 2012-08-16 20:22:47]

PTAL
This fixes the bug.

Randy says that this cannot be the right fix. DownloadServiceFactory DependsOn
HistoryServiceFactory, so the ProfileKeyedService infrastructure *should* ensure
that the HistoryService is created before the DownloadService. I dug into
DependsOn and could not see this mechanism that Randy describes, so I'm starting
a discussion here.

Achuith, do you know of anything in chromeos that might mess around with the
ProfileKeyedService infrastructure and/or DownloadServiceFactory's dependence on
HistoryServiceFactory?
I see that chromeos uses the "user" profile instead of the "Default" profile,
but Randy says that this should not be a problem. Again, I couldn't confirm
this.

[Randy Smith (Not in Mondays), 2012-08-16 20:27:57]

Miranda, can I call you back to code you probably believed you had left behind
forever?  :-}.  I had thought that if you indicated that a service depended on
another service in a ProfileKeyedServiceFactory via DependsOn(), you would
guarantee the initialization order of the two services.   But when I look at the
code, it looks like it only guarantees the destruction order.  Can you confirm
or deny my belief in the dependency chain?

[achuithb, 2012-08-16 21:27:59]

GDataSystemService, which owns everything else, DependsOn
DownloadsServiceFactory here:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/gd...

Satoru-san implemented this stuff so he may have some ideas?

[Randy Smith (Not in Mondays), 2012-08-17 16:16:13]

On 2012/08/16 21:27:59, achuith.bhandarkar wrote:
> GDataSystemService, which owns everything else, DependsOn
> DownloadsServiceFactory here:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/gd...
> 
> Satoru-san implemented this stuff so he may have some ideas?

In the absence of a response from someone knowledgeable about profile
initialization, I'm willing to go with my reading of the code, which is to say
that DependsOn is used for destruction order, and initialization order is
dependent on demand load.  Which means the basic approach of this CL is right. 
I would like to track down the intended meaning of DependsOn(), either through
Miranda or someone else working on profile initialization, but that can be done
in parallel.

[benjhayden, 2012-08-17 16:31:59]

On 2012/08/17 16:16:13, rdsmith wrote:
> On 2012/08/16 21:27:59, achuith.bhandarkar wrote:
> > GDataSystemService, which owns everything else, DependsOn
> > DownloadsServiceFactory here:
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/gd...
> > 
> > Satoru-san implemented this stuff so he may have some ideas?
> 
> In the absence of a response from someone knowledgeable about profile
> initialization, I'm willing to go with my reading of the code, which is to say
> that DependsOn is used for destruction order, and initialization order is
> dependent on demand load.  Which means the basic approach of this CL is right.

> I would like to track down the intended meaning of DependsOn(), either through
> Miranda or someone else working on profile initialization, but that can be
done
> in parallel.

Is that an LGTM?

[Randy Smith (Not in Mondays), 2012-08-17 16:35:27]

On 2012/08/17 16:31:59, benjhayden_chromium wrote:
> On 2012/08/17 16:16:13, rdsmith wrote:
> > On 2012/08/16 21:27:59, achuith.bhandarkar wrote:
> > > GDataSystemService, which owns everything else, DependsOn
> > > DownloadsServiceFactory here:
> > >
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/gd...
> > > 
> > > Satoru-san implemented this stuff so he may have some ideas?
> > 
> > In the absence of a response from someone knowledgeable about profile
> > initialization, I'm willing to go with my reading of the code, which is to
say
> > that DependsOn is used for destruction order, and initialization order is
> > dependent on demand load.  Which means the basic approach of this CL is
right.
> 
> > I would like to track down the intended meaning of DependsOn(), either
through
> > Miranda or someone else working on profile initialization, but that can be
> done
> > in parallel.
> 
> Is that an LGTM?

No, this is :-}.  (I hadn't read the patch itself.)  If you can think of a way
to DCHECK that you aren't calling the history service back into existence during
shutdown, I'd value that, but I couldn't think of a way to do it, so I ain't
blocking the CL on it.

LGTM.

[Randy Smith (Not in Mondays), 2012-08-17 17:02:28]

On 2012/08/17 16:35:27, rdsmith wrote:
> On 2012/08/17 16:31:59, benjhayden_chromium wrote:
> > On 2012/08/17 16:16:13, rdsmith wrote:
> > > On 2012/08/16 21:27:59, achuith.bhandarkar wrote:
> > > > GDataSystemService, which owns everything else, DependsOn
> > > > DownloadsServiceFactory here:
> > > >
> > >
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/gd...
> > > > 
> > > > Satoru-san implemented this stuff so he may have some ideas?
> > > 
> > > In the absence of a response from someone knowledgeable about profile
> > > initialization, I'm willing to go with my reading of the code, which is to
> say
> > > that DependsOn is used for destruction order, and initialization order is
> > > dependent on demand load.  Which means the basic approach of this CL is
> right.
> > 
> > > I would like to track down the intended meaning of DependsOn(), either
> through
> > > Miranda or someone else working on profile initialization, but that can be
> > done
> > > in parallel.
> > 
> > Is that an LGTM?
> 
> No, this is :-}.  (I hadn't read the patch itself.)  If you can think of a way
> to DCHECK that you aren't calling the history service back into existence
during
> shutdown, I'd value that, but I couldn't think of a way to do it, so I ain't
> blocking the CL on it.
> 
> LGTM.

Closing the loop--I just chatted with Miranda by IM, and she confirms: the only
guarantee DependsOn() provides is destruction order.  So this is the right fix.

[commit-bot: I haz the power, 2012-08-17 17:38:23]

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

[achuithb, 2012-08-17 19:06:22]

lgtm

[commit-bot: I haz the power, 2012-08-17 20:30:05]

Change committed as 152146