Support for multiple sync ModelSafeWorkers....

Support for multiple sync ModelSafeWorkers.

- Introduce an equivalence class enum to group sync model types that live on the same
chrome native model together.
- Remove ModelSafeWorkerBridge as it is no longer needed.
- Rename BookmarkModelWorker -> UIModelWorker, and make it use the new stuff.
- Allow syncable entries belonging to an "unsynced" model type to be just
processed "passively" from
the SyncerThread (rather than dispatching), and allow this to change as a result
of enable/disabling.
- Experimenting with a way to complete issue 31909 (the CLASS_UNASSOCIATED
stuff).

BUG=31925, 31911, 31909
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=36835

[tim (not reviewing), 2010-01-19 02:06:24]

This is ready for a look.  I'm fighting with the try server because of a couple
'svn mv' mixups...

This doesn't quite fix any of the associated bugs yet, but my hope is it's a
good checkpoint.  I have a descriptive TODO in sync_session.cc outlining how I
was thinking of getting from here to there.

[tim (not reviewing), 2010-01-19 22:10:37]

CC'ing skrul and albertb FYI

[ncarter (slow), 2010-01-20 19:27:33]

http://codereview.chromium.org/553015/diff/2060/2077
File chrome/browser/sync/engine/apply_updates_command_unittest.cc (right):

http://codereview.chromium.org/553015/diff/2060/2077#newcode48
chrome/browser/sync/engine/apply_updates_command_unittest.cc:48: virtual void
GetWorkers(std::vector<ModelSafeWorker*>* out) {}
// ModelSafeWorkerRegistrar implementation.

http://codereview.chromium.org/553015/diff/2060/2073
File chrome/browser/sync/engine/model_safe_worker.h (right):

http://codereview.chromium.org/553015/diff/2060/2073#newcode17
chrome/browser/sync/engine/model_safe_worker.h:17: enum
ModelSafeEquivalenceClass {
In some sense, this is really just a runtime type tag.  No need to
overcomplicate it by referencing some implicit equivalence relation.

I think choosing "Class" as the noun part of this phrase is a mistake.  'Class'
has a specific meaning in the context of a C++ program, and because it's a
keyword, the obvious default variable name is illegal.  Calling this WorkType,
WorkerType, or ModelWorkGroup might be better.

http://codereview.chromium.org/553015/diff/2060/2073#newcode58
chrome/browser/sync/engine/model_safe_worker.h:58: virtual void
DoWorkAndWaitUntilDone(Closure* work) {}
If you don't do anything to the closure, it won't delete itself, meaning you
leak memory.

http://codereview.chromium.org/553015/diff/2060/2073#newcode81
chrome/browser/sync/engine/model_safe_worker.h:81: ModelSafeWorkerRegistrar() {
}
Define a virtual destructor.

http://codereview.chromium.org/553015/diff/2060/2078
File chrome/browser/sync/engine/syncer_thread_unittest.cc (right):

http://codereview.chromium.org/553015/diff/2060/2078#newcode54
chrome/browser/sync/engine/syncer_thread_unittest.cc:54: 
// ModelSafeWorkerRegistrar implementation.

http://codereview.chromium.org/553015/diff/2060/2080
File chrome/browser/sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/553015/diff/2060/2080#newcode120
chrome/browser/sync/engine/syncer_unittest.cc:120: 
// ModelSafeWorkerRegistrar implementation.

http://codereview.chromium.org/553015/diff/2060/2063
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/553015/diff/2060/2063#newcode40
chrome/browser/sync/glue/sync_backend_host.cc:40: syncable::ModelType
t(syncable::ModelTypeFromInt(i));
Just make i a ModelType.

http://codereview.chromium.org/553015/diff/2060/2063#newcode102
chrome/browser/sync/glue/sync_backend_host.cc:102:
static_cast<UIModelWorker*>(registrar_.workers[CLASS_UI])->Stop();
Smells wrong.  This is a strong signal to me that an array of untyped workers is
the wrong way to go.  If you need access to the exact implementation, why not
just have a scoped_ptr<UIModelWorker>?

http://codereview.chromium.org/553015/diff/2060/2063#newcode251
chrome/browser/sync/glue/sync_backend_host.cc:251:
host_->registrar_.workers[CLASS_UI]);
Again, totally gross.

