I've started looking into this one today

@blundell Could you provide an example of code that would be ported using this new API? I've started porting it but had trouble finding places where such migration would be done, other than AuthService and OAuth2TokenService, both inside 'google_apis':

  $ git grep -l StartRequestWithContext 
  google_apis/drive/auth_service.cc
  google_apis/gaia/oauth2_token_service.cc
  google_apis/gaia/oauth2_token_service.h

As a result, if I attempt to migrate the bits in auth_service.cc to test the changes done in AccessTokenFetcher, I hit a dependency cycle since AccessTokenFetcher, AccessTokenInfo and URLLoaderFactory are all provided by the Identity service's public API:

  $ gn check out/Default
  ERROR Dependency cycle:
    //google_apis:google_apis ->
    //services/identity/public/cpp:cpp ->
    //components/signin/core/browser:browser ->
    //google_apis:google_apis

I have the feeling I'm missing something then... perhaps there are some downstream ports requiring this migration?

Hi Mario,

The intent is indeed to migrate //google_apis/drive/auth_service.cc. Handling the dependency cycle that you describe is listed in its bug:  https://crbug.com/809440 . Does that make sense? Thanks!

Hi Colin,

Your explanation makes sense to me, thanks for clarifying and the pointer!

I still have yet more question, though: Why adding a variant to the PrimaryAccountAccessTokenFetcher constructor as well? 

As far as I can see, there's no place where that would be used at the moment (AuthService would use AccessTokenFetcher directly AFAICS, see [1]) and adding such a method would require adding a new overload for IdentityManager::CreateAccessTokenFetcherForAccount() (see [2]) that would utilize the new constructor added to AccessTokenFetcher (see [3]) before we can add the required overload the PrimaryAccountAccessTokenFetcher (see [4]).

Note: the commits pointed out below are WIP patches, not meant to be proposed yet (e.g. besides checking whether the approach is correct, they need tests and docs at the very least), but I shared them anyway since they help explain my thinking, I hope.

As for  crbug.com/809440 , I'm going to assign that to me now, since it's very much blocking me from proposing the CLs here.

[1] https://github.com/mariospr/chromium/commit/453e6110
[2] https://github.com/mariospr/chromium/commit/dfeb7646
[3] https://github.com/mariospr/chromium/commit/04bf2183
[4] https://github.com/mariospr/chromium/commit/cc299f42

Hi Mario,

Yes, you're right that we don't need the new constructor for PAATF at this time. Note that we should add the new overload for IdentityManager in any case though because AuthService should create the AccessTokenFetcher via IdentityManager. Make sense?

Hi Colin,

Yes, it makes perfect sense, thanks for clarifying once again!

Early CL in https://chromium-review.googlesource.com/c/chromium/src/+/1280553, depending on the CLs currently on review for 809440.

I have some other CLs on the work that will build on top of this one to move AuthService away from O2TS and truly rely on the IdentityManager, but I'll propose those as part of 809440 instead.

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

commit 050feea5abcc66a5a644ad7bf6a6458a52aac2a5
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Wed Oct 17 16:29:49 2018

New constructor for AccessTokenFetcher accepting a SharedURLLoaderFactory

This will be used to migrate AuthService to the Identity service, which
currently relies on OAuth2TokenService::StartRequestWithContext() for that.

Also, add a new unit tests to use the new constructor passing an unmodified
testing SharedURLLoaderFactory instead of relying on the production one.

Bug:  883655 
Change-Id: Ib891b33dbe3ab3aa83e4456d9cabed4f6b26c4f4
Reviewed-on: https://chromium-review.googlesource.com/c/1280553
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600416}
[modify] https://crrev.com/050feea5abcc66a5a644ad7bf6a6458a52aac2a5/components/signin/core/browser/fake_profile_oauth2_token_service.cc
[modify] https://crrev.com/050feea5abcc66a5a644ad7bf6a6458a52aac2a5/components/signin/core/browser/fake_profile_oauth2_token_service.h
[modify] https://crrev.com/050feea5abcc66a5a644ad7bf6a6458a52aac2a5/services/identity/public/cpp/DEPS
[modify] https://crrev.com/050feea5abcc66a5a644ad7bf6a6458a52aac2a5/services/identity/public/cpp/access_token_fetcher.cc
[modify] https://crrev.com/050feea5abcc66a5a644ad7bf6a6458a52aac2a5/services/identity/public/cpp/access_token_fetcher.h
[modify] https://crrev.com/050feea5abcc66a5a644ad7bf6a6458a52aac2a5/services/identity/public/cpp/access_token_fetcher_unittest.cc