New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 910518 link

Starred by 1 user
Status: Started
Owner:
treib@chromium.org
Cc:
mastiz@chromium.org
ios-bugs@chromium.org
█ tschumann@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac
Pri: 3
Type: Task

Blocking:
issue 884159



Sign in to add a comment

ProfileSyncServiceMock is bad

Project Member Reported by treib@chromium.org, Nov 30 
ProfileSyncServiceMock inherits from ProfileSyncService, i.e. from the concrete implementation, and then overrides *some* of its methods with mocks. The result is a partial implementation that's just a mess, and makes refactorings like issue 884159 more difficult.

I'd say the root of the problem is that ProfileSyncServiceFactory should really be just SyncServiceFactory, and return a pointer to the SyncService interface rather than to the concrete ProfileSyncService implementation. As it is, many tests have no choice but to use ProfileSyncServiceMock (or something else derived from ProfileSyncService ), even when they just want a simple FakeSyncService that returns some particular values.
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 3 

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

commit bc505b9f74b3a4aef054577dda93a0117233bdfa
Author: Marc Treib <treib@chromium.org>
Date: Mon Dec 03 12:44:07 2018

ProfileSyncServiceMock cleanups

- Reorder all overrides to be grouped by interface, and match the
  interface's ordering.
- Remove a second ctor that isn't necessary anymore.
- Remove some unnecessary includes.

Bug: 910518
Change-Id: I4123e4ce9cd8eeac4fd2cef051e094cccc8c7d14
Reviewed-on: https://chromium-review.googlesource.com/c/1356702
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613059}
[modify] https://crrev.com/bc505b9f74b3a4aef054577dda93a0117233bdfa/chrome/browser/sync/sync_ui_util_unittest.cc
[modify] https://crrev.com/bc505b9f74b3a4aef054577dda93a0117233bdfa/components/browser_sync/profile_sync_service_mock.cc
[modify] https://crrev.com/bc505b9f74b3a4aef054577dda93a0117233bdfa/components/browser_sync/profile_sync_service_mock.h
[modify] https://crrev.com/bc505b9f74b3a4aef054577dda93a0117233bdfa/ios/chrome/browser/ui/settings/passphrase_collection_view_controller_test.mm
[modify] https://crrev.com/bc505b9f74b3a4aef054577dda93a0117233bdfa/ios/chrome/browser/ui/settings/sync_encryption_table_view_controller_unittest.mm
[modify] https://crrev.com/bc505b9f74b3a4aef054577dda93a0117233bdfa/ios/chrome/browser/ui/settings/sync_settings_collection_view_controller_unittest.mm
[modify] https://crrev.com/bc505b9f74b3a4aef054577dda93a0117233bdfa/ios/web_view/internal/sync/cwv_sync_controller_unittest.mm
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 5 

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

commit 41b306b38a7e8b5f28add0dfb5670190a3c05862
Author: Marc Treib <treib@chromium.org>
Date: Wed Dec 05 09:28:25 2018

sync_ui_util: Use SyncService rather than ProfileSyncService

Generally, everything should use the interface (SyncService) rather than
the concrete implementation (ProfileSyncService). In particular, this
will make it easier to write tests for this code, since they can now use
TestSyncService instead of ProfileSyncServiceMock.

Bug: 884159, 910518
Change-Id: I794e0af45b4fc576536265cbc049358d37196cbd
Reviewed-on: https://chromium-review.googlesource.com/c/1357087
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613916}
[modify] https://crrev.com/41b306b38a7e8b5f28add0dfb5670190a3c05862/chrome/browser/browsing_data/counters/browsing_data_counter_utils.cc
[modify] https://crrev.com/41b306b38a7e8b5f28add0dfb5670190a3c05862/chrome/browser/extensions/api/browsing_data/browsing_data_api.cc
[modify] https://crrev.com/41b306b38a7e8b5f28add0dfb5670190a3c05862/chrome/browser/sync/sync_ui_util.cc
[modify] https://crrev.com/41b306b38a7e8b5f28add0dfb5670190a3c05862/chrome/browser/sync/sync_ui_util.h
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 5 

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

commit d3b0f265311997fa2f264cf3c9054e69d85b7832
Author: Marc Treib <treib@chromium.org>
Date: Wed Dec 05 15:10:42 2018

