ExtensionUpdater: Abstract ExtensionDownloader creation

ExtensionUpdater: Abstract ExtensionDownloader creation

ExtensionUpdater likes to create and destroy ExtensionDownloader
instances as needed. This is fine, but it means duplicating efforts
any time new dependencies need to be injected into ExtensionDownloader,
because then they also need to be injected into ExtensionUpdater.

This CL adds a factory callback to ExtensionUpdater's constructor
so that ExtensionUpdater creators can opaquely inform the instance
about how it should construct new ExtensionDownloaders.

This also reverts the change to ExtensionDownloader's constructor
which took an (optionally NULL) IdentityProvider; this can now
instead be attached to new ExtensionDownloader instances by the
factory responsible for their construction via the
SetWebstoreIdentityProvider method. ExtensionService
provides an example of this.

BUG=398671
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288964

[Ken Rockot(use gerrit already), 2014-08-09 00:17:26]

Yoyo, could you please take a look?

[Yoyo Zhou, 2014-08-11 23:19:28]

LGTM with nits

https://chromiumcodereview.appspot.com/456063002/diff/20001/chrome/browser/ex...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/456063002/diff/20001/chrome/browser/ex...
chrome/browser/extensions/extension_service.cc:334: base::Unretained(this))));
Why not use weak pointers?

https://chromiumcodereview.appspot.com/456063002/diff/20001/chrome/browser/ex...
File chrome/browser/extensions/updater/extension_downloader.cc (right):

https://chromiumcodereview.appspot.com/456063002/diff/20001/chrome/browser/ex...
chrome/browser/extensions/updater/extension_downloader.cc:282:
identity_provider_ = identity_provider.Pass();
Use reset() or swap() here.

https://chromiumcodereview.appspot.com/456063002/diff/20001/chrome/browser/ex...
chrome/browser/extensions/updater/extension_downloader.cc:752:
DCHECK(identity_provider_ != NULL);
nit: I think this is usually written DCHECK(identity_provider.get())

https://chromiumcodereview.appspot.com/456063002/diff/20001/chrome/browser/ex...
File chrome/browser/extensions/updater/extension_updater.cc (right):

https://chromiumcodereview.appspot.com/456063002/diff/20001/chrome/browser/ex...
chrome/browser/extensions/updater/extension_updater.cc:171: }
nit: newline after

https://chromiumcodereview.appspot.com/456063002/diff/20001/chrome/browser/ex...
File chrome/browser/extensions/updater/extension_updater_unittest.cc (right):

https://chromiumcodereview.appspot.com/456063002/diff/20001/chrome/browser/ex...
chrome/browser/extensions/updater/extension_updater_unittest.cc:374:
scoped_ptr<ExtensionDownloader> downloader(new ExtensionDownloader(
Use CreateExtensionDownloader here.

[Ken Rockot(use gerrit already), 2014-08-11 23:48:25]

Thanks

https://codereview.chromium.org/456063002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/456063002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:334: base::Unretained(this))));
On 2014/08/11 23:19:28, Yoyo Zhou wrote:
> Why not use weak pointers?

Not allowed to use weak pointers when binding to methods that have a non-void
return value. We could work around this by instead having a void method populate
a scoped_ptr output argument, but that seemed ugly too. Given that we know the
ExtensionDownloader's lifetime can't extend beyond that of ExtensionService
anyway, I opted for Unretained.

https://codereview.chromium.org/456063002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/updater/extension_downloader.cc (right):

https://codereview.chromium.org/456063002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/updater/extension_downloader.cc:282:
identity_provider_ = identity_provider.Pass();
On 2014/08/11 23:19:28, Yoyo Zhou wrote:
> Use reset() or swap() here.

Done.

https://codereview.chromium.org/456063002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/updater/extension_downloader.cc:752:
DCHECK(identity_provider_ != NULL);
On 2014/08/11 23:19:28, Yoyo Zhou wrote:
> nit: I think this is usually written DCHECK(identity_provider.get())

Done.

https://codereview.chromium.org/456063002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/updater/extension_updater.cc (right):

https://codereview.chromium.org/456063002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/updater/extension_updater.cc:171: }
On 2014/08/11 23:19:28, Yoyo Zhou wrote:
> nit: newline after

Woops. Done.

https://codereview.chromium.org/456063002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/updater/extension_updater_unittest.cc (right):

https://codereview.chromium.org/456063002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/updater/extension_updater_unittest.cc:374:
scoped_ptr<ExtensionDownloader> downloader(new ExtensionDownloader(
On 2014/08/11 23:19:28, Yoyo Zhou wrote:
> Use CreateExtensionDownloader here.

Good catch, thanks. Done.

[Ken Rockot(use gerrit already), 2014-08-12 00:59:47]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-08-12 01:04:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rockot@chromium.org/456063002/40001

[commit-bot: I haz the power, 2014-08-12 14:48:47]

Change committed as 288964