New issue
Advanced search Search tips

Issue 887438 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 21
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocked on:
issue 887441

Blocking:
issue 887433
issue 887448
issue 903870



Sign in to add a comment

Provide API to replace SigningManagerBase::Observer::GoogleSigninSucceededWithPassword

Project Member Reported by sdefresne@chromium.org, Sep 20

Issue description

Required to convert the following classes to move away from SigninManagerBase::Observer:
- PasswordStoreSigninNotifierImpl

This is also used by those test classes:
- TestSigninManagerObserver

Note that the way the interface is exposed is terrible. The method is marked private as if it would prevent classes not marked as friend from implementing it, but this is not the case.

It would be probably better to move this to a separate interface than IdentityManager::Observer. Or to see if there is a way to remove this.
 
Blockedon: 887441
Blocking: 887433
Blocking: 887448
Blocking: 903870
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 21

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

commit 8cdfa6767f4e028cd0555db84fe27ab460a61abc
Author: Sylvain Defresne <sdefresne@chromium.org>
Date: Wed Nov 21 13:27:31 2018

Convert PasswordStoreSigninNotifierImpl to IdentityManager

Add a new method OnPrimaryAccountSetWithPassword to
IdentityManager::Observer (with comments indicating
it only works for legacy sign-in workflow) and port
PasswordStoreSigninNotifierImpl to it.

Bug:  903870 ,  887438 ,  887441 
Change-Id: Ic79cff4ffdd341aedb22ee2c8abd40d309ee6fa4
Reviewed-on: https://chromium-review.googlesource.com/c/1344089
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610029}
[modify] https://crrev.com/8cdfa6767f4e028cd0555db84fe27ab460a61abc/chrome/browser/password_manager/password_store_signin_notifier_impl.cc
[modify] https://crrev.com/8cdfa6767f4e028cd0555db84fe27ab460a61abc/chrome/browser/password_manager/password_store_signin_notifier_impl.h
[modify] https://crrev.com/8cdfa6767f4e028cd0555db84fe27ab460a61abc/components/signin/core/browser/signin_manager.cc
[modify] https://crrev.com/8cdfa6767f4e028cd0555db84fe27ab460a61abc/components/signin/core/browser/signin_manager_base.h
[modify] https://crrev.com/8cdfa6767f4e028cd0555db84fe27ab460a61abc/components/signin/core/browser/signin_manager_unittest.cc
[modify] https://crrev.com/8cdfa6767f4e028cd0555db84fe27ab460a61abc/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/8cdfa6767f4e028cd0555db84fe27ab460a61abc/services/identity/public/cpp/identity_manager.h

Status: Fixed (was: Assigned)

Sign in to add a comment