New issue
Advanced search Search tips

Issue 796545 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 796544



Sign in to add a comment

Convert //components/history to use IdentityManager rather than //components/signin classes

Project Member Reported by blundell@chromium.org, Dec 20 2017

Issue description

In //components/history, WebHistoryServices fetches access tokens for the primary account via SigninManager and ProfileOAuth2TokenService. As part of isolating these classes inside the Identity Service, we need to convert this usage to be of IdentityManager/PrimaryAccountAccessTokenFetcher (see crbug.com/796544).
 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 20 2017

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

commit 1b43007318e334b5f16e910309b28f85f0d7d3b4
Author: Colin Blundell <blundell@chromium.org>
Date: Wed Dec 20 15:04:55 2017

Remove unneeded test dependencies on //components/signin classes

WebHistoryService is a client of SigninManager and
ProfileOAuth2TokenService. Its unittest creates fakes of these classes
to supply them to the WebHistoryService instance under test. However,
these dependencies are actually unnecessary:
- WebHistoryService's only usage of these classes is to pass them to
an internal RequestImpl class that makes access token requests.
- The unittest deliberately eschews usage of RequestImpl, replacing it
with a TestRequest class that makes no usage of //components/signin.

Note that changing the tests to actually exercise RequestImpl would be
a non-trivial amount of work, as RequestImpl has substantial other
interactions that would need to be stubbed out (e.g., with the network).
This CL simply eliminates the unused dependencies from the test.

Bug:  796545 
Change-Id: I45c55684e119ece7172c8ee406486a46faca815e
Reviewed-on: https://chromium-review.googlesource.com/835109
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525331}
[modify] https://crrev.com/1b43007318e334b5f16e910309b28f85f0d7d3b4/components/history/core/browser/web_history_service_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Dec 22 2017

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

commit 6df05934e12d675b744939abeb9b185e7ad5d89c
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Dec 22 08:54:30 2017

Remove signin-related args from FakeWebHistoryService constructor

FakeWebHistoryService takes in signin-related classes in its
constructor to pass them to the base class WebHistoryService constructor.
However, these arguments are actually unnecessary:
- WebHistoryService's only usage of these classes is to pass them to
an internal RequestImpl class that makes access token requests.
- The fake deliberately eschews usage of RequestImpl, replacing it
with a FakeRequest class that makes no usage of //components/signin.

TBR=jochen@chromium.org

Bug:  796545 
Change-Id: Ie82ec68f1e4d676d8dbcad1a3016d39b7999e06c
Reviewed-on: https://chromium-review.googlesource.com/840029
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525960}
[modify] https://crrev.com/6df05934e12d675b744939abeb9b185e7ad5d89c/chrome/browser/browsing_data/history_counter_browsertest.cc
[modify] https://crrev.com/6df05934e12d675b744939abeb9b185e7ad5d89c/chrome/browser/browsing_data/sync_aware_counter_browsertest.cc
[modify] https://crrev.com/6df05934e12d675b744939abeb9b185e7ad5d89c/chrome/browser/ui/webui/browsing_history_handler_unittest.cc
[modify] https://crrev.com/6df05934e12d675b744939abeb9b185e7ad5d89c/components/browsing_data/core/history_notice_utils_unittest.cc
[modify] https://crrev.com/6df05934e12d675b744939abeb9b185e7ad5d89c/components/history/core/browser/browsing_history_service_unittest.cc
[modify] https://crrev.com/6df05934e12d675b744939abeb9b185e7ad5d89c/components/history/core/test/fake_web_history_service.cc
[modify] https://crrev.com/6df05934e12d675b744939abeb9b185e7ad5d89c/components/history/core/test/fake_web_history_service.h

Project Member

Comment 3 by bugdroid1@chromium.org, Jan 10 2018

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

commit df272efb8f38b128da124c2b1ef9330346a187eb
Author: Colin Blundell <blundell@chromium.org>
Date: Wed Jan 10 10:35:46 2018

Convert history to use Identity Service client library

This CL converts //components/history from using //components/signin
to using the Identity Service client library. The conversion is
straightforward:

- Obtain the primary account info via the IdentityManager rather than
  the SigninManager.
- Invalidate access tokens via the IdentityManager rather than
  ProfileOAuth2TokenService.
- Obtain access tokens via a one-shot PrimaryAccountAccessTokenFetcher
  rather than via ProfileOAuth2TokenService.

There is a slight behavioral change as the Identity Service client lib
interacts with ProfileOAuth2TokenService asynchronously to invalidate
access tokens and fetch access tokens. This behavioral change should
not cause any user-visible changes in behavior, as access token fetching
is already asynchronous from the consumer POV and ordering of a call
to invalidate an access token with a subsequent access token fetch is
preserved by design.

I tested the relevant behavior manually by going to chrome://history
for a signed-in and syncing test user and confirming that their
synced history showed up after this change identically to before this
change.

Bug:  796545 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Ic2cbf966f5bd7c88512ff710bf5d5dba3da613ad
Reviewed-on: https://chromium-review.googlesource.com/846879
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528275}
[modify] https://crrev.com/df272efb8f38b128da124c2b1ef9330346a187eb/chrome/browser/history/web_history_service_factory.cc
[modify] https://crrev.com/df272efb8f38b128da124c2b1ef9330346a187eb/components/history/DEPS
[modify] https://crrev.com/df272efb8f38b128da124c2b1ef9330346a187eb/components/history/core/browser/BUILD.gn
[modify] https://crrev.com/df272efb8f38b128da124c2b1ef9330346a187eb/components/history/core/browser/DEPS
[modify] https://crrev.com/df272efb8f38b128da124c2b1ef9330346a187eb/components/history/core/browser/web_history_service.cc
[modify] https://crrev.com/df272efb8f38b128da124c2b1ef9330346a187eb/components/history/core/browser/web_history_service.h
[modify] https://crrev.com/df272efb8f38b128da124c2b1ef9330346a187eb/components/history/core/browser/web_history_service_unittest.cc
[modify] https://crrev.com/df272efb8f38b128da124c2b1ef9330346a187eb/components/history/core/test/fake_web_history_service.cc
[modify] https://crrev.com/df272efb8f38b128da124c2b1ef9330346a187eb/ios/chrome/browser/history/web_history_service_factory.cc

Status: Fixed (was: Started)

Sign in to add a comment