sync: beginnings of MessageLoop based SyncerThread

sync: beginnings of MessageLoop based SyncerThread

This is just a checkpoint.  It's not wired up to anything yet, and not fully implemented (note several NOTIMPLEMENTED() checks).

For design and overview, see https://docs.google.com/document/d/1bFqqtpA7TZuwtyEqlSxgkCCRrHvYRnnH6HlHou2LwVo/edit

BUG=26339
TEST=sync_unit_tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72663

[tim (not reviewing), 2010-12-21 21:20:04]

As you can see, I'm in the process of writing tests for the limited
functionality added in this patch.  Will keep working on it on and off over the
next week or so, but probably won't be all done until after the break.  So
consider this just food for thought for now :)

[Nicolas Zea, 2010-12-23 21:49:20]

Some early, higher level comments.

http://codereview.chromium.org/5939006/diff/8001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_thread2.cc (right):

http://codereview.chromium.org/5939006/diff/8001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_thread2.cc:70: if (!wait_interval_->had_nudge)
Why don't we back off if we haven't had a nudge?

http://codereview.chromium.org/5939006/diff/8001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_thread2.cc:80: // Mode / purpose contract.
An explanation of when the contract might be violated and hence return false
would be useful here.

http://codereview.chromium.org/5939006/diff/8001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_thread2.h (right):

http://codereview.chromium.org/5939006/diff/8001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_thread2.h:94: THROTTLED,
I'm currently working on a patch to add support for per-datatype throttling. In
that scenario, we may want multiple wait intervals, one for each datatype (my
current implementation stored a silenced_until_ value for each datatype). That
might be something to bear in mind in rewriting this.

http://codereview.chromium.org/5939006/diff/8001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_thread2.h:108: struct SyncSessionJob {
Would there be any benefit from differentiating between user side nudges and
server side nudges (ie from invalidations)?

[tim (not reviewing), 2010-12-24 04:23:47]

thanks for the feedback!

http://codereview.chromium.org/5939006/diff/8001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_thread2.cc (right):

http://codereview.chromium.org/5939006/diff/8001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_thread2.cc:70: if (!wait_interval_->had_nudge)
On 2010/12/23 21:49:20, nzea wrote:
> Why don't we back off if we haven't had a nudge?

the code allows one nudge per backoff interval (akin to clicking the 'Try Now'
button when your gTalk is complaining about not being able to connect, trying
again in XX seconds).  The current code is designed to do this, but the behavior
became broken as soon as we started coalescing nudges (somewhat recent).

http://codereview.chromium.org/5939006/diff/8001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_thread2.cc:80: // Mode / purpose contract.
On 2010/12/23 21:49:20, nzea wrote:
> An explanation of when the contract might be violated and hence return false
> would be useful here.

Ok.  I'll adapt the 'Mode' comment in the header and add something here.

http://codereview.chromium.org/5939006/diff/8001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_thread2.h (right):

http://codereview.chromium.org/5939006/diff/8001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_thread2.h:94: THROTTLED,
On 2010/12/23 21:49:20, nzea wrote:
> I'm currently working on a patch to add support for per-datatype throttling.
In
> that scenario, we may want multiple wait intervals, one for each datatype (my
> current implementation stored a silenced_until_ value for each datatype). That
> might be something to bear in mind in rewriting this.

Oh, good to know you're working on that.  I think one wait interval might
suffice but it all depends on what semantics we want with per-type throttling...
we should chat about this!

http://codereview.chromium.org/5939006/diff/8001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_thread2.h:108: struct SyncSessionJob {
On 2010/12/23 21:49:20, nzea wrote:
> Would there be any benefit from differentiating between user side nudges and
> server side nudges (ie from invalidations)?

This is actually stored in the session itself (the GetUpdatesCallerInfo source
param) if we want to know.  The job purpose is a higher level classification
that sort of matches up with the current 'Mode' of operation.

[tim (not reviewing), 2011-01-12 19:29:29]

Just getting back to this now, should have a new patch up with more tests by
EOD.

