Issue 1486363002: USS: Introduce dependency of SharedModelTypeProcessor on ModelTypeService

Issue 1486363002: USS: Introduce dependency of SharedModelTypeProcessor on ModelTypeService (Closed)

Created:
5 years ago by stanisc

Modified:
5 years ago

Reviewers:
maxbogue, Gang Wu, skym, pavely

CC:
chromium-reviews, tim+watch_chromium.org, pvalenzuela+watch_chromium.org, maxbogue+watch_chromium.org, plaree+watch_chromium.org, zea+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

USS: Introduce dependency of SharedModelTypeProcessor on ModelTypeService BUG=561821 Committed: https://crrev.com/c490a4d2c3b423494f54d169c538d641af4cca64 Cr-Commit-Position: refs/heads/master@{#363066}

Patch Set 1 #

Patch Set 2 : Fixed unit tests #

Total comments: 8

Patch Set 3 : Addressed CR feedback #

Created: 5 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+133 lines, -39 lines)			Patch
M	components/sync_driver/non_blocking_data_type_controller_unittest.cc	View	1 2	3 chunks	+5 lines, -3 lines	0 comments	Download
M	sync/BUILD.gn	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	sync/api/model_type_service.h	View	1 2	1 chunk	+1 line, -4 lines	0 comments	Download
M	sync/internal_api/public/shared_model_type_processor.h	View		4 chunks	+6 lines, -7 lines	0 comments	Download
A	sync/internal_api/public/test/fake_model_type_service.h	View		1 chunk	+43 lines, -0 lines	0 comments	Download
M	sync/internal_api/shared_model_type_processor.cc	View		2 chunks	+8 lines, -7 lines	0 comments	Download
M	sync/internal_api/shared_model_type_processor_unittest.cc	View		5 chunks	+13 lines, -4 lines	0 comments	Download
M	sync/internal_api/sync_context_proxy_impl_unittest.cc	View	1	3 chunks	+3 lines, -3 lines	0 comments	Download
A	sync/internal_api/test/fake_model_type_service.cc	View		1 chunk	+41 lines, -0 lines	0 comments	Download
M	sync/sessions/model_type_registry_unittest.cc	View	1	3 chunks	+9 lines, -11 lines	0 comments	Download
M	sync/sync_tests.gypi	View		1 chunk	+2 lines, -0 lines	0 comments	Download

Messages

Total messages: 13 (4 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

skym

https://codereview.chromium.org/1486363002/diff/40001/components/sync_driver/non_blocking_data_type_controller_unittest.cc File components/sync_driver/non_blocking_data_type_controller_unittest.cc (right): https://codereview.chromium.org/1486363002/diff/40001/components/sync_driver/non_blocking_data_type_controller_unittest.cc#newcode165 components/sync_driver/non_blocking_data_type_controller_unittest.cc:165: class MockService : public syncer_v2::ModelTypeService { This looks the ...

5 years ago (2015-12-02 19:23:15 UTC) #4

stanisc

PTAL! https://codereview.chromium.org/1486363002/diff/40001/components/sync_driver/non_blocking_data_type_controller_unittest.cc File components/sync_driver/non_blocking_data_type_controller_unittest.cc (right): https://codereview.chromium.org/1486363002/diff/40001/components/sync_driver/non_blocking_data_type_controller_unittest.cc#newcode165 components/sync_driver/non_blocking_data_type_controller_unittest.cc:165: class MockService : public syncer_v2::ModelTypeService { On 2015/12/02 ...

5 years ago (2015-12-03 01:41:45 UTC) #5

PTAL!

https://codereview.chromium.org/1486363002/diff/40001/components/sync_driver/...
File components/sync_driver/non_blocking_data_type_controller_unittest.cc
(right):

https://codereview.chromium.org/1486363002/diff/40001/components/sync_driver/...
components/sync_driver/non_blocking_data_type_controller_unittest.cc:165: class
MockService : public syncer_v2::ModelTypeService {
On 2015/12/02 19:23:15, skym wrote:
> This looks the exact same as FakeModelTypeService. Can you reuse it here?

Yeah. For some reason I thought I couldn't share the class across two unit test
binaries. In fact reusing FakeModelTypeService isn't a problem. Fixed this.

https://codereview.chromium.org/1486363002/diff/40001/components/sync_driver/...
components/sync_driver/non_blocking_data_type_controller_unittest.cc:230: //
SyncClient.
On 2015/12/02 19:23:15, skym wrote:
> Nit: do you really need this new line? I think the following is only 76
> characters:
> 
> ....// TODO(stanisc): Controller should discover the service via SyncClient.

Done.

https://codereview.chromium.org/1486363002/diff/40001/sync/api/model_type_ser...
File sync/api/model_type_service.h (right):

https://codereview.chromium.org/1486363002/diff/40001/sync/api/model_type_ser...
sync/api/model_type_service.h:16: #include "sync/api/sync_error.h"
On 2015/12/02 19:23:15, skym wrote:
> What was the reasoning for needing to include this instead of forward declare?

I did this to work around some layering dependency restrictions.
Since SyncError is a part of API it can't be included into most parts of the
code. The only exception is anything under internal_api/public.
To solve this I could include sync_error.h into fake_model_type_service.h but it
seemed reasonable to just include it here instead. All files that include
model_type_service.h will very likely have to have SyncError definition as well.

https://codereview.chromium.org/1486363002/diff/40001/sync/api/model_type_ser...
sync/api/model_type_service.h:20: class SyncError;
On 2015/12/02 19:23:15, skym wrote:
> If you're adding the #include, remove forward declaration?

Done.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1486363002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1486363002/60001

5 years ago (2015-12-03 21:14:55 UTC) #10

commit-bot: I haz the power

5 years ago (2015-12-03 21:40:27 UTC) #13

Message was sent while issue was closed.

Patchset 3 (id:??) landed as
https://crrev.com/c490a4d2c3b423494f54d169c538d641af4cca64
Cr-Commit-Position: refs/heads/master@{#363066}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages