Issue 451743002: Pass args to SyncManager::Init via a struct.

Issue 451743002: Pass args to SyncManager::Init via a struct. (Closed)

Created:
6 years, 4 months ago by maniscalco

Modified:
6 years, 4 months ago

Reviewers:
tim (not reviewing)

CC:
chromium-reviews, tim+watch_chromium.org, haitaol+watch_chromium.org, zea+watch_chromium.org, maniscalco+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Project:
chromium

Visibility:
Public.

More Reviews

Description

Refactor SyncManager::Init to receive args via a struct. SyncManager::Init is implemented in several places, takes a whole lot of args, and in the near future is going to get a few more. In some cases, many of the args are optional and it is tedious to pass a bunch of NULLs or empty strings. Use a struct instead of an arg list. BUG= Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288280

Patch Set 1 #

Patch Set 2 : Improve comments for SyncManager::Init's args. #

Patch Set 3 : Remove unnecessary forward declaration. #

Patch Set 4 : Add SYNC_EXPORT tag to InitArgs. #

Total comments: 4

Patch Set 5 : Use scoped_ptr instead of raw pointer. #

Created: 6 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+217 lines, -358 lines)			Patch
M	chrome/browser/sync/glue/sync_backend_host_core.cc	View	1 2 3 4	1 chunk	+23 lines, -16 lines	0 comments	Download
M	sync/internal_api/public/sync_manager.h	View	1 2 3 4	3 chunks	+57 lines, -43 lines	0 comments	Download
M	sync/internal_api/public/sync_manager.cc	View	1 2 3 4	1 chunk	+9 lines, -0 lines	0 comments	Download
M	sync/internal_api/public/test/fake_sync_manager.h	View		1 chunk	+1 line, -17 lines	0 comments	Download
M	sync/internal_api/sync_backup_manager.h	View		1 chunk	+1 line, -17 lines	0 comments	Download
M	sync/internal_api/sync_backup_manager.cc	View	1 2 3 4	1 chunk	+5 lines, -21 lines	0 comments	Download
M	sync/internal_api/sync_backup_manager_unittest.cc	View	1 2 3 4	1 chunk	+9 lines, -18 lines	0 comments	Download
M	sync/internal_api/sync_manager_impl.h	View		1 chunk	+1 line, -17 lines	0 comments	Download
M	sync/internal_api/sync_manager_impl.cc	View	1 2 3 4	7 chunks	+44 lines, -55 lines	0 comments	Download
M	sync/internal_api/sync_manager_impl_unittest.cc	View	1 2 3 4	1 chunk	+15 lines, -19 lines	0 comments	Download
M	sync/internal_api/sync_rollback_manager.h	View		1 chunk	+1 line, -17 lines	0 comments	Download
M	sync/internal_api/sync_rollback_manager.cc	View	1 2 3 4	1 chunk	+10 lines, -26 lines	0 comments	Download
M	sync/internal_api/sync_rollback_manager_base_unittest.cc	View	1 2 3 4	1 chunk	+12 lines, -39 lines	0 comments	Download
M	sync/internal_api/sync_rollback_manager_unittest.cc	View	1 2 3 4	1 chunk	+8 lines, -19 lines	0 comments	Download
M	sync/internal_api/test/fake_sync_manager.cc	View		1 chunk	+1 line, -17 lines	0 comments	Download
M	sync/tools/sync_client.cc	View	1 2 3 4	1 chunk	+20 lines, -17 lines	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

maniscalco

Tim, this is the refactoring change I mentioned earlier today. PTAL.

6 years, 4 months ago (2014-08-07 17:49:07 UTC) #1

tim (not reviewing)

LGTM https://codereview.chromium.org/451743002/diff/80001/sync/internal_api/public/sync_manager.h File sync/internal_api/public/sync_manager.h (right): https://codereview.chromium.org/451743002/diff/80001/sync/internal_api/public/sync_manager.h#newcode240 sync/internal_api/public/sync_manager.h:240: // TODO(akalin): Replace post_factory parameter with a URLFetcher. ...

6 years, 4 months ago (2014-08-07 20:10:15 UTC) #2

maniscalco

Thanks for the speedy review. https://codereview.chromium.org/451743002/diff/80001/sync/internal_api/public/sync_manager.h File sync/internal_api/public/sync_manager.h (right): https://codereview.chromium.org/451743002/diff/80001/sync/internal_api/public/sync_manager.h#newcode240 sync/internal_api/public/sync_manager.h:240: // TODO(akalin): Replace post_factory ...

6 years, 4 months ago (2014-08-07 20:55:08 UTC) #3

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maniscalco@chromium.org/451743002/100001

6 years, 4 months ago (2014-08-07 23:01:57 UTC) #5

Message was sent while issue was closed.

Change committed as 288280

Expand Messages | Collapse Messages