sync: Remove SyncSourceInfo

sync: Remove SyncSourceInfo

The SyncSourceInfo was a struct that contained a GetUpdatesSource and a
ModelTypeInvalidationMap.  Both of these types are in the process of
being deprecated.  The SyncSourceInfo itself was used only for debugging
(about:sync), tests (mostly sync_scheduler_unittest.cc) and maintaining
compatibility with some old function signatures.

Removing the SyncSourceInfo allow us to remove dependencies on
ModelTypeInvalidationMap, which is a step towards enabling invalidation
"trickles" support.

BUG=233437
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215446

[rlarocque, 2013-07-24 19:58:45]

Strangely enough, this is related to the trickles work.  The
ModelTypeInvalidationMap is heading into obsolescence, and the SyncSourceInfo is
one of the last remaining uses of the type.

Please review.

[tim (not reviewing), 2013-08-01 17:16:38]

https://codereview.chromium.org/19982002/diff/14001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

https://codereview.chromium.org/19982002/diff/14001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_harness.cc:505:
snap.model_neutral_state().commit_request_types;
Am I correct that we only need commit_request_types for p2p / integration tests?
 Would this be interesting for about:sync?

https://codereview.chromium.org/19982002/diff/14001/sync/engine/sync_schedule...
File sync/engine/sync_scheduler_unittest.cc (right):

https://codereview.chromium.org/19982002/diff/14001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_unittest.cc:302: EXPECT_EQ(NORMAL,
records.sources[0]);
This expectation doesn't fill the shoes of the previous code. Previously, the
GetUpdatesCallerValue was set by scheduler / syncer code and we'd verify that;
now we're not really verifying anything at all besides our own test code (unless
I'm misunderstanding?).

For parity, one thing you could do is pre-bind an output GetUpdatesCallerInfo
parameter on SimulateNormalSuccess to a local variable, assign it in the test
utility, and verify *that*, to get the value that would have been sent to the
server.

https://codereview.chromium.org/19982002/diff/14001/sync/engine/syncer.h
File sync/engine/syncer.h (right):

https://codereview.chromium.org/19982002/diff/14001/sync/engine/syncer.h#newc...
sync/engine/syncer.h:51: sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source,
Can you comment the different *SyncShare functions and their parameters?  It's
more critical now that the signatures are different and we're specifying each
input individually.

https://codereview.chromium.org/19982002/diff/14001/sync/internal_api/public/...
File sync/internal_api/public/sessions/sync_session_snapshot.h (right):

https://codereview.chromium.org/19982002/diff/14001/sync/internal_api/public/...
sync/internal_api/public/sessions/sync_session_snapshot.h:45:
SyncSessionSnapshot(
Style guide forbids overloading functions (constructors included).  You should
make one of these a factory function (or both with a single private ctor).

https://codereview.chromium.org/19982002/diff/14001/sync/internal_api/public/...
sync/internal_api/public/sessions/sync_session_snapshot.h:78:
sync_pb::GetUpdatesCallerInfo::GetUpdatesSource legacy_updates_source() const;
Comment what this is / why it is considered "legacy".

https://codereview.chromium.org/19982002/diff/14001/sync/sessions/sync_session.h
File sync/sessions/sync_session.h (right):

https://codereview.chromium.org/19982002/diff/14001/sync/sessions/sync_sessio...
sync/sessions/sync_session.h:112: SyncSessionSnapshot TakeSnapshot(
Style guide forbids overloading functions.  You'll need to rename this something
like TakeLegacySnapshot or something.

https://codereview.chromium.org/19982002/diff/14001/sync/sessions/sync_sessio...
sync/sessions/sync_session.h:118:
sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source);
nit- indent.

https://codereview.chromium.org/19982002/diff/14001/sync/sessions/sync_sessio...
sync/sessions/sync_session.h:133: Delegate* delegate);
does this fit on line above?

[rlarocque, 2013-08-01 21:01:41]

Patch updated.  PTAL.

https://codereview.chromium.org/19982002/diff/14001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

https://codereview.chromium.org/19982002/diff/14001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_harness.cc:505:
snap.model_neutral_state().commit_request_types;
On 2013/08/01 17:16:38, timsteele wrote:
> Am I correct that we only need commit_request_types for p2p / integration
tests?

Yep, it's only for tests.  I wish I could have avoided it, but I didn't see any
better way to keep the tests running.

>  Would this be interesting for about:sync?  

The information might be useful, but I'm not sure where I'd put it.  If we ever
get around to replacing the counters with a concise event log, we might be able
to use it then.

https://codereview.chromium.org/19982002/diff/14001/sync/engine/sync_schedule...
File sync/engine/sync_scheduler_unittest.cc (right):

