New issue
Advanced search Search tips

Issue 907782 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 859882



Sign in to add a comment

Eliminate FakeGaiaCookieManagerService

Project Member Reported by blundell@chromium.org, Nov 22

Issue description

As described in the design doc, this class presents a blocker to the conversion of the codebase to IdentityManager.

For the motivation of the problem and design of the solution, see the design doc:

https://docs.google.com/document/d/1t0ZtuV7h-znzdItFgBW0aKPscAwWXIBuNZnNlEGgi7g/edit
 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 10

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

commit a3b72976938f9fdc1438b1e7543c767a539ee0c6
Author: Lowell Manners <lowell@chromium.org>
Date: Mon Dec 10 13:47:26 2018

Update FakeGaiaCookieManagerService constructor to accept a TestUrlLoaderFactory

FakeGaiaCookieManagerService's constructor takes in a boolean that allows the client to specify
whether FakeGCMS should internally create a TestUrlLoaderFactory or implicitly use the one
provided by SigninClient. This boolean defaults to the internal creation.

This CL adds a constructor that takes in a TestUrlLoaderFactory*, which FakeGCMS will use.
This replaces the case where true is (implicitly or explicitly) passed, with the difference
that the caller is now responsible for owning the TestUrlLoaderFactory passed in.

This CL also converts a few clients to show the pattern.

This is step 1a of the plan to eliminate FakeGCMS entirely:
https://docs.google.com/document/d/1t0ZtuV7h-znzdItFgBW0aKPscAwWXIBuNZnNlEGgi7g/edit

Bug: 907782
Change-Id: I6dc32bc71e292288cef5bf8ada868b9a55c821cd
Reviewed-on: https://chromium-review.googlesource.com/c/1356944
Commit-Queue: Lowell Manners <lowell@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615108}
[modify] https://crrev.com/a3b72976938f9fdc1438b1e7543c767a539ee0c6/components/search_provider_logos/logo_service_impl_unittest.cc
[modify] https://crrev.com/a3b72976938f9fdc1438b1e7543c767a539ee0c6/components/signin/core/browser/account_investigator_unittest.cc
[modify] https://crrev.com/a3b72976938f9fdc1438b1e7543c767a539ee0c6/components/signin/core/browser/fake_gaia_cookie_manager_service.cc
[modify] https://crrev.com/a3b72976938f9fdc1438b1e7543c767a539ee0c6/components/signin/core/browser/fake_gaia_cookie_manager_service.h
[modify] https://crrev.com/a3b72976938f9fdc1438b1e7543c767a539ee0c6/components/signin/core/browser/signin_manager_unittest.cc
[modify] https://crrev.com/a3b72976938f9fdc1438b1e7543c767a539ee0c6/components/signin/core/browser/signin_tracker_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Dec 12

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

commit 2558150eb6c6c00632f2d066e44ef9ddec476863
Author: Lowell Manners <lowell@chromium.org>
Date: Wed Dec 12 09:28:15 2018

Update two tests to use new FakeGaiaCookieManagerService constructor.

The difference with the new constructor is that the test is now responsible for owning the
TestUrlLoaderFactory passed in, rather than having a new TestUrlLoaderFactory implicitly
created.

This is part of step 1a of the plan to eliminate FakeGCMS entirely:
https://docs.google.com/document/d/1t0ZtuV7h-znzdItFgBW0aKPscAwWXIBuNZnNlEGgi7g/edit

Bug: 907782
Change-Id: If3a45a2b075c9f6fbf3c3274b80fd64b5e021c08
Reviewed-on: https://chromium-review.googlesource.com/c/1371391
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Lowell Manners <lowell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615833}
[modify] https://crrev.com/2558150eb6c6c00632f2d066e44ef9ddec476863/components/signin/core/browser/account_reconcilor_unittest.cc
[modify] https://crrev.com/2558150eb6c6c00632f2d066e44ef9ddec476863/services/identity/public/cpp/identity_manager_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 17

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

commit 67ea1b2eaabd78fb15d46681a37dfb731d8261e6
Author: Lowell Manners <lowell@chromium.org>
Date: Mon Dec 17 13:03:13 2018

No longer create a TestUrlLoaderFactory by default for IdentityTestEnvironment in //chrome.

Once FakeGaiaCookieManagerService is eliminated, tests will need to own the TestUrlLoaderFactory
themselves, so it's better not to have a TestUrlLoaderFactory be created except when it will
actually be used. We can tell it wasn't being used because the tests still pass.

This CL also deletes the create_fake_url_loader_factory_for_cookie_requests param from
AppendIdentityTestEnvironmentFactories, as it was never used.

