New issue
Advanced search Search tips

Issue 919482 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 7
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Switch AccountInvestigator from ListedAccount to AccountInfo

Project Member Reported by toniki...@chromium.org, Jan 7

Issue description

As AccountInvestigator code is switched to using IdentityManager, its logic got into a in-flux state of using both ListedAccount and AccountInfo structures, with conversion back and forth happening when needed.

Quoting David Rogers, "This is dangerous, because AccountInfo does not have all the information for the conversion: the 'valid' and 'verified' fields are not populated. Looking at the code here, it seems that these fields are not currently used, so this is probably fine. The TODO should be addressed though, to guarantee that the fields are really not used (currently and in the future). It should be pretty easy hopefully."

This bug aims to track fixing it.
 
Status: Started (was: Assigned)
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 7

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

commit 39a9ec69135fe27625328b35cca5ee18cf55ba6b
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Mon Jan 07 17:56:17 2019

Switch AccountInvestigator from ListedAccount to AccountInfo

This is a follow up of [1], that switched AccountInvestigator
away from GaiaCookieManagerService::Observer (in favor of
IdentityManager::Observer), but needed to add temporarty
ListedAccount <-> AccountInfo conversion logic helpers.

Now, AccountInvestigator operates exclusively on AccountInfo.

[1] https://crrev.com/c/1396240 (Remove GaiaCookieManagerService
dependency from AccountsInvestigator)

BUG= 919482 

Change-Id: I03a474e5cc6d5021477816285ccc0b15d807a787
Reviewed-on: https://chromium-review.googlesource.com/c/1398041
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620370}
[modify] https://crrev.com/39a9ec69135fe27625328b35cca5ee18cf55ba6b/components/signin/core/browser/account_investigator.cc
[modify] https://crrev.com/39a9ec69135fe27625328b35cca5ee18cf55ba6b/components/signin/core/browser/account_investigator.h
[modify] https://crrev.com/39a9ec69135fe27625328b35cca5ee18cf55ba6b/components/signin/core/browser/account_investigator_unittest.cc

Status: Fixed (was: Started)
Project Member

Comment 4 by bugdroid1@chromium.org, Jan 10

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

commit 44a910031d52a8d1987f02823f7a5934d574c87b
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Thu Jan 10 12:11:03 2019

Clean up pass on account_investigator_unittest.cc

This is a follow up of [1], from blundell@.

[1] https://crrev.com/c/1398041/3/components/signin/core/browser/account_investigator_unittest.cc#183

Bug= 919482 

Change-Id: I5dbca667c7c4c8152fab529df976eb1e04acb983
Reviewed-on: https://chromium-review.googlesource.com/c/1403156
Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Cr-Commit-Position: refs/heads/master@{#621549}
[modify] https://crrev.com/44a910031d52a8d1987f02823f7a5934d574c87b/components/signin/core/browser/account_investigator_unittest.cc

Sign in to add a comment