Delay DownloadManager creation by adding a callback queue to DownloadService

Delay DownloadManager creation by adding a callback queue to DownloadService.
ExtensionDownloadsEventRouter's ctor pushes a Callback to this queue instead of calling GetDownloadManager(), which would create the manager. EDER doesn't really need the manager in its ctor, it just needs the manager before the manager does something interesting.

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

[benjhayden, 2011-12-05 19:02:17]

PTAL

[Randy Smith (Not in Mondays), 2011-12-05 22:29:29]

What happens if someone calls a downloads extension function before the download
manager is initialized?

[benjhayden, 2011-12-05 22:41:38]

On 2011/12/05 22:29:29, rdsmith wrote:
> What happens if someone calls a downloads extension function before the
download
> manager is initialized?

ExtensionDownloadsEventRouter only implements the events and has nothing to do
with the functions. Functions call GetDownloadManager(), which creates a manager
if necessary. If an extension listens for an event before the manager is
created, then the ExtensionService system will remember that listener for
whenever a manager is created and sends an event through EDER.

[Randy Smith (Not in Mondays), 2011-12-06 17:12:41]

LGTM *but*: If you're willing, I'd like you to check with the extensions folks
to see if there's any way to get notification if anyone ever registers for a
download event.  I suspect there isn't, but I do think it'd be cleaner to have
things go that way (get the DownloadManager in response to an event listener
notification) for delayed initialization.

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

http://codereview.chromium.org/8804018/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_extension_api.cc:483:
DownloadServiceFactory::GetForProfile(profile)->OnManagerCreated(base::Bind(
nit: Add a comment explaining what's going on here, and how we believe that
there's no way for any methods on this class to be called before we get the
callback.

[benjhayden, 2011-12-06 18:12:07]

On 2011/12/06 17:12:41, rdsmith wrote:
> LGTM *but*: If you're willing, I'd like you to check with the extensions folks
> to see if there's any way to get notification if anyone ever registers for a
> download event.  I suspect there isn't, but I do think it'd be cleaner to have
> things go that way (get the DownloadManager in response to an event listener
> notification) for delayed initialization.

Can you explain that a little more? I don't understand why we want to create the
manager just because an extension starts up and registers for events. It seems
like we want to wait to create the manager until when an extension calls a
function (e.g. query() when the user opens the popup/panel) or when a user
downloads a file.

Oh, or are you suggesting waiting to even call
DownloadService->OnManagerCreated() until an extension registers an event
handler? That makes more sense, but I don't understand what it would buy us.



> 
>
http://codereview.chromium.org/8804018/diff/1/chrome/browser/download/downloa...
> File chrome/browser/download/download_extension_api.cc (right):
> 
>
http://codereview.chromium.org/8804018/diff/1/chrome/browser/download/downloa...
> chrome/browser/download/download_extension_api.cc:483:
> DownloadServiceFactory::GetForProfile(profile)->OnManagerCreated(base::Bind(
> nit: Add a comment explaining what's going on here, and how we believe that
> there's no way for any methods on this class to be called before we get the
> callback.

[Randy Smith (Not in Mondays), 2011-12-06 18:16:42]

On 2011/12/06 18:12:07, b s h wrote:
> On 2011/12/06 17:12:41, rdsmith wrote:
> > LGTM *but*: If you're willing, I'd like you to check with the extensions
folks
> > to see if there's any way to get notification if anyone ever registers for a
> > download event.  I suspect there isn't, but I do think it'd be cleaner to
have
> > things go that way (get the DownloadManager in response to an event listener
> > notification) for delayed initialization.
> 
> Can you explain that a little more? I don't understand why we want to create
the
> manager just because an extension starts up and registers for events. It seems
> like we want to wait to create the manager until when an extension calls a
> function (e.g. query() when the user opens the popup/panel) or when a user
> downloads a file.

Hmmm.  I think the answer is "because I didn't think it through" :-}.  I hadn't
fully played out the use case in my mind, and had the sense that registering for
an event was a fairly good indicator that the event was going to happen.  But
you're right, if you install a downloads related extension, my suggestion would
just put us back where we were--initializing the DM on startup.

NM.  LGTM.


> 
> Oh, or are you suggesting waiting to even call
> DownloadService->OnManagerCreated() until an extension registers an event
> handler? That makes more sense, but I don't understand what it would buy us.
> 
> 
> 
> > 
> >
>
http://codereview.chromium.org/8804018/diff/1/chrome/browser/download/downloa...
> > File chrome/browser/download/download_extension_api.cc (right):
> > 
> >
>
http://codereview.chromium.org/8804018/diff/1/chrome/browser/download/downloa...
> > chrome/browser/download/download_extension_api.cc:483:
> > DownloadServiceFactory::GetForProfile(profile)->OnManagerCreated(base::Bind(
> > nit: Add a comment explaining what's going on here, and how we believe that
> > there's no way for any methods on this class to be called before we get the
> > callback.

[benjhayden, 2011-12-06 18:45:05]

Submitting.

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

http://codereview.chromium.org/8804018/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_extension_api.cc:483:
DownloadServiceFactory::GetForProfile(profile)->OnManagerCreated(base::Bind(
On 2011/12/06 17:12:41, rdsmith wrote:
> nit: Add a comment explaining what's going on here, and how we believe that
> there's no way for any methods on this class to be called before we get the
> callback.

Done.

[commit-bot: I haz the power, 2011-12-09 22:17:18]

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

[commit-bot: I haz the power, 2011-12-10 00:17:19]

Change committed as 113892