Rewrite sync_ui_util unittests in terms of TestSyncService

instead of ProfileSyncServiceMock (which wasn't used as a mock)

Bug: 910518
Change-Id: I5aeab4ffa4c399d5d411e09d88b9b26e0db974cf
Reviewed-on: https://chromium-review.googlesource.com/c/1358493
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613969}
[modify] https://crrev.com/d3b0f265311997fa2f264cf3c9054e69d85b7832/chrome/browser/sync/sync_ui_util_unittest.cc
[modify] https://crrev.com/d3b0f265311997fa2f264cf3c9054e69d85b7832/components/sync/driver/test_sync_service.cc
[modify] https://crrev.com/d3b0f265311997fa2f264cf3c9054e69d85b7832/components/sync/driver/test_sync_service.h
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 7 

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

commit 49f80a482f5f426a03c09bc262dd9d645a008223
Author: Marc Treib <treib@chromium.org>
Date: Fri Dec 07 12:26:39 2018

Sync: Expose GetRegistered/ForcedDataTypes in SyncService

Previously, GetRegisteredDataTypes and GetForcedDataTypes were only
exposed from the (concrete) ProfileSyncService, not from the (interface)
SyncService, as opposed to GetPreferredDataTypes and GetActiveDataTypes.
It seems right and sensible to expose the registered and forced data
types too.
Concretely, this will allow us to write more code (e.g. PeopleHandler)
in terms of SyncService rather than ProfileSyncService.

Bug: 884159, 910518
Change-Id: I1d9176d4d2ced1dc5defda1b85b6ced2fa51afba
Reviewed-on: https://chromium-review.googlesource.com/c/1365594
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614676}
[modify] https://crrev.com/49f80a482f5f426a03c09bc262dd9d645a008223/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/49f80a482f5f426a03c09bc262dd9d645a008223/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/49f80a482f5f426a03c09bc262dd9d645a008223/components/sync/driver/fake_sync_service.cc
[modify] https://crrev.com/49f80a482f5f426a03c09bc262dd9d645a008223/components/sync/driver/fake_sync_service.h
[modify] https://crrev.com/49f80a482f5f426a03c09bc262dd9d645a008223/components/sync/driver/sync_service.h
[modify] https://crrev.com/49f80a482f5f426a03c09bc262dd9d645a008223/components/sync/driver/test_sync_service.cc
[modify] https://crrev.com/49f80a482f5f426a03c09bc262dd9d645a008223/components/sync/driver/test_sync_service.h

Comment 5 by treib@google.com, Dec 7

Blocking: 884159
Project Member

Comment 6 by bugdroid1@chromium.org, Dec 10 

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

commit a9175b104b6b66650a614e62419da47bb9f2977d
Author: Marc Treib <treib@chromium.org>
Date: Mon Dec 10 09:46:09 2018

PeopleHandler: Use base SyncService instead of ProfileSyncService

Ideally, all clients should use the base interface instead of the
concrete implementation. As one benefit, that will make testing much
easier.

Bug: 910518
Change-Id: I2231a592dd7c21d48db6ee7b601f8a605a57f8a7
Reviewed-on: https://chromium-review.googlesource.com/c/1366292
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615070}
[modify] https://crrev.com/a9175b104b6b66650a614e62419da47bb9f2977d/chrome/browser/ui/webui/settings/people_handler.cc
[modify] https://crrev.com/a9175b104b6b66650a614e62419da47bb9f2977d/chrome/browser/ui/webui/settings/people_handler.h
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 13 

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

commit b80dded50e658266e4d718a00fd147e1212dbd45
Author: Marc Treib <treib@chromium.org>
Date: Thu Dec 13 09:54:06 2018

Simplify SyncStartupTracker: don't use Profile (just SyncService)

Previously, SyncStartupTracker checked Profile::IsSyncAllowed before
getting a SyncService from the factory, presumably to avoid creating it
in case it doesn't exist yet. However, in all places where it's
constructed, we do already have a SyncService around.
So this CL changes SyncStartupTracker to just take a SyncService. This
generally makes things simpler, and it'll ease testing since we can
now use TestSyncService instead of ProfileSyncServiceMock (and don't
have to go through the factory, but can instead just inject the
dependency directly).