This is part of step 1a of the plan to eliminate FakeGCMS entirely:
https://docs.google.com/document/d/1t0ZtuV7h-znzdItFgBW0aKPscAwWXIBuNZnNlEGgi7g/edit

Bug: 907782
Change-Id: I2ce861a4b053045a55c4d834d081b51d68a69534
Reviewed-on: https://chromium-review.googlesource.com/c/1375872
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Lowell Manners <lowell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617099}
[modify] https://crrev.com/67ea1b2eaabd78fb15d46681a37dfb731d8261e6/chrome/browser/extensions/api/identity/identity_apitest.cc
[modify] https://crrev.com/67ea1b2eaabd78fb15d46681a37dfb731d8261e6/chrome/browser/signin/identity_test_environment_profile_adaptor.cc
[modify] https://crrev.com/67ea1b2eaabd78fb15d46681a37dfb731d8261e6/chrome/browser/signin/identity_test_environment_profile_adaptor.h

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 17

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

commit ddce7e097d574e0d80876dbcbced50b57c3fd309
Author: Lowell Manners <lowell@chromium.org>
Date: Mon Dec 17 14:04:08 2018

Update BuildFakeGaiaCookieManagerService to no longer create a TestUrlLoaderFactory by default.

Once FakeGaiaCookieManagerService is eliminated, tests will need to own the TestUrlLoaderFactory
themselves, so it's better not to have a TestUrlLoaderFactory be created except when it will
actually be used.

For tests which actually need a TestUrlLoaderFactory we now set
create_fake_url_loader_factory_for_cookie_requests explicitly to true.

This is part of step 1a of the plan to eliminate FakeGCMS entirely:
https://docs.google.com/document/d/1t0ZtuV7h-znzdItFgBW0aKPscAwWXIBuNZnNlEGgi7g/edit

TBR=menegola@chromium.org,mamir@chromium.org,avi@chromium.org,bsep@chromium.org

Bug: 907782
Change-Id: I23e7dcb176f384ebd6b165a489a427be54a47399
Reviewed-on: https://chromium-review.googlesource.com/c/1378091
Commit-Queue: Lowell Manners <lowell@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617111}
[modify] https://crrev.com/ddce7e097d574e0d80876dbcbced50b57c3fd309/chrome/browser/signin/fake_gaia_cookie_manager_service_builder.cc
[modify] https://crrev.com/ddce7e097d574e0d80876dbcbced50b57c3fd309/chrome/browser/signin/signin_ui_util_unittest.cc
[modify] https://crrev.com/ddce7e097d574e0d80876dbcbced50b57c3fd309/chrome/browser/supervised_user/child_accounts/child_account_service_unittest.cc
[modify] https://crrev.com/ddce7e097d574e0d80876dbcbced50b57c3fd309/chrome/browser/sync/test/integration/secondary_account_helper.cc
[modify] https://crrev.com/ddce7e097d574e0d80876dbcbced50b57c3fd309/chrome/browser/ui/cocoa/test/cocoa_profile_test.mm
[modify] https://crrev.com/ddce7e097d574e0d80876dbcbced50b57c3fd309/chrome/browser/ui/views/frame/test_with_browser_view.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Dec 18

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

commit 4d42e59e277db69d3b4b30b73caa24f80d3a36b7
Author: Lowell Manners <lowell@chromium.org>
Date: Tue Dec 18 12:48:40 2018

Don't set a default value for use_fake_url_fetcher in FakeGaiaCookieManagerService constructor.

This CL also removes the only remaining caller which didn't explicitly set a value for
use_fake_url_fetcher.

Removing the default value for use_fake_url_fetcher gives this constructor a unique method
signature, which will make it easier to delete later.

This is part of step 1a of the plan to eliminate FakeGCMS entirely:
https://docs.google.com/document/d/1t0ZtuV7h-znzdItFgBW0aKPscAwWXIBuNZnNlEGgi7g/edit

TBR=michaeldo@chromium.org

Bug: 907782
Change-Id: Id6ae19838218d8bb8947a2638d3048d67af783bc
Reviewed-on: https://chromium-review.googlesource.com/c/1380092
Commit-Queue: Lowell Manners <lowell@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617459}
[modify] https://crrev.com/4d42e59e277db69d3b4b30b73caa24f80d3a36b7/components/signin/core/browser/fake_gaia_cookie_manager_service.h
[modify] https://crrev.com/4d42e59e277db69d3b4b30b73caa24f80d3a36b7/ios/web_view/internal/sync/cwv_sync_controller_unittest.mm

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 18

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

commit daa6f9da01399618188765b047f791bd1d85799b
Author: Lowell Manners <lowell@chromium.org>
Date: Tue Dec 18 13:48:55 2018

