Taking this...

Hey Sergio, thanks! Make sure to set bugs to "Started" if you've started working on them :).

Sure. Now that I got your attention :) ... I am not sure what to do with some APIs we don't have an equivalent version in IdentityManager for:

* OAuth2TokenService::DiagnosticsObserver
 - only OnAccessTokenRequested is supported in IdentityManager

* OAuth2TokenServiceDelegate::Observer

* SigninErrorController::Observer

* SigninManager::SigninDiagnosticsObserver

Are there plans to add them? Are there any other way to retrieve the very same information?

Hi Sergio,

Thanks for the investigation here! Let's let this bug be for now and come back when we're further along. Let's stick to the P1 Proj-Servicification-VendorBug bugs as those ones should be fully analyzed (i.e., if there are no blocking bugs then they should be ready to be worked on). Let me know if at any point that list is running short and I'll prioritize repopulating it!

I had already started to migrate some other APIs. Would it make sense to upload CLs for those changes even though the other APIs are not designed yet?

Hi Sergio,

Sorry, I'm not sure that I understand your question. Are you saying that you had already started to migrate parts of about_signin_internals.cc that can be migrated now?

Correct. It's just one observer but maybe it pays off to land it as it's already done.

Can you upload it so I can take a look? Thanks!

Uploaded it to https://crrev.com/c/1276606

The bots don't like it because I'd need to add the identity service dependency but then the problem is that a dependency cycle appears:
ERROR Dependency cycle:
  //components/signin/core/browser:browser ->
  //services/identity/public/cpp:cpp ->
  //components/signin/core/browser:browser

Hi Sergio,

This is great, thank you! Two responses to your CL:

(1) Having looked at it, I agree that it's nice to land this even with the complete conversion being blocked on further APIs. Moving from observing SigninManager to observing IdentityManager is a nice change just on its own.

(2) You've exposed the fact that //components/signin/core/browser needs to be split into two targets: the "implementation of IdentityManager" target, which //services/identity/public/cpp will depend on, and the "consumer of IdentityManager" target, which will depend on //services/identity/public/cpp. This fact is obvious in retrospect but I hadn't thought about it. Apart from anything else, the fact that your CL exposed that issue now is hugely valuable!

I'll file a bug for (2) and block this bug on it. I'll look at (2) but I won't put up something immediately; in the meantime we should continue working on conversions of classes outside //components/signin.

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

commit cb9b70203c1811fcf4a90e33760e56dcb8ec723b
Author: Sergio Villar Senin <svillar@igalia.com>
Date: Tue Nov 20 09:22:55 2018

AboutSigninInternals: remove the SigninManagerBase::Observer dependency

It can observe IdentityManager::Observer instead. This is a step
forward in the migration to IdentityManager API.

Most usages of SigninManager were also replaced by their equivalent
IdentityManager API. It's also observing
IdentityManager::DiagnosticsObserver from now on.

BUG: 809539

Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: I9e7319a522f298298ab6a1a512bad73675050e70
Reviewed-on: https://chromium-review.googlesource.com/c/1276606
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Sergio Villar <svillar@igalia.com>
Cr-Commit-Position: refs/heads/master@{#609640}
[modify] https://crrev.com/cb9b70203c1811fcf4a90e33760e56dcb8ec723b/chrome/browser/signin/about_signin_internals_factory.cc
[modify] https://crrev.com/cb9b70203c1811fcf4a90e33760e56dcb8ec723b/chrome/browser/signin/dice_response_handler_unittest.cc
[modify] https://crrev.com/cb9b70203c1811fcf4a90e33760e56dcb8ec723b/components/signin/core/browser/about_signin_internals.cc
[modify] https://crrev.com/cb9b70203c1811fcf4a90e33760e56dcb8ec723b/components/signin/core/browser/about_signin_internals.h
[modify] https://crrev.com/cb9b70203c1811fcf4a90e33760e56dcb8ec723b/ios/chrome/browser/signin/about_signin_internals_factory.cc

I'll leave this open as there are still some pending APIs to be migrated

The dependencies on SigninManager are gone thanks to Sergio, but there are still dependencies on PO2TS. I need to analyze these dependencies, as they look pretty hairy. Once I've done that this can likely become a VendorBug again :).

Marking this as Available to reflect the fact that I'm not currently doing this analysis.