make new syncer thread the default.

make new syncer thread the default. Fixed all the test cases that failed.


BUG=26339
TEST=unit_tests.exe, sync_integration_tests.exe, sync_unit_tests.exe. manual testcases:
1. Set up a brand new profile to sync and make sure it syncs.
2. enable/disable a datatype and make sure it syncs.
3. Make change to a datatype locally and make sure it gets propagated.
4. make change to a datatype remotely and make sure it syncs down.

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

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

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

[lipalani1, 2011-04-17 06:46:44]

Making syncer thread the default. Fixed all the unit tests. All the try bots
pass on the server itself.

Things that are yet to be done but I plan to tackle as a seperate review(as
these are not essential to make syncer thread default):
1. Some unit test files still have the name with the 2 suffix.
2. Did not remove the deprecated datastructure from the serverconnectionmanager.
Some unit tests depend on it.

[tim (not reviewing), 2011-04-18 16:11:47]

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/n...
File chrome/browser/sync/engine/nudge_source.h (right):

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/n...
chrome/browser/sync/engine/nudge_source.h:20: CLEAR_USER_PRIVATE_DATA
What's this about?

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:1676: 0,
browser_sync::CLEAR_USER_PRIVATE_DATA, FROM_HERE);
SyncerThread has a "ScheduleClear..." operation, you should use that.

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:1751: syncer_thread_.reset(new
SyncerThreadAdapter(context));
Can we just yank SyncerThreadAdapter now too?

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_thread.h:11: #include
"base/memory/linked_ptr.h"
I'm assuming there were no changes to syncer_thread2.h/cc as part of this patch?

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_thread_adapter.h (right):

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_thread_adapter.h:21: explicit
SyncerThreadAdapter(sessions::SyncSessionContext* context);
This class only existed to choose between implementations.  Theres only one now
so it should be removed.

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_types.h (right):

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_types.h:86: // SyncerThread generated events.
remove comment

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/glue/dat...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_manager_impl.cc:363: DataTypeManager::State
DataTypeManagerImpl::state() {
I'm assuming you made no changes to DataTypeManagerImpl2 in this patch?

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:602: // When unrecoverable error is
detected we post a task to shutdown the
I don't quite understand a) how or if this arose as part of this patch and b) if
this is needed here is it needed elsewhere?

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service_password_unittest.cc (right):

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_password_unittest.cc:222:
NotificationType::SYNC_CONFIGURE_UPDATES_DOWNLOADED),_,_))
This was the kind of thing I expected to arise when "making the unit tests work"
and why I thought it'd be a little trickier than just pulling pause/resume out
:)

I'd prefer to not add a notification just for the test. But if we have to, note
that it's only a matter of time before code is built on top of it because "it
sounds like the right event", so I wouldn't pick "updates downloaded".  It's too
ambiguous and overloaded in this context.  

Additionally, it also seems racy because couldn't the cryptographer transition
to 'ready' due to the set passphrase call below?

Did you consider SYNC_CONFIGURE_BLOCKED instead?  I think it mitigates the two
issues above and also is nice because it teases out the fact that we only need
this for password tests, so hopefully it wouldn't be mistakenly included in
other tests that should be using SYNC_CONFIGURE_DONE in the future.

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/sync_session_context.h (right):

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session_context.h:95: void
set_last_snapshot(const SyncSessionSnapshot& snapshot);
is this still needed? I can't recall

[tim (not reviewing), 2011-04-18 16:13:10]

in addition to comments, please update BUG= to reference 26339, and update the
TEST=.

[lipalani1, 2011-04-18 20:36:41]

Fixed or commented on the CR feedback.

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/n...
File chrome/browser/sync/engine/nudge_source.h (right):

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/n...
chrome/browser/sync/engine/nudge_source.h:20: CLEAR_USER_PRIVATE_DATA
Was needed to communicate with the adapter class. now removed.
On 2011/04/18 16:11:47, timsteele wrote:
> What's this about?

Done.

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:1676: 0,
browser_sync::CLEAR_USER_PRIVATE_DATA, FROM_HERE);
On 2011/04/18 16:11:47, timsteele wrote:
> SyncerThread has a "ScheduleClear..." operation, you should use that.

Done.

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:1751: syncer_thread_.reset(new
SyncerThreadAdapter(context));
On 2011/04/18 16:11:47, timsteele wrote:
> Can we just yank SyncerThreadAdapter now too?

Done.

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_thread.h:11: #include
"base/memory/linked_ptr.h"
Yes it was a straight port.
On 2011/04/18 16:11:47, timsteele wrote:
> I'm assuming there were no changes to syncer_thread2.h/cc as part of this
patch?

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_thread_adapter.h (right):

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_thread_adapter.h:21: explicit
SyncerThreadAdapter(sessions::SyncSessionContext* context);
On 2011/04/18 16:11:47, timsteele wrote:
> This class only existed to choose between implementations.  Theres only one
now
> so it should be removed.