Bug: 910518
Change-Id: Iae3d92f7605d42b97d51dcff3afc9dcc29666cad
Reviewed-on: https://chromium-review.googlesource.com/c/1367654
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616254}
[modify] https://crrev.com/b80dded50e658266e4d718a00fd147e1212dbd45/chrome/browser/sync/sync_startup_tracker.cc
[modify] https://crrev.com/b80dded50e658266e4d718a00fd147e1212dbd45/chrome/browser/sync/sync_startup_tracker.h
[modify] https://crrev.com/b80dded50e658266e4d718a00fd147e1212dbd45/chrome/browser/sync/sync_startup_tracker_unittest.cc
[modify] https://crrev.com/b80dded50e658266e4d718a00fd147e1212dbd45/chrome/browser/ui/webui/settings/people_handler.cc
[modify] https://crrev.com/b80dded50e658266e4d718a00fd147e1212dbd45/chrome/browser/ui/webui/signin/dice_turn_sync_on_helper.cc

Comment 8 by treib@chromium.org, Dec 13

Cc: -treib@chromium.org
Owner: treib@chromium.org
Status: Started (was: Available)
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 13 

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

commit b6553d262c79312ae8a052dceda3713144233171
Author: Marc Treib <treib@chromium.org>
Date: Thu Dec 13 16:04:06 2018

Prefer using SyncService (the interface) over ProfileSyncService (the impl)

It's better to use the interface rather than the concrete implementation.
For one, this will make testing easier.

This CL trivially switches a bunch of uses of ProfileSyncService over to
SyncService. There are still many left.

Bug: 910518
Change-Id: I6890db739b3333c056c6b6631d2074a8fd3f2efe
Reviewed-on: https://chromium-review.googlesource.com/c/1375718
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616325}
[modify] https://crrev.com/b6553d262c79312ae8a052dceda3713144233171/chrome/browser/profiles/profile.cc
[modify] https://crrev.com/b6553d262c79312ae8a052dceda3713144233171/chrome/browser/profiles/profile_manager.cc
[modify] https://crrev.com/b6553d262c79312ae8a052dceda3713144233171/chrome/browser/search/suggestions/suggestions_service_factory.cc
[modify] https://crrev.com/b6553d262c79312ae8a052dceda3713144233171/chrome/browser/sync/glue/sync_start_util.cc
[modify] https://crrev.com/b6553d262c79312ae8a052dceda3713144233171/chrome/browser/sync/glue/sync_start_util.h
[modify] https://crrev.com/b6553d262c79312ae8a052dceda3713144233171/chrome/browser/sync/sync_error_notifier_ash.cc
[modify] https://crrev.com/b6553d262c79312ae8a052dceda3713144233171/chrome/browser/sync/sync_error_notifier_ash.h
[modify] https://crrev.com/b6553d262c79312ae8a052dceda3713144233171/chrome/browser/sync/sync_error_notifier_factory_ash.cc
[modify] https://crrev.com/b6553d262c79312ae8a052dceda3713144233171/chrome/browser/ui/avatar_button_error_controller.cc
[modify] https://crrev.com/b6553d262c79312ae8a052dceda3713144233171/chrome/browser/ui/search/search_tab_helper.cc
[modify] https://crrev.com/b6553d262c79312ae8a052dceda3713144233171/chrome/browser/ui/sync/one_click_signin_sync_observer.cc
[modify] https://crrev.com/b6553d262c79312ae8a052dceda3713144233171/chrome/browser/ui/sync/one_click_signin_sync_observer.h
[modify] https://crrev.com/b6553d262c79312ae8a052dceda3713144233171/chrome/browser/ui/sync/one_click_signin_sync_starter.cc
[modify] https://crrev.com/b6553d262c79312ae8a052dceda3713144233171/chrome/browser/ui/sync/one_click_signin_sync_starter.h
[modify] https://crrev.com/b6553d262c79312ae8a052dceda3713144233171/chrome/browser/ui/webui/signin/dice_turn_sync_on_helper.cc
[modify] https://crrev.com/b6553d262c79312ae8a052dceda3713144233171/chrome/browser/ui/webui/signin/dice_turn_sync_on_helper.h
[modify] https://crrev.com/b6553d262c79312ae8a052dceda3713144233171/chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc
►

Sign in to add a comment