Relieve SyncerSession,SyncCycleState, SyncProcessState, SyncerSession, Syncer...

Add browser_sync 'sessions' to relieve SyncCycleState, SyncProcessState, SyncerSession,
SyncerStatus, and ConflictResolutionView of duty.
Main impact is factors all status munging to 'StatusController', adds SyncSessionContext
to wrap various engine parts needed by different components, removes duplicated methods by a
factor of ~3 making it easier to reason about, and adds a 'Controller' to the session object to
give a way to delegate session-global (i.e affecting any session) occurrences such as throttling.

Also adds testing for 'HasMoreToSync' and other session related code.

BUG=25266
TEST=SyncSessionTest(added), StatusControllerTest(added)
     various sync_unit_tests in this CL


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

[ncarter (slow), 2009-11-14 00:13:17]

http://codereview.chromium.org/386030/diff/1/29
File chrome/browser/sync/engine/all_status.cc (right):

http://codereview.chromium.org/386030/diff/1/29#newcode96
Line 96: sessions::SyncSessionSnapshot* snapshot(event.snapshot);
Use = for initialization of a scalar type.  This should be const.

http://codereview.chromium.org/386030/diff/1/44
File chrome/browser/sync/engine/syncer.h (right):

http://codereview.chromium.org/386030/diff/1/44#newcode17
Line 17: #include "chrome/browser/sync/sessions/sync_session.h"
These files are not present in the change.

[tesigoadondequieras_gmail.com, 2009-11-14 00:15:37]

Remover
----- Original Message ----- 
From: <nick@chromium.org>
To: <tim@chromium.org>; <chron@chromium.org>
Cc: <chromium-reviews@googlegroups.com>; <ben+cc@chromium.org>; 
<paul@chromium.org>; <idana@chromium.org>; <phajdan.jr@chromium.org>; 
<tim@chromium.org>
Sent: Friday, November 13, 2009 10:13 PM
Subject: [chromium-reviews] Re: Relieve SyncerSession,SyncCycleState, 
SyncProcessState, SyncerSession, Syncer...


>
> http://codereview.chromium.org/386030/diff/1/29
> File chrome/browser/sync/engine/all_status.cc (right):
>
> http://codereview.chromium.org/386030/diff/1/29#newcode96
> Line 96: sessions::SyncSessionSnapshot* snapshot(event.snapshot);
> Use = for initialization of a scalar type.  This should be const.
>
> http://codereview.chromium.org/386030/diff/1/44
> File chrome/browser/sync/engine/syncer.h (right):
>
> http://codereview.chromium.org/386030/diff/1/44#newcode17
> Line 17: #include "chrome/browser/sync/sessions/sync_session.h"
> These files are not present in the change.
>
> http://codereview.chromium.org/386030 

[tim (not reviewing), 2009-11-14 00:47:01]

Please see the description. I mention that this CL requires the other. I wanted
to check the other one in on its own, originally because it was reviewable on
its own and because it could have went in to the beta branch inactive, though
the latter is not a concern now.  I can add the gyp changes to it, though.

[tim (not reviewing), 2009-11-16 16:41:17]

http://codereview.chromium.org/386030/diff/1/29
File chrome/browser/sync/engine/all_status.cc (right):

http://codereview.chromium.org/386030/diff/1/29#newcode96
Line 96: sessions::SyncSessionSnapshot* snapshot(event.snapshot);
On 2009/11/14 00:13:17, nick wrote:
> Use = for initialization of a scalar type.  This should be const.

Done.

[ncarter (slow), 2009-11-16 16:47:57]

I see my confusion; you said 'This requires
http://codereview.chromium.org/385103 ' but I think you meant a different
link.  Anyhow, I'm up to speed.  I think it'll be better to combine them so
that we can review the new data structure alongside its usage.

 - nick

On Fri, Nov 13, 2009 at 4:47 PM, <tim@chromium.org> wrote:

> Please see the description. I mention that this CL requires the other. I
> wanted
> to check the other one in on its own, originally because it was reviewable
> on
> its own and because it could have went in to the beta branch inactive,
> though
> the latter is not a concern now.  I can add the gyp changes to it, though.
>
>
>
> http://codereview.chromium.org/386030
>

[tim (not reviewing), 2009-11-16 17:05:47]

As you wish.

[chron_chromium.org, 2009-11-17 23:09:17]

http://codereview.chromium.org/386030/diff/4002/9028
File chrome/browser/sync/engine/all_status.cc (right):

http://codereview.chromium.org/386030/diff/4002/9028#newcode104
Line 104: // Show a syncer as syncing if it's got stalled updates.
Remove this comment about stalled updates. It was about "blocked" items, which
should no longer occur.

http://codereview.chromium.org/386030/diff/4002/9027
File chrome/browser/sync/engine/apply_updates_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9027#newcode25
Line 25: virtual void ModelChangingExecuteImpl(sessions::SyncSession* session);
include the .h file for this structure

http://codereview.chromium.org/386030/diff/4002/9016
File chrome/browser/sync/engine/apply_updates_command_unittest.cc (right):

http://codereview.chromium.org/386030/diff/4002/9016#newcode41
Line 41: }
Add consistent newlines between functions, or consistently no newlines.

http://codereview.chromium.org/386030/diff/4002/9002
File chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc
(right):

http://codereview.chromium.org/386030/diff/4002/9002#newcode51
Line 51: session->context()->resolver(), session->status_controller());
does this fit on three lines like the above dir lookup?

http://codereview.chromium.org/386030/diff/4002/9038
File chrome/browser/sync/engine/build_and_process_conflict_sets_command.h
(right):

http://codereview.chromium.org/386030/diff/4002/9038#newcode11
Line 11: #include "chrome/browser/sync/engine/model_changing_syncer_command.h"
Does this file need to import syncersession.h?

http://codereview.chromium.org/386030/diff/4002/9015
File chrome/browser/sync/engine/build_commit_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9015#newcode10
Line 10: #include "chrome/browser/sync/engine/syncproto.h"
Does syncproto include sync session? Should sync session still be explicitly
included? Or forward declared?

http://codereview.chromium.org/386030/diff/4002/9003
File chrome/browser/sync/engine/download_updates_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9003#newcode20
Line 20: virtual void ExecuteImpl(sessions::SyncSession* session);
Why was the .h removed from here? Should you forward declare?

http://codereview.chromium.org/386030/diff/4002/9034
File chrome/browser/sync/engine/get_commit_ids_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9034#newcode159
Line 159: // later. ValidateCommitEntry writes to the DB, and we add the blocked
can you remove this comment about blocked items?

http://codereview.chromium.org/386030/diff/4002/9009
File chrome/browser/sync/engine/post_commit_message_command.cc (right):

http://codereview.chromium.org/386030/diff/4002/9009#newcode23
Line 23: if (session->status_controller()->commit_ids().empty())
Why is commit_ids part of status controller?  Is status controller weirdly
named?