Done.

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_types.h (right):

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_types.h:86: // SyncerThread generated events.
On 2011/04/18 16:11:47, timsteele wrote:
> remove comment

Done.

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/glue/dat...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_manager_impl.cc:363: DataTypeManager::State
DataTypeManagerImpl::state() {
Yep straight port.
On 2011/04/18 16:11:47, timsteele wrote:
> I'm assuming you made no changes to DataTypeManagerImpl2 in this patch?

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:602: // When unrecoverable error is
detected we post a task to shutdown the
This arose because of this patch and how the threading model changed with out
the pause command. it is needed elsewhere as well. But first I wanted to figure
out the reason for posting a message to do shut down. I guess they wanted to
unwind the stack. But not sure.
Assuming it is right then we need this check else where as well. For now I will
add a TODO. Let me know if you think otherwise.
On 2011/04/18 16:11:47, timsteele wrote:
> I don't quite understand a) how or if this arose as part of this patch and b)
if
> this is needed here is it needed elsewhere?

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service_password_unittest.cc (right):

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_password_unittest.cc:222:
NotificationType::SYNC_CONFIGURE_UPDATES_DOWNLOADED),_,_))
ha ha! yes and it took a while to figure out as well.

Any way the reason I put that in was to allow things to be built around
updates_downloaded if needed(similar to pause earlier). But I see your point as
well. If need arises others can add it later. I have made it very specific. Now
it is called sync_configure_needs_crypto. Look at the implementation.
On 2011/04/18 16:11:47, timsteele wrote:
> This was the kind of thing I expected to arise when "making the unit tests
work"
> and why I thought it'd be a little trickier than just pulling pause/resume out
> :)
> 
> I'd prefer to not add a notification just for the test. But if we have to,
note
> that it's only a matter of time before code is built on top of it because "it
> sounds like the right event", so I wouldn't pick "updates downloaded".  It's
too
> ambiguous and overloaded in this context.  
> 
> Additionally, it also seems racy because couldn't the cryptographer transition
> to 'ready' due to the set passphrase call below?
> 
> Did you consider SYNC_CONFIGURE_BLOCKED instead?  I think it mitigates the two
> issues above and also is nice because it teases out the fact that we only need
> this for password tests, so hopefully it wouldn't be mistakenly included in
> other tests that should be using SYNC_CONFIGURE_DONE in the future.

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/sync_session_context.h (right):

http://codereview.chromium.org/6874018/diff/5001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session_context.h:95: void
set_last_snapshot(const SyncSessionSnapshot& snapshot);
On 2011/04/18 16:11:47, timsteele wrote:
> is this still needed? I can't recall

Done.

[lipalani1, 2011-04-18 22:15:02]

Fixing Tim's comments.

[lipalani1, 2011-04-18 22:22:26]

Updating feedback.

[tim (not reviewing), 2011-04-18 22:23:05]

one more nit, then LGTM

http://codereview.chromium.org/6874018/diff/6045/chrome/browser/sync/glue/dat...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

http://codereview.chromium.org/6874018/diff/6045/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_manager_impl.cc:367: state_ = state;
unless you're going to call this everywhere we set state_ = to something, you
should just call it SetBlockedAndNotify, or remove the method.  It's confusing
to have state_ = and SetState methods.

[lipalani1, 2011-04-18 22:28:48]

Fixed the feeback.

[lipalani1, 2011-04-18 22:30:01]

marked the feeback as done.(no code change)

http://codereview.chromium.org/6874018/diff/6045/chrome/browser/sync/glue/dat...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

http://codereview.chromium.org/6874018/diff/6045/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_manager_impl.cc:367: state_ = state;
On 2011/04/18 22:23:05, timsteele wrote:
> unless you're going to call this everywhere we set state_ = to something, you
> should just call it SetBlockedAndNotify, or remove the method.  It's confusing
> to have state_ = and SetState methods.

Done.

[lipalani1, 2011-04-18 23:00:37]

need Brett's owners approval for chrome/content/common/notification_type.h

[brettw, 2011-04-18 23:10:28]

I didn't really check the code but fewer notification types are better. LGTM

[lipalani1, 2011-04-19 04:13:59]

need LGTM on the test file.

[Raghu Simha, 2011-04-19 04:25:37]

Test change LGTM, based on the fact that it's a temporary change meant only for
debugging on the sync buildbots :)

http://codereview.chromium.org/6874018/diff/6046/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sync_test.cc (right):

http://codereview.chromium.org/6874018/diff/6046/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_test.cc:111:
logging::SetMinLogLevel(logging::LOG_VERBOSE);
Ideally, this should require an include of "base/logging.h". However, I'm fine
with leaving this as it is if it is a temporary change that you intend to back
out.