New issue
Advanced search Search tips

Issue 897113 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 26
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 880848



Sign in to add a comment

Port OAuth2TokenService::Observer::On{Start, End}BatchChanges to IdentityManager::Observer

Project Member Reported by blundell@chromium.org, Oct 19

Issue description

We can name it IdentityManager::Observer::On{Start,End}BatchOfRefreshTokenStateChanges. It should be unittested the way that e.g. IdentityManager::Observer::OnRefreshTokenRemovedForAccount() is unittested.
 
Do you want also a ScopedBatchChange class on IdentityManager?

OnRefreshTokenRemovedForAccount uses the DiagnosticObserver to guarantee order - do you also want this here? It seems like there's only a single non-test implementation of EndBatchChanges(), so I guess it doesn't really matter.
Thanks!

- We don't need a ScopedBatchChange class on IdentityManager at this time because we'll just be forwarding the calls from the token service.

- After https://bugs.chromium.org/p/chromium/issues/detail?id=883722, we don't need to worry about ensuring ordering as we're just forwarding the observer callbacks on.

BTW, there are several impls of OnEndBatchChanges():

blundell:src((0f884208b23a...)) $  git grep "OnEndBatchChanges() override" | g "\.h"
(standard input):3:components/signin/core/browser/about_signin_internals.h:  void OnEndBatchChanges() override;
(standard input):4:components/signin/core/browser/account_reconcilor.h:  void OnEndBatchChanges() override;
(standard input):5:components/signin/ios/browser/oauth2_token_service_observer_bridge.h:  void OnEndBatchChanges() override;
(standard input):6:ios/chrome/browser/signin/authentication_service.h:  void OnEndBatchChanges() override; 
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 26

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

commit 2ba0fb79d117a8be692907da4e64e35fb7419aad
Author: Jochen Eisinger <jochen@chromium.org>
Date: Fri Oct 26 13:20:11 2018

Add IdentityManager::Observer::On{Start,End}BatchOfRefreshTokenStateChanges

This mirrors the calls on
OAuth2TokenService::Observer::On{Start,End}BatchChanges.

Bug:  897113 
Change-Id: I6a436baf28f4bf629da8a9b037cd5ab04c644fe1
Reviewed-on: https://chromium-review.googlesource.com/c/1296249
Commit-Queue: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603071}
[modify] https://crrev.com/2ba0fb79d117a8be692907da4e64e35fb7419aad/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/2ba0fb79d117a8be692907da4e64e35fb7419aad/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/2ba0fb79d117a8be692907da4e64e35fb7419aad/services/identity/public/cpp/identity_manager_unittest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment