Ensure SigninManager::Initialize() always receives a non-null PrefService* |
||||
Issue descriptionAs it was discussed in crbug.com/906085 (and in more detail from comment #4 [1]), the implementation of SigninManager::Initialize() explicitly states that the local_state passed may be null when calling it from unit tests. This is causing some trouble now that we're migrating SigningManagerFactory::IsAllowedUsername() since we need to access the browser process' local state to check the value of a preferences path (|kGoogleServicesUsernamePattern| in that case) to pass it to identity::IsUsernameAllowedByPattern(), but that might not be possible when such local state hasn't been set for those tests beforehand. As explained by Sylvain in [1], in the particular case of we should not check whether |kGoogleServicesUsernamePattern| is available or not before trying to access it but, instead, we should make sure that the preference has always being set, and the browser process' local state registered. So, this ticket is for that (of course, a blocker for crbug.com/906085 ). [1] https://bugs.chromium.org/p/chromium/issues/detail?id=906085#c4
,
Nov 26
As discussed in the mailing list with Sylvain based on the investigation I did with CL1349695 (which unveiled that 3000+ tests would need to be manually fixed using |ScopedTestingLocalState|), I'm unblocking 906085 from this bug now by doing this: * Provide a "transitional" identity::IsUsernameAllowedByPatternFromPrefs() method that checks whether a preference exists before trying to read it (similar to what SigninManager::IsAllowedUsername() already does) and use that one to finish the CL for bug 906085 . * Mark that transitional method deprecated with a comment pointing to this bug, to decouple that migration from the CL here, and keep working on it in parallel to other bugs. Also, JFTR, Sylvain confirmed that using |ScopedTestingLocalState| to fix those tests is the way to go.
,
Nov 26
This should probably be really low-priority as the pay-off is low (removing one check for preference registered) while the cost is really high (thousands of tests to fix).
,
Nov 26
Agreed. I'll stop working actively in CL 1349695 then for now, since I have other P1 tickets assigned to me that I need to take care of. |
||||
►
Sign in to add a comment |
||||
Comment 1 by ma...@igalia.com
, Nov 23