Add constructor to FakeGaiaCookieManagerService that does not create a TestUrlLoaderFactory.

This new constructor is meant to replace the constructor that accepts a use_fake_url_fetcher
argument in the cases where use_fake_url_fetcher=false.

Also updated one caller to use this new constructor.

This is part of step 1a of the plan to eliminate FakeGCMS entirely:
https://docs.google.com/document/d/1t0ZtuV7h-znzdItFgBW0aKPscAwWXIBuNZnNlEGgi7g/edit

Bug: 907782
Change-Id: I89d3cb25493bbe9fe47b22b01c5ac942bbfae34f
Reviewed-on: https://chromium-review.googlesource.com/c/1379770
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Lowell Manners <lowell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617470}
[modify] https://crrev.com/daa6f9da01399618188765b047f791bd1d85799b/chrome/browser/signin/dice_response_handler_unittest.cc
[modify] https://crrev.com/daa6f9da01399618188765b047f791bd1d85799b/components/signin/core/browser/fake_gaia_cookie_manager_service.cc
[modify] https://crrev.com/daa6f9da01399618188765b047f791bd1d85799b/components/signin/core/browser/fake_gaia_cookie_manager_service.h

Project Member

Comment 7 by bugdroid1@chromium.org, Dec 19

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

commit 52ab02b72f671904c784d26cbcffa25b5443ede2
Author: Lowell Manners <lowell@chromium.org>
Date: Wed Dec 19 11:50:42 2018

Fix formatting for files related to GaiaCookieManagerService.

This is a formatting only change. I have planned changes for these
files; by fixing the formatting in advance the diff on the actual change
will be smaller.

Bug: 907782 Change-Id: Ia60ad2f1d571d79bb2de27f351e823562a9cf4e8
Change-Id: Ia60ad2f1d571d79bb2de27f351e823562a9cf4e8
Reviewed-on: https://chromium-review.googlesource.com/c/1382482
Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: Lowell Manners <lowell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617801}
[modify] https://crrev.com/52ab02b72f671904c784d26cbcffa25b5443ede2/components/signin/core/browser/gaia_cookie_manager_service.cc
[modify] https://crrev.com/52ab02b72f671904c784d26cbcffa25b5443ede2/components/signin/core/browser/gaia_cookie_manager_service.h
[modify] https://crrev.com/52ab02b72f671904c784d26cbcffa25b5443ede2/components/signin/core/browser/gaia_cookie_manager_service_unittest.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Dec 19

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

commit 7607d0f2bf472952960beb0a8d9c3918be97339b
Author: Lowell Manners <lowell@chromium.org>
Date: Wed Dec 19 11:50:51 2018

[iOS] BuildFakeGCMS no longer creates a TestUrlLoaderFactory by default.

Once FakeGaiaCookieManagerService is eliminated, tests will need to own
the TestUrlLoaderFactory themselves, so it's better not to have a
TestUrlLoaderFactory be created except when it will actually be used.

This change is equivalent to https://crrev.com/c/1378091, but for iOS.

This is part of step 1a of the plan to eliminate FakeGCMS entirely:
https://docs.google.com/document/d/1t0ZtuV7h-znzdItFgBW0aKPscAwWXIBuNZnNlEGgi7g/edit

Bug: 907782 Change-Id: I314cbd904b8c60a9f6feb5ce626728e06aeee01e
Change-Id: I314cbd904b8c60a9f6feb5ce626728e06aeee01e
Reviewed-on: https://chromium-review.googlesource.com/c/1382475
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: Lowell Manners <lowell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617802}
[modify] https://crrev.com/7607d0f2bf472952960beb0a8d9c3918be97339b/ios/chrome/browser/signin/fake_gaia_cookie_manager_service_builder.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Dec 19

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

commit cc203f6d847d3fd4031997c91665b66fc72c7081
Author: Lowell Manners <lowell@chromium.org>
Date: Wed Dec 19 14:59:17 2018

Update IdentityTestEnvironment to take a TestURLLoaderFactory.

This CL removes the boolean option
use_fake_url_loader_for_gaia_cookie_manager.  Now instead of passing
true, callers pass a TestUrlLoaderFactory themselves.

This is part of step 1a of the plan to eliminate FakeGCMS entirely:
https://docs.google.com/document/d/1t0ZtuV7h-znzdItFgBW0aKPscAwWXIBuNZnNlEGgi7g/edit

TBR=kristipark@chromium.org

