Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(49)

Issue 6286067: sync: add a SyncSessionJobPurpose for clearing sync user data. (Closed)

Created:
9 years, 10 months ago by tim (not reviewing)
Modified:
9 years, 6 months ago
CC:
chromium-reviews, Raghu Simha, ncarter (slow), Paweł Hajdan Jr., lipalani1, tim (not reviewing), idana
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 4

Patch Set 2 : review #

Patch Set 3 : whitespace #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+169 lines, -69 lines) Patch
M chrome/browser/sync/engine/nudge_source.h View 1 chunk +0 lines, -3 lines 0 comments Download
M chrome/browser/sync/engine/syncer.h View 1 2 chunks +8 lines, -9 lines 0 comments Download
M chrome/browser/sync/engine/syncer.cc View 4 chunks +17 lines, -11 lines 0 comments Download
M chrome/browser/sync/engine/syncer_thread2.h View 5 chunks +12 lines, -1 line 0 comments Download
M chrome/browser/sync/engine/syncer_thread2.cc View 1 2 8 chunks +52 lines, -9 lines 6 comments Download
M chrome/browser/sync/engine/syncer_thread2_unittest.cc View 23 chunks +63 lines, -28 lines 0 comments Download
M chrome/browser/sync/sessions/test_util.h View 2 chunks +9 lines, -4 lines 0 comments Download
M chrome/browser/sync/sessions/test_util.cc View 2 chunks +8 lines, -4 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
tim (not reviewing)
9 years, 10 months ago (2011-02-03 00:04:34 UTC) #1
tim (not reviewing)
9 years, 10 months ago (2011-02-03 00:06:08 UTC) #2
Nicolas Zea
LG with a nit and a test http://codereview.chromium.org/6286067/diff/1/chrome/browser/sync/engine/syncer_thread2.cc File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6286067/diff/1/chrome/browser/sync/engine/syncer_thread2.cc#newcode126 chrome/browser/sync/engine/syncer_thread2.cc:126: if (purpose ...
9 years, 10 months ago (2011-02-03 00:39:07 UTC) #3
tim (not reviewing)
http://codereview.chromium.org/6286067/diff/1/chrome/browser/sync/engine/syncer_thread2.cc File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6286067/diff/1/chrome/browser/sync/engine/syncer_thread2.cc#newcode126 chrome/browser/sync/engine/syncer_thread2.cc:126: if (purpose != POLL && purpose != NUDGE && ...
9 years, 10 months ago (2011-02-03 00:56:15 UTC) #4
Raz Mathias
Some observations (given my limited knowledge of the design) http://codereview.chromium.org/6286067/diff/6003/chrome/browser/sync/engine/syncer_thread2.cc File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6286067/diff/6003/chrome/browser/sync/engine/syncer_thread2.cc#newcode120 chrome/browser/sync/engine/syncer_thread2.cc:120: ...
9 years, 10 months ago (2011-02-03 01:42:17 UTC) #5
tim (not reviewing)
Thanks for the review Raz. http://codereview.chromium.org/6286067/diff/6003/chrome/browser/sync/engine/syncer_thread2.cc File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6286067/diff/6003/chrome/browser/sync/engine/syncer_thread2.cc#newcode120 chrome/browser/sync/engine/syncer_thread2.cc:120: switch (mode_) { On ...
9 years, 10 months ago (2011-02-03 18:18:52 UTC) #6
Raz Mathias
9 years, 10 months ago (2011-02-03 19:21:17 UTC) #7
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
:)

Powered by Google App Engine
This is Rietveld 408576698