Get resolution on the API surface of AccountsMutator and fill it in |
||||||||||||||
Issue descriptionRemaining APIs to add (with unittests): // Updates the refresh token of the account with the given information, first // adding that account to the system if it is not known. accound_id AddOrUpdateAccount(gaia_id, email, child_status, refresh_token); // Updates the refresh token of |account_id|, which must be a known account. void UpdateRefreshToken(account_id, refresh_token); // Removes the account given by |account_id|. Also revokes the token server-side if needed. void RemoveAccount(account_id) // Removes all accounts. void RemoveAllAccounts(); } The implementations are sketched in https://docs.google.com/document/d/1kLjnxPnBAX0G6W-3u6OF_VguuJc0pXdgCRVR0z3yn-g/edit#heading=h.jukhj7v4no6w.
,
Dec 10
,
Dec 10
,
Dec 10
,
Dec 10
This is the last stepping stone to fix crbug.com/898810 , assigning to me.
,
Dec 13
,
Dec 13
,
Dec 17
Heads-up: There's some ongoing discussion still happening in https://docs.google.com/document/d/1kLjnxPnBAX0G6W-3u6OF_VguuJc0pXdgCRVR0z3yn-g/edit?pli=1# regarding to how to implement the AddOrUpdateAccount() and UpdateRefreshToken() methods, so I've split this effort into 2 CLs: * Implement methods to remove accounts in the AccountsMutator's API: https://crrev.com/c/1373828 * Implement remaining methods from the AccountsMutator's desired API: https://crrev.com/c/1379952 ...where the first one is almost in a reviewable state (need to figure out the failure on the Android bots) and the second one is pretty much me backing up what I had written before stopping to work on those other methods while the discussion is still ongoing.
,
Dec 18
,
Dec 20
I'm going to take responsibility for resolving the design questions that are blocking us moving forward here. Will update this bug once we have clarity there (including hopefully putting back the VendorBug label).
,
Dec 20
Thanks Colin. Only one quick note, though: I'll be away from Dec 24th to Jan 6th, so please feel free to resume my work in the CLs mentioned above if needed, otherwise I'll continue after that date if nobody beats me to it.
,
Dec 20
,
Jan 7
,
Jan 7
,
Jan 7
,
Jan 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c1485520dd0b14725862d7df7bc866e968610848 commit c1485520dd0b14725862d7df7bc866e968610848 Author: Mario Sanchez Prada <mario@igalia.com> Date: Wed Jan 09 18:41:57 2019 Implement methods to remove accounts in the AccountsMutator's API This patchset implements the RemoveAccount and RemoveAllAccounts methods as per the API described in the "Supporting account seeding and refresh token mutation in Identity Manager" document [1], along with new unit tests for both of them. A follow-up CL will implement the remaining methods to add or update accounts and refresh tokens, once the discussion in [1] is settled. [1] https://docs.google.com/document/d/1kLjnxPnBAX0G6W-3u6OF_VguuJc0pXdgCRVR0z3yn-g Bug: 887870 , 907901 Change-Id: I827cee4f339dd378b5aefe95b372ad2d115e71cc Reviewed-on: https://chromium-review.googlesource.com/c/1373828 Commit-Queue: Mario Sanchez Prada <mario@igalia.com> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Reviewed-by: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#621241} [modify] https://crrev.com/c1485520dd0b14725862d7df7bc866e968610848/services/identity/BUILD.gn [modify] https://crrev.com/c1485520dd0b14725862d7df7bc866e968610848/services/identity/public/cpp/DEPS [modify] https://crrev.com/c1485520dd0b14725862d7df7bc866e968610848/services/identity/public/cpp/accounts_mutator.h [modify] https://crrev.com/c1485520dd0b14725862d7df7bc866e968610848/services/identity/public/cpp/accounts_mutator_impl.cc [modify] https://crrev.com/c1485520dd0b14725862d7df7bc866e968610848/services/identity/public/cpp/accounts_mutator_impl.h [modify] https://crrev.com/c1485520dd0b14725862d7df7bc866e968610848/services/identity/public/cpp/accounts_mutator_impl_unittest.cc
,
Jan 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/abd11dcff4afcd5cd6fa0cb27ae5078c6b3975f1 commit abd11dcff4afcd5cd6fa0cb27ae5078c6b3975f1 Author: Mario Sanchez Prada <mario@igalia.com> Date: Thu Jan 10 14:04:46 2019 Avoid using pure virtual functions with default parameters in AccountsMutator Using default parameters in pure virtual functions is in general a bad idea, as it makes it possible that different defaults will end up being used in different places. Thus, and even though in this case it might not be as bad since the only subclass that will ever exist is a private implementation, let's remove those defaults and force clients to be explicit on what the source is. For cases where that's not relevant (i.e. tests) we can always pass the previous default value, |signin_metrics::SourceForRefreshTokenOperation::kUnknown|. Bug: 907901 Change-Id: I6d7770cfb46cd6ce067c88aaa013dffc9f8a0655 Reviewed-on: https://chromium-review.googlesource.com/c/1404074 Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Commit-Queue: Mario Sanchez Prada <mario@igalia.com> Cr-Commit-Position: refs/heads/master@{#621571} [modify] https://crrev.com/abd11dcff4afcd5cd6fa0cb27ae5078c6b3975f1/services/identity/public/cpp/accounts_mutator.h [modify] https://crrev.com/abd11dcff4afcd5cd6fa0cb27ae5078c6b3975f1/services/identity/public/cpp/accounts_mutator_impl.h [modify] https://crrev.com/abd11dcff4afcd5cd6fa0cb27ae5078c6b3975f1/services/identity/public/cpp/accounts_mutator_impl_unittest.cc
,
Jan 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b5721fc683c9684d589f482e39e0ba21703cc608 commit b5721fc683c9684d589f482e39e0ba21703cc608 Author: Mario Sanchez Prada <mario@igalia.com> Date: Mon Jan 14 11:52:43 2019 Implement remaining method from the AccountsMutator's API: AddOrUpdateAccount() This patchset implements the remaining method as per the API described in the "Supporting account seeding and refresh token mutation in Identity Manager" document [1], along with new unit tests to test different use cases. [1] https://docs.google.com/document/d/1kLjnxPnBAX0G6W-3u6OF_VguuJc0pXdgCRVR0z3yn-g Bug: 887870 , 907901 Change-Id: I906521369775c1844f4dfaaf8154d092c1f38307 Reviewed-on: https://chromium-review.googlesource.com/c/1379952 Commit-Queue: Mario Sanchez Prada <mario@igalia.com> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Reviewed-by: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#622427} [modify] https://crrev.com/b5721fc683c9684d589f482e39e0ba21703cc608/chrome/browser/signin/identity_manager_factory.cc [modify] https://crrev.com/b5721fc683c9684d589f482e39e0ba21703cc608/services/identity/public/cpp/accounts_mutator.h [modify] https://crrev.com/b5721fc683c9684d589f482e39e0ba21703cc608/services/identity/public/cpp/accounts_mutator_impl.cc [modify] https://crrev.com/b5721fc683c9684d589f482e39e0ba21703cc608/services/identity/public/cpp/accounts_mutator_impl.h [modify] https://crrev.com/b5721fc683c9684d589f482e39e0ba21703cc608/services/identity/public/cpp/accounts_mutator_impl_unittest.cc [modify] https://crrev.com/b5721fc683c9684d589f482e39e0ba21703cc608/services/identity/public/cpp/identity_test_environment.cc
,
Jan 14
Fixed! |
||||||||||||||
►
Sign in to add a comment |
||||||||||||||
Comment 1 by blundell@chromium.org
, Nov 22