Bug: 907782
Change-Id: I103ce003b364c6ba0233515d674b8a791028174d
Reviewed-on: https://chromium-review.googlesource.com/c/1384204
Commit-Queue: Lowell Manners <lowell@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617831}
[modify] https://crrev.com/cc203f6d847d3fd4031997c91665b66fc72c7081/chrome/browser/search/one_google_bar/one_google_bar_service_unittest.cc
[modify] https://crrev.com/cc203f6d847d3fd4031997c91665b66fc72c7081/components/browser_sync/profile_sync_test_util.cc
[modify] https://crrev.com/cc203f6d847d3fd4031997c91665b66fc72c7081/components/browser_sync/sync_auth_manager_unittest.cc
[modify] https://crrev.com/cc203f6d847d3fd4031997c91665b66fc72c7081/services/identity/public/cpp/identity_test_environment.cc
[modify] https://crrev.com/cc203f6d847d3fd4031997c91665b66fc72c7081/services/identity/public/cpp/identity_test_environment.h
[modify] https://crrev.com/cc203f6d847d3fd4031997c91665b66fc72c7081/services/identity/public/cpp/primary_account_mutator_unittest.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Jan 3

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

commit 388e52eba646c82655f971175cf8455c2596d942
Author: Lowell Manners <lowell@chromium.org>
Date: Thu Jan 03 15:32:01 2019

[iOS] Don't create a TestUrlLoaderFactory for IdentityTestEnvironment.

None of the iOS tests need a TestUrlLoaderFactory, so we should not
create one by default.

Once FakeGaiaCookieManagerService is eliminated, tests will need to own
the TestUrlLoaderFactory themselves, so it's better not to have a
TestUrlLoaderFactory be created except when it will actually be used.

This is part of step 1a of the plan to eliminate FakeGCMS entirely:
https://docs.google.com/document/d/1t0ZtuV7h-znzdItFgBW0aKPscAwWXIBuNZnNlEGgi7g/edit

Bug: 907782
Change-Id: I420748e43f468858c33bd3d6c8b32f182e50a88f
Reviewed-on: https://chromium-review.googlesource.com/c/1382497
Commit-Queue: Lowell Manners <lowell@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619640}
[modify] https://crrev.com/388e52eba646c82655f971175cf8455c2596d942/ios/chrome/browser/signin/identity_test_environment_chrome_browser_state_adaptor.h

Project Member

Comment 11 by bugdroid1@chromium.org, Jan 3

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

commit 357909fe63f449e025b97561d3143bb19d1d2b7b
Author: Lowell Manners <lowell@chromium.org>
Date: Thu Jan 03 15:38:49 2019

[iOS] Delete unused method BuildFakeGaiaCookieManagerServiceWithOptions.

iOS tests don't use BuildFakeGaiaCookieManagerServiceWithOptions, so it
is dead code and can be deleted.

This functionality can always be added later if/when there is a test
which needs it.

This is part of step 1a of the plan to eliminate FakeGCMS entirely:
https://docs.google.com/document/d/1t0ZtuV7h-znzdItFgBW0aKPscAwWXIBuNZnNlEGgi7g/edit

Bug: 907782
Change-Id: Ia924bd8f41e6f7090646eae02841a01743f104a2
Reviewed-on: https://chromium-review.googlesource.com/c/1384370
Commit-Queue: Lowell Manners <lowell@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619644}
[modify] https://crrev.com/357909fe63f449e025b97561d3143bb19d1d2b7b/ios/chrome/browser/signin/fake_gaia_cookie_manager_service_builder.cc
[modify] https://crrev.com/357909fe63f449e025b97561d3143bb19d1d2b7b/ios/chrome/browser/signin/fake_gaia_cookie_manager_service_builder.h
[modify] https://crrev.com/357909fe63f449e025b97561d3143bb19d1d2b7b/ios/chrome/browser/signin/identity_test_environment_chrome_browser_state_adaptor.cc
[modify] https://crrev.com/357909fe63f449e025b97561d3143bb19d1d2b7b/ios/chrome/browser/signin/identity_test_environment_chrome_browser_state_adaptor.h

Project Member

Comment 12 by bugdroid1@chromium.org, Jan 7

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

commit 81cd042ce12ec77b2bb90d3081c2fadbe8820f74
Author: Lowell Manners <lowell@chromium.org>
Date: Mon Jan 07 13:42:37 2019

Create list_accounts_test_utils helpers.

This CL moves test logic which was previously part of
FakeGaiaCookieManagerService to standalone helper methods in
list_accounts_test_utils.

Eventually all code that uses FakeGaiaCookieManagerService to set fake
responses will be updated to call these new helpers, and
FakeGaiaCookieManagerService will be deleted.

This change is step 3 of the plan to eliminate FakeGCMS entirely:
https://docs.google.com/document/d/1t0ZtuV7h-znzdItFgBW0aKPscAwWXIBuNZnNlEGgi7g/edit