http://codereview.chromium.org/553015/diff/2060/2074
File chrome/browser/sync/sessions/sync_session.cc (right):

http://codereview.chromium.org/553015/diff/2060/2074#newcode18
chrome/browser/sync/sessions/sync_session.cc:18: // We're in the ctor for Pete's
sake!
What is this comment trying to say?

http://codereview.chromium.org/553015/diff/2060/2075
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/553015/diff/2060/2075#newcode144
chrome/browser/sync/sessions/sync_session.h:144: ModelSafetyRestriction
model_safety_restriction_;
Maybe just eliminate the struct, and have a pair of variables:

model_restriction_type_
model_restriction_in_effect_.

A special value for the type (PASSIVE?) would eliminate the need for a bool; I'm
guessing you considered that, though.

http://codereview.chromium.org/553015/diff/2060/2075#newcode170
chrome/browser/sync/sessions/sync_session.h:170: class
ScopedModelSafetyRestriction {
Ultimately, you're not restricting safety, you're restricting model types (to
achieve safety).  Consider the name 'ScopedModelTypeRestriction' or
'ScopedModelRestriction'.

http://codereview.chromium.org/553015/diff/2060/2069
File chrome/browser/sync/syncable/model_type.h (right):

http://codereview.chromium.org/553015/diff/2060/2069#newcode22
chrome/browser/sync/syncable/model_type.h:22: return static_cast<ModelType>(i);
Don't need this.

[tim (not reviewing), 2010-01-21 00:04:25]

Removed GROUP_UNASSOCIATED since it wasn't used currently and I think Nick
convinced me it is not the way to go :)

http://codereview.chromium.org/553015/diff/2060/2077
File chrome/browser/sync/engine/apply_updates_command_unittest.cc (right):

http://codereview.chromium.org/553015/diff/2060/2077#newcode48
chrome/browser/sync/engine/apply_updates_command_unittest.cc:48: virtual void
GetWorkers(std::vector<ModelSafeWorker*>* out) {}
On 2010/01/20 19:27:33, nick wrote:
> // ModelSafeWorkerRegistrar implementation.

Done.

http://codereview.chromium.org/553015/diff/2060/2073
File chrome/browser/sync/engine/model_safe_worker.h (right):

http://codereview.chromium.org/553015/diff/2060/2073#newcode17
chrome/browser/sync/engine/model_safe_worker.h:17: enum
ModelSafeEquivalenceClass {
On 2010/01/20 19:27:33, nick wrote:
> In some sense, this is really just a runtime type tag.  No need to
> overcomplicate it by referencing some implicit equivalence relation.
> 
> I think choosing "Class" as the noun part of this phrase is a mistake. 
'Class'
> has a specific meaning in the context of a C++ program, and because it's a
> keyword, the obvious default variable name is illegal.  Calling this WorkType,
> WorkerType, or ModelWorkGroup might be better.

Sounds better, I like ModelSafeGroup?  (Though it's just a tag now, I need this
as a key for the next step (siloing session data by MSG) so it will have a more
important role.  At least, the way I currently see it).
I also moved it to syncable (keep it close to ModelType), and so the values
become syncable::GROUP_UI, syncable::GROUP_PASSIVE, etc.

http://codereview.chromium.org/553015/diff/2060/2073#newcode58
chrome/browser/sync/engine/model_safe_worker.h:58: virtual void
DoWorkAndWaitUntilDone(Closure* work) {}
On 2010/01/20 19:27:33, nick wrote:
> If you don't do anything to the closure, it won't delete itself, meaning you
> leak memory.

Good catch!

http://codereview.chromium.org/553015/diff/2060/2073#newcode81
chrome/browser/sync/engine/model_safe_worker.h:81: ModelSafeWorkerRegistrar() {
}
On 2010/01/20 19:27:33, nick wrote:
> Define a virtual destructor.

Done, protected though since we don't want to delete through a pointer of this
type.

http://codereview.chromium.org/553015/diff/2060/2078
File chrome/browser/sync/engine/syncer_thread_unittest.cc (right):

http://codereview.chromium.org/553015/diff/2060/2078#newcode54
chrome/browser/sync/engine/syncer_thread_unittest.cc:54: 
On 2010/01/20 19:27:33, nick wrote:
> // ModelSafeWorkerRegistrar implementation.

Done.

http://codereview.chromium.org/553015/diff/2060/2080
File chrome/browser/sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/553015/diff/2060/2080#newcode120
chrome/browser/sync/engine/syncer_unittest.cc:120: 
On 2010/01/20 19:27:33, nick wrote:
> // ModelSafeWorkerRegistrar implementation.

Done.

http://codereview.chromium.org/553015/diff/2060/2063
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/553015/diff/2060/2063#newcode40
chrome/browser/sync/glue/sync_backend_host.cc:40: syncable::ModelType
t(syncable::ModelTypeFromInt(i));
On 2010/01/20 19:27:33, nick wrote:
> Just make i a ModelType.

Hmm.. ++ isn't defined and '=' requires a cast... not sure how do this
otherwise? :\

http://codereview.chromium.org/553015/diff/2060/2063#newcode102
chrome/browser/sync/glue/sync_backend_host.cc:102:
static_cast<UIModelWorker*>(registrar_.workers[CLASS_UI])->Stop();
On 2010/01/20 19:27:33, nick wrote:
> Smells wrong.  This is a strong signal to me that an array of untyped workers
is
> the wrong way to go.  If you need access to the exact implementation, why not
> just have a scoped_ptr<UIModelWorker>?

Well, here's the thing.  I don't know for sure, but I have a hunch that we'll
need to Stop()/OnSyncerShutdownComplete() more model workers than just the UI
model worker.  In that case we'll need either add or extend the MSW interface
with these methods.  Although I admit, my morals may be broken here because I
don't find this kind of list downcast bad.  (Maybe it was all my pre-java1.5 use
of Vector.get() returning 'Object' that corrupted me.) If they are separate
members, then GetWorkers needs to be updated every time you add a worker to SBH,
as does every place we call Stop/OnSyncerShutdown, to account for each member.  