http://codereview.chromium.org/386030/diff/4002/9009#newcode32
Line 32: status->mutable_commit_message(), &response, session)) {
It seems weird that status has a mutable commit message.

http://codereview.chromium.org/386030/diff/4002/9011
File chrome/browser/sync/engine/post_commit_message_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9011#newcode19
Line 19: virtual void ExecuteImpl(sessions::SyncSession* session);
Does this need forward decl or include?

http://codereview.chromium.org/386030/diff/4002/9023
File chrome/browser/sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/386030/diff/4002/9023#newcode65
Line 65: monitor->PutRecords(session->extensions_activity());
this extensions activity monitor here does seem kind of like a weird side
effect, wish it was like an observer pattern

http://codereview.chromium.org/386030/diff/4002/9042
File chrome/browser/sync/engine/process_commit_response_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9042#newcode29
Line 29: virtual void ModelChangingExecuteImpl(sessions::SyncSession* session);
Do we need to include or declare?

http://codereview.chromium.org/386030/diff/4002/9010
File chrome/browser/sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/386030/diff/4002/9010#newcode37
Line 37: session->status_controller()->updates_response().get_updates();
the response coming back in the status controller is a little weird..

http://codereview.chromium.org/386030/diff/4002/9041
File chrome/browser/sync/engine/process_updates_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9041#newcode34
Line 34: virtual void ModelChangingExecuteImpl(sessions::SyncSession* session);
why no forward decl or .h?

http://codereview.chromium.org/386030/diff/4002/9048
File chrome/browser/sync/engine/resolve_conflicts_command.cc (right):

http://codereview.chromium.org/386030/diff/4002/9048#newcode19
Line 19: DCHECK(resolver);
These semantics changed. Can resolver no longer be null?

http://codereview.chromium.org/386030/diff/4002/9019
File chrome/browser/sync/engine/resolve_conflicts_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9019#newcode25
Line 25: virtual void ModelChangingExecuteImpl(sessions::SyncSession* session);
forward decl? or .h?

http://codereview.chromium.org/386030/diff/4002/9040
File chrome/browser/sync/engine/syncer.cc (right):

http://codereview.chromium.org/386030/diff/4002/9040#newcode176
Line 176: sessions::ScopedSetSessionWriteTransaction set_trans(session, &trans);
Why SetSessionWriteTransaction?
Does it write to a set, or to a session?

http://codereview.chromium.org/386030/diff/4002/9006
File chrome/browser/sync/engine/syncer_command.cc (right):

http://codereview.chromium.org/386030/diff/4002/9006#newcode32
Line 32: if (session->status_controller()->TestAndResetIsDirty()) {
TestAndClear might be clearer
or just plain ClearIsDirty

http://codereview.chromium.org/386030/diff/4002/9006#newcode44
Line 44: if (session->auth_failure_occurred()) {
why not follow the same pattern?

http://codereview.chromium.org/386030/diff/4002/9054
File chrome/browser/sync/engine/syncer_end_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9054#newcode25
Line 25: virtual void ExecuteImpl(sessions::SyncSession* session);
.h or decl?

http://codereview.chromium.org/386030/diff/4002/9032
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/386030/diff/4002/9032#newcode157
Line 157: session->controller()->OnSilencedUntil(base::TimeTicks::Now() +
What's the difference between controller and status controller?

http://codereview.chromium.org/386030/diff/4002/9022
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/386030/diff/4002/9022#newcode429
Line 429: // Clean slate for SyncSession::Controller impl.
What does this comment mean?

http://codereview.chromium.org/386030/diff/4002/9037
File chrome/browser/sync/engine/syncer_thread.h (left):

http://codereview.chromium.org/386030/diff/4002/9037#oldcode144
Line 144: SyncerThread();  // Necessary for temporary pthreads-based PIMPL impl.
Aww. We lost our PIMP.

http://codereview.chromium.org/386030/diff/4002/9037
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/386030/diff/4002/9037#newcode296
Line 296: base::TimeTicks silenced_until_;
While I think this does belong here, how did this end up here in this CL?

http://codereview.chromium.org/386030/diff/4002/9021
File chrome/browser/sync/engine/syncer_thread_unittest.cc (right):

http://codereview.chromium.org/386030/diff/4002/9021#newcode58
Line 58: // throttled.  Only call this if a throttle is supposed to occur!
Should there be some preconditions for this call? Variable wise?

http://codereview.chromium.org/386030/diff/4002/9021#newcode76
Line 76: base::WaitableEvent sync_cycle_ended_event_;
Sort of confusing that the WaitableEvent is named similarly to the hookup.

http://codereview.chromium.org/386030/diff/4002/9021#newcode192
Line 192: SyncSessionContext* context = new SyncSessionContext(NULL, NULL,
NULL);
Would it make sense to have a default constructor?

http://codereview.chromium.org/386030/diff/4002/9007
File chrome/browser/sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/386030/diff/4002/9007#newcode106
Line 106: }
Consistent endl after functions, or no endl after functions

http://codereview.chromium.org/386030/diff/4002/9007#newcode205
Line 205: // tests.?!?!?!?!?!
Replace with an interrobang. On second thought, remove comment about
brittleness. It's brittle.

http://codereview.chromium.org/386030/diff/4002/9007#newcode311
Line 311: ConflictProgress* progress =
Conflict Progress? Maybe a better name for this.

http://codereview.chromium.org/386030/diff/4002/9007#newcode346
Line 346: void FailTest(const std::string& msg){
Is this better than the FAIL() << MSG?

http://codereview.chromium.org/386030/diff/4002/9007#newcode2223
Line 2223: EXPECT_TRUE(0 == progress->ConflictingItemsSize());
This naming has lost the essence of "conflicting updates". Items != updates? If
the previous naming left out something then this is fine.

http://codereview.chromium.org/386030/diff/4002/9055
File chrome/browser/sync/engine/verify_updates_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9055#newcode27
Line 27: // SyncerCommand implementation.
Forward decl or include used?

http://codereview.chromium.org/386030/diff/4002/9059
File chrome/browser/sync/sessions/session_state.cc (right):

http://codereview.chromium.org/386030/diff/4002/9059#newcode164
Line 164: applied_updates_.begin();
should this indent a little?

http://codereview.chromium.org/386030/diff/4002/9061
File chrome/browser/sync/sessions/session_state.h (right):

http://codereview.chromium.org/386030/diff/4002/9061#newcode201
Line 201: #endif  // CHROME_BROWSER_SYNC_SESSIONS_SESSION_STATE_H_
this is better than before :)

http://codereview.chromium.org/386030/diff/4002/9064
File chrome/browser/sync/sessions/status_controller.h (right):

http://codereview.chromium.org/386030/diff/4002/9064#newcode133
Line 133: class Dirtyable {
Maybe the testandreset thing should go in here?

http://codereview.chromium.org/386030/diff/4002/9062
File chrome/browser/sync/sessions/status_controller_unittest.cc (right):

http://codereview.chromium.org/386030/diff/4002/9062#newcode10
Line 10: namespace {
Is this extra namespace necessary for the whole file scope?

http://codereview.chromium.org/386030/diff/4002/9057
File chrome/browser/sync/sessions/sync_session.cc (right):

http://codereview.chromium.org/386030/diff/4002/9057#newcode20
Line 20: bool is_share_usable = false;
move down and combine with assignment, set const

http://codereview.chromium.org/386030/diff/4002/9058
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/386030/diff/4002/9058#newcode143
Line 143: ~ScopedSetSessionWriteTransaction() { session_->write_transaction_ =
NULL; }
endl between this and the next line might be more clear

http://codereview.chromium.org/386030/diff/4002/9060
File chrome/browser/sync/sessions/sync_session_context.h (right):

http://codereview.chromium.org/386030/diff/4002/9060#newcode47
Line 47: // In unittests, there may be no UI thread, so the above will fail.
Usually you comment before the line. So move this up and talk about the below
line?

http://codereview.chromium.org/386030/diff/4002/9060#newcode145
Line 145: DCHECK(!context->syncer_event_channel_);
NULL == slightly more clear

[ncarter (slow), 2009-11-18 00:21:40]

It only took me 12 hours to review this change.

http://codereview.chromium.org/386030/diff/4002/9016
File chrome/browser/sync/engine/apply_updates_command_unittest.cc (right):

http://codereview.chromium.org/386030/diff/4002/9016#newcode29
Line 29: 
public:

http://codereview.chromium.org/386030/diff/4002/9016#newcode46
Line 46: }
This whole block needs +1 space of indentation.

http://codereview.chromium.org/386030/diff/4002/9016#newcode162
Line 162: EXPECT_EQ(2, status->conflict_progress()->ConflictingItemsSize())
Seems a bit weird here that we're writing into conflict progress during update
application, but after looking at how this field is used during conflict
resolution, I suppose it makes sense.

http://codereview.chromium.org/386030/diff/4002/9002
File chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc
(right):

http://codereview.chromium.org/386030/diff/4002/9002#newcode24
Line 24: using sessions::ConflictProgress;
sort.

http://codereview.chromium.org/386030/diff/4002/9015
File chrome/browser/sync/engine/build_commit_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9015#newcode20
Line 20: virtual void ExecuteImpl(sessions::SyncSession *session);
"* "

http://codereview.chromium.org/386030/diff/4002/9020
File chrome/browser/sync/engine/conflict_resolver.h (right):

http://codereview.chromium.org/386030/diff/4002/9020#newcode80
Line 80: syncable::Id id);
Pass id by reference.

http://codereview.chromium.org/386030/diff/4002/9051
File chrome/browser/sync/engine/get_commit_ids_command.cc (right):

http://codereview.chromium.org/386030/diff/4002/9051#newcode132
Line 132: &ordered_commit_set_);
This loop is approaching the unreadable.  Hopefully the simplifications I
suggested will allow it to use fewer lines.  If not, please try to make it so
that the loop setup isn't a five-line-long comma-and-semicolon-delimited blob of
goo.

x 3 because this goo is replicated below.

http://codereview.chromium.org/386030/diff/4002/9034
File chrome/browser/sync/engine/get_commit_ids_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9034#newcode37
Line 37: sessions::ConflictProgress* progress);
You plumb ConflictProgress all over the place in this h/cc file.  But I don't
see you ever use it.

http://codereview.chromium.org/386030/diff/4002/9034#newcode111
Line 111: CommitMetahandleIterator(vector<int64>::const_iterator unsynced_begin,
Why not pass in a reference to the container, rather than a pair of iterators?

http://codereview.chromium.org/386030/diff/4002/9034#newcode175
Line 175: sessions::ConflictProgress* const conflict_progress_;
It's unused, I think.

http://codereview.chromium.org/386030/diff/4002/9009
File chrome/browser/sync/engine/post_commit_message_command.cc (right):

http://codereview.chromium.org/386030/diff/4002/9009#newcode32
Line 32: status->mutable_commit_message(), &response, session)) {
Is it possible to avoid using the mutable variant here?

http://codereview.chromium.org/386030/diff/4002/9023
File chrome/browser/sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/386030/diff/4002/9023#newcode50
Line 50: status->increment_num_consecutive_errors();
Originally I freaked out when I saw this diff -- SyncerStatus was passed BY
VALUE in the original code.  But then I realized that it worked okay because the
only member was a pointer to the state, which was preserved on a copy.

http://codereview.chromium.org/386030/diff/4002/9010
File chrome/browser/sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/386030/diff/4002/9010#newcode36
Line 36: const GetUpdatesResponse& updates =
Thanks for eliminating the copy.

http://codereview.chromium.org/386030/diff/4002/9010#newcode70
Line 70: for (it = progress.VerifiedUpdatesBegin(); it <
progress.VerifiedUpdatesEnd();
< works fine because they're random access iterators.  != is more
idiomatic/familiar to me.  WDYT?

http://codereview.chromium.org/386030/diff/4002/9014
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/386030/diff/4002/9014#newcode1020
Line 1020: SyncSessionContext * context = new SyncSessionContext(
"* " not " * "

http://codereview.chromium.org/386030/diff/4002/9006
File chrome/browser/sync/engine/syncer_command.cc (right):

http://codereview.chromium.org/386030/diff/4002/9006#newcode34
Line 34: sessions::SyncSessionSnapshot snapshot(session->TakeSnapshot());
Why make a copy to take a snapshot, if it's going to be consumed synchronously
by the listeners?

http://codereview.chromium.org/386030/diff/4002/9024
File chrome/browser/sync/engine/syncer_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9024#newcode37
Line 37: virtual void ExecuteImpl(sessions::SyncSession* session) = 0;
I wonder if it would improve things, if Execute stashed the session away as a
member, and ExecuteImpl took no args.  This would get rid of a lot of the
passing-around we do.

http://codereview.chromium.org/386030/diff/4002/9022
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/386030/diff/4002/9022#newcode141
Line 141: disable_idle_detection_(false) {
Should we initialize silenced_until_ to Now()?

http://codereview.chromium.org/386030/diff/4002/9037
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/386030/diff/4002/9037#newcode210
Line 210: const base::TimeDelta& new_interval);
Should these really be private, if they're invoked through an interface?

http://codereview.chromium.org/386030/diff/4002/9037#newcode289
Line 289: // happening. Is this strict event handling order needed?!
Wow.  I had no idea.  That's jacked up.

http://codereview.chromium.org/386030/diff/4002/9021
File chrome/browser/sync/engine/syncer_thread_unittest.cc (right):

http://codereview.chromium.org/386030/diff/4002/9021#newcode623
Line 623: EXPECT_TRUE(syncer_thread()->IsSyncingCurrentlySilenced());
Was there an underlying behavior change that prompted this change to the test?

http://codereview.chromium.org/386030/diff/4002/9035
File chrome/browser/sync/engine/syncer_types.h (right):

http://codereview.chromium.org/386030/diff/4002/9035#newcode102
Line 102: SyncerEvent(EventCause cause) : what_happened(cause) {}
All the data members (int, Snapshot*) must be initialized by every constructor. 
the C-style struct initialization "Foo foo = { x }" guarantees default
initialization for scalar types even if they aren't matched with an element from
the RHS.  C++ ctors do not.  That's why the code historically used the C style
syntax.

http://codereview.chromium.org/386030/diff/4002/9007
File chrome/browser/sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/386030/diff/4002/9007#newcode98
Line 98: SyncerTest() {}
syncer_(NULL)

http://codereview.chromium.org/386030/diff/4002/9007#newcode182
Line 182: delete syncer_;
add line "syncer_ = NULL;"

http://codereview.chromium.org/386030/diff/4002/9007#newcode433
Line 433: status->ResetTransientState();
Why is it necessary to reset when the test has just begun?

http://codereview.chromium.org/386030/diff/4002/9007#newcode3609
Line 3609: SyncRepeatedlyToTriggerConflictResolution(session_.get());
Is it weird to switch between the SyncShare(Controller*) and SyncShare(Session*)
variants, mid-test?

http://codereview.chromium.org/386030/diff/4002/9053
File chrome/browser/sync/engine/verify_updates_command.cc (right):

http://codereview.chromium.org/386030/diff/4002/9053#newcode38
Line 38: const GetUpdatesResponse& updates =
status->updates_response().get_updates();
Thanks for removing the copy.

http://codereview.chromium.org/386030/diff/4002/9061
File chrome/browser/sync/sessions/session_state.h (right):

http://codereview.chromium.org/386030/diff/4002/9061#newcode5
Line 5: // The 'sessions' namespace is comprised of all the pieces of state that
are
"is composed of" or "comprises" but not "is comprised of".  Or in this case:
"wraps", "contains", "houses", "binds together", "collects".

The parts compose the whole.
The whole comprises the parts.
The whole is composed of several parts.

http://codereview.chromium.org/386030/diff/4002/9061#newcode37
Line 37: struct SyncerStatus {
Add a comment for the class.

http://codereview.chromium.org/386030/diff/4002/9061#newcode44
Line 44: // True iff we're stuck. User should contact support.
"contact support"

http://codereview.chromium.org/386030/diff/4002/9061#newcode50
Line 50: struct ErrorCounters {
Add a comment for the class.

http://codereview.chromium.org/386030/diff/4002/9061#newcode78
Line 78: SyncSessionSnapshot(const SyncerStatus& status,
Consider naming the params identically to the members.  Seems like you do a lot
of dancing here to pick alternate names; it's not necessary.

http://codereview.chromium.org/386030/diff/4002/9061#newcode83
Line 83: : syncer_status(status), errors(error_counters),
One-per-line is easier to read, IMO, but I'm not insisting.

http://codereview.chromium.org/386030/diff/4002/9061#newcode127
Line 127: bool progress_changed() const { return progress_changed_; }
Is this redundant to the dirty bit stored?

http://codereview.chromium.org/386030/diff/4002/9061#newcode181
Line 181: // within a while that session is in flight.  This is useful when
multiple
"within a while" doesn't parse.

http://codereview.chromium.org/386030/diff/4002/9061#newcode185
Line 185: items_committed(false), timestamp_dirty(false) {}
Move : down to the next line; one per line.

http://codereview.chromium.org/386030/diff/4002/9063
File chrome/browser/sync/sessions/status_controller.cc (right):

http://codereview.chromium.org/386030/diff/4002/9063#newcode22
Line 22: template <class FieldType, class DirtyableContainer>
nit: i'd use 'typename FieldType' since it's often bool/int.

http://codereview.chromium.org/386030/diff/4002/9063#newcode33
Line 33: conflict_progress_.progress_changed();
Why track 5 dirty bits if they're just ||ed together at the end of the day?  If
you were to unify them, you could make SetAndMarkDirtyIfChanged a nontemplatized
class member and drop its awkward third argument.

http://codereview.chromium.org/386030/diff/4002/9063#newcode115
Line 115: unsynced_handles, transient_.get());
Why is transient state dirtyable?  If it gets reconstructed each pass, then what
does it mean to be dirty?  Also -- none of the other transient fields seem to be
tracked by the dirty bit.

http://codereview.chromium.org/386030/diff/4002/9063#newcode143
Line 143: // These don't cause notifications to fire.
What notifications?  Please elaborate.

http://codereview.chromium.org/386030/diff/4002/9063#newcode178
Line 178: it < transient_->value()->update_progress.AppliedUpdatesEnd();
< or != ?

http://codereview.chromium.org/386030/diff/4002/9063#newcode180
Line 180: if (it->first == CONFLICT) {
De-indent.

http://codereview.chromium.org/386030/diff/4002/9064
File chrome/browser/sync/sessions/status_controller.h (right):

http://codereview.chromium.org/386030/diff/4002/9064#newcode93
Line 93: bool HasConflictingUpdates() const;
It seems inconsistent that this lives here, but SuccessfullyAppliedUpdateCount()
lives in the UpdateProgress -- they're nearly counterparts.  What's the
philosophy surrounding whether functionality lives in the Controller or in a
Progress group?

http://codereview.chromium.org/386030/diff/4002/9064#newcode119
Line 119: void set_unsynced_handles(const std::vector<int64> unsynced_handles);
Passing a vector by value?

For the vector members, a mutable_unsynced_handles() style accessor would be
more efficient.  Just be sure to clear() it before writing into it.

http://codereview.chromium.org/386030/diff/4002/9064#newcode140
Line 140: const T& value() const { return t_; }
I'm a bit cynical of this.  In my mind the need to add value() calls everywhere
is an inconvenience whose cost might outweigh the convenience this template
adds.

Dirtyable could potentially inherit from T, though this would probably be still
less comprehensible.

http://codereview.chromium.org/386030/diff/4002/9064#newcode146
Line 146: // Various pieces of state we track dirtyness of.
"dirtiness"

http://codereview.chromium.org/386030/diff/4002/9064#newcode156
Line 156: ConflictProgress conflict_progress_;
ConflictProgress tracks its own dirtiness.  Why have it be different from the
others?

http://codereview.chromium.org/386030/diff/4002/9062
File chrome/browser/sync/sessions/status_controller_unittest.cc (right):

http://codereview.chromium.org/386030/diff/4002/9062#newcode12
Line 12: typedef testing::Test StatusControllerTest;
Is there precedent for this?

http://codereview.chromium.org/386030/diff/4002/9062#newcode229
Line 229: sync_pb::SyncEntity* entity1 =
response->mutable_get_updates()->add_entries();
You don't use entity1 or entity2.  That's a gcc warning, which is treated as a
gcc error.

http://codereview.chromium.org/386030/diff/4002/9058
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/386030/diff/4002/9058#newcode136
Line 136: public:
indent.

http://codereview.chromium.org/386030/diff/4002/9058#newcode139
Line 139: : session_(session) {
indent.

http://codereview.chromium.org/386030/diff/4002/9058#newcode144
Line 144: private:
indent.

http://codereview.chromium.org/386030/diff/4002/9060
File chrome/browser/sync/sessions/sync_session_context.h (right):

http://codereview.chromium.org/386030/diff/4002/9060#newcode6
Line 6: // that are useful when processing a SyncSession.
This description could apply to just about anything; it's not very informative. 
Please be more concrete.

http://codereview.chromium.org/386030/diff/4002/9060#newcode87
Line 87: friend class ScopedSessionContextSyncerEventChannel;
Why do these need to be available only on a temporary basis?  Couldn't they be
permanent?

http://codereview.chromium.org/386030/diff/4002/9060#newcode138
Line 138: // one SyncerEventChannel in the system, so it is an error to use this
if the
Per your other comment, there are typically two SyncerEventChannels.

http://codereview.chromium.org/386030/diff/4002/9056
File chrome/browser/sync/sessions/sync_session_unittest.cc (right):

http://codereview.chromium.org/386030/diff/4002/9056#newcode35
Line 35: FailControllerInvocationIfDisabled("OnSilencedUntil");
Weird indent.

http://codereview.chromium.org/386030/diff/4002/9056#newcode38
Line 38: FailControllerInvocationIfDisabled("IsSyncingCurrentlySilenced");
Weird indent.

http://codereview.chromium.org/386030/diff/4002/9056#newcode42
Line 42: const base::TimeDelta& new_interval) {
Do you need to include something to use TimeDelta?

http://codereview.chromium.org/386030/diff/4002/9056#newcode56
Line 56: bool fail_controller_invocations_;
totally optional: consider inverting the sense of this so that it's
controller_invocations_expected_.

http://codereview.chromium.org/386030/diff/4002/9056#newcode89
Line 89: EXPECT_FALSE(session.write_transaction());
compare with NULL

http://codereview.chromium.org/386030/diff/4002/9056#newcode93
Line 93: EXPECT_TRUE(session.write_transaction());
compare with &trans

http://codereview.chromium.org/386030/diff/4002/9056#newcode135
Line 135: status()->mutable_updates_response()->Clear();
Hmm.  What happens if the client gets a 500 error on GetUpdates.  there's no
updates response in that case ...

[tim (not reviewing), 2009-11-18 05:51:36]

On Tue, Nov 17, 2009 at 4:21 PM, <nick@chromium.org> wrote:

> It only took me 12 hours to review this change.


I'm sorry for throwing this giant CL at you, and thank you for the review.
 I can tell you that I spent many, many, MANY weekday+weeknight+weekend
hours on this, which Id describe as a planar embedding of what we had
before; not even  the perfect end game, necessarily. I wanted to give up
several times. I think fear of that, coupled with the inevitably dreadful
review of something that touches so many files and isn't feasible to split
up [at least I couldn't do better than my first try with two CLs] are the
two main reasons this never happened before.  I felt it was necessary. I'm
sorry :(

I just got back to Seattle from NYC and I'm exhausted, I'll reply to your
comments first thing tomorrow.

>
>
>
> http://codereview.chromium.org/386030/diff/4002/9016
> File chrome/browser/sync/engine/apply_updates_command_unittest.cc
> (right):
>
> http://codereview.chromium.org/386030/diff/4002/9016#newcode29
> Line 29:
> public:
>
> http://codereview.chromium.org/386030/diff/4002/9016#newcode46
> Line 46: }
> This whole block needs +1 space of indentation.
>
> http://codereview.chromium.org/386030/diff/4002/9016#newcode162
> Line 162: EXPECT_EQ(2,
> status->conflict_progress()->ConflictingItemsSize())
> Seems a bit weird here that we're writing into conflict progress during
> update application, but after looking at how this field is used during
> conflict resolution, I suppose it makes sense.
>
>
> http://codereview.chromium.org/386030/diff/4002/9002
> File
> chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc
> (right):
>
> http://codereview.chromium.org/386030/diff/4002/9002#newcode24
> Line 24: using sessions::ConflictProgress;
> sort.
>
>
> http://codereview.chromium.org/386030/diff/4002/9015
> File chrome/browser/sync/engine/build_commit_command.h (right):
>
> http://codereview.chromium.org/386030/diff/4002/9015#newcode20
>
> Line 20: virtual void ExecuteImpl(sessions::SyncSession *session);
> "* "
>
> http://codereview.chromium.org/386030/diff/4002/9020
> File chrome/browser/sync/engine/conflict_resolver.h (right):
>
> http://codereview.chromium.org/386030/diff/4002/9020#newcode80
> Line 80: syncable::Id id);
> Pass id by reference.
>
> http://codereview.chromium.org/386030/diff/4002/9051
> File chrome/browser/sync/engine/get_commit_ids_command.cc (right):
>
> http://codereview.chromium.org/386030/diff/4002/9051#newcode132
> Line 132: &ordered_commit_set_);
> This loop is approaching the unreadable.  Hopefully the simplifications
> I suggested will allow it to use fewer lines.  If not, please try to
> make it so that the loop setup isn't a five-line-long
> comma-and-semicolon-delimited blob of goo.
>
> x 3 because this goo is replicated below.
>
>
> http://codereview.chromium.org/386030/diff/4002/9034
> File chrome/browser/sync/engine/get_commit_ids_command.h (right):
>
> http://codereview.chromium.org/386030/diff/4002/9034#newcode37
> Line 37: sessions::ConflictProgress* progress);
> You plumb ConflictProgress all over the place in this h/cc file.  But I
> don't see you ever use it.
>
> http://codereview.chromium.org/386030/diff/4002/9034#newcode111
> Line 111: CommitMetahandleIterator(vector<int64>::const_iterator
> unsynced_begin,
> Why not pass in a reference to the container, rather than a pair of
> iterators?
>
> http://codereview.chromium.org/386030/diff/4002/9034#newcode175
> Line 175: sessions::ConflictProgress* const conflict_progress_;
> It's unused, I think.
>
>
> http://codereview.chromium.org/386030/diff/4002/9009
> File chrome/browser/sync/engine/post_commit_message_command.cc (right):
>
> http://codereview.chromium.org/386030/diff/4002/9009#newcode32
> Line 32: status->mutable_commit_message(), &response, session)) {
> Is it possible to avoid using the mutable variant here?
>
>
> http://codereview.chromium.org/386030/diff/4002/9023
> File chrome/browser/sync/engine/process_commit_response_command.cc
> (right):
>
> http://codereview.chromium.org/386030/diff/4002/9023#newcode50
> Line 50: status->increment_num_consecutive_errors();
> Originally I freaked out when I saw this diff -- SyncerStatus was passed
> BY VALUE in the original code.  But then I realized that it worked okay
> because the only member was a pointer to the state, which was preserved
> on a copy.
>
>
> http://codereview.chromium.org/386030/diff/4002/9010
> File chrome/browser/sync/engine/process_updates_command.cc (right):
>
> http://codereview.chromium.org/386030/diff/4002/9010#newcode36
> Line 36: const GetUpdatesResponse& updates =
> Thanks for eliminating the copy.
>
> http://codereview.chromium.org/386030/diff/4002/9010#newcode70
> Line 70: for (it = progress.VerifiedUpdatesBegin(); it <
> progress.VerifiedUpdatesEnd();
> < works fine because they're random access iterators.  != is more
> idiomatic/familiar to me.  WDYT?
>
> http://codereview.chromium.org/386030/diff/4002/9014
> File chrome/browser/sync/engine/syncapi.cc (right):
>
> http://codereview.chromium.org/386030/diff/4002/9014#newcode1020
> Line 1020: SyncSessionContext * context = new SyncSessionContext(
> "* " not " * "
>
>
> http://codereview.chromium.org/386030/diff/4002/9006
> File chrome/browser/sync/engine/syncer_command.cc (right):
>
> http://codereview.chromium.org/386030/diff/4002/9006#newcode34
> Line 34: sessions::SyncSessionSnapshot
> snapshot(session->TakeSnapshot());
> Why make a copy to take a snapshot, if it's going to be consumed
> synchronously by the listeners?
>
> http://codereview.chromium.org/386030/diff/4002/9024
> File chrome/browser/sync/engine/syncer_command.h (right):
>
> http://codereview.chromium.org/386030/diff/4002/9024#newcode37
> Line 37: virtual void ExecuteImpl(sessions::SyncSession* session) = 0;
> I wonder if it would improve things, if Execute stashed the session away
> as a member, and ExecuteImpl took no args.  This would get rid of a lot
> of the passing-around we do.
>
>
> http://codereview.chromium.org/386030/diff/4002/9022
> File chrome/browser/sync/engine/syncer_thread.cc (right):
>
> http://codereview.chromium.org/386030/diff/4002/9022#newcode141
> Line 141: disable_idle_detection_(false) {
> Should we initialize silenced_until_ to Now()?
>
>
> http://codereview.chromium.org/386030/diff/4002/9037
> File chrome/browser/sync/engine/syncer_thread.h (right):
>
> http://codereview.chromium.org/386030/diff/4002/9037#newcode210
> Line 210: const base::TimeDelta& new_interval);
> Should these really be private, if they're invoked through an interface?
>
> http://codereview.chromium.org/386030/diff/4002/9037#newcode289
> Line 289: // happening. Is this strict event handling order needed?!
> Wow.  I had no idea.  That's jacked up.
>
>
> http://codereview.chromium.org/386030/diff/4002/9021
> File chrome/browser/sync/engine/syncer_thread_unittest.cc (right):
>
> http://codereview.chromium.org/386030/diff/4002/9021#newcode623
> Line 623: EXPECT_TRUE(syncer_thread()->IsSyncingCurrentlySilenced());
> Was there an underlying behavior change that prompted this change to the
> test?
>
> http://codereview.chromium.org/386030/diff/4002/9035
> File chrome/browser/sync/engine/syncer_types.h (right):
>
> http://codereview.chromium.org/386030/diff/4002/9035#newcode102
> Line 102: SyncerEvent(EventCause cause) : what_happened(cause) {}
> All the data members (int, Snapshot*) must be initialized by every
> constructor.  the C-style struct initialization "Foo foo = { x }"
> guarantees default initialization for scalar types even if they aren't
> matched with an element from the RHS.  C++ ctors do not.  That's why the
> code historically used the C style syntax.
>
>
> http://codereview.chromium.org/386030/diff/4002/9007
> File chrome/browser/sync/engine/syncer_unittest.cc (right):
>
> http://codereview.chromium.org/386030/diff/4002/9007#newcode98
> Line 98: SyncerTest() {}
> syncer_(NULL)
>
> http://codereview.chromium.org/386030/diff/4002/9007#newcode182
> Line 182: delete syncer_;
> add line "syncer_ = NULL;"
>
> http://codereview.chromium.org/386030/diff/4002/9007#newcode433
> Line 433: status->ResetTransientState();
> Why is it necessary to reset when the test has just begun?
>
> http://codereview.chromium.org/386030/diff/4002/9007#newcode3609
> Line 3609: SyncRepeatedlyToTriggerConflictResolution(session_.get());
> Is it weird to switch between the SyncShare(Controller*) and
> SyncShare(Session*) variants, mid-test?
>
> http://codereview.chromium.org/386030/diff/4002/9053
> File chrome/browser/sync/engine/verify_updates_command.cc (right):
>
> http://codereview.chromium.org/386030/diff/4002/9053#newcode38
> Line 38: const GetUpdatesResponse& updates =
> status->updates_response().get_updates();
> Thanks for removing the copy.
>
>
> http://codereview.chromium.org/386030/diff/4002/9061
> File chrome/browser/sync/sessions/session_state.h (right):
>
> http://codereview.chromium.org/386030/diff/4002/9061#newcode5
> Line 5: // The 'sessions' namespace is comprised of all the pieces of
> state that are
> "is composed of" or "comprises" but not "is comprised of".  Or in this
> case: "wraps", "contains", "houses", "binds together", "collects".
>
> The parts compose the whole.
> The whole comprises the parts.
> The whole is composed of several parts.
>
> http://codereview.chromium.org/386030/diff/4002/9061#newcode37
> Line 37: struct SyncerStatus {
> Add a comment for the class.
>
> http://codereview.chromium.org/386030/diff/4002/9061#newcode44
> Line 44: // True iff we're stuck. User should contact support.
> "contact support"
>
> http://codereview.chromium.org/386030/diff/4002/9061#newcode50
> Line 50: struct ErrorCounters {
> Add a comment for the class.
>
> http://codereview.chromium.org/386030/diff/4002/9061#newcode78
> Line 78: SyncSessionSnapshot(const SyncerStatus& status,
> Consider naming the params identically to the members.  Seems like you
> do a lot of dancing here to pick alternate names; it's not necessary.
>
> http://codereview.chromium.org/386030/diff/4002/9061#newcode83
> Line 83: : syncer_status(status), errors(error_counters),
> One-per-line is easier to read, IMO, but I'm not insisting.
>
> http://codereview.chromium.org/386030/diff/4002/9061#newcode127
> Line 127: bool progress_changed() const { return progress_changed_; }
> Is this redundant to the dirty bit stored?
>
> http://codereview.chromium.org/386030/diff/4002/9061#newcode181
> Line 181: // within a while that session is in flight.  This is useful
> when multiple
> "within a while" doesn't parse.
>
> http://codereview.chromium.org/386030/diff/4002/9061#newcode185
> Line 185: items_committed(false), timestamp_dirty(false) {}
> Move : down to the next line; one per line.
>
> http://codereview.chromium.org/386030/diff/4002/9063
> File chrome/browser/sync/sessions/status_controller.cc (right):
>
> http://codereview.chromium.org/386030/diff/4002/9063#newcode22
> Line 22: template <class FieldType, class DirtyableContainer>
> nit: i'd use 'typename FieldType' since it's often bool/int.
>
> http://codereview.chromium.org/386030/diff/4002/9063#newcode33
> Line 33: conflict_progress_.progress_changed();
> Why track 5 dirty bits if they're just ||ed together at the end of the
> day?  If you were to unify them, you could make SetAndMarkDirtyIfChanged
> a nontemplatized class member and drop its awkward third argument.
>
> http://codereview.chromium.org/386030/diff/4002/9063#newcode115
> Line 115: unsynced_handles, transient_.get());
> Why is transient state dirtyable?  If it gets reconstructed each pass,
> then what does it mean to be dirty?  Also -- none of the other transient
> fields seem to be tracked by the dirty bit.
>
> http://codereview.chromium.org/386030/diff/4002/9063#newcode143
> Line 143: // These don't cause notifications to fire.
> What notifications?  Please elaborate.
>
> http://codereview.chromium.org/386030/diff/4002/9063#newcode178
> Line 178: it < transient_->value()->update_progress.AppliedUpdatesEnd();
> < or != ?
>
> http://codereview.chromium.org/386030/diff/4002/9063#newcode180
> Line 180: if (it->first == CONFLICT) {
> De-indent.
>
>
> http://codereview.chromium.org/386030/diff/4002/9064
> File chrome/browser/sync/sessions/status_controller.h (right):
>
> http://codereview.chromium.org/386030/diff/4002/9064#newcode93
> Line 93: bool HasConflictingUpdates() const;
> It seems inconsistent that this lives here, but
> SuccessfullyAppliedUpdateCount() lives in the UpdateProgress -- they're
> nearly counterparts.  What's the philosophy surrounding whether
> functionality lives in the Controller or in a Progress group?
>
> http://codereview.chromium.org/386030/diff/4002/9064#newcode119
> Line 119: void set_unsynced_handles(const std::vector<int64>
> unsynced_handles);
> Passing a vector by value?
>
> For the vector members, a mutable_unsynced_handles() style accessor
> would be more efficient.  Just be sure to clear() it before writing into
> it.
>
> http://codereview.chromium.org/386030/diff/4002/9064#newcode140
> Line 140: const T& value() const { return t_; }
> I'm a bit cynical of this.  In my mind the need to add value() calls
> everywhere is an inconvenience whose cost might outweigh the convenience
> this template adds.
>
> Dirtyable could potentially inherit from T, though this would probably
> be still less comprehensible.
>
> http://codereview.chromium.org/386030/diff/4002/9064#newcode146
> Line 146: // Various pieces of state we track dirtyness of.
> "dirtiness"
>
> http://codereview.chromium.org/386030/diff/4002/9064#newcode156
> Line 156: ConflictProgress conflict_progress_;
> ConflictProgress tracks its own dirtiness.  Why have it be different
> from the others?
>
>
> http://codereview.chromium.org/386030/diff/4002/9062
> File chrome/browser/sync/sessions/status_controller_unittest.cc (right):
>
> http://codereview.chromium.org/386030/diff/4002/9062#newcode12
> Line 12: typedef testing::Test StatusControllerTest;
> Is there precedent for this?
>
> http://codereview.chromium.org/386030/diff/4002/9062#newcode229
> Line 229: sync_pb::SyncEntity* entity1 =
> response->mutable_get_updates()->add_entries();
> You don't use entity1 or entity2.  That's a gcc warning, which is
> treated as a gcc error.
>
>
> http://codereview.chromium.org/386030/diff/4002/9058
> File chrome/browser/sync/sessions/sync_session.h (right):
>
> http://codereview.chromium.org/386030/diff/4002/9058#newcode136
> Line 136: public:
> indent.
>
> http://codereview.chromium.org/386030/diff/4002/9058#newcode139
> Line 139: : session_(session) {
> indent.
>
> http://codereview.chromium.org/386030/diff/4002/9058#newcode144
> Line 144: private:
> indent.
>
>
> http://codereview.chromium.org/386030/diff/4002/9060
> File chrome/browser/sync/sessions/sync_session_context.h (right):
>
> http://codereview.chromium.org/386030/diff/4002/9060#newcode6
> Line 6: // that are useful when processing a SyncSession.
> This description could apply to just about anything; it's not very
> informative.  Please be more concrete.
>
> http://codereview.chromium.org/386030/diff/4002/9060#newcode87
> Line 87: friend class ScopedSessionContextSyncerEventChannel;
> Why do these need to be available only on a temporary basis?  Couldn't
> they be permanent?
>
> http://codereview.chromium.org/386030/diff/4002/9060#newcode138
> Line 138: // one SyncerEventChannel in the system, so it is an error to
> use this if the
> Per your other comment, there are typically two SyncerEventChannels.
>
> http://codereview.chromium.org/386030/diff/4002/9056
> File chrome/browser/sync/sessions/sync_session_unittest.cc (right):
>
> http://codereview.chromium.org/386030/diff/4002/9056#newcode35
> Line 35: FailControllerInvocationIfDisabled("OnSilencedUntil");
> Weird indent.
>
> http://codereview.chromium.org/386030/diff/4002/9056#newcode38
> Line 38:
> FailControllerInvocationIfDisabled("IsSyncingCurrentlySilenced");
> Weird indent.
>
> http://codereview.chromium.org/386030/diff/4002/9056#newcode42
> Line 42: const base::TimeDelta& new_interval) {
> Do you need to include something to use TimeDelta?
>
> http://codereview.chromium.org/386030/diff/4002/9056#newcode56
> Line 56: bool fail_controller_invocations_;
> totally optional: consider inverting the sense of this so that it's
> controller_invocations_expected_.
>
> http://codereview.chromium.org/386030/diff/4002/9056#newcode89
> Line 89: EXPECT_FALSE(session.write_transaction());
> compare with NULL
>
> http://codereview.chromium.org/386030/diff/4002/9056#newcode93
> Line 93: EXPECT_TRUE(session.write_transaction());
> compare with &trans
>
> http://codereview.chromium.org/386030/diff/4002/9056#newcode135
> Line 135: status()->mutable_updates_response()->Clear();
> Hmm.  What happens if the client gets a 500 error on GetUpdates.
> there's no updates response in that case ...
>
>
> http://codereview.chromium.org/386030
>

[tim (not reviewing), 2009-11-19 04:08:20]

Brace for impact.

http://codereview.chromium.org/386030/diff/4002/9028
File chrome/browser/sync/engine/all_status.cc (right):

http://codereview.chromium.org/386030/diff/4002/9028#newcode104
Line 104: // Show a syncer as syncing if it's got stalled updates.
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Remove this comment about stalled updates. It was about "blocked" items, which
> should no longer occur.

Done.

http://codereview.chromium.org/386030/diff/4002/9027
File chrome/browser/sync/engine/apply_updates_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9027#newcode25
Line 25: virtual void ModelChangingExecuteImpl(sessions::SyncSession* session);
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> include the .h file for this structure

This is the implementation of the interface prototype defined in
model_changing_syncer_command.h. Including everything that the interface file
includes to declare the prototypes doesn't scale very well in general, and the
style guide says to prefer minimizing includes.

http://codereview.chromium.org/386030/diff/4002/9016
File chrome/browser/sync/engine/apply_updates_command_unittest.cc (right):

http://codereview.chromium.org/386030/diff/4002/9016#newcode29
Line 29: 
On 2009/11/18 00:21:40, nick wrote:
> public:

Done.

http://codereview.chromium.org/386030/diff/4002/9016#newcode41
Line 41: }
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Add consistent newlines between functions, or consistently no newlines.

Done.

http://codereview.chromium.org/386030/diff/4002/9016#newcode46
Line 46: }
On 2009/11/18 00:21:40, nick wrote:
> This whole block needs +1 space of indentation.

Done.

http://codereview.chromium.org/386030/diff/4002/9016#newcode162
Line 162: EXPECT_EQ(2, status->conflict_progress()->ConflictingItemsSize())
On 2009/11/18 00:21:40, nick wrote:
> Seems a bit weird here that we're writing into conflict progress during update
> application, but after looking at how this field is used during conflict
> resolution, I suppose it makes sense.

Yeah. My argument here is that at least now we "realize" that we're writing into
and affecting conflict progress state during update application.

http://codereview.chromium.org/386030/diff/4002/9002
File chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc
(right):

http://codereview.chromium.org/386030/diff/4002/9002#newcode24
Line 24: using sessions::ConflictProgress;
On 2009/11/18 00:21:40, nick wrote:
> sort.

Done.

http://codereview.chromium.org/386030/diff/4002/9002#newcode51
Line 51: session->context()->resolver(), session->status_controller());
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> does this fit on three lines like the above dir lookup?

Done.

http://codereview.chromium.org/386030/diff/4002/9038
File chrome/browser/sync/engine/build_and_process_conflict_sets_command.h
(right):

http://codereview.chromium.org/386030/diff/4002/9038#newcode11
Line 11: #include "chrome/browser/sync/engine/model_changing_syncer_command.h"
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Does this file need to import syncersession.h?

No; See comment in apply_updates_command.h.

http://codereview.chromium.org/386030/diff/4002/9015
File chrome/browser/sync/engine/build_commit_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9015#newcode10
Line 10: #include "chrome/browser/sync/engine/syncproto.h"
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Does syncproto include sync session? Should sync session still be explicitly
> included? Or forward declared?

No; See comment in apply_updates_command.h.

http://codereview.chromium.org/386030/diff/4002/9015#newcode20
Line 20: virtual void ExecuteImpl(sessions::SyncSession *session);
On 2009/11/18 00:21:40, nick wrote:
> "* "

Done.

http://codereview.chromium.org/386030/diff/4002/9020
File chrome/browser/sync/engine/conflict_resolver.h (right):

http://codereview.chromium.org/386030/diff/4002/9020#newcode80
Line 80: syncable::Id id);
On 2009/11/18 00:21:40, nick wrote:
> Pass id by reference.

Done.

http://codereview.chromium.org/386030/diff/4002/9003
File chrome/browser/sync/engine/download_updates_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9003#newcode20
Line 20: virtual void ExecuteImpl(sessions::SyncSession* session);
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Why was the .h removed from here? Should you forward declare?

No; See comment in apply_updates_command.h.

http://codereview.chromium.org/386030/diff/4002/9051
File chrome/browser/sync/engine/get_commit_ids_command.cc (right):

http://codereview.chromium.org/386030/diff/4002/9051#newcode132
Line 132: &ordered_commit_set_);
On 2009/11/18 00:21:40, nick wrote:
> This loop is approaching the unreadable.  Hopefully the simplifications I
> suggested will allow it to use fewer lines.  If not, please try to make it so
> that the loop setup isn't a five-line-long comma-and-semicolon-delimited blob
of
> goo.
> 
> x 3 because this goo is replicated below.

Blob of goo gone.  Now each of the 3 left-aligned lines are one part of the
for-construct (declare, condition, operation) so I think it is back to a
readable state.

http://codereview.chromium.org/386030/diff/4002/9034
File chrome/browser/sync/engine/get_commit_ids_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9034#newcode37
Line 37: sessions::ConflictProgress* progress);
On 2009/11/18 00:21:40, nick wrote:
> You plumb ConflictProgress all over the place in this h/cc file.  But I don't
> see you ever use it.

Amazing observation! Thank you. Done.

http://codereview.chromium.org/386030/diff/4002/9034#newcode111
Line 111: CommitMetahandleIterator(vector<int64>::const_iterator unsynced_begin,
On 2009/11/18 00:21:40, nick wrote:
> Why not pass in a reference to the container, rather than a pair of iterators?

Wow. Yes. Done.

http://codereview.chromium.org/386030/diff/4002/9034#newcode159
Line 159: // later. ValidateCommitEntry writes to the DB, and we add the blocked
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> can you remove this comment about blocked items?

Done.

http://codereview.chromium.org/386030/diff/4002/9034#newcode175
Line 175: sessions::ConflictProgress* const conflict_progress_;
On 2009/11/18 00:21:40, nick wrote:
> It's unused, I think.

Done.

http://codereview.chromium.org/386030/diff/4002/9009
File chrome/browser/sync/engine/post_commit_message_command.cc (right):

http://codereview.chromium.org/386030/diff/4002/9009#newcode23
Line 23: if (session->status_controller()->commit_ids().empty())
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Why is commit_ids part of status controller?  Is status controller weirdly
> named?

Perhaps I should call it 'SessionStateController'?
commit_ids is transient state.

http://codereview.chromium.org/386030/diff/4002/9009#newcode32
Line 32: status->mutable_commit_message(), &response, session)) {
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> It seems weird that status has a mutable commit message.
"Status" includes the messages for the session. Perhaps my above rename would
alleviate your concern.

http://codereview.chromium.org/386030/diff/4002/9009#newcode32
Line 32: status->mutable_commit_message(), &response, session)) {
On 2009/11/18 00:21:40, nick wrote:
> Is it possible to avoid using the mutable variant here?

PostClientToServerMessage sets the store birthday.  We'd have to move that out
and make sure all callers still set it...

http://codereview.chromium.org/386030/diff/4002/9011
File chrome/browser/sync/engine/post_commit_message_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9011#newcode19
Line 19: virtual void ExecuteImpl(sessions::SyncSession* session);
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Does this need forward decl or include?

No; See comment in apply_updates_command.h.

http://codereview.chromium.org/386030/diff/4002/9023
File chrome/browser/sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/386030/diff/4002/9023#newcode50
Line 50: status->increment_num_consecutive_errors();
On 2009/11/18 00:21:40, nick wrote:
> Originally I freaked out when I saw this diff -- SyncerStatus was passed BY
> VALUE in the original code.  But then I realized that it worked okay because
the
> only member was a pointer to the state, which was preserved on a copy.

A good example the sort of propaganda the incumbent implementation exposes us to
that I set out to destroy :)

http://codereview.chromium.org/386030/diff/4002/9023#newcode65
Line 65: monitor->PutRecords(session->extensions_activity());
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> this extensions activity monitor here does seem kind of like a weird side
> effect, wish it was like an observer pattern

Hm. Well, this didn't change here, but maybe you're right. We could have the EAM
listen to STATUS_CHANGED SyncerEvents, but because we can't currently
differentiate which command raised the event (e.g so we would know it was a
"commit", and that 'had successful commits' is meaningful), this is kind of
awkward. We could maybe add a COMMIT_ATTEMPTED event  that we would fire from
SendNotifications if we determined it was a commit or add state to SyncerEvent
to depict which command was the trigger or? Just adding an event type event and
raising it from within process_commit_response_command would seem to circumvent
the whole SendNotifications construct of SyncerCommand.

http://codereview.chromium.org/386030/diff/4002/9042
File chrome/browser/sync/engine/process_commit_response_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9042#newcode29
Line 29: virtual void ModelChangingExecuteImpl(sessions::SyncSession* session);
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Do we need to include or declare?

No; See comment in apply_updates_command.h.

http://codereview.chromium.org/386030/diff/4002/9010
File chrome/browser/sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/386030/diff/4002/9010#newcode36
Line 36: const GetUpdatesResponse& updates =
On 2009/11/18 00:21:40, nick wrote:
> Thanks for eliminating the copy.

Done.

http://codereview.chromium.org/386030/diff/4002/9010#newcode37
Line 37: session->status_controller()->updates_response().get_updates();
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> the response coming back in the status controller is a little weird..

Again I refer to my comment about renaming.  I think it makes sense to be a part
of the same structure whatever it is called because this structure controls
access to all mutable sync process related state in SyncSession.

http://codereview.chromium.org/386030/diff/4002/9010#newcode70
Line 70: for (it = progress.VerifiedUpdatesBegin(); it <
progress.VerifiedUpdatesEnd();
On 2009/11/18 00:21:40, nick wrote:
> < works fine because they're random access iterators.  != is more
> idiomatic/familiar to me.  WDYT?

Fully agree. Done.

http://codereview.chromium.org/386030/diff/4002/9041
File chrome/browser/sync/engine/process_updates_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9041#newcode34
Line 34: virtual void ModelChangingExecuteImpl(sessions::SyncSession* session);
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> why no forward decl or .h?

See comment in apply_updates_command.h.

http://codereview.chromium.org/386030/diff/4002/9048
File chrome/browser/sync/engine/resolve_conflicts_command.cc (right):

http://codereview.chromium.org/386030/diff/4002/9048#newcode19
Line 19: DCHECK(resolver);
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> These semantics changed. Can resolver no longer be null?

AFAICT, it could never be null before. However, I probably should have kept the
early-out for release. Added it back.

http://codereview.chromium.org/386030/diff/4002/9019
File chrome/browser/sync/engine/resolve_conflicts_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9019#newcode25
Line 25: virtual void ModelChangingExecuteImpl(sessions::SyncSession* session);
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> forward decl? or .h?

See comment in apply_updates_command.h.

http://codereview.chromium.org/386030/diff/4002/9019#newcode25
Line 25: virtual void ModelChangingExecuteImpl(sessions::SyncSession* session);
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> forward decl? or .h?

See comment in apply_updates_command.h.

http://codereview.chromium.org/386030/diff/4002/9014
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/386030/diff/4002/9014#newcode1020
Line 1020: SyncSessionContext * context = new SyncSessionContext(
On 2009/11/18 00:21:40, nick wrote:
> "* " not " * "

Done.

http://codereview.chromium.org/386030/diff/4002/9040
File chrome/browser/sync/engine/syncer.cc (right):

http://codereview.chromium.org/386030/diff/4002/9040#newcode176
Line 176: sessions::ScopedSetSessionWriteTransaction set_trans(session, &trans);
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Why SetSessionWriteTransaction?
> Does it write to a set, or to a session?

Not sure I follow.  Are you suggesting maybe
sessions::ScopedSessionWriteTransaction? In fact, I think that's a much better
name! does that work for you?

http://codereview.chromium.org/386030/diff/4002/9006
File chrome/browser/sync/engine/syncer_command.cc (right):

http://codereview.chromium.org/386030/diff/4002/9006#newcode32
Line 32: if (session->status_controller()->TestAndResetIsDirty()) {
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> TestAndClear might be clearer
> or just plain ClearIsDirty

Renamed to TestAndClearIsDirty()

http://codereview.chromium.org/386030/diff/4002/9006#newcode34
Line 34: sessions::SyncSessionSnapshot snapshot(session->TakeSnapshot());
On 2009/11/18 00:21:40, nick wrote:
> Why make a copy to take a snapshot, if it's going to be consumed synchronously
> by the listeners?

Done.

http://codereview.chromium.org/386030/diff/4002/9006#newcode44
Line 44: if (session->auth_failure_occurred()) {
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> why not follow the same pattern?

I guess I couldn't find a good place for this one bit in session_state.  I
didn't want to group it with status because I didn't want to change this
separation for now, but it sort of feels like this type of error could be raised
when it happens rather than here.

http://codereview.chromium.org/386030/diff/4002/9024
File chrome/browser/sync/engine/syncer_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9024#newcode37
Line 37: virtual void ExecuteImpl(sessions::SyncSession* session) = 0;
On 2009/11/18 00:21:40, nick wrote:
> I wonder if it would improve things, if Execute stashed the session away as a
> member, and ExecuteImpl took no args.  This would get rid of a lot of the
> passing-around we do.

This might be nice, I agree. Does anything call Execute repatedly and change
state in between? I seem to think the unittests do this? Or at least I feel like
it would be a big change syntactically, could I try it in an isolated CL?

http://codereview.chromium.org/386030/diff/4002/9054
File chrome/browser/sync/engine/syncer_end_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9054#newcode25
Line 25: virtual void ExecuteImpl(sessions::SyncSession* session);
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> .h or decl?

See comment in apply_updates_command.h.

http://codereview.chromium.org/386030/diff/4002/9032
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/386030/diff/4002/9032#newcode157
Line 157: session->controller()->OnSilencedUntil(base::TimeTicks::Now() +
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> What's the difference between controller and status controller?

The former is SyncSession::Controller; an external entity that controls the
session and session initiation, that the session can delegate to when a
session-global action must absolutely be taken (see comment in sync_session.h).
I had named it 'Delegate' before, akin to URLRequest::Delegate and perhaps more
common lingo in Chrome. Maybe I should name it back.

OTOH, SyncSession Is Composed Of (in the UML sense) a StatusController (or
'SessionStateController') which is more of an MVC type controller for the model
data in session_state.

http://codereview.chromium.org/386030/diff/4002/9022
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/386030/diff/4002/9022#newcode141
Line 141: disable_idle_detection_(false) {
On 2009/11/18 00:21:40, nick wrote:
> Should we initialize silenced_until_ to Now()?

Hm, not sure I follow? I think the default of '0' aka is_null() is what we want.

http://codereview.chromium.org/386030/diff/4002/9022#newcode429
Line 429: // Clean slate for SyncSession::Controller impl.
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> What does this comment mean?

"Since we are initiating a new session for which we are the controller, we are
not currently silenced so reset this state for the next session which may need
to use it."

http://codereview.chromium.org/386030/diff/4002/9037
File chrome/browser/sync/engine/syncer_thread.h (left):

http://codereview.chromium.org/386030/diff/4002/9037#oldcode144
Line 144: SyncerThread();  // Necessary for temporary pthreads-based PIMPL impl.
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Aww. We lost our PIMP.

Yes, the pimp is gone.

http://codereview.chromium.org/386030/diff/4002/9037
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/386030/diff/4002/9037#newcode210
Line 210: const base::TimeDelta& new_interval);
On 2009/11/18 00:21:40, nick wrote:
> Should these really be private, if they're invoked through an interface?

I don't know. It worked and I felt there was something satisfying about not
letting you call these through a SyncerThread*, from an abstraction point of
view.  Maybe I should just move it to the public section to be normal.

http://codereview.chromium.org/386030/diff/4002/9037#newcode289
Line 289: // happening. Is this strict event handling order needed?!
On 2009/11/18 00:21:40, nick wrote:
> Wow.  I had no idea.  That's jacked up.

http://www.strangedangers.com/images/content/108533.jpg

http://codereview.chromium.org/386030/diff/4002/9037#newcode296
Line 296: base::TimeTicks silenced_until_;
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> While I think this does belong here, how did this end up here in this CL?

SyncerThread is now the SyncSession::Controller.

http://codereview.chromium.org/386030/diff/4002/9021
File chrome/browser/sync/engine/syncer_thread_unittest.cc (right):

http://codereview.chromium.org/386030/diff/4002/9021#newcode58
Line 58: // throttled.  Only call this if a throttle is supposed to occur!
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Should there be some preconditions for this call? Variable wise?

Hmm. I think the name of the function is an implicit precondition; don't wait
for throttle unless a throttle is coming. There isn't much we can do as a
precondition check.. we could maybe check that the mock connection manager is
set to throttle a request, but that's racy the way it is now because the
throttle could have occurred already.

http://codereview.chromium.org/386030/diff/4002/9021#newcode76
Line 76: base::WaitableEvent sync_cycle_ended_event_;
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Sort of confusing that the WaitableEvent is named similarly to the hookup.

Appended hookup_ to the hookup.

http://codereview.chromium.org/386030/diff/4002/9021#newcode192
Line 192: SyncSessionContext* context = new SyncSessionContext(NULL, NULL,
NULL);
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Would it make sense to have a default constructor?

The class would then have multiple constructors, and style guide says not to
overload to simulate default parameters.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/386030/diff/4002/9021#newcode623
Line 623: EXPECT_TRUE(syncer_thread()->IsSyncingCurrentlySilenced());
On 2009/11/18 00:21:40, nick wrote:
> Was there an underlying behavior change that prompted this change to the test?

This test was basically open to race conditions before.  It waited for 10 poll
intervals and would fail if a sync occurred, effectively saying "well, if we
haven't seen a sync share in this much time it *must* be throttled". But that's
not true. The test could easily succeed without having proved anything.  Now, it
can't.

http://codereview.chromium.org/386030/diff/4002/9035
File chrome/browser/sync/engine/syncer_types.h (right):

http://codereview.chromium.org/386030/diff/4002/9035#newcode102
Line 102: SyncerEvent(EventCause cause) : what_happened(cause) {}
On 2009/11/18 00:21:40, nick wrote:
> All the data members (int, Snapshot*) must be initialized by every
constructor. 
> the C-style struct initialization "Foo foo = { x }" guarantees default
> initialization for scalar types even if they aren't matched with an element
from
> the RHS.  C++ ctors do not.  That's why the code historically used the C style
> syntax.

Fixed to init all members and to not overload the ctor.

http://codereview.chromium.org/386030/diff/4002/9007
File chrome/browser/sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/386030/diff/4002/9007#newcode98
Line 98: SyncerTest() {}
On 2009/11/18 00:21:40, nick wrote:
> syncer_(NULL)

Done.

http://codereview.chromium.org/386030/diff/4002/9007#newcode106
Line 106: }
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Consistent endl after functions, or no endl after functions

Done.

http://codereview.chromium.org/386030/diff/4002/9007#newcode182
Line 182: delete syncer_;
On 2009/11/18 00:21:40, nick wrote:
> add line "syncer_ = NULL;"

Done.

http://codereview.chromium.org/386030/diff/4002/9007#newcode205
Line 205: // tests.?!?!?!?!?!
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Replace with an interrobang. On second thought, remove comment about
> brittleness. It's brittle.

Heh, honestly, I don't know what this comment meant; I added the ?!?... to
remind myself to look at it. Anyway, removed the 'brittle' part.

http://codereview.chromium.org/386030/diff/4002/9007#newcode311
Line 311: ConflictProgress* progress =
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Conflict Progress? Maybe a better name for this.

It's a wrapper for conflict-related progress state tracking. I'm kinda stuck on
a better name.

http://codereview.chromium.org/386030/diff/4002/9007#newcode346
Line 346: void FailTest(const std::string& msg){
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Is this better than the FAIL() << MSG?

No, but it's needed because you can't call ASSERT_* or FAIL from non-void
methods. (The macros are designed to be called from TEST_Fs which are void and
do 'return;', so you get a compile error if your method returns bool for
example, which IsSyncingCurrentlySilenced does).

http://codereview.chromium.org/386030/diff/4002/9007#newcode433
Line 433: status->ResetTransientState();
On 2009/11/18 00:21:40, nick wrote:
> Why is it necessary to reset when the test has just begun?

Ok, so when I first started in here it looked like it was grabbing global state_
and allocating a SyncCycleState for the transient bits that should be reset
(which is equivalent to ResetTransientState now). I didn't know whether I was
going to have to reset session_ in SetUp. It turns out I do, so you are correct
that this call is not needed.  Removed.

http://codereview.chromium.org/386030/diff/4002/9007#newcode2223
Line 2223: EXPECT_TRUE(0 == progress->ConflictingItemsSize());
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> This naming has lost the essence of "conflicting updates". Items != updates?
If
> the previous naming left out something then this is fine.

In the previous world, ConflictingUpdatesSize invoked ConflictingItemsSize
directly; they were aliases whether or not they were intended to be.  Now it is
explicit that we are talking about the same thing; before when I looked at this
quantity I would ask "what are conflicting updates" and have to dig through to
find that they are in fact conflicting items via SyncerStatus synonymizing.

http://codereview.chromium.org/386030/diff/4002/9007#newcode3609
Line 3609: SyncRepeatedlyToTriggerConflictResolution(session_.get());
On 2009/11/18 00:21:40, nick wrote:
> Is it weird to switch between the SyncShare(Controller*) and
SyncShare(Session*)
> variants, mid-test?

There is a difference in that the former creates an entirely new session, and
here we instead continue with the existing session but reset transient state as
the comment above this [private] variant of SyncShare mentions. There is another
difference in that that the latter method lets you examine the session after,
which some tests make use of.

http://codereview.chromium.org/386030/diff/4002/9053
File chrome/browser/sync/engine/verify_updates_command.cc (right):

http://codereview.chromium.org/386030/diff/4002/9053#newcode38
Line 38: const GetUpdatesResponse& updates =
status->updates_response().get_updates();
On 2009/11/18 00:21:40, nick wrote:
> Thanks for removing the copy.

Done.

http://codereview.chromium.org/386030/diff/4002/9055
File chrome/browser/sync/engine/verify_updates_command.h (right):

http://codereview.chromium.org/386030/diff/4002/9055#newcode27
Line 27: // SyncerCommand implementation.
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Forward decl or include used?

See comment in apply_updates_command.h.

http://codereview.chromium.org/386030/diff/4002/9059
File chrome/browser/sync/sessions/session_state.cc (right):

http://codereview.chromium.org/386030/diff/4002/9059#newcode164
Line 164: applied_updates_.begin();
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> should this indent a little?

Hm. I always have an issue with this case. I don't know. The closest style
guideline I find is 'wrapped parameters have a 4 space indent', where 4 spaces
is from the start of the line, but that just looks silly with 'for' constructs.
I suppose I could apply the 4 space indent from the start of the variable
declaration, but that seemed more ad-hoc than lining it up directly under.

http://codereview.chromium.org/386030/diff/4002/9061
File chrome/browser/sync/sessions/session_state.h (right):

http://codereview.chromium.org/386030/diff/4002/9061#newcode5
Line 5: // The 'sessions' namespace is comprised of all the pieces of state that
are
On 2009/11/18 00:21:40, nick wrote:
> "is composed of" or "comprises" but not "is comprised of".  Or in this case:
> "wraps", "contains", "houses", "binds together", "collects".
> 
> The parts compose the whole.
> The whole comprises the parts.
> The whole is composed of several parts.

Done.

http://codereview.chromium.org/386030/diff/4002/9061#newcode44
Line 44: // True iff we're stuck. User should contact support.
On 2009/11/18 00:21:40, nick wrote:
> "contact support"

Not sure what you mean but I removed this (was copy pasted).

http://codereview.chromium.org/386030/diff/4002/9061#newcode50
Line 50: struct ErrorCounters {
On 2009/11/18 00:21:40, nick wrote:
> Add a comment for the class.

Done.

http://codereview.chromium.org/386030/diff/4002/9061#newcode78
Line 78: SyncSessionSnapshot(const SyncerStatus& status,
On 2009/11/18 00:21:40, nick wrote:
> Consider naming the params identically to the members.  Seems like you do a
lot
> of dancing here to pick alternate names; it's not necessary.

Done. I thought it was necessary for some reason.

http://codereview.chromium.org/386030/diff/4002/9061#newcode83
Line 83: : syncer_status(status), errors(error_counters),
On 2009/11/18 00:21:40, nick wrote:
> One-per-line is easier to read, IMO, but I'm not insisting.

Done.

http://codereview.chromium.org/386030/diff/4002/9061#newcode127
Line 127: bool progress_changed() const { return progress_changed_; }
On 2009/11/18 00:21:40, nick wrote:
> Is this redundant to the dirty bit stored?

There is no dirty bit stored for ConflictProgress. It stores its own because it
has mutators. I could make it inherit from Dirtyable<ConflictProgress> a la
RefCounted<foo>, but I don't think the benefit is worth exposing Dirtyable
outside of the controller and exposing more than just the const progress_changed
accessor.

http://codereview.chromium.org/386030/diff/4002/9061#newcode181
Line 181: // within a while that session is in flight.  This is useful when
multiple
On 2009/11/18 00:21:40, nick wrote:
> "within a while" doesn't parse.

Done.

http://codereview.chromium.org/386030/diff/4002/9061#newcode185
Line 185: items_committed(false), timestamp_dirty(false) {}
On 2009/11/18 00:21:40, nick wrote:
> Move : down to the next line; one per line.

Done.

http://codereview.chromium.org/386030/diff/4002/9061#newcode201
Line 201: #endif  // CHROME_BROWSER_SYNC_SESSIONS_SESSION_STATE_H_
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> this is better than before :)

Thank you for the motivating comment :)

http://codereview.chromium.org/386030/diff/4002/9063
File chrome/browser/sync/sessions/status_controller.cc (right):

http://codereview.chromium.org/386030/diff/4002/9063#newcode22
Line 22: template <class FieldType, class DirtyableContainer>
On 2009/11/18 00:21:40, nick wrote:
> nit: i'd use 'typename FieldType' since it's often bool/int.

Done.

http://codereview.chromium.org/386030/diff/4002/9063#newcode33
Line 33: conflict_progress_.progress_changed();
On 2009/11/18 00:21:40, nick wrote:
> Why track 5 dirty bits if they're just ||ed together at the end of the day? 
If
> you were to unify them, you could make SetAndMarkDirtyIfChanged a
nontemplatized
> class member and drop its awkward third argument.
I still think, as my comment in the .h says, this is more contained and explicit
and less error prone.
It was pretty hard to tell before finishing this which fields caused what to
happen (you had to check the .cc file, and then each mutating method to see if
UpdateDirty was called).  I'm willing to add a single dirty bit to the
controller and have Dirtyable< take a pointer to the bit, which would eliminate
the 3rd param to SetAndMark, but it would still be templatized [which all things
considered affects only one single line of code.]

http://codereview.chromium.org/386030/diff/4002/9063#newcode115
Line 115: unsynced_handles, transient_.get());
On 2009/11/18 00:21:40, nick wrote:
> Why is transient state dirtyable?  If it gets reconstructed each pass, then
what
> does it mean to be dirty?  Also -- none of the other transient fields seem to
be
> tracked by the dirty bit.

It is not reconstructed between each SyncerCommand.
Something is dirtyable if it contains something that can change and you want to
know that happened. It would be nice, I admit, to separate the dirtyable part of
transient state out. Alas, I was stuck with this decision at the very start- I
had two traits, dirtyable and transient, and had to decide how to identify each.
I went with putting transient in a struct which was more closely related to the
previous implementation; as it turned out only one field was both transient and
dirtyable, and I decided not to optimize for it. The unfortunate part is it
defeats one of the two main advantages to this approach that I point out in my
above comment.

http://codereview.chromium.org/386030/diff/4002/9063#newcode143
Line 143: // These don't cause notifications to fire.
On 2009/11/18 00:21:40, nick wrote:
> What notifications?  Please elaborate.

Amended comment to reflect what I explain above.

http://codereview.chromium.org/386030/diff/4002/9063#newcode178
Line 178: it < transient_->value()->update_progress.AppliedUpdatesEnd();
On 2009/11/18 00:21:40, nick wrote:
> < or != ?

Done. (moved to session_state.cc)

http://codereview.chromium.org/386030/diff/4002/9063#newcode180
Line 180: if (it->first == CONFLICT) {
On 2009/11/18 00:21:40, nick wrote:
> De-indent.

Done. (moved to session_state.cc)

http://codereview.chromium.org/386030/diff/4002/9064
File chrome/browser/sync/sessions/status_controller.h (right):

http://codereview.chromium.org/386030/diff/4002/9064#newcode93
Line 93: bool HasConflictingUpdates() const;
On 2009/11/18 00:21:40, nick wrote:
> It seems inconsistent that this lives here, but
SuccessfullyAppliedUpdateCount()
> lives in the UpdateProgress -- they're nearly counterparts.  What's the
> philosophy surrounding whether functionality lives in the Controller or in a
> Progress group?

You are right, it belongs in UpdateProgress.

http://codereview.chromium.org/386030/diff/4002/9064#newcode119
Line 119: void set_unsynced_handles(const std::vector<int64> unsynced_handles);
On 2009/11/18 00:21:40, nick wrote:
> Passing a vector by value?
> 
> For the vector members, a mutable_unsynced_handles() style accessor would be
> more efficient.  Just be sure to clear() it before writing into it.

Shouldn't have been by value.  The issue with mutable_ is that you can't compare
equality for dirtiness "after you're done".

http://codereview.chromium.org/386030/diff/4002/9064#newcode133
Line 133: class Dirtyable {
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Maybe the testandreset thing should go in here?

Yeah, that could work. Done.

http://codereview.chromium.org/386030/diff/4002/9064#newcode140
Line 140: const T& value() const { return t_; }
On 2009/11/18 00:21:40, nick wrote:
> I'm a bit cynical of this.  In my mind the need to add value() calls
everywhere
> is an inconvenience whose cost might outweigh the convenience this template
> adds.
> 
> Dirtyable could potentially inherit from T, though this would probably be
still
> less comprehensible.

It adds convenience, also some level of explicitness, and encapsulation. I don't
see the need to add value calls as a hindrance. It is tiny, and completely
isolated to the implementation of StatusController whose sole purpose is to
track this state. If we didn't have it we would have to add a bool for each
piece, and open ourselves up to bugs (like we had before, setting one and
accidentally dirtying another bool); seems to break encapsulation. And I don't
think the bool should be stuck inside the structs, because it makes sense to
have one of those on their own without any dirty bit; it's just the
StatusController that needs it.  We could rename value to get() if that is more
familiar.

http://codereview.chromium.org/386030/diff/4002/9064#newcode146
Line 146: // Various pieces of state we track dirtyness of.
On 2009/11/18 00:21:40, nick wrote:
> "dirtiness"

Done.

http://codereview.chromium.org/386030/diff/4002/9064#newcode156
Line 156: ConflictProgress conflict_progress_;
On 2009/11/18 00:21:40, nick wrote:
> ConflictProgress tracks its own dirtiness.  Why have it be different from the
> others?

It's not just a container of state, it has a brain and a beating heart (and
accessor/mutators, etc) to perform deeper calculations for dirtiness. The others
are just plain state, embedded in a generic multipurpose brain.

http://codereview.chromium.org/386030/diff/4002/9062
File chrome/browser/sync/sessions/status_controller_unittest.cc (right):

http://codereview.chromium.org/386030/diff/4002/9062#newcode10
Line 10: namespace {
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Is this extra namespace necessary for the whole file scope?

Removed the namespace.

http://codereview.chromium.org/386030/diff/4002/9062#newcode12
Line 12: typedef testing::Test StatusControllerTest;
On 2009/11/18 00:21:40, nick wrote:
> Is there precedent for this?

Yes, a bunch. Search solution for 'typedef testing::Test'.
In fact I start out all my tests like this and only add if I need to.

http://codereview.chromium.org/386030/diff/4002/9062#newcode229
Line 229: sync_pb::SyncEntity* entity1 =
response->mutable_get_updates()->add_entries();
On 2009/11/18 00:21:40, nick wrote:
> You don't use entity1 or entity2.  That's a gcc warning, which is treated as a
> gcc error.

Done.

http://codereview.chromium.org/386030/diff/4002/9057
File chrome/browser/sync/sessions/sync_session.cc (right):

http://codereview.chromium.org/386030/diff/4002/9057#newcode20
Line 20: bool is_share_usable = false;
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> move down and combine with assignment, set const

Done.

http://codereview.chromium.org/386030/diff/4002/9058
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/386030/diff/4002/9058#newcode136
Line 136: public:
On 2009/11/18 00:21:40, nick wrote:
> indent.

Done.

http://codereview.chromium.org/386030/diff/4002/9058#newcode139
Line 139: : session_(session) {
On 2009/11/18 00:21:40, nick wrote:
> indent.

Done.

http://codereview.chromium.org/386030/diff/4002/9058#newcode143
Line 143: ~ScopedSetSessionWriteTransaction() { session_->write_transaction_ =
NULL; }
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> endl between this and the next line might be more clear

Done.

http://codereview.chromium.org/386030/diff/4002/9058#newcode144
Line 144: private:
On 2009/11/18 00:21:40, nick wrote:
> indent.

Done.

http://codereview.chromium.org/386030/diff/4002/9060
File chrome/browser/sync/sessions/sync_session_context.h (right):

http://codereview.chromium.org/386030/diff/4002/9060#newcode6
Line 6: // that are useful when processing a SyncSession.
On 2009/11/18 00:21:40, nick wrote:
> This description could apply to just about anything; it's not very
informative. 
> Please be more concrete.

That doesn't mean that anything could benefit from this description :) I feel
this class is pretty simple with an accurate name, but I tried to boost the
comment a bit.

http://codereview.chromium.org/386030/diff/4002/9060#newcode47
Line 47: // In unittests, there may be no UI thread, so the above will fail.
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> Usually you comment before the line. So move this up and talk about the below
> line?

Done.

http://codereview.chromium.org/386030/diff/4002/9060#newcode87
Line 87: friend class ScopedSessionContextSyncerEventChannel;
On 2009/11/18 00:21:40, nick wrote:
> Why do these need to be available only on a temporary basis?  Couldn't they be
> permanent?
As my comment at the top of the file says, the context doesn't take ownership of
anything not explicitly created by this object. I did not see any reason to
extend the lifetime of the ConflictResolver or SyncerEventChannel beyond what it
is today. It seems natural for the Syncer to own and manage lifetimes for these.

http://codereview.chromium.org/386030/diff/4002/9060#newcode138
Line 138: // one SyncerEventChannel in the system, so it is an error to use this
if the
On 2009/11/18 00:21:40, nick wrote:
> Per your other comment, there are typically two SyncerEventChannels.

Oh, yeah. The error part was right, but fixed the 'system' bit to say 'context'.

http://codereview.chromium.org/386030/diff/4002/9060#newcode145
Line 145: DCHECK(!context->syncer_event_channel_);
On 2009/11/17 23:09:17, chron_chromium.org wrote:
> NULL == slightly more clear

Fixed here and above.

http://codereview.chromium.org/386030/diff/4002/9056
File chrome/browser/sync/sessions/sync_session_unittest.cc (right):

http://codereview.chromium.org/386030/diff/4002/9056#newcode35
Line 35: FailControllerInvocationIfDisabled("OnSilencedUntil");
On 2009/11/18 00:21:40, nick wrote:
> Weird indent.

Done.

http://codereview.chromium.org/386030/diff/4002/9056#newcode38
Line 38: FailControllerInvocationIfDisabled("IsSyncingCurrentlySilenced");
On 2009/11/18 00:21:40, nick wrote:
> Weird indent.

Done.

http://codereview.chromium.org/386030/diff/4002/9056#newcode42
Line 42: const base::TimeDelta& new_interval) {
On 2009/11/18 00:21:40, nick wrote:
> Do you need to include something to use TimeDelta?

I don't make use of it beyond what the interface does, so I think 'minimize
includes' rule applies.

http://codereview.chromium.org/386030/diff/4002/9056#newcode56
Line 56: bool fail_controller_invocations_;
On 2009/11/18 00:21:40, nick wrote:
> totally optional: consider inverting the sense of this so that it's
> controller_invocations_expected_.

Done.

http://codereview.chromium.org/386030/diff/4002/9056#newcode89
Line 89: EXPECT_FALSE(session.write_transaction());
On 2009/11/18 00:21:40, nick wrote:
> compare with NULL

Done.

http://codereview.chromium.org/386030/diff/4002/9056#newcode93
Line 93: EXPECT_TRUE(session.write_transaction());
On 2009/11/18 00:21:40, nick wrote:
> compare with &trans

Done.

http://codereview.chromium.org/386030/diff/4002/9056#newcode135
Line 135: status()->mutable_updates_response()->Clear();
On 2009/11/18 00:21:40, nick wrote:
> Hmm.  What happens if the client gets a 500 error on GetUpdates.  there's no
> updates response in that case ...

I think you found a bug. A particularly messy one if it occurs on the first
sync, because we mark Directory::initial_sync_ended to true.
In the steady-state, it's not so bad because the order in HasMoreToSync is
important, and if the result depends on 'got zero updates', several other things
must be true. So the worst that could happen (I think) is we miss some updates
that we should have seen, but at least we're likely in the READY state).

But on initial sync... I don't know. I tried to trace through and see what would
catch this and so far I'm empty handed. timestamp_dirty won't get set because
updates.has_new_timestamp() will be false.

[ncarter (slow), 2009-11-20 01:29:54]

LGTM after you respond to the following (particularly the question about the
snapshot)

http://codereview.chromium.org/386030/diff/4002/9032
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/386030/diff/4002/9032#newcode157
Line 157: session->controller()->OnSilencedUntil(base::TimeTicks::Now() +
On 2009/11/19 04:08:21, timsteele wrote:
> On 2009/11/17 23:09:17, http://chron_chromium.org wrote:
> > What's the difference between controller and status controller?
> 
> The former is SyncSession::Controller; an external entity that controls the
> session and session initiation, that the session can delegate to when a
> session-global action must absolutely be taken (see comment in
sync_session.h).
> I had named it 'Delegate' before, akin to URLRequest::Delegate and perhaps
more
> common lingo in Chrome. Maybe I should name it back.
> 
> OTOH, SyncSession Is Composed Of (in the UML sense) a StatusController (or
> 'SessionStateController') which is more of an MVC type controller for the
model
> data in session_state.
> 

I suggest not obsessing over the names, but I think having two Controllers with
very different responsibilities is a potential source of confusion.

http://codereview.chromium.org/386030/diff/4002/9022
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/386030/diff/4002/9022#newcode141
Line 141: disable_idle_detection_(false) {
On 2009/11/19 04:08:21, timsteele wrote:
> On 2009/11/18 00:21:40, nick wrote:
> > Should we initialize silenced_until_ to Now()?
> 
> Hm, not sure I follow? I think the default of '0' aka is_null() is what we
want.

I was wrong -- didn't realize that 0 was a special time value that could be
checked via is_null(); I mistakenly thought it always meant some random date in
the 1970s.  While those were good times, history has moved on.

http://codereview.chromium.org/386030/diff/4002/9037
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/386030/diff/4002/9037#newcode210
Line 210: const base::TimeDelta& new_interval);
On 2009/11/19 04:08:21, timsteele wrote:
> On 2009/11/18 00:21:40, nick wrote:
> > Should these really be private, if they're invoked through an interface?
> 
> I don't know. It worked and I felt there was something satisfying about not
> letting you call these through a SyncerThread*, from an abstraction point of
> view.  Maybe I should just move it to the public section to be normal.

http://www.parashift.com/c++-faq-lite/proper-inheritance.html#faq-21.1

http://codereview.chromium.org/386030/diff/4002/9021
File chrome/browser/sync/engine/syncer_thread_unittest.cc (right):

http://codereview.chromium.org/386030/diff/4002/9021#newcode192
Line 192: SyncSessionContext* context = new SyncSessionContext(NULL, NULL,
NULL);
On 2009/11/19 04:08:21, timsteele wrote:
> On 2009/11/17 23:09:17, http://chron_chromium.org wrote:
> > Would it make sense to have a default constructor?
> 
> The class would then have multiple constructors, and style guide says not to
> overload to simulate default parameters.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

You can, however, use the named constructor idiom.

http://codereview.chromium.org/386030/diff/4002/9007
File chrome/browser/sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/386030/diff/4002/9007#newcode311
Line 311: ConflictProgress* progress =
On 2009/11/19 04:08:21, timsteele wrote:
> On 2009/11/17 23:09:17, http://chron_chromium.org wrote:
> > Conflict Progress? Maybe a better name for this.
> 
> It's a wrapper for conflict-related progress state tracking. I'm kinda stuck
on
> a better name.

I like it.

http://codereview.chromium.org/386030/diff/4002/9007#newcode346
Line 346: void FailTest(const std::string& msg){
On 2009/11/19 04:08:21, timsteele wrote:
> On 2009/11/17 23:09:17, http://chron_chromium.org wrote:
> > Is this better than the FAIL() << MSG?
> 
> No, but it's needed because you can't call ASSERT_* or FAIL from non-void
> methods. (The macros are designed to be called from TEST_Fs which are void and
> do 'return;', so you get a compile error if your method returns bool for
> example, which IsSyncingCurrentlySilenced does).

What you're looking for is ADD_FAILURE().  It logs a failure like FAIL(),
without returning.

http://codereview.chromium.org/386030/diff/4002/9063
File chrome/browser/sync/sessions/status_controller.cc (right):

http://codereview.chromium.org/386030/diff/4002/9063#newcode22
Line 22: template <class FieldType, class DirtyableContainer>
On 2009/11/19 04:08:21, timsteele wrote:
> On 2009/11/18 00:21:40, nick wrote:
> > nit: i'd use 'typename FieldType' since it's often bool/int.
> 
> Done.

Not done.

http://codereview.chromium.org/386030/diff/4002/9056
File chrome/browser/sync/sessions/sync_session_unittest.cc (right):

http://codereview.chromium.org/386030/diff/4002/9056#newcode135
Line 135: status()->mutable_updates_response()->Clear();
On 2009/11/19 04:08:21, timsteele wrote:
> On 2009/11/18 00:21:40, nick wrote:
> > Hmm.  What happens if the client gets a 500 error on GetUpdates.  there's no
> > updates response in that case ...
> 
> I think you found a bug. A particularly messy one if it occurs on the first
> sync, because we mark Directory::initial_sync_ended to true.
> In the steady-state, it's not so bad because the order in HasMoreToSync is
> important, and if the result depends on 'got zero updates', several other
things
> must be true. So the worst that could happen (I think) is we miss some updates
> that we should have seen, but at least we're likely in the READY state).
> 
> But on initial sync... I don't know. I tried to trace through and see what
would
> catch this and so far I'm empty handed. timestamp_dirty won't get set because
> updates.has_new_timestamp() will be false.

If we made it as far as the ModelAssociator without crashing, we'd be caught
without the tagged permanent nodes.  That's an unrecoverable error.

Filed http://crbug.com/28315

http://codereview.chromium.org/386030/diff/13001/13003
File chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc
(right):

http://codereview.chromium.org/386030/diff/13001/13003#newcode50
Line 50: session->context()->resolver(), session->status_controller());
Reduce indentation.

http://codereview.chromium.org/386030/diff/13001/13007
File chrome/browser/sync/engine/syncer_command.cc (right):

http://codereview.chromium.org/386030/diff/13001/13007#newcode34
Line 34: const sessions::SyncSessionSnapshot& snapshot(session->TakeSnapshot());
On 2009/11/19 04:08:21, timsteele wrote:
> On 2009/11/18 00:21:40, nick wrote:
> > Why make a copy to take a snapshot, if it's going to be consumed
synchronously
> > by the listeners?
> 
> Done.

You didn't answer the question.

http://codereview.chromium.org/386030/diff/13001/13025
File chrome/browser/sync/engine/syncer_command.h (right):

http://codereview.chromium.org/386030/diff/13001/13025#newcode38
Line 38: private:
On 2009/11/19 04:08:21, timsteele wrote:
> On 2009/11/18 00:21:40, nick wrote:
> > I wonder if it would improve things, if Execute stashed the session away as
a
> > member, and ExecuteImpl took no args.  This would get rid of a lot of the
> > passing-around we do.
> 
> This might be nice, I agree. Does anything call Execute repatedly and change
> state in between? I seem to think the unittests do this? Or at least I feel
like
> it would be a big change syntactically, could I try it in an isolated CL?
> 

Fine to defer.  AFAIK the only unittest that actually cares or knows about
SyncerCommands is the apply updates test.  All the tests in syncer_unittest.cc
go through SyncShare.

[ncarter (slow), 2009-11-20 01:30:40]

Also: You must fix the trybots.

[tim (not reviewing), 2009-11-20 02:13:54]

On 2009/11/20 01:30:40, nick wrote:
> Also: You must fix the trybots.

All done, also made Syncer and SyncerEvent ctors explicit.
Waiting for try servers.

[tim (not reviewing), 2009-11-20 02:14:18]

Gah, here are my comments.

http://codereview.chromium.org/386030/diff/4002/9032
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/386030/diff/4002/9032#newcode157
Line 157: session->controller()->OnSilencedUntil(base::TimeTicks::Now() +
On 2009/11/20 01:29:55, nick wrote:
> On 2009/11/19 04:08:21, timsteele wrote:
> > On 2009/11/17 23:09:17, http://chron_chromium.org wrote:
> > > What's the difference between controller and status controller?
> > 
> > The former is SyncSession::Controller; an external entity that controls the
> > session and session initiation, that the session can delegate to when a
> > session-global action must absolutely be taken (see comment in
> sync_session.h).
> > I had named it 'Delegate' before, akin to URLRequest::Delegate and perhaps
> more
> > common lingo in Chrome. Maybe I should name it back.
> > 
> > OTOH, SyncSession Is Composed Of (in the UML sense) a StatusController (or
> > 'SessionStateController') which is more of an MVC type controller for the
> model
> > data in session_state.
> > 
> 
> I suggest not obsessing over the names, but I think having two Controllers
with
> very different responsibilities is a potential source of confusion.

Made SyncSession::Controller SyncSession::Delegate instead and updated comments.

http://codereview.chromium.org/386030/diff/4002/9021
File chrome/browser/sync/engine/syncer_thread_unittest.cc (right):

http://codereview.chromium.org/386030/diff/4002/9021#newcode192
Line 192: SyncSessionContext* context = new SyncSessionContext(NULL, NULL,
NULL);
On 2009/11/20 01:29:55, nick wrote:
> On 2009/11/19 04:08:21, timsteele wrote:
> > On 2009/11/17 23:09:17, http://chron_chromium.org wrote:
> > > Would it make sense to have a default constructor?
> > 
> > The class would then have multiple constructors, and style guide says not to
> > overload to simulate default parameters.
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> 
> You can, however, use the named constructor idiom.

Frankly, I like the explicitness in this case and since this ctor would only be
useful for unittests I don't want to add it.

http://codereview.chromium.org/386030/diff/4002/9007
File chrome/browser/sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/386030/diff/4002/9007#newcode346
Line 346: void FailTest(const std::string& msg){
On 2009/11/20 01:29:55, nick wrote:
> On 2009/11/19 04:08:21, timsteele wrote:
> > On 2009/11/17 23:09:17, http://chron_chromium.org wrote:
> > > Is this better than the FAIL() << MSG?
> > 
> > No, but it's needed because you can't call ASSERT_* or FAIL from non-void
> > methods. (The macros are designed to be called from TEST_Fs which are void
and
> > do 'return;', so you get a compile error if your method returns bool for
> > example, which IsSyncingCurrentlySilenced does).
> 
> What you're looking for is ADD_FAILURE().  It logs a failure like FAIL(),
> without returning.

Awesome! used that instead.

http://codereview.chromium.org/386030/diff/4002/9063
File chrome/browser/sync/sessions/status_controller.cc (right):

http://codereview.chromium.org/386030/diff/4002/9063#newcode22
Line 22: template <class FieldType, class DirtyableContainer>
On 2009/11/20 01:29:55, nick wrote:
> On 2009/11/19 04:08:21, timsteele wrote:
> > On 2009/11/18 00:21:40, nick wrote:
> > > nit: i'd use 'typename FieldType' since it's often bool/int.
> > 
> > Done.
> 
> Not done.

Done.

http://codereview.chromium.org/386030/diff/13001/13003
File chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc
(right):

http://codereview.chromium.org/386030/diff/13001/13003#newcode50
Line 50: session->context()->resolver(), session->status_controller());
On 2009/11/20 01:29:55, nick wrote:
> Reduce indentation.

Done.

http://codereview.chromium.org/386030/diff/13001/13007
File chrome/browser/sync/engine/syncer_command.cc (right):

http://codereview.chromium.org/386030/diff/13001/13007#newcode34
Line 34: const sessions::SyncSessionSnapshot& snapshot(session->TakeSnapshot());
On 2009/11/20 01:29:55, nick wrote:
> On 2009/11/19 04:08:21, timsteele wrote:
> > On 2009/11/18 00:21:40, nick wrote:
> > > Why make a copy to take a snapshot, if it's going to be consumed
> synchronously
> > > by the listeners?
> > 
> > Done.
> 
> You didn't answer the question.

I removed the copy, though.  Unless you're referring to the internal copy of the
fields in SyncerStatus and the counters in ErrorCounters done inside
TakeSnapshot.  I had a const& originally, but decided the true immutability of
the snapshot was a nice property; for example, event though this is consumed
synchronously, it's totally possible to cause some event to happen (or directly
modify the SyncSession object) that would cause the values referred to by the
const& references in the snapshot to change from under you.  This way, const
does not lie; it is strictly immutable.  As a side note it's also more flexible
in that we could, if we wanted in the future, kick a Snapshot to the UI thread
for something like aboot:sync or other UI purposes, instead of going through all
the intermediaries we do now, and this way it would "just work".

http://codereview.chromium.org/386030/diff/13001/13025
File chrome/browser/sync/engine/syncer_command.h (right):

http://codereview.chromium.org/386030/diff/13001/13025#newcode38
Line 38: private:
On 2009/11/20 01:29:55, nick wrote:
> On 2009/11/19 04:08:21, timsteele wrote:
> > On 2009/11/18 00:21:40, nick wrote:
> > > I wonder if it would improve things, if Execute stashed the session away
as
> a
> > > member, and ExecuteImpl took no args.  This would get rid of a lot of the
> > > passing-around we do.
> > 
> > This might be nice, I agree. Does anything call Execute repatedly and change
> > state in between? I seem to think the unittests do this? Or at least I feel
> like
> > it would be a big change syntactically, could I try it in an isolated CL?
> > 
> 
> Fine to defer.  AFAIK the only unittest that actually cares or knows about
> SyncerCommands is the apply updates test.  All the tests in syncer_unittest.cc
> go through SyncShare.

Done.

[Fred, 2009-11-22 20:48:51]

Drive-by.

Now that this is checked in, you should make a follow-up changelist to clean up
all the lint errors.

Watch out for gcc's signed/unsigned warnings when changing EXPECT_TRUE to
EXPECT_EQ!

On 2009/11/20 02:14:18, timsteele wrote:
> Gah, here are my comments.
> 
> http://codereview.chromium.org/386030/diff/4002/9032
> File chrome/browser/sync/engine/syncer_proto_util.cc (right):
> 
> http://codereview.chromium.org/386030/diff/4002/9032#newcode157
> Line 157: session->controller()->OnSilencedUntil(base::TimeTicks::Now() +
> On 2009/11/20 01:29:55, nick wrote:
> > On 2009/11/19 04:08:21, timsteele wrote:
> > > On 2009/11/17 23:09:17, http://chron_chromium.org wrote:
> > > > What's the difference between controller and status controller?
> > > 
> > > The former is SyncSession::Controller; an external entity that controls
the
> > > session and session initiation, that the session can delegate to when a
> > > session-global action must absolutely be taken (see comment in
> > sync_session.h).
> > > I had named it 'Delegate' before, akin to URLRequest::Delegate and perhaps
> > more
> > > common lingo in Chrome. Maybe I should name it back.
> > > 
> > > OTOH, SyncSession Is Composed Of (in the UML sense) a StatusController (or
> > > 'SessionStateController') which is more of an MVC type controller for the
> > model
> > > data in session_state.
> > > 
> > 
> > I suggest not obsessing over the names, but I think having two Controllers
> with
> > very different responsibilities is a potential source of confusion.
> 
> Made SyncSession::Controller SyncSession::Delegate instead and updated
comments.
> 
> http://codereview.chromium.org/386030/diff/4002/9021
> File chrome/browser/sync/engine/syncer_thread_unittest.cc (right):
> 
> http://codereview.chromium.org/386030/diff/4002/9021#newcode192
> Line 192: SyncSessionContext* context = new SyncSessionContext(NULL, NULL,
> NULL);
> On 2009/11/20 01:29:55, nick wrote:
> > On 2009/11/19 04:08:21, timsteele wrote:
> > > On 2009/11/17 23:09:17, http://chron_chromium.org wrote:
> > > > Would it make sense to have a default constructor?
> > > 
> > > The class would then have multiple constructors, and style guide says not
to
> > > overload to simulate default parameters.
> > >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> > 
> > You can, however, use the named constructor idiom.
> 
> Frankly, I like the explicitness in this case and since this ctor would only
be
> useful for unittests I don't want to add it.
> 
> http://codereview.chromium.org/386030/diff/4002/9007
> File chrome/browser/sync/engine/syncer_unittest.cc (right):
> 
> http://codereview.chromium.org/386030/diff/4002/9007#newcode346
> Line 346: void FailTest(const std::string& msg){
> On 2009/11/20 01:29:55, nick wrote:
> > On 2009/11/19 04:08:21, timsteele wrote:
> > > On 2009/11/17 23:09:17, http://chron_chromium.org wrote:
> > > > Is this better than the FAIL() << MSG?
> > > 
> > > No, but it's needed because you can't call ASSERT_* or FAIL from non-void
> > > methods. (The macros are designed to be called from TEST_Fs which are void
> and
> > > do 'return;', so you get a compile error if your method returns bool for
> > > example, which IsSyncingCurrentlySilenced does).
> > 
> > What you're looking for is ADD_FAILURE().  It logs a failure like FAIL(),
> > without returning.
> 
> Awesome! used that instead.
> 
> http://codereview.chromium.org/386030/diff/4002/9063
> File chrome/browser/sync/sessions/status_controller.cc (right):
> 
> http://codereview.chromium.org/386030/diff/4002/9063#newcode22
> Line 22: template <class FieldType, class DirtyableContainer>
> On 2009/11/20 01:29:55, nick wrote:
> > On 2009/11/19 04:08:21, timsteele wrote:
> > > On 2009/11/18 00:21:40, nick wrote:
> > > > nit: i'd use 'typename FieldType' since it's often bool/int.
> > > 
> > > Done.
> > 
> > Not done.
> 
> Done.
> 
> http://codereview.chromium.org/386030/diff/13001/13003
> File chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc
> (right):
> 
> http://codereview.chromium.org/386030/diff/13001/13003#newcode50
> Line 50: session->context()->resolver(), session->status_controller());
> On 2009/11/20 01:29:55, nick wrote:
> > Reduce indentation.
> 
> Done.
> 
> http://codereview.chromium.org/386030/diff/13001/13007
> File chrome/browser/sync/engine/syncer_command.cc (right):
> 
> http://codereview.chromium.org/386030/diff/13001/13007#newcode34
> Line 34: const sessions::SyncSessionSnapshot&
snapshot(session->TakeSnapshot());
> On 2009/11/20 01:29:55, nick wrote:
> > On 2009/11/19 04:08:21, timsteele wrote:
> > > On 2009/11/18 00:21:40, nick wrote:
> > > > Why make a copy to take a snapshot, if it's going to be consumed
> > synchronously
> > > > by the listeners?
> > > 
> > > Done.
> > 
> > You didn't answer the question.
> 
> I removed the copy, though.  Unless you're referring to the internal copy of
the
> fields in SyncerStatus and the counters in ErrorCounters done inside
> TakeSnapshot.  I had a const& originally, but decided the true immutability of
> the snapshot was a nice property; for example, event though this is consumed
> synchronously, it's totally possible to cause some event to happen (or
directly
> modify the SyncSession object) that would cause the values referred to by the
> const& references in the snapshot to change from under you.  This way, const
> does not lie; it is strictly immutable.  As a side note it's also more
flexible
> in that we could, if we wanted in the future, kick a Snapshot to the UI thread
> for something like aboot:sync or other UI purposes, instead of going through
all
> the intermediaries we do now, and this way it would "just work".
> 
> http://codereview.chromium.org/386030/diff/13001/13025
> File chrome/browser/sync/engine/syncer_command.h (right):
> 
> http://codereview.chromium.org/386030/diff/13001/13025#newcode38
> Line 38: private:
> On 2009/11/20 01:29:55, nick wrote:
> > On 2009/11/19 04:08:21, timsteele wrote:
> > > On 2009/11/18 00:21:40, nick wrote:
> > > > I wonder if it would improve things, if Execute stashed the session away
> as
> > a
> > > > member, and ExecuteImpl took no args.  This would get rid of a lot of
the
> > > > passing-around we do.
> > > 
> > > This might be nice, I agree. Does anything call Execute repatedly and
change
> > > state in between? I seem to think the unittests do this? Or at least I
feel
> > like
> > > it would be a big change syntactically, could I try it in an isolated CL?
> > > 
> > 
> > Fine to defer.  AFAIK the only unittest that actually cares or knows about
> > SyncerCommands is the apply updates test.  All the tests in
syncer_unittest.cc
> > go through SyncShare.
> 
> Done.