Introduce the plumbing necessary to report Unrecoverable error from model safe workers.

This is the first patch in a series of patches. This patch introduces the unrecoverable error info class which is plumbed up all ModelSafeWorker subclasses. 

The subclasses of ModelChangingSyncer command would be converted to return an unrecoverable error info in another CL.


BUG=100374
TEST=


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

[lipalani1, 2011-10-21 02:48:44]

Tim - this is for you to do a high level review of the DESIGN to recover users
from corrupted store.

I will have to fix unit tests etc and upload another patch sometime tmw. But I
would like to make sure you dont find any glaring issues in this design.

Basically we do the following:
We could encounter a store corruption executing on any thread. So syncable layer
passes the error(as a bool but we could change it a struct later) to the syncer
layer(like ProcessUpdateCommand). Then that layer will set the error into a
variable in sync_session object and return right away.
Then when the execution returns to the syncer_thread on the
ModelChangingSyncerCommand object we check the session object for any
unrecoverable error and if found we use the delegate to inform of the
unrecoverable error to the upper layer and also to request_early_exit.
We also store the presence of unrecoverable_error in the DirectoryObject. This
is so that during sync shutdown(as a result of unrecoverable error) we dont
perform any store operations(which might crash because the store is corrupted).

[lipalani1, 2011-10-21 21:55:41]

Adding Fred.

[akalin, 2011-10-21 22:18:26]

http://codereview.chromium.org/8366030/diff/2001/chrome/browser/sync/engine/m...
File chrome/browser/sync/engine/model_changing_syncer_command.cc (right):