Bug: 907782
Change-Id: I1aab793705a326e2cf1baa43670aa120dd5df105
Reviewed-on: https://chromium-review.googlesource.com/c/1394590
Commit-Queue: Lowell Manners <lowell@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620310}
[modify] https://crrev.com/81cd042ce12ec77b2bb90d3081c2fadbe8820f74/components/signin/core/browser/BUILD.gn
[modify] https://crrev.com/81cd042ce12ec77b2bb90d3081c2fadbe8820f74/components/signin/core/browser/account_reconcilor_unittest.cc
[modify] https://crrev.com/81cd042ce12ec77b2bb90d3081c2fadbe8820f74/components/signin/core/browser/fake_gaia_cookie_manager_service.cc
[modify] https://crrev.com/81cd042ce12ec77b2bb90d3081c2fadbe8820f74/components/signin/core/browser/fake_gaia_cookie_manager_service.h
[add] https://crrev.com/81cd042ce12ec77b2bb90d3081c2fadbe8820f74/components/signin/core/browser/list_accounts_test_utils.cc
[add] https://crrev.com/81cd042ce12ec77b2bb90d3081c2fadbe8820f74/components/signin/core/browser/list_accounts_test_utils.h
[modify] https://crrev.com/81cd042ce12ec77b2bb90d3081c2fadbe8820f74/services/identity/public/cpp/identity_test_utils.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Jan 9

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

commit 213446d0215138261ed8deb050528d106bfad43c
Author: Lowell Manners <lowell@chromium.org>
Date: Wed Jan 09 17:31:03 2019

Remove integration test dependencies on FakeGaiaCookieManagerService.

Instead of creating a FakeGaiaCookieManagerService, we now set mock
responses using the TestURLLoaderFactory that already existed in
SyncTest.

As part of this change, the helpers that need access to the
TestURLLoaderFactory in SyncTest were moved from
secondary_account_helper to a test class which inherits from SyncTest.

Removing this dependency will make it easier to eliminate FakeGCMS
entirely:
http://doc/1t0ZtuV7h-znzdItFgBW0aKPscAwWXIBuNZnNlEGgi7g#heading=h.7dt3rezdvj2u

Bug: 907782
Change-Id: Id60b56ecc76bf0d4bbe0089deec53e0d4225d4f0
Reviewed-on: https://chromium-review.googlesource.com/c/1396148
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621200}
[modify] https://crrev.com/213446d0215138261ed8deb050528d106bfad43c/chrome/browser/metrics/ukm_browsertest.cc
[delete] https://crrev.com/aa323fc134b020e3ddb77767e34382fe6264089e/chrome/browser/sync/test/integration/secondary_account_helper.h
[rename] https://crrev.com/213446d0215138261ed8deb050528d106bfad43c/chrome/browser/sync/test/integration/secondary_account_sync_test.cc
[add] https://crrev.com/213446d0215138261ed8deb050528d106bfad43c/chrome/browser/sync/test/integration/secondary_account_sync_test.h
[modify] https://crrev.com/213446d0215138261ed8deb050528d106bfad43c/chrome/browser/sync/test/integration/single_client_secondary_account_sync_test.cc
[modify] https://crrev.com/213446d0215138261ed8deb050528d106bfad43c/chrome/browser/sync/test/integration/single_client_wallet_sync_test.cc
[modify] https://crrev.com/213446d0215138261ed8deb050528d106bfad43c/chrome/browser/sync/test/integration/sync_test.h
[modify] https://crrev.com/213446d0215138261ed8deb050528d106bfad43c/chrome/test/BUILD.gn
[modify] https://crrev.com/213446d0215138261ed8deb050528d106bfad43c/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/213446d0215138261ed8deb050528d106bfad43c/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/213446d0215138261ed8deb050528d106bfad43c/services/identity/public/cpp/identity_test_utils.cc
[modify] https://crrev.com/213446d0215138261ed8deb050528d106bfad43c/services/identity/public/cpp/identity_test_utils.h

Project Member

Comment 14 by bugdroid1@chromium.org, Jan 10

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

commit 51085ff0b1f0c0563b9b0a00e885ff89e04d274b
Author: Patti <patricialor@chromium.org>
Date: Thu Jan 10 02:51:05 2019

Revert "Remove integration test dependencies on FakeGaiaCookieManagerService."

This reverts commit 213446d0215138261ed8deb050528d106bfad43c.

Reason for revert:

FindIt has found this change to be the culprit of the flaky tests on 
chromium.linux	linux-xenial-rel
and
chromium.win	Win 7 Tests x64 (1)

See  crbug.com/920485  for more information.

