Since IsUsernameAllowedByPolicy is only used internally, there is no need to expose it via IdentityManager.

@sdefresne since this is the other API that is currently blocking some of the tickets I've got assigned (i.e. 890794), I feel like I need to ask here as well: any update on this one?

If not, is there any way I could help with this one?

Thanks in advance

Sylvain: What direction do you think that we should take here? Is the situation similar enough to IsSigninAllowed() that we can take the same approach? If so, I think that those changes should be something that the Igalia folks could take on with guidance, right?

Assigning to you to outline the direction :).

I think the plan should be the following:

1. convert SigninManager::IsUsernameAllowedByPolicy from a static method to a free function identity::IsUsernameAllowedByPattern

2. move the free function from //components/signin/core/browser:internals to //components/signin/core/browser:shared

3. code in the browser can then use this free function after fetching the pattern from prefs, by converting the following

  SigningManagerFactory::GetForProfile(profile)
    ->IsAllowedUsername(username);

to

  identity::IsUsernameAllowedByPattern(
      username,
      profile->GetPrefs()->GetString(prefs::kGoogleServicesUsernamePattern));

4. for code in components/signin, the pattern should be set and the preference not accessed.

Each step should probably be converted to individual bugs blocking the current bug (and bug N blocks N+1 too).

I'll create the individual bugs later.

I need this functionality to unblock work on  crbug.com/890794  (and thus on  crbug.com/898810 ), so self-assigning

@sdefresne: I have a few questions about the outlined plan, that are kind of causing some confusion for me when it comes to working on this ticket:

> I think the plan should be the following:
> 
> 1. convert SigninManager::IsUsernameAllowedByPolicy from a static method to a free function identity::IsUsernameAllowedByPattern

Where would you put declare/implement such free function?

My first thought would be in to do it in |components/signin/core/browser/signin_manager.[h|cc]|, but it seems a bit strange to me to declare something belonging to the |identity| namespace from there; I would assume somewhere under //services/identity would be a better fit? (e.g. identity_manager.[h|cc]).

> 2. move the free function from //components/signin/core/browser:internals to //components/signin/core/browser:shared

Related to my previous question, if the new free function IsUsernameAllowedByPattern() will be part of the |identity| namespace... wouldn't it need to be added to a target outside //components/signin/core? Maybe to //services/identity/public/cpp (if added to identity_manager.[h|cc])?

> > 1. convert SigninManager::IsUsernameAllowedByPolicy from a static method to a free function identity::IsUsernameAllowedByPattern
> Where would you put declare/implement such free function?
> 
> My first thought would be in to do it in |components/signin/core/browser/signin_manager.[h|cc]|, but it seems a bit strange to me to declare something belonging to the |identity| namespace from there; I would assume somewhere under //services/identity would be a better fit? (e.g. identity_manager.[h|cc]).

I would initially put it in signin_manager.cc to reduce the size of the CL (which could potentially be largish depending on how many calls to this function are made). Regarding using identity namespace in that file, I need to skip to your second question to answer it.

> > 2. move the free function from //components/signin/core/browser:internals to //components/signin/core/browser:shared
> 
> Related to my previous question, if the new free function IsUsernameAllowedByPattern() will be part of the |identity| namespace... wouldn't it need to be added to a target outside //components/signin/core? Maybe to //services/identity/public/cpp (if added to identity_manager.[h|cc])?

We cannot move the method to //services/identity as it is called from SigninManager and this would create a dependency cycle. This is why we need to keep the code in //components/signin (at least until we are done with the conversion).

Regarding using the namespace outside of //services/identity, IMO, namespaces are a logical way to organize code. There is no rule in C++ that say namespace foo needs to be a in directory named foo. So I am personally not bothered by (hopefully temporarily) having things defined in identity namespace in //component/signin.

blundell: what do you think? Should we avoid using "identity" namespace in component/signin? If so, any recommendation on naming this method (maybe IsUsernameAllowedForPrimaryAccountByPattern, but then it is such a mouth-full and I do not really like it).

What is the timeframe for being able to eliminate the method's usage by SigninManager and move it to //services/identity/public/cpp?

Probably the same time frame as removing the SigninManager class completely.

OK I read through the bug in detail and I'm up to speed now. IIUC, once SigninManager isn't used by any consumers outside of //services/identity, we can then move it into //services/identity to be part of IdentityManager (and eventually Identity Service) internals, and at that time we could move this free function to //services/identity/public/cpp. That time actually isn't that far away based on our current progress on removing SigninManager usage. So I am fine using the identity namespace in //components/signin. If we get pushback from //components/signin OWNERS, we can equally well use the signin namespace with a TODO and bug to convert it to the identity namespace at the time that we move it to //services/identity/public/cpp.

Mario, does that coupled with Sylvain's comments in c#16 unblock you on this bug? Thanks!

@blundell @sdefresne Thanks for the clarifications. Yes, I think this information unblocks me for now. The mismatch between using the identity namespace in //components/signin was very confusing to me because, even if nothing stops me from doing that, seemed counter-intuitive considering other changes we've been doing so far.

But your explanations made sense to me, especially considering this as a temporary move before removing the SigninManager class completely, so I think I have everything I need to keep moving. Thanks!

Old the blocking bugs have been fixed now => fixed