http://codereview.chromium.org/8366030/diff/2001/chrome/browser/sync/engine/m...
chrome/browser/sync/engine/model_changing_syncer_command.cc:52: if
(session->unrecoverable_error()) {
Why not just make the (optional) unrecoverable error a return value of
DoWorkAndWaitUntilDone?

Then, why not make that error a return value of ExecuteImpl and have the syncer
check it, then propagate up to the SyncScheduler, etc.?

[lipalani1, 2011-10-21 23:40:03]

Summarizing for Tim:
I discussed this with Fred. I agree with the suggestion here. It would make the
code more linear. 
he also suggested splitting up the CLs into few major pieces which sounds good
to me as well.

I think I would have the split up patches ready to be reviewed soon.

http://codereview.chromium.org/8366030/diff/2001/chrome/browser/sync/engine/m...
File chrome/browser/sync/engine/model_changing_syncer_command.cc (right):

http://codereview.chromium.org/8366030/diff/2001/chrome/browser/sync/engine/m...
chrome/browser/sync/engine/model_changing_syncer_command.cc:52: if
(session->unrecoverable_error()) {
Agreed
On 2011/10/21 22:18:27, akalin wrote:
> Why not just make the (optional) unrecoverable error a return value of
> DoWorkAndWaitUntilDone?
> 
> Then, why not make that error a return value of ExecuteImpl and have the
syncer
> check it, then propagate up to the SyncScheduler, etc.?

[lipalani1, 2011-10-24 23:10:30]

OK the first patch is ready for review. Can either or both of you review. Quick
response is appreciated as this is just the first in a series of patches.

[akalin, 2011-10-24 23:20:49]

http://codereview.chromium.org/8366030/diff/12005/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/model_safe_worker.h (right):

http://codereview.chromium.org/8366030/diff/12005/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_safe_worker.h:56: virtual void
DoWorkAndWaitUntilDone(
It doesn't make sense to do the base class first and the derived class later
since you're changing the type signature.  That just means that all function
calls will go to this one and that'll break.

http://codereview.chromium.org/8366030/diff/12005/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_safe_worker.h:57:
Callback1<sessions::UnrecoverableErrorInfo*>::Type* work,
the callbacktype should be ErrorInfo(void) (i.e., the work function should
return the struct directly)

http://codereview.chromium.org/8366030/diff/12005/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_safe_worker.h:58:
sessions::UnrecoverableErrorInfo* error_info);
make this return the ErrorInfo class directly

http://codereview.chromium.org/8366030/diff/12005/chrome/browser/sync/session...
File chrome/browser/sync/sessions/unrecoverable_error_info.h (right):

http://codereview.chromium.org/8366030/diff/12005/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.h:21: void Set(const
tracked_objects::Location& location,
add constructor that takes in the arguments instead

http://codereview.chromium.org/8366030/diff/12005/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.h:30:
scoped_ptr<tracked_objects::Location> location_;
no need for scoped_ptr now (Location objs are now assignable)

http://codereview.chromium.org/8366030/diff/12005/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.h:33:
DISALLOW_COPY_AND_ASSIGN(UnrecoverableErrorInfo);
since there's no more scoped_ptr, this can be assignable

[lipalani1, 2011-10-25 02:29:46]

Added OVERRIDE keyword to DoWorkandWaitUntilDone so that the overriden
characteristic is obvious and such mistakes wont be made again :)

http://codereview.chromium.org/8366030/diff/12005/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/model_safe_worker.h (right):

http://codereview.chromium.org/8366030/diff/12005/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_safe_worker.h:56: virtual void
DoWorkAndWaitUntilDone(
On 2011/10/24 23:20:49, akalin wrote:
> It doesn't make sense to do the base class first and the derived class later
> since you're changing the type signature.  That just means that all function
> calls will go to this one and that'll break.

Done.

http://codereview.chromium.org/8366030/diff/12005/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_safe_worker.h:57:
Callback1<sessions::UnrecoverableErrorInfo*>::Type* work,
As discussed we have to pass the pointer because it is in another thread..
On 2011/10/24 23:20:49, akalin wrote:
> the callbacktype should be ErrorInfo(void) (i.e., the work function should
> return the struct directly)

http://codereview.chromium.org/8366030/diff/12005/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_safe_worker.h:58:
sessions::UnrecoverableErrorInfo* error_info);
On 2011/10/24 23:20:49, akalin wrote:
> make this return the ErrorInfo class directly

Done.

http://codereview.chromium.org/8366030/diff/12005/chrome/browser/sync/session...
File chrome/browser/sync/sessions/unrecoverable_error_info.h (right):

http://codereview.chromium.org/8366030/diff/12005/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.h:21: void Set(const
tracked_objects::Location& location,
On 2011/10/24 23:20:49, akalin wrote:
> add constructor that takes in the arguments instead

Done.

http://codereview.chromium.org/8366030/diff/12005/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.h:30:
scoped_ptr<tracked_objects::Location> location_;
On 2011/10/24 23:20:49, akalin wrote:
> no need for scoped_ptr now (Location objs are now assignable)

Done.

http://codereview.chromium.org/8366030/diff/12005/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.h:33:
DISALLOW_COPY_AND_ASSIGN(UnrecoverableErrorInfo);
On 2011/10/24 23:20:49, akalin wrote:
> since there's no more scoped_ptr, this can be assignable

Done.

[akalin, 2011-10-25 03:20:06]

Also, please fix CL description/summary

http://codereview.chromium.org/8366030/diff/12005/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/model_safe_worker.h (right):

http://codereview.chromium.org/8366030/diff/12005/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_safe_worker.h:57:
Callback1<sessions::UnrecoverableErrorInfo*>::Type* work,
On 2011/10/25 02:29:46, lipalani1 wrote:
> As discussed we have to pass the pointer because it is in another thread..
> On 2011/10/24 23:20:49, akalin wrote:
> > the callbacktype should be ErrorInfo(void) (i.e., the work function should
> > return the struct directly)

Actually, it's only CallDoWorkAndSignalTask (e.g.) that must take a pointer. 
The callback itself can return the struct directly.  To clarify, it'll look like
this:

PostTask(..., CallDoWorkAndSIgnalTask(done, event, &error_info));

....

void CallDoWorkAndSignalTask(Callback1 ... done, ..., ErrorInfo* error_info) {
  *error_info = done->Run();
  Signal();
}

To put it in another way, you don't want the fact that some subclasses of
ModelSafeWorker posts on other threads to leak into the ModelSafeWorker
interface.

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/model_changing_syncer_command.cc (right):

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_changing_syncer_command.cc:54:
worker->DoWorkAndWaitUntilDone(c.get());
Plumb up the error_info all the way up to StartChangingModel.

I'm surprised the compiler doesn't warn/error on the unused variable.

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/model_changing_syncer_command.h (right):

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_changing_syncer_command.h:36: void
StartChangingModel(sessions::UnrecoverableErrorInfo* error_info) {
if you're going to change this, this should return the struct directly, and
callers should explicitly ignore_result() and have a TODO.

Or you could leave this as is and ignore_result() the return value of
ModelChangingExecuteImpl.

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/model_safe_worker.cc (right):

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_safe_worker.cc:83: work->Run(&error_info);  //
For GROUP_PASSIVE,
put this as a single-line comment above this line

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/model_safe_worker.h (right):

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_safe_worker.h:57:
Callback1<sessions::UnrecoverableErrorInfo*>::Type* work);
probably easier to provide a typedef for the callback, like:

typedef Callback1<...>::Type WorkCallback;

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/session...
File chrome/browser/sync/sessions/unrecoverable_error_info.cc (right):

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.cc:27: DCHECK(IsSet() ==
false);
i think it's fine to allow resetting, i.e. this DCHECK isn't needed

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.cc:42: const std::string&
UnrecoverableErrorInfo::message() const {
DCHECK(set_) here, too

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/session...
File chrome/browser/sync/sessions/unrecoverable_error_info.h (right):

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.h:5: #ifndef
CHROME_BROWSER_SYNC_SESSIONS_UNRECOVERABLE_ERROR_INFO_H_
I don't think this class really belongs in sessions/.  What about util/?

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.h:12: #include
"base/memory/scoped_ptr.h"
remove this include

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.h:24: void Set(const
tracked_objects::Location& location,
Reset is probably a better name

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.h:35: bool set_;
probably should be named is_set_

[akalin, 2011-10-25 03:29:41]

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/model_changing_syncer_command.h (right):

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_changing_syncer_command.h:36: void
StartChangingModel(sessions::UnrecoverableErrorInfo* error_info) {
On 2011/10/25 03:20:06, akalin wrote:
> if you're going to change this, this should return the struct directly, and
> callers should explicitly ignore_result() and have a TODO.
> 
> Or you could leave this as is and ignore_result() the return value of
> ModelChangingExecuteImpl.

Sorry, I got confused.  This is the function that gets posted.  But it should
return the struct directly anyway.

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_changing_syncer_command.h:37: //
TODO(lipalani): Pass in error_info parameter to this function.
The TODO should read: Change ModelChangingExecuteImpl to return an
UnrecoverableErrorInfo struct.

[lipalani1, 2011-10-26 00:12:13]

The main problem was old callbacks did not support the functionality to return
arguments and take more than zero argument. As obvious from your feedback it is
clear that any new reader of the code would have the same questions. So changed
the functions to use the new callback.

PTAL.

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/model_changing_syncer_command.h (right):

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_changing_syncer_command.h:36: void
StartChangingModel(sessions::UnrecoverableErrorInfo* error_info) {
I dont want to change this internally just yet. Let me know what you think.
On 2011/10/25 03:29:41, akalin wrote:
> On 2011/10/25 03:20:06, akalin wrote:
> > if you're going to change this, this should return the struct directly, and
> > callers should explicitly ignore_result() and have a TODO.
> > 
> > Or you could leave this as is and ignore_result() the return value of
> > ModelChangingExecuteImpl.
> 
> Sorry, I got confused.  This is the function that gets posted.  But it should
> return the struct directly anyway.

Done.

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_changing_syncer_command.h:36: void
StartChangingModel(sessions::UnrecoverableErrorInfo* error_info) {
On 2011/10/25 03:20:06, akalin wrote:
> if you're going to change this, this should return the struct directly, and
> callers should explicitly ignore_result() and have a TODO.
> 
> Or you could leave this as is and ignore_result() the return value of
> ModelChangingExecuteImpl.

Done.

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_changing_syncer_command.h:37: //
TODO(lipalani): Pass in error_info parameter to this function.
On 2011/10/25 03:29:41, akalin wrote:
> The TODO should read: Change ModelChangingExecuteImpl to return an
> UnrecoverableErrorInfo struct.

Done.

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/model_safe_worker.cc (right):

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_safe_worker.cc:83: work->Run(&error_info);  //
For GROUP_PASSIVE,
On 2011/10/25 03:20:06, akalin wrote:
> put this as a single-line comment above this line

Done.

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/model_safe_worker.h (right):

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_safe_worker.h:57:
Callback1<sessions::UnrecoverableErrorInfo*>::Type* work);
On 2011/10/25 03:20:06, akalin wrote:
> probably easier to provide a typedef for the callback, like:
> 
> typedef Callback1<...>::Type WorkCallback;

Done.

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/session...
File chrome/browser/sync/sessions/unrecoverable_error_info.cc (right):

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.cc:27: DCHECK(IsSet() ==
false);
On 2011/10/25 03:20:06, akalin wrote:
> i think it's fine to allow resetting, i.e. this DCHECK isn't needed

Done.

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.cc:42: const std::string&
UnrecoverableErrorInfo::message() const {
On 2011/10/25 03:20:06, akalin wrote:
> DCHECK(set_) here, too

Done.

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/session...
File chrome/browser/sync/sessions/unrecoverable_error_info.h (right):

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.h:5: #ifndef
CHROME_BROWSER_SYNC_SESSIONS_UNRECOVERABLE_ERROR_INFO_H_
added a todo.
On 2011/10/25 03:20:06, akalin wrote:
> I don't think this class really belongs in sessions/.  What about util/?

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.h:12: #include
"base/memory/scoped_ptr.h"
On 2011/10/25 03:20:06, akalin wrote:
> remove this include

Done.

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.h:24: void Set(const
tracked_objects::Location& location,
On 2011/10/25 03:20:06, akalin wrote:
> Reset is probably a better name

Done.

http://codereview.chromium.org/8366030/diff/16002/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.h:35: bool set_;
On 2011/10/25 03:20:06, akalin wrote:
> probably should be named is_set_

Done.

[akalin, 2011-10-26 00:25:04]

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/model_changing_syncer_command.cc (left):

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_changing_syncer_command.cc:50:
worker->DoWorkAndWaitUntilDone(c.get());
Add a comment saying that it's safe to use Unretained here, since we block until
after the callback is called.

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/model_changing_syncer_command.cc (right):

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_changing_syncer_command.cc:54:
UnrecoverableErrorInfo error_info = worker->DoWorkAndWaitUntilDone(c);
this will probably trigger a compiler warning/error since you're not using it.

#include basictypes.h and do:

// TODO(lipalani): Check the return value for an unrecoverable error ...
ignore_result(worker->DoWorkAndWiatUntilDone(c));

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/model_changing_syncer_command.h (right):

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_changing_syncer_command.h:40: //
UnrecoverableErrorInfo struct..
.. -> .

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/model_safe_worker.h (right):

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_safe_worker.h:62: virtual ModelSafeGroup
GetModelSafeGroup();
please mark all overrides of GetModelSafeGroup in the subclasses (since you're
already adding it for DoWorkAndWaitUntilDone)

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/glue/br...
File chrome/browser/sync/glue/browser_thread_model_worker_unittest.cc (right):

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/glue/br...
chrome/browser/sync/glue/browser_thread_model_worker_unittest.cc:43:
WorkCallback c = base::Bind(&BrowserThreadModelWorkerTest::DoWork,
comment about unretained

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/glue/ui...
File chrome/browser/sync/glue/ui_model_worker_unittest.cc (right):

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/glue/ui...
chrome/browser/sync/glue/ui_model_worker_unittest.cc:51:
browser_sync::WorkCallback c = base::Bind(&UIModelWorkerVisitor::DoWork,
comment about unretained

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/session...
File chrome/browser/sync/sessions/unrecoverable_error_info.cc (right):

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.cc:6: namespace
browser_sync {
space after #include

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/session...
File chrome/browser/sync/sessions/unrecoverable_error_info.h (right):

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.h:16: namespace sessions {
space before class

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.h:26: const std::string&
message);
indent

[lipalani1, 2011-10-26 01:39:11]

PTAL.

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/model_changing_syncer_command.cc (left):

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_changing_syncer_command.cc:50:
worker->DoWorkAndWaitUntilDone(c.get());
On 2011/10/26 00:25:05, akalin wrote:
> Add a comment saying that it's safe to use Unretained here, since we block
until
> after the callback is called.

Done.

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/model_changing_syncer_command.cc (right):

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_changing_syncer_command.cc:54:
UnrecoverableErrorInfo error_info = worker->DoWorkAndWaitUntilDone(c);
On 2011/10/26 00:25:05, akalin wrote:
> this will probably trigger a compiler warning/error since you're not using it.
> 
> #include basictypes.h and do:
> 
> // TODO(lipalani): Check the return value for an unrecoverable error ...
> ignore_result(worker->DoWorkAndWiatUntilDone(c));

Done.

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/model_changing_syncer_command.h (right):

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_changing_syncer_command.h:40: //
UnrecoverableErrorInfo struct..
On 2011/10/26 00:25:05, akalin wrote:
> .. -> .

Done.

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/model_safe_worker.h (right):

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_safe_worker.h:62: virtual ModelSafeGroup
GetModelSafeGroup();
On 2011/10/26 00:25:05, akalin wrote:
> please mark all overrides of GetModelSafeGroup in the subclasses (since you're
> already adding it for DoWorkAndWaitUntilDone)

Done.

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/glue/br...
File chrome/browser/sync/glue/browser_thread_model_worker_unittest.cc (right):

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/glue/br...
chrome/browser/sync/glue/browser_thread_model_worker_unittest.cc:43:
WorkCallback c = base::Bind(&BrowserThreadModelWorkerTest::DoWork,
On 2011/10/26 00:25:05, akalin wrote:
> comment about unretained

Done.

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/glue/ui...
File chrome/browser/sync/glue/ui_model_worker_unittest.cc (right):

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/glue/ui...
chrome/browser/sync/glue/ui_model_worker_unittest.cc:51:
browser_sync::WorkCallback c = base::Bind(&UIModelWorkerVisitor::DoWork,
On 2011/10/26 00:25:05, akalin wrote:
> comment about unretained

Done.

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/session...
File chrome/browser/sync/sessions/unrecoverable_error_info.cc (right):

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.cc:6: namespace
browser_sync {
On 2011/10/26 00:25:05, akalin wrote:
> space after #include

Done.

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/session...
File chrome/browser/sync/sessions/unrecoverable_error_info.h (right):

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.h:16: namespace sessions {
On 2011/10/26 00:25:05, akalin wrote:
> space before class

Done.

http://codereview.chromium.org/8366030/diff/19012/chrome/browser/sync/session...
chrome/browser/sync/sessions/unrecoverable_error_info.h:26: const std::string&
message);
On 2011/10/26 00:25:05, akalin wrote:
> indent

Done.

[akalin, 2011-10-26 01:44:45]

LGTM

http://codereview.chromium.org/8366030/diff/26002/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/model_changing_syncer_command.cc (right):

http://codereview.chromium.org/8366030/diff/26002/chrome/browser/sync/engine/...
chrome/browser/sync/engine/model_changing_syncer_command.cc:8: #include
"base/basictypes.h"
put before callback_old.h

http://codereview.chromium.org/8366030/diff/26002/chrome/browser/sync/glue/br...
File chrome/browser/sync/glue/browser_thread_model_worker_unittest.cc (right):

http://codereview.chromium.org/8366030/diff/26002/chrome/browser/sync/glue/br...
chrome/browser/sync/glue/browser_thread_model_worker_unittest.cc:44: // We wait
until the callback is done. So it is safe to use unretained.
You can just put the comment above the expression, and indent the line properly.

[commit-bot: I haz the power, 2011-10-27 05:28:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lipalani@chromium.org/8366030/28005

[commit-bot: I haz the power, 2011-10-27 06:34:37]

Change committed as 107535