Original change's description:
> Remove integration test dependencies on FakeGaiaCookieManagerService.
> 
> Instead of creating a FakeGaiaCookieManagerService, we now set mock
> responses using the TestURLLoaderFactory that already existed in
> SyncTest.
> 
> As part of this change, the helpers that need access to the
> TestURLLoaderFactory in SyncTest were moved from
> secondary_account_helper to a test class which inherits from SyncTest.
> 
> Removing this dependency will make it easier to eliminate FakeGCMS
> entirely:
> http://doc/1t0ZtuV7h-znzdItFgBW0aKPscAwWXIBuNZnNlEGgi7g#heading=h.7dt3rezdvj2u
> 
> Bug: 907782
> Change-Id: Id60b56ecc76bf0d4bbe0089deec53e0d4225d4f0
> Reviewed-on: https://chromium-review.googlesource.com/c/1396148
> Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
> Reviewed-by: Marc Treib <treib@chromium.org>
> Reviewed-by: Colin Blundell <blundell@chromium.org>
> Commit-Queue: Colin Blundell <blundell@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#621200}

TBR=rkaplow@chromium.org,blundell@chromium.org,treib@chromium.org,lowell@chromium.org

Change-Id: Iee0882a9e96118dd99c7f0249a6c7b6eacbf74c9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 907782
Reviewed-on: https://chromium-review.googlesource.com/c/1404540
Reviewed-by: Patti <patricialor@chromium.org>
Commit-Queue: Patti <patricialor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621437}
[modify] https://crrev.com/51085ff0b1f0c0563b9b0a00e885ff89e04d274b/chrome/browser/metrics/ukm_browsertest.cc
[rename] https://crrev.com/51085ff0b1f0c0563b9b0a00e885ff89e04d274b/chrome/browser/sync/test/integration/secondary_account_helper.cc
[add] https://crrev.com/51085ff0b1f0c0563b9b0a00e885ff89e04d274b/chrome/browser/sync/test/integration/secondary_account_helper.h
[delete] https://crrev.com/235f070f47771c62dcb0dd12974dcef075440a99/chrome/browser/sync/test/integration/secondary_account_sync_test.h
[modify] https://crrev.com/51085ff0b1f0c0563b9b0a00e885ff89e04d274b/chrome/browser/sync/test/integration/single_client_secondary_account_sync_test.cc
[modify] https://crrev.com/51085ff0b1f0c0563b9b0a00e885ff89e04d274b/chrome/browser/sync/test/integration/single_client_wallet_sync_test.cc
[modify] https://crrev.com/51085ff0b1f0c0563b9b0a00e885ff89e04d274b/chrome/browser/sync/test/integration/sync_test.h
[modify] https://crrev.com/51085ff0b1f0c0563b9b0a00e885ff89e04d274b/chrome/test/BUILD.gn
[modify] https://crrev.com/51085ff0b1f0c0563b9b0a00e885ff89e04d274b/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/51085ff0b1f0c0563b9b0a00e885ff89e04d274b/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/51085ff0b1f0c0563b9b0a00e885ff89e04d274b/services/identity/public/cpp/identity_test_utils.cc
[modify] https://crrev.com/51085ff0b1f0c0563b9b0a00e885ff89e04d274b/services/identity/public/cpp/identity_test_utils.h

Project Member

Comment 15 by bugdroid1@chromium.org, Jan 16 (6 days ago)

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

commit 9b4e49741e91a48a3a3eccd25c100354de3c5f07
Author: Lowell Manners <lowell@chromium.org>
Date: Wed Jan 16 10:31:18 2019

Add a factory method to create a FakeGCMS with a TestURLLoaderFactory.

This new factory method is meant to replace
BuildFakeGaiaCookieManagerServiceWithOptions in the cases where
create_fake_url_loader_factory_for_cookie_requests is set to true.

Callers of BuildFakeGaiaCookieManagerServiceWithOptions will be migrated
to use this new factory method in a future CL.

This is part of step 1a of the plan to eliminate FakeGCMS entirely:

https: //docs.google.com/document/d/1t0ZtuV7h-znzdItFgBW0aKPscAwWXIBuNZnNlEGgi7g/edit
Change-Id: Ia26dbe429743e30b86130106d3909a3320e4ba2d
Bug: 907782
Reviewed-on: https://chromium-review.googlesource.com/c/1412456
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Lowell Manners <lowell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623180}
[modify] https://crrev.com/9b4e49741e91a48a3a3eccd25c100354de3c5f07/chrome/browser/signin/fake_gaia_cookie_manager_service_builder.cc
[modify] https://crrev.com/9b4e49741e91a48a3a3eccd25c100354de3c5f07/chrome/browser/signin/fake_gaia_cookie_manager_service_builder.h

