sync: add a SyncSessionJobPurpose for clearing sync user data.

sync: add a SyncSessionJobPurpose for clearing private data.

Removes the nudge source which wasn't used server-side.

BUG=71616, 26339
TEST=SyncerThread2Test

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

[Nicolas Zea, 2011-02-03 00:39:07]

LG with a nit and a test

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

http://codereview.chromium.org/6286067/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncer_thread2.cc:126: if (purpose != POLL && purpose
!= NUDGE && purpose != CLEAR_USER_DATA)
if (purpose == CONFIGURATION)?

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

http://codereview.chromium.org/6286067/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncer_thread2_unittest.cc:690: done.Reset();
Test configuration as well?

[tim (not reviewing), 2011-02-03 00:56:15]

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

http://codereview.chromium.org/6286067/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncer_thread2.cc:126: if (purpose != POLL && purpose
!= NUDGE && purpose != CLEAR_USER_DATA)
On 2011/02/03 00:39:07, nzea wrote:
> if (purpose == CONFIGURATION)?

Done.

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

http://codereview.chromium.org/6286067/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncer_thread2_unittest.cc:690: done.Reset();
On 2011/02/03 00:39:07, nzea wrote:
> Test configuration as well?

Well... I didn't actually implement configuration yet.  So there's still a
NOTIMPLEMENTED in scheduleconfigimpl.

[Raz Mathias, 2011-02-03 01:42:17]

Some observations (given my limited knowledge of the design)

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

http://codereview.chromium.org/6286067/diff/6003/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_thread2.cc:120: switch (mode_) {
It seems like you are switching on Purpose a bunch -- would it make make the
purpose a class and use polymorphic methods to get a better encapsulation?

http://codereview.chromium.org/6286067/diff/6003/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_thread2.cc:328: void
SyncerThread::SetSyncerStepsForPurpose(SyncSessionJobPurpose purpose,
It seems a little strange that the syncer, which seems to be a general state
machine executor, hardcodes/controls the start/end states.  I wonder if it would
make sense to encapsulate this info in a separate class?  Without knowing more
about the design, it almost feels like the Purpose  itself could be that
class...

[tim (not reviewing), 2011-02-03 18:18:52]

Thanks for the review Raz.

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

http://codereview.chromium.org/6286067/diff/6003/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_thread2.cc:120: switch (mode_) {
On 2011/02/03 01:42:17, Raz Mathias wrote:
> It seems like you are switching on Purpose a bunch -- would it make make the
> purpose a class and use polymorphic methods to get a better encapsulation?

I like this suggestion, except that right now the code actually only switches on
|purpose| once!  There are a few if purpose == NUDGE branches though, but
they're so far pretty specific and I'm not sure right now is the best time to
build this encapsulation... will definitely keep it in mind though for next
time.  That sound ok?

http://codereview.chromium.org/6286067/diff/6003/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_thread2.cc:328: void
SyncerThread::SetSyncerStepsForPurpose(SyncSessionJobPurpose purpose,
On 2011/02/03 01:42:17, Raz Mathias wrote:
> It seems a little strange that the syncer, which seems to be a general state
> machine executor, hardcodes/controls the start/end states.  I wonder if it
would
> make sense to encapsulate this info in a separate class?  Without knowing more
> about the design, it almost feels like the Purpose  itself could be that
> class...

Did you mean SyncerThread vs Syncer?  Because this patch actually deprecates the
Syncer method that hardcodes/controls the start/end states and makes public the
method to allow the controller (SyncerThread) to make the initial/end state
decisions.

[Raz Mathias, 2011-02-03 19:21:17]

LGTM++

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

http://codereview.chromium.org/6286067/diff/6003/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_thread2.cc:120: switch (mode_) {
On 2011/02/03 18:18:52, timsteele wrote:
> On 2011/02/03 01:42:17, Raz Mathias wrote:
> > It seems like you are switching on Purpose a bunch -- would it make make the
> > purpose a class and use polymorphic methods to get a better encapsulation?
> 
> I like this suggestion, except that right now the code actually only switches
on
> |purpose| once!  There are a few if purpose == NUDGE branches though, but
> they're so far pretty specific and I'm not sure right now is the best time to
> build this encapsulation... will definitely keep it in mind though for next
> time.  That sound ok?

What I meant here was implementing a method on the purpose class that took a
current_mode (and naming it so well that you could remove the comment above the
switch block :), something like shouldRunInMode().  The if checks and
switch(purpose) below would be additional candidates for encapsulation.

http://codereview.chromium.org/6286067/diff/6003/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_thread2.cc:328: void
SyncerThread::SetSyncerStepsForPurpose(SyncSessionJobPurpose purpose,
On 2011/02/03 18:18:52, timsteele wrote:
> On 2011/02/03 01:42:17, Raz Mathias wrote:
> > It seems a little strange that the syncer, which seems to be a general state
> > machine executor, hardcodes/controls the start/end states.  I wonder if it
> would
> > make sense to encapsulate this info in a separate class?  Without knowing
more
> > about the design, it almost feels like the Purpose  itself could be that
> > class...
> 
> Did you mean SyncerThread vs Syncer?  Because this patch actually deprecates
the
> Syncer method that hardcodes/controls the start/end states and makes public
the
> method to allow the controller (SyncerThread) to make the initial/end state
> decisions.

Yes, sorry, SyncerThread -- This switch block with hard-coded constants seems to
belong elsewhere, perhaps in the purpose class itself (when it becomes a class
:)