sync: Per-type update application

sync: Per-type update application

This change moves the update application functionality from the
ApplyUpdatesAndResolveConflictsCommand into the
SyncDirectoryUpdateHandler class.  This change will allow us to
implement update application differently for different types.

Because update application happens on the model threads, the
ApplyUpdatesAndResolveConflictsCommand had to be aware of
ModelSafeRoutingInfo, ModelSafeWorkers, and other concepts intended to
hide threading details.  The new code takes a different approach.  It
hides the threading details specific to each type inside its
SyncDirectoryUpateHandler by initializing it with a scoped_refptr to its
associated ModelSafeWorker.

The ApplyUpdatesAndResolveConflictsCommand was the last SyncerCommand.
With its removal, we can also remove the definitions of SyncerCommand,
ModelChangingSyncerCommand and SyncerCommandTest.

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

[rlarocque, 2013-11-15 23:44:38]

Nicolas: Please review.

+CC Pavel, since this might bump into his SyncScheduler refactoring.
+CC Tim, since he wanted to know where this SyncerCommand refactor was heading.

This is the last big patch in this series.  It removes the last remaining
{ModelChanging,}SyncerCommand.  I don't expect this to include any significant
behavioral changes.  Let me know if you see any; they could be signs of a bug.

After this is committed, I plan to commit a few smaller CLs to tidy up some
things.  In particular, I'd like to move some of the free functions back into
classes and limit the scope of some state variables.

Once that's done, I will start working on the new-style-sync-equivalents of
SyncDirectoryUpdateHandler and SyncDirectoryCommitContributor.

[Nicolas Zea, 2013-11-19 22:45:26]

https://codereview.chromium.org/72403003/diff/1/sync/engine/sync_directory_up...
File sync/engine/sync_directory_update_handler.cc (right):

https://codereview.chromium.org/72403003/diff/1/sync/engine/sync_directory_up...
sync/engine/sync_directory_update_handler.cc:46: return;  // We don't process
control types here.
What's the plan for control types? Do they not have a
SyncDirectoryUpdateHandler?

https://codereview.chromium.org/72403003/diff/1/sync/engine/sync_directory_up...
File sync/engine/sync_directory_update_handler.h (right):

https://codereview.chromium.org/72403003/diff/1/sync/engine/sync_directory_up...
sync/engine/sync_directory_update_handler.h:15: #include
"sync/internal_api/public/engine/model_safe_worker.h"
I think you can probably forward declare this

https://codereview.chromium.org/72403003/diff/1/sync/engine/sync_directory_up...
File sync/engine/sync_directory_update_handler_unittest.cc (right):

