New issue
Advanced search Search tips

Issue 903870 link

Starred by 3 users
Status: Fixed
Owner:
sdefresne@chromium.org
Closed: Nov 21
Cc:
blundell@chromium.org
sdefresne@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocked on:
issue 887438


Sign in to add a comment

Migrate chrome/browser/password_manager/password_store_signin_notifier_impl.cc to IdentityManager::Observer

Project Member Reported by ma...@igalia.com, Nov 9 
Implemented:
  - GoogleSignedOut()
  - GoogleSigninSucceededWithPassword()

Note that there's no mapping for GoogleSigninSucceededWithPassword() yet, so not setting the Proj-Servicification-VendorBug tag as per the following comment in the migration document:

"""
GoogleSigninSucceededWithPassword → TODO
This is only implemented by PasswordStoreSigninNotifierImpl; probably better to implement and port ourselves instead of delegating to contractors
"""

Comment 1 by blundell@chromium.org, Nov 15

Cc: sdefresne@chromium.org
Labels: -Pri-3 Pri-1
Sylvain, had you had thoughts on what we should do here?

Comment 2 by blundell@chromium.org, Nov 15

Owner: sdefresne@chromium.org
Status: Assigned (was: Untriaged)
Assigning to Sylvain to respond to c#1; once we have an approach here, we can see whether the technical work is a VendorBug.

Comment 3 by sdefresne@chromium.org, Nov 16

Status: Started (was: Assigned)
I am currently discussing with the OWNERS of the code whether it is worth supporting GoogleSigninSucceededWithPassword or not. Depending on the outcome of the discussion, I'll post a plan on how to address this conversion.

Comment 4 by sdefresne@chromium.org, Nov 20 

This method is still needed, so I'll add similar API to IdentityManager::Observer (marked as deprecated as this will only be meaningful for legacy sign-in workflow).

Comment 5 by sdefresne@chromium.org, Nov 20 

 Issue 887448  has been merged into this issue.

Comment 6 by sdefresne@chromium.org, Nov 20

Blockedon: 887438
Project Member

Comment 7 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

Comment 8 by sdefresne@chromium.org, Nov 21

Status: Fixed (was: Started)
►

Sign in to add a comment