Project Member

Comment 16 by bugdroid1@chromium.org, Jan 16 (6 days ago)

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

commit 49be60bf098342ba3242522312d02cdd2ed1e149
Author: Lowell Manners <lowell@chromium.org>
Date: Wed Jan 16 10:49:39 2019

Make secondary account sync tests share a single TestURLLoaderFactory.

Before this change, these tests had multiple TestURLLoaderFactory
instances. One, which is declared in SyncTest, plus an additional
instance each time BuildFakeGaiaCookieManagerServiceWithOptions was
invoked.

Eventually, FakeGCMS will be eliminated entirely in these tests. This
change separates out one of the logical side effects of eliminating
FakeGCMS, which that there will be a single TestURLLoaderFactory
instance.

We are splitting out this change to be able to verify whether the switch
to a single shared factory was the cause of the flake in
https://chromium-review.googlesource.com/c/chromium/src/+/1396148. Our
suspicion is that the flake was rather caused by the fact that in that
CL, the TestURLLoaderFactory was *not* used by the main profile, whereas
prior to that CL FakeGCMS instances had been installed for all Profiles.

Assuming that this change sticks, we will follow it up by hiding the
usage of |test_url_loader_factory_| inside a new SecondarySyncTest
class, thus eliminating the passing of the raw pointer.

This is part of step 1a of the plan to eliminate FakeGCMS entirely:
TBR=rkaplow@chromium.org

https: //docs.google.com/document/d/1t0ZtuV7h-znzdItFgBW0aKPscAwWXIBuNZnNlEGgi7g/edit
Change-Id: I86837090af84e94bdd9887ac9ee5e9c96bbef8da
Bug: 907782
Reviewed-on: https://chromium-review.googlesource.com/c/1411877
Commit-Queue: Lowell Manners <lowell@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623188}
[modify] https://crrev.com/49be60bf098342ba3242522312d02cdd2ed1e149/chrome/browser/metrics/ukm_browsertest.cc
[modify] https://crrev.com/49be60bf098342ba3242522312d02cdd2ed1e149/chrome/browser/sync/test/integration/secondary_account_helper.cc
[modify] https://crrev.com/49be60bf098342ba3242522312d02cdd2ed1e149/chrome/browser/sync/test/integration/secondary_account_helper.h
[modify] https://crrev.com/49be60bf098342ba3242522312d02cdd2ed1e149/chrome/browser/sync/test/integration/single_client_secondary_account_sync_test.cc
[modify] https://crrev.com/49be60bf098342ba3242522312d02cdd2ed1e149/chrome/browser/sync/test/integration/single_client_wallet_sync_test.cc
[modify] https://crrev.com/49be60bf098342ba3242522312d02cdd2ed1e149/chrome/browser/sync/test/integration/sync_test.h

Project Member

Comment 17 by bugdroid1@chromium.org, Jan 18 (4 days ago)

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

commit 38edde83f309551ec623098ab65cfd25ca5058d4
Author: Lowell Manners <lowell@chromium.org>
Date: Fri Jan 18 12:09:21 2019

Update BuildFakeGaiaCookieManagerServiceWithOptions not to take a bool.

All existing callers of BuildFakeGaiaCookieManagerServiceWithOptions
already pass create_fake_url_loader_factory_for_cookie_requests=true, so
we can update them to instead pass a TestURLLoaderFactory that they own.

With this change, we also delete the deprecated constructor for
FakeGaiaCookieManagerService that accepted a boolean, since it has no
remaining callers.

This is part of step 1a of the plan to eliminate FakeGCMS entirely:
https://docs.google.com/document/d/1t0ZtuV7h-znzdItFgBW0aKPscAwWXIBuNZnNlEGgi7g/edit

TBR=thakis@chromium.org,michaeldo@chromium.org,sky@chromium.org,pkasting@chromium.org