https://codereview.chromium.org/72403003/diff/1/sync/engine/sync_directory_up...
sync/engine/sync_directory_update_handler_unittest.cc:32: class
SyncDirectoryUpdateHandlerProcessUpdateTest : public ::testing::Test {
Comment about what parts of the update handler this is capable of testing?

https://codereview.chromium.org/72403003/diff/1/sync/engine/sync_directory_up...
sync/engine/sync_directory_update_handler_unittest.cc:230: class
SyncDirectoryUpdateHandlerApplyUpdateTest : public ::testing::Test {
Here too? (and why it's separate/different)

https://codereview.chromium.org/72403003/diff/1/sync/engine/sync_directory_up...
sync/engine/sync_directory_update_handler_unittest.cc:306: EXPECT_EQ(0,
status.num_encryption_conflicts())
Check the is_unsynced/is_unapplied bits are set appropriately? (here and below)

https://codereview.chromium.org/72403003/diff/1/sync/engine/sync_engine_event.h
File sync/engine/sync_engine_event.h (right):

https://codereview.chromium.org/72403003/diff/1/sync/engine/sync_engine_event...
sync/engine/sync_engine_event.h:26: // Sent at several points during the sync
cycle.
Can we get more specific? Maybe "sent anytime progress is made during a sync
cycle"?

https://codereview.chromium.org/72403003/diff/1/sync/internal_api/sync_manage...
File sync/internal_api/sync_manager_impl_unittest.cc (right):

https://codereview.chromium.org/72403003/diff/1/sync/internal_api/sync_manage...
sync/internal_api/sync_manager_impl_unittest.cc:819: // This works only because
all routing info types are GROUP_PASSIVE.
It's not clear to me what this comment is for (what works?)

https://codereview.chromium.org/72403003/diff/1/sync/sessions/status_controll...
File sync/sessions/status_controller.h (right):

https://codereview.chromium.org/72403003/diff/1/sync/sessions/status_controll...
sync/sessions/status_controller.h:12: // blocked until update application
completes.
Does the status controller only hold model neutral information now? Perhaps
mention that no datatype specific state is being tracked?

https://codereview.chromium.org/72403003/diff/1/sync/syncable/directory.h
File sync/syncable/directory.h (right):

https://codereview.chromium.org/72403003/diff/1/sync/syncable/directory.h#new...
sync/syncable/directory.h:318: // Returns whether or not this type has unapplied
updates.
nit: this type -> |type|

[rlarocque, 2013-11-21 18:28:47]

Patch updated.  PTAL.

https://codereview.chromium.org/72403003/diff/1/sync/engine/sync_directory_up...
File sync/engine/sync_directory_update_handler.cc (right):

https://codereview.chromium.org/72403003/diff/1/sync/engine/sync_directory_up...
sync/engine/sync_directory_update_handler.cc:46: return;  // We don't process
control types here.
On 2013/11/19 22:45:26, Nicolas Zea wrote:
> What's the plan for control types? Do they not have a
> SyncDirectoryUpdateHandler?

No, they do have SyncDirectoryUpdateHandlers.  That's just the problem,
actually.

Control types will have their updates downloaded through the handlers, just like
any of the other types.  But their update application logic is special, so we
can't let them run this generic update application code.  Their updates will be
applied in ApplyControlDataUpdates() (as before), and they'll run the function
to apply updates normally but jump out of it early (as before).

In the future, it might be nice to have a Nigori-specific or DeviceInfo-specific
derivations of the SyncDirectoryUpdateHandler that include specific update
logic.  We'd still need some special case logic to ensure that control types get
their updates applied first, but that solution would probably be less
complicated overall than what we have now.

For this CL, though, I think it makes more sense to keep doing things in a
manner similar to the old way.

https://codereview.chromium.org/72403003/diff/1/sync/engine/sync_directory_up...
File sync/engine/sync_directory_update_handler.h (right):

https://codereview.chromium.org/72403003/diff/1/sync/engine/sync_directory_up...
sync/engine/sync_directory_update_handler.h:15: #include
"sync/internal_api/public/engine/model_safe_worker.h"
On 2013/11/19 22:45:26, Nicolas Zea wrote:
> I think you can probably forward declare this

Done.

https://codereview.chromium.org/72403003/diff/1/sync/engine/sync_directory_up...
File sync/engine/sync_directory_update_handler_unittest.cc (right):

https://codereview.chromium.org/72403003/diff/1/sync/engine/sync_directory_up...
sync/engine/sync_directory_update_handler_unittest.cc:32: class
SyncDirectoryUpdateHandlerProcessUpdateTest : public ::testing::Test {
On 2013/11/19 22:45:26, Nicolas Zea wrote:
> Comment about what parts of the update handler this is capable of testing?

Done.

https://codereview.chromium.org/72403003/diff/1/sync/engine/sync_directory_up...
sync/engine/sync_directory_update_handler_unittest.cc:230: class
SyncDirectoryUpdateHandlerApplyUpdateTest : public ::testing::Test {
On 2013/11/19 22:45:26, Nicolas Zea wrote:
> Here too? (and why it's separate/different)

Done.

The reasons are:
- History.
- Update application involves threads.  I could see these tests becoming more
complicated if for some reason the FakeModelWorkers become insufficient for
testing.

Also, I didn't mention it in the comment, but I could see a justification for
having different classes for update application and processing updates.  The
argument against is that the two concepts are somewhat tied to each other;
Bismarck types will have a no-op update application step.  On the other hand,
should we really need to instantiate a FakeModelWorker that we're not going to
use just to test update processing?

https://codereview.chromium.org/72403003/diff/1/sync/engine/sync_directory_up...
sync/engine/sync_directory_update_handler_unittest.cc:306: EXPECT_EQ(0,
status.num_encryption_conflicts())
On 2013/11/19 22:45:26, Nicolas Zea wrote:
> Check the is_unsynced/is_unapplied bits are set appropriately? (here and
below)

Good idea.

We won't be able to rely on StatusController for much longer.  I've added a
bunch of assertions based on the entries' state, rather than the status
controller.

https://codereview.chromium.org/72403003/diff/1/sync/engine/sync_engine_event.h
File sync/engine/sync_engine_event.h (right):

https://codereview.chromium.org/72403003/diff/1/sync/engine/sync_engine_event...
sync/engine/sync_engine_event.h:26: // Sent at several points during the sync
cycle.
On 2013/11/19 22:45:26, Nicolas Zea wrote:
> Can we get more specific? Maybe "sent anytime progress is made during a sync
> cycle"?

Done.

https://codereview.chromium.org/72403003/diff/1/sync/internal_api/sync_manage...
File sync/internal_api/sync_manager_impl_unittest.cc (right):

https://codereview.chromium.org/72403003/diff/1/sync/internal_api/sync_manage...
sync/internal_api/sync_manager_impl_unittest.cc:819: // This works only because
all routing info types are GROUP_PASSIVE.
On 2013/11/19 22:45:26, Nicolas Zea wrote:
> It's not clear to me what this comment is for (what works?)

Comment updated.

https://codereview.chromium.org/72403003/diff/1/sync/sessions/status_controll...
File sync/sessions/status_controller.h (right):

https://codereview.chromium.org/72403003/diff/1/sync/sessions/status_controll...
sync/sessions/status_controller.h:12: // blocked until update application
completes.
On 2013/11/19 22:45:26, Nicolas Zea wrote:
> Does the status controller only hold model neutral information now? Perhaps
> mention that no datatype specific state is being tracked?

It hasn't tracked any data type specific state for a while now.  I've added that
fact to the file comment.

In case you're wondering, yes, this does mean that we could merge
ModelNeutralState into this class.  But it gets complicated because the
ModelNeutralState is copied directly into the snapshot, while other counters,
like num encryption conflicts, are copied one at a time.  We'd need to refactor
the code so its definition isn't required all over the place before we can
remove it.

https://codereview.chromium.org/72403003/diff/1/sync/syncable/directory.h
File sync/syncable/directory.h (right):

https://codereview.chromium.org/72403003/diff/1/sync/syncable/directory.h#new...
sync/syncable/directory.h:318: // Returns whether or not this type has unapplied
updates.
On 2013/11/19 22:45:26, Nicolas Zea wrote:
> nit: this type -> |type|

Done.

[Nicolas Zea, 2013-12-02 21:09:32]

LGTM

https://codereview.chromium.org/72403003/diff/370001/sync/engine/sync_directo...
File sync/engine/sync_directory_update_handler_unittest.cc (right):

https://codereview.chromium.org/72403003/diff/370001/sync/engine/sync_directo...
sync/engine/sync_directory_update_handler_unittest.cc:34: // Update processing
is what occurs when we first download updats.  It converts
nit: updats -> updates

[commit-bot: I haz the power, 2013-12-02 23:30:56]

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

[commit-bot: I haz the power, 2013-12-03 02:02:57]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-03 17:51:10]

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

[commit-bot: I haz the power, 2013-12-03 19:10:38]

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

[commit-bot: I haz the power, 2013-12-03 23:36:48]

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

[commit-bot: I haz the power, 2013-12-04 02:51:39]

Change committed as 238532