Issue 809967  has been merged into this issue.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0610146f3b67f0e78c0415ecf098f3a92ac34ac1

commit 0610146f3b67f0e78c0415ecf098f3a92ac34ac1
Author: Colin Blundell <blundell@chromium.org>
Date: Wed May 23 10:23:23 2018

Port ExtensionDownloader away from IdentityProvider

IdentityProvider is deprecated as a general-purpose means of interacting
with the user's Google accounts. This CL ports ExtensionDownloader
away from it, having it instead take in the OAuth2TokenService directly
as well as a callback that returns the account to use with the webstore.
It also clarifies the lifetime relationship between ExtensionDownloader
and the ProfileOAuth2TokenService/SigninManager instances on which it
depends for authentication.

In the long term ExtensionDownloader will be ported to interact with the
Identity Service for its use case. This CL is an incremental step on the
path that also fills the more near-term goal of eliminating usages of
IdentityProvider.

Bug:  809966 , 809927
Change-Id: I6b82cc318bd807d559e1701391ed3e02fcb489aa
Reviewed-on: https://chromium-review.googlesource.com/1051810
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561024}
[modify] https://crrev.com/0610146f3b67f0e78c0415ecf098f3a92ac34ac1/chrome/browser/extensions/extension_system_factory.cc
[modify] https://crrev.com/0610146f3b67f0e78c0415ecf098f3a92ac34ac1/chrome/browser/extensions/updater/chrome_extension_downloader_factory.cc
[modify] https://crrev.com/0610146f3b67f0e78c0415ecf098f3a92ac34ac1/chrome/browser/extensions/updater/extension_updater_unittest.cc
[modify] https://crrev.com/0610146f3b67f0e78c0415ecf098f3a92ac34ac1/extensions/browser/updater/extension_downloader.cc
[modify] https://crrev.com/0610146f3b67f0e78c0415ecf098f3a92ac34ac1/extensions/browser/updater/extension_downloader.h

The remaining work here is to have ExtensionDownloader take in IdentityManager rather than taking in a callback to get the primary account ID and a TokenService pointer. AFAIK, there is no dependency issue with having //extensions/browser/updater depend directly on //services/identity/public/cpp. I did a quick check that adding the latter as a dependency of the former does not cause any GN problems.
Assuming that that is true, this conversion should be straightforward.

Devlin, can you confirm whether adding //services/identity/public/cpp as a dependency of //extensions/browser/updater is a fine thing to do? Please feel free to mark Available once you've answered :). Thanks!

@blundell: I'm not terribly opposed to adding services/identity as a DEP, but I'm a bit more concerned about the complexity in extension downloader itself.  The class is fairly monstrous, and testing it is very difficult.  Would it be possible to have the identity code abstracted out and use dependency injection instead, providing a callback (or value, if we can synchronously)?  That would make it a lot cleaner to test, and would also avoid the need to add the new dependency.

How much do we need from the IdentityManager?

Hi Devlin,

Thanks for the reply! Making this change without referencing IdentityManager in ExtensionDownloader would be complex, as we need to replace ExtensionDownloader's usage of |token_service_|, from which it fetches and invalidates access tokens as well as checking whether the pointer is null to short-circuit out of certain operations. The most complexity would be in the fact that the fetching of access tokens is done via creating an object that the caller then owns until the access token fetch is complete or no longer relevant (e.g., the caller dies). We could of course indirect all of this interaction, but my opinion is that it would make the code pretty opaque.

That said, the change to have ExtensionDownloader interact with IdentityManager directly shouldn't make ExtensionDownloader *harder* to test. We'd be stripping out the dependency on OAuth2TokenService and the passed-in callback to get the webstore account, and there are extensive facilities for creating and interacting with IdentityManager in test contexts via identity_test_environment.h.

Hi Devlin,

Friendly ping here :). Thanks!

Okay, I think I'm fine with adding the dep.  In the course of the CL review, I'll see if anything jumps out at me as an easy opportunity for abstraction. :)

Thanks, Devlin! So the work here is that outlined in c#3 above.

CL https://chromium-review.googlesource.com/c/chromium/src/+/1378988.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/99be931540a37767321f11a9dead5a1dfb35a493

commit 99be931540a37767321f11a9dead5a1dfb35a493
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Wed Dec 19 14:55:33 2018

[s13n] Convert ExtensionDownloader and ChromeExtensionDownloaderFactory to use IdentityManager

PO2TS is going to be an implementation detail of the IdentityManager, and should not be exposed to
clients out of //services/identity.

In practice, the CL changes ExtensionDownloader::SetWebstoreAuthenticationCapabilities to take in
an IdentityManager instance rather than taking in a callback to get the primary account ID and
a TokenService pointer.

Additionally, it also replaces the inheritance to OAuth2TokenService::Consumer by the use of
identity::AccessTokenFetcher.

BUG= 809966 

Change-Id: I93ee4464e4ff9afd1ffab15d983e475667d54e9d
Reviewed-on: https://chromium-review.googlesource.com/c/1378988
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Cr-Commit-Position: refs/heads/master@{#617829}
[modify] https://crrev.com/99be931540a37767321f11a9dead5a1dfb35a493/chrome/browser/extensions/extension_system_factory.cc
[modify] https://crrev.com/99be931540a37767321f11a9dead5a1dfb35a493/chrome/browser/extensions/updater/chrome_extension_downloader_factory.cc
[modify] https://crrev.com/99be931540a37767321f11a9dead5a1dfb35a493/chrome/browser/extensions/updater/extension_updater_unittest.cc
[modify] https://crrev.com/99be931540a37767321f11a9dead5a1dfb35a493/extensions/browser/DEPS
[modify] https://crrev.com/99be931540a37767321f11a9dead5a1dfb35a493/extensions/browser/updater/extension_downloader.cc
[modify] https://crrev.com/99be931540a37767321f11a9dead5a1dfb35a493/extensions/browser/updater/extension_downloader.h