https://codereview.chromium.org/19982002/diff/14001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_unittest.cc:302: EXPECT_EQ(NORMAL,
records.sources[0]);
On 2013/08/01 17:16:38, timsteele wrote:
> This expectation doesn't fill the shoes of the previous code. Previously, the
> GetUpdatesCallerValue was set by scheduler / syncer code and we'd verify that;
> now we're not really verifying anything at all besides our own test code
(unless
> I'm misunderstanding?).
> 
> For parity, one thing you could do is pre-bind an output GetUpdatesCallerInfo
> parameter on SimulateNormalSuccess to a local variable, assign it in the test
> utility, and verify *that*, to get the value that would have been sent to the
> server.

You're right that this code tests less than it used to.

However, I don't think that checking the GetUpdatesCallerInfo value is as
important as it used to be.  The mocks can now assert that the scheduler calls
NormalSyncShare() and would fail if the scheduler decided to run a configure
instead.

Verifying the recorded cycles is still valuable in cases where the mock can't
make the sort of assertions we're looking for, as in the BackoffElevation test. 
You're right that it doesn't accomplish much in this test, though.

I think what we've lost here is the ability to detect errors where the
GetUpdatesCallerInfo is set wrong but it's still within the same category (ie.
NOTIFICATION instead of LOCAL), or where the SyncSource types are set wrong. 
The NudgeTracker unit tests should do a good job of detecting these kinds of
issues, so this CL shouldn't cost us too much coverage.

https://codereview.chromium.org/19982002/diff/14001/sync/engine/syncer.h
File sync/engine/syncer.h (right):

https://codereview.chromium.org/19982002/diff/14001/sync/engine/syncer.h#newc...
sync/engine/syncer.h:51: sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source,
On 2013/08/01 17:16:38, timsteele wrote:
> Can you comment the different *SyncShare functions and their parameters?  It's
> more critical now that the signatures are different and we're specifying each
> input individually.

Done.

https://codereview.chromium.org/19982002/diff/14001/sync/internal_api/public/...
File sync/internal_api/public/sessions/sync_session_snapshot.h (right):

https://codereview.chromium.org/19982002/diff/14001/sync/internal_api/public/...
sync/internal_api/public/sessions/sync_session_snapshot.h:45:
SyncSessionSnapshot(
On 2013/08/01 17:16:38, timsteele wrote:
> Style guide forbids overloading functions (constructors included).  You should
> make one of these a factory function (or both with a single private ctor).

Apparently the second constructor is only used in tests.  I removed the second
constructor and updated the two invocations of it to use the first.

https://codereview.chromium.org/19982002/diff/14001/sync/internal_api/public/...
sync/internal_api/public/sessions/sync_session_snapshot.h:78:
sync_pb::GetUpdatesCallerInfo::GetUpdatesSource legacy_updates_source() const;
On 2013/08/01 17:16:38, timsteele wrote:
> Comment what this is / why it is considered "legacy".

Done.

https://codereview.chromium.org/19982002/diff/14001/sync/sessions/sync_session.h
File sync/sessions/sync_session.h (right):

https://codereview.chromium.org/19982002/diff/14001/sync/sessions/sync_sessio...
sync/sessions/sync_session.h:112: SyncSessionSnapshot TakeSnapshot(
On 2013/08/01 17:16:38, timsteele wrote:
> Style guide forbids overloading functions.  You'll need to rename this
something
> like TakeLegacySnapshot or something.

I renamed the second to TakeSnapshotWithSource.

https://codereview.chromium.org/19982002/diff/14001/sync/sessions/sync_sessio...
sync/sessions/sync_session.h:118:
sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source);
On 2013/08/01 17:16:38, timsteele wrote:
> nit- indent.

Done.

https://codereview.chromium.org/19982002/diff/14001/sync/sessions/sync_sessio...
sync/sessions/sync_session.h:133: Delegate* delegate);
On 2013/08/01 17:16:38, timsteele wrote:
> does this fit on line above?

Done.

[rlarocque, 2013-08-02 21:37:28]

It turns out the SyncSchedulerTests didn't actually need to assert on the
"sources" field, though I had to rearrange some of the mocks to avoid losing
coverage.  I also converted the tests to use a strict mock so they should fail
if we accidentally call a SyncShare function too often or at the wrong time.

PTAL.

[tim (not reviewing), 2013-08-02 22:49:38]

lgtm

https://codereview.chromium.org/19982002/diff/27001/sync/engine/sync_schedule...
File sync/engine/sync_scheduler_unittest.cc (right):

https://codereview.chromium.org/19982002/diff/27001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_unittest.cc:52: std::vector<TimeTicks> times;
This could just be made a typedef.

[commit-bot: I haz the power, 2013-08-03 00:30:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/19982002/33001

[commit-bot: I haz the power, 2013-08-03 01:41:47]

Change committed as 215446