With that background, if you still want me to change it, for now, I will - WDYS?

http://codereview.chromium.org/553015/diff/2060/2074
File chrome/browser/sync/sessions/sync_session.cc (right):

http://codereview.chromium.org/553015/diff/2060/2074#newcode18
chrome/browser/sync/sessions/sync_session.cc:18: // We're in the ctor for Pete's
sake!
On 2010/01/20 19:27:33, nick wrote:
> What is this comment trying to say?

Err, I'll remove it.  I just thought there may have been a way to assign a const
member given that we're in the ctor (but syntactically I can't use the
initializer list... or I don't know how)

http://codereview.chromium.org/553015/diff/2060/2075
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/553015/diff/2060/2075#newcode144
chrome/browser/sync/sessions/sync_session.h:144: ModelSafetyRestriction
model_safety_restriction_;
On 2010/01/20 19:27:33, nick wrote:
> Maybe just eliminate the struct, and have a pair of variables:
> 
> model_restriction_type_
> model_restriction_in_effect_.
> 
> A special value for the type (PASSIVE?) would eliminate the need for a bool;
I'm
> guessing you considered that, though.

Removed the struct.  Left the bool though, PASSIVE is a valid state,
unfortunately.  I could add an INVALID one..

http://codereview.chromium.org/553015/diff/2060/2075#newcode170
chrome/browser/sync/sessions/sync_session.h:170: class
ScopedModelSafetyRestriction {
On 2010/01/20 19:27:33, nick wrote:
> Ultimately, you're not restricting safety, you're restricting model types (to
> achieve safety).  Consider the name 'ScopedModelTypeRestriction' or
> 'ScopedModelRestriction'.

Done.

http://codereview.chromium.org/553015/diff/2060/2079
File chrome/browser/sync/sessions/sync_session_unittest.cc (right):

http://codereview.chromium.org/553015/diff/2060/2079#newcode50
chrome/browser/sync/sessions/sync_session_unittest.cc:50: 
Added comment.

http://codereview.chromium.org/553015/diff/2060/2069
File chrome/browser/sync/syncable/model_type.h (right):

http://codereview.chromium.org/553015/diff/2060/2069#newcode22
chrome/browser/sync/syncable/model_type.h:22: return static_cast<ModelType>(i);
On 2010/01/20 19:27:33, nick wrote:
> Don't need this.

As we discussed, left it here.

[tim (not reviewing), 2010-01-21 00:05:39]

> I also moved it to syncable (keep it close to ModelType), and so the values
> become syncable::GROUP_UI, syncable::GROUP_PASSIVE, etc.

I did not do this.  Stale comment.  Sorry.

[tim (not reviewing), 2010-01-21 00:09:43]

> Well, here's the thing.  I don't know for sure, but I have a hunch that we'll
> need to Stop()/OnSyncerShutdownComplete() more model workers than just the UI
> model worker.  In that case we'll need either add or extend the MSW interface
> with these methods.  Although I admit, my morals may be broken here because I
> don't find this kind of list downcast bad.  (Maybe it was all my pre-java1.5
use
> of Vector.get() returning 'Object' that corrupted me.) If they are separate
> members, then GetWorkers needs to be updated every time you add a worker to
SBH,
> as does every place we call Stop/OnSyncerShutdown, to account for each member.
 