[akalin, 2011-01-13 00:36:34]

Couple of comments.  Still looking.

http://codereview.chromium.org/5939006/diff/8001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_thread2.h (right):

http://codereview.chromium.org/5939006/diff/8001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_thread2.h:27: class SyncerThread : public
base::RefCountedThreadSafe<SyncerThread>,
If SyncerThread owns its own thread, there's no reason for it to be refcounted.

http://codereview.chromium.org/5939006/diff/8001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/5939006/diff/8001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session.h:42: class SyncSession : public
base::RefCountedThreadSafe<SyncSession> {
Why does this have to be refcounted?

[tim (not reviewing), 2011-01-13 18:26:56]

http://codereview.chromium.org/5939006/diff/8001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_thread2.h (right):

http://codereview.chromium.org/5939006/diff/8001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_thread2.h:27: class SyncerThread : public
base::RefCountedThreadSafe<SyncerThread>,
On 2011/01/13 00:36:34, akalin wrote:
> If SyncerThread owns its own thread, there's no reason for it to be
refcounted.

Good point. Removed.

http://codereview.chromium.org/5939006/diff/8001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/5939006/diff/8001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session.h:42: class SyncSession : public
base::RefCountedThreadSafe<SyncSession> {
On 2011/01/13 00:36:34, akalin wrote:
> Why does this have to be refcounted?

I discussed this tradeoff in the design doc (linked in CL description).  I
prefer this over defining a new custom task and callback decl just to explicitly
declare ownership when NewRunnableMethod gives us that for free.  

Do you see another option not mentioned in the doc?

[akalin, 2011-01-13 21:49:48]

> I discussed this tradeoff in the design doc (linked in CL description).  I
> prefer this over defining a new custom task and callback decl just to
explicitly
> declare ownership when NewRunnableMethod gives us that for free.  
> 
> Do you see another option not mentioned in the doc?

What about making SyncSessionJob refcounted and making it hold SyncSession by
value?  That seems cleaner since it's the one that's being passed around and not
SyncSession directly.  Also, it doesn't need to be ThreadSafe right?  It looks
like all of the stuff happens on the worker thread.

[tim (not reviewing), 2011-01-20 00:08:41]

On 2011/01/13 21:49:48, akalin wrote:
> > I discussed this tradeoff in the design doc (linked in CL description).  I
> > prefer this over defining a new custom task and callback decl just to
> explicitly
> > declare ownership when NewRunnableMethod gives us that for free.  
> > 
> > Do you see another option not mentioned in the doc?
> 
> What about making SyncSessionJob refcounted and making it hold SyncSession by
> value?  That seems cleaner since it's the one that's being passed around and
not
> SyncSession directly.  Also, it doesn't need to be ThreadSafe right?  It looks
> like all of the stuff happens on the worker thread.

Removed the refcounting in favor of using linked_ptr.  Also added some tests,
more on the way.

[akalin, 2011-01-20 00:47:57]

LGTM, modulo nits and the rest of the tests!

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/mock_model_safe_workers.h (right):

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/mock_model_safe_workers.h:9: #include
"base/scoped_vector.h"
needed?

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread2.cc (right):

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:65: } else {
no need for else since the if returns.

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:68: switch (purpose) {
isn't this clearer as:

DCHECK(purpose == POLL || purpose == NUDGE);
if ((purpose == POLL) || wait_interval_->had_nudge) {
  return false;
}

Pretty sure that's equivalent to what you have.  Or maybe (purpose != NUDGE)

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:106:
GetUpdatesCallerInfo::GetUpdatesSource GetUpdatesFromNudgeSource(
anon namespace?

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:134: // Functor for std::find_if to
search by ModelSafeGroup.
anon namespace?

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:191: DCHECK(w_tmp.end() != it);
maybe:

if (it != w_tmp.end() {
  workers->push_back(*it):
} else {
  NOTREACHED();
}

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:197: DCHECK(w_tmp.end() != it);
here, too

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:420: bool
SyncerThread::IsSyncingCurrentlySilenced() {
const method?

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread2.h (right):

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.h:103: WaitInterval(Mode mode,
base::TimeDelta length)
move the body into the .cc file

[Nicolas Zea, 2011-01-20 20:01:56]

LGTM with nits (and a clarification question). Unfortunately, this is definitely
going to conflict with my nudge refactor, which will hopefully be landing soon
(everything related to nudges and syncsourceinfo gets touched).

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/nudge_source.h (right):

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/nudge_source.h:13: enum NudgeSource {
Some comments about what these correspond to might be useful.

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/nudge_source.h:24: #endif  //
CHROME_BROWSER_SYNC_ENGINE_NUDGE_SOURCE_H_
newline at end of file? (not sure if it shows it here or not)

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/polling_constants.cc (right):

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/polling_constants.cc:25: }  // namespace browser_sync
newline?

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/polling_constants.h (right):

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/polling_constants.h:11: namespace browser_sync {
extra newline after namespace? (or remove the newline before the close
namespace)

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/polling_constants.h:19: #endif  //
CHROME_BROWSER_SYNC_ENGINE_POLLING_CONSTANTS_H_
newline at end of file?

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread2.cc (right):

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:310: NUDGE_SOURCE_CONTINUATION,
Just for my understanding here, is this called when the nudge succeeded and
completed? Or if the nudge that we just performed hasn't completed yet, but made
forward progress? I'm a bit confused as the whether the normal (everything goes
well) code path will go through here, and if so why we maintain the nudge info.

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread2.h (right):

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.h:46: // We don't use a lock when
changing modes, so it modes won't cause currently
unnecessary "it"?

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.h:134: void DoSyncSessionJob(const
SyncSessionJob& job);
Might be more consistent if named SyncSessionJobImpl (following the convention
used by the other methods that are posted)?

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_types.h (right):

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_types.h:90: // TODO(tim): Deprecated.
Hurray, you're fixing my SyncerThreadWithSyncerTest.Pause bugs!

[tim (not reviewing), 2011-01-25 03:23:05]

Whew.  The tests were tricky but I think they look good now!
They uncovered a decent number of bugs in the code :) 
I had to add a DelayProvider interface to mock out exponential backoff delays.
Had to change part of ScheduleSyncSessionJob to deal with linked_ptr, it's a bit
ugly but I think for now still makes the overall impl simpler than RefCounting
the class.  May want to change it later if it gets too cumbersome due to linked
> raw > linked ptr conversions turning up.

Also added test for SyncSession::Coalesce, and of course, addressed comments!

There are still tests that can be added, and I left some in there with empty
bodies (marked DISABLED) just so we remember as more of the currently
NOTIMPLEMENTED functionality gets put in.

Thanks!

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/mock_model_safe_workers.h (right):

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/mock_model_safe_workers.h:9: #include
"base/scoped_vector.h"
On 2011/01/20 00:47:57, akalin wrote:
> needed?

Done.

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/nudge_source.h (right):

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/nudge_source.h:13: enum NudgeSource {
On 2011/01/20 20:01:57, nzea wrote:
> Some comments about what these correspond to might be useful.

Done.

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/polling_constants.cc (right):

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/polling_constants.cc:25: }  // namespace browser_sync
On 2011/01/20 20:01:57, nzea wrote:
> newline?

Done.

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/polling_constants.h (right):

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/polling_constants.h:11: namespace browser_sync {
On 2011/01/20 20:01:57, nzea wrote:
> extra newline after namespace? (or remove the newline before the close
> namespace)

Done. Also decreased indent.

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/polling_constants.h:19: #endif  //
CHROME_BROWSER_SYNC_ENGINE_POLLING_CONSTANTS_H_
On 2011/01/20 20:01:57, nzea wrote:
> newline at end of file?

Done.

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread2.cc (right):

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:65: } else {
On 2011/01/20 00:47:57, akalin wrote:
> no need for else since the if returns.

Done.

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:68: switch (purpose) {
On 2011/01/20 00:47:57, akalin wrote:
> isn't this clearer as:
> 
> DCHECK(purpose == POLL || purpose == NUDGE);
> if ((purpose == POLL) || wait_interval_->had_nudge) {
>   return false;
> }
> 
> Pretty sure that's equivalent to what you have.  Or maybe (purpose != NUDGE)

Done.

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:106:
GetUpdatesCallerInfo::GetUpdatesSource GetUpdatesFromNudgeSource(
On 2011/01/20 00:47:57, akalin wrote:
> anon namespace?

Done.

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:134: // Functor for std::find_if to
search by ModelSafeGroup.
On 2011/01/20 00:47:57, akalin wrote:
> anon namespace?

Done.

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:191: DCHECK(w_tmp.end() != it);
On 2011/01/20 00:47:57, akalin wrote:
> maybe:
> 
> if (it != w_tmp.end() {
>   workers->push_back(*it):
> } else {
>   NOTREACHED();
> }

Yeah, probably safer.

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:197: DCHECK(w_tmp.end() != it);
On 2011/01/20 00:47:57, akalin wrote:
> here, too

Done.

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:310: NUDGE_SOURCE_CONTINUATION,
On 2011/01/20 20:01:57, nzea wrote:
> Just for my understanding here, is this called when the nudge succeeded and
> completed? Or if the nudge that we just performed hasn't completed yet, but
made
> forward progress? I'm a bit confused as the whether the normal (everything
goes
> well) code path will go through here, and if so why we maintain the nudge
info.

Typically this means some sort of error occurred but we still may make forward
progress.  Practically speaking it is when SyncSession::HasMoreToSync returns
false, but we're not truly done.

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:420: bool
SyncerThread::IsSyncingCurrentlySilenced() {
On 2011/01/20 00:47:57, akalin wrote:
> const method?

Yeah, probably should be - I started to make this change but it requires
bringing in a pile of other files (this is part of an interface def'n).  Mind if
I file a TODO instead just to keep this patch from getting any larger than it
already is?

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread2.h (right):

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.h:46: // We don't use a lock when
changing modes, so it modes won't cause currently
On 2011/01/20 20:01:57, nzea wrote:
> unnecessary "it"?

Done.

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.h:103: WaitInterval(Mode mode,
base::TimeDelta length)
On 2011/01/20 00:47:57, akalin wrote:
> move the body into the .cc file

Done.

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.h:134: void DoSyncSessionJob(const
SyncSessionJob& job);
On 2011/01/20 20:01:57, nzea wrote:
> Might be more consistent if named SyncSessionJobImpl (following the convention
> used by the other methods that are posted)?

Hmm.. well the others have the same prefix.  Here SyncSessionJob is not a verb
but a noun.  It's also called on its own in the impl (see DoCanaryJob)...

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_types.h (right):

http://codereview.chromium.org/5939006/diff/38001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_types.h:90: // TODO(tim): Deprecated.
On 2011/01/20 20:01:57, nzea wrote:
> Hurray, you're fixing my SyncerThreadWithSyncerTest.Pause bugs!

:)

[akalin, 2011-01-25 06:09:53]

A few more comments

http://codereview.chromium.org/5939006/diff/47001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/mock_model_safe_workers.h (right):

http://codereview.chromium.org/5939006/diff/47001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/mock_model_safe_workers.h:25: class
MockModelSafeWorkerRegistrar : public ModelSafeWorkerRegistrar {
Move the bodies into a .cc file, please (and move all the other ones if you feel
like it)

http://codereview.chromium.org/5939006/diff/47001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread2_unittest.cc (right):

http://codereview.chromium.org/5939006/diff/47001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2_unittest.cc:366:
EXPECT_FALSE(GetBackoffAndReset(&done));
according to the gmock docs, results are undefined if you interleave
EXPECT_CALLs and actions.  You may need to split up these tests.

http://codereview.chromium.org/5939006/diff/47001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2_unittest.cc:538: // Test config tasks
don't run during normal mode.
Do you need a TODO here?

http://codereview.chromium.org/5939006/diff/47001/chrome/browser/sync/session...
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/5939006/diff/47001/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_session.h:98: void Coalesce(const SyncSession*
s);
why not a const ref?

http://codereview.chromium.org/5939006/diff/47001/chrome/browser/sync/session...
File chrome/browser/sync/sessions/sync_session_unittest.cc (right):

http://codereview.chromium.org/5939006/diff/47001/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_session_unittest.cc:7: #include
"base/scoped_vector.h"
Do you still need this?

Probably want ref_counted.h for scoped_refptr, too

http://codereview.chromium.org/5939006/diff/47001/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_session_unittest.cc:280: SyncSession
one(&*context_, this, source_one, routes_one, workers_one);
why not just context_.get()?

http://codereview.chromium.org/5939006/diff/47001/chrome/browser/sync/session...
File chrome/browser/sync/sessions/test_util.h (right):

http://codereview.chromium.org/5939006/diff/47001/chrome/browser/sync/session...
chrome/browser/sync/sessions/test_util.h:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
Can you move the bodies of these functions into a .cc file?  (whatever you can)

[tim (not reviewing), 2011-01-25 17:15:22]

http://codereview.chromium.org/5939006/diff/47001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread2_unittest.cc (right):

http://codereview.chromium.org/5939006/diff/47001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2_unittest.cc:366:
EXPECT_FALSE(GetBackoffAndReset(&done));
On 2011/01/25 06:09:53, akalin wrote:
> according to the gmock docs, results are undefined if you interleave
> EXPECT_CALLs and actions.  You may need to split up these tests.

Not sure what you mean here, can you elaborate?
Note that I call VerifyAndClearExpectations as part of GetBackoffAndReset.

[akalin, 2011-01-25 19:20:14]

http://codereview.chromium.org/5939006/diff/47001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread2_unittest.cc (right):

http://codereview.chromium.org/5939006/diff/47001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2_unittest.cc:366:
EXPECT_FALSE(GetBackoffAndReset(&done));
Oh, I was referring to this line from the gmock docs at
http://code.google.com/p/googlemock/wiki/ForDummies :

Important note: Google Mock requires expectations to be set before the mock
functions are called, otherwise the behavior is undefined. In particular, you
mustn't interleave EXPECT_CALL()s and calls to the mock functions.

Although I guess if you call VerifyAndClearExpectations it may be okay.  I'd add
a comment, though.

On 2011/01/25 17:15:22, timsteele wrote:
> On 2011/01/25 06:09:53, akalin wrote:
> > according to the gmock docs, results are undefined if you interleave
> > EXPECT_CALLs and actions.  You may need to split up these tests.
> 
> Not sure what you mean here, can you elaborate?
> Note that I call VerifyAndClearExpectations as part of GetBackoffAndReset.

[tim (not reviewing), 2011-01-25 23:11:36]

Addressed all comments.

http://codereview.chromium.org/5939006/diff/47001/chrome/browser/sync/session...
File chrome/browser/sync/sessions/sync_session_unittest.cc (right):

http://codereview.chromium.org/5939006/diff/47001/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_session_unittest.cc:280: SyncSession
one(&*context_, this, source_one, routes_one, workers_one);
On 2011/01/25 06:09:53, akalin wrote:
> why not just context_.get()?

shorter and, previously, let me fit on one line

I think I was gunning for one-line in an earlier incarnation :P changed.

[akalin, 2011-01-26 10:49:19]

LGTM after the nits below

http://codereview.chromium.org/5939006/diff/63001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/mock_model_safe_workers.h (right):

http://codereview.chromium.org/5939006/diff/63001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/mock_model_safe_workers.h:35:
scoped_refptr<ModelSafeWorker> passive_worker_;
include ref_counted.h for this

http://codereview.chromium.org/5939006/diff/63001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/mock_model_safe_workers.h:36: ModelSafeRoutingInfo
routes_;
do you need an include for this?

http://codereview.chromium.org/5939006/diff/63001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread2.h (right):

http://codereview.chromium.org/5939006/diff/63001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.h:221: scoped_ptr<Syncer> syncer_;
need include for scoped_ptr