Bug: 907782
Change-Id: I3538b94b1c869a8e4d9c7d07de5e3a1ff0986021
Reviewed-on: https://chromium-review.googlesource.com/c/1392943
Commit-Queue: Lowell Manners <lowell@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624080}
[modify] https://crrev.com/38edde83f309551ec623098ab65cfd25ca5058d4/chrome/browser/signin/fake_gaia_cookie_manager_service_builder.cc
[modify] https://crrev.com/38edde83f309551ec623098ab65cfd25ca5058d4/chrome/browser/signin/fake_gaia_cookie_manager_service_builder.h
[modify] https://crrev.com/38edde83f309551ec623098ab65cfd25ca5058d4/chrome/browser/supervised_user/child_accounts/child_account_service_unittest.cc
[modify] https://crrev.com/38edde83f309551ec623098ab65cfd25ca5058d4/chrome/browser/ui/cocoa/test/cocoa_profile_test.h
[modify] https://crrev.com/38edde83f309551ec623098ab65cfd25ca5058d4/chrome/browser/ui/cocoa/test/cocoa_profile_test.mm
[modify] https://crrev.com/38edde83f309551ec623098ab65cfd25ca5058d4/chrome/browser/ui/views/frame/test_with_browser_view.cc
[modify] https://crrev.com/38edde83f309551ec623098ab65cfd25ca5058d4/chrome/browser/ui/views/frame/test_with_browser_view.h
[modify] https://crrev.com/38edde83f309551ec623098ab65cfd25ca5058d4/chrome/test/base/browser_with_test_window_test.h
[modify] https://crrev.com/38edde83f309551ec623098ab65cfd25ca5058d4/components/signin/core/browser/fake_gaia_cookie_manager_service.cc
[modify] https://crrev.com/38edde83f309551ec623098ab65cfd25ca5058d4/components/signin/core/browser/fake_gaia_cookie_manager_service.h
[modify] https://crrev.com/38edde83f309551ec623098ab65cfd25ca5058d4/ios/web_view/internal/sync/cwv_sync_controller_unittest.mm

Project Member

Comment 18 by bugdroid1@chromium.org, Yesterday (45 hours ago)

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

commit 3508bcbf1fc3898af04575cffdcfd83ae852b5a6
Author: Lowell Manners <lowell@chromium.org>
Date: Mon Jan 21 10:25:09 2019

Change how GaiaCookieManagerService fetches its SharedURLLoaderFactory.

Before this CL, GaiaCookieManagerService would always get its
SharedURLLoaderFactory by calling signin_client_->GetURLLoaderFactory().
This made it difficult for unit tests to tell GaiaCookieManagerService
to use a TestURLLoaderFactory (for fake responses).

With this change, GaiaCookieManagerService constructor accepts a
callback (shared_url_loader_factory_getter) that allows the code which
constructed GaiaCookieManagerService to control what
SharedURLLoaderFactory will be used.

Note: This is implemented as a callback, because
signin_client_->GetURLLoaderFactory() has side effects, so we don't want
to change *when* it is called for the first time. This change should be
a pure refactor.

This is part of step 2 of the plan to eliminate FakeGCMS entirely:
https://docs.google.com/document/d/1t0ZtuV7h-znzdItFgBW0aKPscAwWXIBuNZnNlEGgi7g/edit

Bug: 907782
Change-Id: Ic8ea8d73e19713b382859436f131a7985f61dbbf
Reviewed-on: https://chromium-review.googlesource.com/c/1392346
Commit-Queue: Lowell Manners <lowell@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624545}
[modify] https://crrev.com/3508bcbf1fc3898af04575cffdcfd83ae852b5a6/components/signin/core/browser/fake_gaia_cookie_manager_service.cc
[modify] https://crrev.com/3508bcbf1fc3898af04575cffdcfd83ae852b5a6/components/signin/core/browser/fake_gaia_cookie_manager_service.h
[modify] https://crrev.com/3508bcbf1fc3898af04575cffdcfd83ae852b5a6/components/signin/core/browser/gaia_cookie_manager_service.cc
[modify] https://crrev.com/3508bcbf1fc3898af04575cffdcfd83ae852b5a6/components/signin/core/browser/gaia_cookie_manager_service.h
[modify] https://crrev.com/3508bcbf1fc3898af04575cffdcfd83ae852b5a6/components/signin/core/browser/gaia_cookie_manager_service_unittest.cc

Project Member

Comment 19 by bugdroid1@chromium.org, Today (22 hours ago)

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

commit 75ac4524813d10892e342ca52e61ae0c96c5b475
Author: Lowell Manners <lowell@chromium.org>
Date: Tue Jan 22 08:44:15 2019

Remove an unnecessary usage of FakeGaiaCookieManagerService.

signin_manager_unit_test.cc doesn't set any fake resonses, so it
doesn't need to use FakeGaiaCookieManagerService.

Bug: 907782
Change-Id: Ibd19cf1acc02fa23bc3e7de56b84cdca8127910e
Reviewed-on: https://chromium-review.googlesource.com/c/1402884
Reviewed-by: Colin Blundell (slow until Jan 24 due to travel) <blundell@chromium.org>
Commit-Queue: Lowell Manners <lowell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624746}
[modify] https://crrev.com/75ac4524813d10892e342ca52e61ae0c96c5b475/components/signin/core/browser/signin_manager_unittest.cc

Sign in to add a comment