> 
> With that background, if you still want me to change it, for now, I will -
WDYS?

And on this note, I've been talking with skrul about how to do multi-type
shutdown, and we *will* in fact need Stop on all of them.  So just having a list
to walk through seems best.

[tim (not reviewing), 2010-01-21 06:27:01]

One last thing, I replaced SyncDataType with ModelType; those changes on their
own are visible here http://codereview.chromium.org/555031/show, but I decided
it didn't make sense to check in this without de-duping the enum.

[ncarter (slow), 2010-01-21 18:16:39]

LGTM with the following addressed.

http://codereview.chromium.org/553015/diff/5010/10017
File chrome/browser/sync/engine/apply_updates_command_unittest.cc (right):

http://codereview.chromium.org/553015/diff/5010/10017#newcode47
chrome/browser/sync/engine/apply_updates_command_unittest.cc:47: 
// ModelSafeWorkerRegistrar implementation.

http://codereview.chromium.org/553015/diff/5010/10020
File chrome/browser/sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/553015/diff/5010/10020#newcode130
chrome/browser/sync/engine/syncer_unittest.cc:130: }
Should this be factored out into a test_model_safe_worker_registrar?

http://codereview.chromium.org/553015/diff/5010/10003
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/553015/diff/5010/10003#newcode86
chrome/browser/sync/glue/sync_backend_host.cc:86: void
SyncBackendHost::Shutdown(bool sync_disabled) {
Shutdown can be called when Initialize() didn't complete fully.  Thus, you
should be sure not to assume that the workers are non-NULL.   Check the pointers
before you deref them.

http://codereview.chromium.org/553015/diff/5010/10003#newcode100
chrome/browser/sync/glue/sync_backend_host.cc:100:
static_cast<UIModelWorker*>(registrar_.workers[GROUP_UI])->Stop();
This is still gross.  At the very least, can you move the cast into an accessor
in the registrar_ struct, use down_cast<> in the implementation, and use the
accessor in all the places you've put the cast?

Alternatively, if you expect Stop() will be part of the MSW interface, why not
make it part of the MSW interface?

http://codereview.chromium.org/553015/diff/5010/10008
File chrome/browser/sync/glue/ui_model_worker.h (right):

http://codereview.chromium.org/553015/diff/5010/10008#newcode70
chrome/browser/sync/glue/ui_model_worker.h:70: // ModelSafeWorkerInterface
implementation. Called on syncapi SyncerThread.
This should be ModelSafeWorker (no Interface).

http://codereview.chromium.org/553015/diff/5010/10008#newcode74
chrome/browser/sync/glue/ui_model_worker.h:74: // Upon receiving this idempotent
call, the ModelSafeWorkerInterface can
Same here.  Please grep.

http://codereview.chromium.org/553015/diff/5010/10015
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/553015/diff/5010/10015#newcode105
chrome/browser/sync/sessions/sync_session.h:105: const
std::vector<ModelSafeWorker*>& workers() { return workers_; }
() const {

http://codereview.chromium.org/553015/diff/5010/10009
File chrome/browser/sync/syncable/model_type.h (right):

http://codereview.chromium.org/553015/diff/5010/10009#newcode12
chrome/browser/sync/syncable/model_type.h:12: // TODO(tim): Make this the only
data type enum (remove SyncDataType).
TODO(tim): remove this todo.

[tim (not reviewing), 2010-01-21 19:28:06]

http://codereview.chromium.org/553015/diff/5010/10017
File chrome/browser/sync/engine/apply_updates_command_unittest.cc (right):

http://codereview.chromium.org/553015/diff/5010/10017#newcode47
chrome/browser/sync/engine/apply_updates_command_unittest.cc:47: 
On 2010/01/21 18:16:39, nick wrote:
> // ModelSafeWorkerRegistrar implementation.

Done.

http://codereview.chromium.org/553015/diff/5010/10020
File chrome/browser/sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/553015/diff/5010/10020#newcode130
chrome/browser/sync/engine/syncer_unittest.cc:130: }
On 2010/01/21 18:16:39, nick wrote:
> Should this be factored out into a test_model_safe_worker_registrar?

Two tests share this impl, but others have a different impl, and this stuff
isn't used all over yet so it's hard to say exactly what the most common test
use will be... I'll hold off for now till I have a better picture.

http://codereview.chromium.org/553015/diff/5010/10003
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/553015/diff/5010/10003#newcode86
chrome/browser/sync/glue/sync_backend_host.cc:86: void
SyncBackendHost::Shutdown(bool sync_disabled) {
On 2010/01/21 18:16:39, nick wrote:
> Shutdown can be called when Initialize() didn't complete fully.  Thus, you
> should be sure not to assume that the workers are non-NULL.   Check the
pointers
> before you deref them.

Good catch.  This has been like that since the beginning.

http://codereview.chromium.org/553015/diff/5010/10003#newcode100
chrome/browser/sync/glue/sync_backend_host.cc:100:
static_cast<UIModelWorker*>(registrar_.workers[GROUP_UI])->Stop();
On 2010/01/21 18:16:39, nick wrote:
> This is still gross.  At the very least, can you move the cast into an
accessor
> in the registrar_ struct, use down_cast<> in the implementation, and use the
> accessor in all the places you've put the cast?
> 
> Alternatively, if you expect Stop() will be part of the MSW interface, why not
> make it part of the MSW interface?

Respectfully, I disagree that this is so bad as to warrant adding an accessor in
a struct (which, afaik, is a style guide violation), and I don't know what
down_cast is but I see no reference to it in the entire chrome tree (and I don't
see it in the public c++ style guide).

I tried to checkpoint this CL to make it a manageable review; think of this as
Tyvek wrapping for now.  I need to continue working on this exact area in my
next CL. I don't want to change the interface just yet.  For now this is limited
to one LOC.

http://codereview.chromium.org/553015/diff/5010/10008
File chrome/browser/sync/glue/ui_model_worker.h (right):

http://codereview.chromium.org/553015/diff/5010/10008#newcode70
chrome/browser/sync/glue/ui_model_worker.h:70: // ModelSafeWorkerInterface
implementation. Called on syncapi SyncerThread.
On 2010/01/21 18:16:39, nick wrote:
> This should be ModelSafeWorker (no Interface).

Done.

http://codereview.chromium.org/553015/diff/5010/10008#newcode74
chrome/browser/sync/glue/ui_model_worker.h:74: // Upon receiving this idempotent
call, the ModelSafeWorkerInterface can
On 2010/01/21 18:16:39, nick wrote:
> Same here.  Please grep.

Done.  These were the only two I missed.

http://codereview.chromium.org/553015/diff/5010/10015
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/553015/diff/5010/10015#newcode105
chrome/browser/sync/sessions/sync_session.h:105: const
std::vector<ModelSafeWorker*>& workers() { return workers_; }
On 2010/01/21 18:16:39, nick wrote:
> () const {

Done.

http://codereview.chromium.org/553015/diff/5010/10009
File chrome/browser/sync/syncable/model_type.h (right):

http://codereview.chromium.org/553015/diff/5010/10009#newcode12
chrome/browser/sync/syncable/model_type.h:12: // TODO(tim): Make this the only
data type enum (remove SyncDataType).
On 2010/01/21 18:16:39, nick wrote:
> TODO(tim): remove this todo.

Done.

[Munjal (Google), 2010-01-22 00:27:59]

One nit I stumbled upon.

http://codereview.chromium.org/553015/diff/1038/2110
File chrome/browser/sync/glue/ui_model_worker.cc (right):

http://codereview.chromium.org/553015/diff/1038/2110#newcode21
chrome/browser/sync/glue/ui_model_worker.cc:21: DLOG(WARNING) <<
"CallDoWorkFromModelSafeThreadAndWait called from "
Change CallDoWorkFromModelSafeThreadAndWait to oWorkAndWaitUntilDone