sync: Implement NonBlockingTypeProcessorCore

sync: Implement NonBlockingTypeProcessorCore

Introduces the second half of the non-blocking sync engine.  For now,
most of the classes invovled are never instantiated outside of tests.

Adds NonBlockingTypeProcessorCore, the sync thread component of
non-blocking sync.  It coordinates between the sync server and the
NonBlockingTypeProcessor that lives on the model thread.  The
SyncThreadSyncEntity exists to help it handle keep track of the
in-flight sync entities.

The NonBlockingTypeProcessorCore interacts with the sync thread
components by implementing both the UpdateHandler and CommitContributor
interfaces.  This allows it to take part in commit and update operations
that are managed by the syncer.

As part of its implementation of the CommitContributor interface, the
NonBlockingTypeProcessorCore introduces
a NonBlockingTypeProcessorCommitContribution class to manage its
contribution to a commit request and associated it with the response.

This CL includes a large amount of test framework code  to help test the
NonBlockingTypeProcessorCore.

Makes the SyncEntityToValue function in proto_value_conversions.h
public to enable more informative debug messages.

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

[rlarocque, 2014-05-24 00:08:51]

Here's part two.  It's slightly bigger than the first.  If it's any consolation,
more than half the code is tests.

If you'd like, I can try to break this up into smaller chunks.  Personally, I
think it will be easier to review this as is, since a lot of this code is
inter-dependent.

Either way, please review.

[Nicolas Zea, 2014-05-28 23:56:15]

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_syn...
File sync/engine/non_blocking_sync_common.h (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_syn...
sync/engine/non_blocking_sync_common.h:17: static const int64
kUncommittedVersion = -1;
Any reason this was changed from 0 to -1?

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
File sync/engine/non_blocking_type_commit_contribution.cc (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_commit_contribution.cc:44: bool serious_error =
false;
How about unknown error instead, as that better reflects what we return.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_commit_contribution.cc:54: LOG(ERROR) << "Server
reports our commit message is invalid.";
nit: remove "our", here and below

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_commit_contribution.cc:60: LOG(ERROR) << "Server
reports conflict for our commit message.";
Conflicts are valid responses, why treat this as an error?

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_commit_contribution.cc:91: // This is where we let
the scheduler know about the failures.
nit: remove "This is where we"

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
File sync/engine/non_blocking_type_commit_contribution.h (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_commit_contribution.h:21:
NonBlockingTypeProcessorCore* parent,
Parent is a pretty overloaded/ambiguous term. How about type_core or something?

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_commit_contribution.h:23: const
google::protobuf::RepeatedPtrField<sync_pb::SyncEntity>& entities,
I wonder if it's worth passing this as vector of some wrapper struct that
encapsulates an entity and a sequence number. It might be useful to have a
tighter coupling of the two.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_commit_contribution.h:37:
NonBlockingTypeProcessorCore* parent_;
make a const pointer?

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_commit_contribution.h:39: // The type-global
context inforamtion.
nit: information

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_commit_contribution.h:40: sync_pb::DataTypeContext
context_;
make const?

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
File sync/engine/non_blocking_type_processor_core.cc (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:13: #include
"sync/engine/non_blocking_type_processor.h"
Why is NonBlockingTypeProcessor needed, if we have a
NonBlockingTypeProcessorInterface?

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:44: *progress_marker =
data_type_state_.progress_marker;
Why the difference between the context and progress marker (copyfrom vs
assignment op)

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:76:
DCHECK_EQ(data_type_state_.type_root_id, update_entity->id_string());
nit: DCHECK(..empty() || ..type_root_id == ...)

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:80: // More normal updates are
handled here.
nit: remove "More"

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:90:
entities_.insert(std::make_pair(client_tag_hash, entity));
When do we remove from the entities map?

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:111: // updates to the model
thread so it can do the rest.
nit: you can just say "Forward these updates.."

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:159: int64 sequence_number;
initialize to 0 (or -1 or something invalid)

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:179: DCHECK_EQ(type_,
GetModelTypeFromSpecifics(request.specifics));
don't we always want the model type now?

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:247: // We'll get it in the
first update response.
Where will be the logic to determine if a particular entity is ready to commit?
(i.e. that checks conflict, encryption, etc. state)

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:257:
data_type_state_.next_client_id++;
nit: can combine with previous line

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
File sync/engine/non_blocking_type_processor_core.h (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.h:14: #include
"sync/engine/non_blocking_type_processor_interface.h"
can forward declare

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.h:55: DataTypeState initial_state,
const ref?

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.h:82: void
ProcessCommitResponses(const CommitResponseDataList& response_list);
OnCommitResponse?

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
File sync/engine/non_blocking_type_processor_core_unittest.cc (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:76: static const
std::string kTypeParentId;
Why make this a static part of the test class? Why not just have it be a const
variable in the anon namespace above?

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:96: // Modifications on
the model thread that get sent to us.
nit: using "us" is kind of ambiguous here, and elsewhere in this test file.
Prefer explicitly saying "the core" or something similar

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:101: void
TypeRootUpdateFromServer();
possibly rename these Trigger*, to make it clearer that it's an action being
taken, not an object

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:107: // Callbacks from
the mock processor.  Called when the core_ tries to send
nit: |core_|

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:109: void
ModelThreadReceivesCommitResponse(
nit: Receives -> Received seems clearer. Or perhaps
OnModelThreadReceivedCommitResponse

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:127: bool WillCommit();
Given the number of helper methods here, perhaps it would be cleaner to break
them into two separate helper classes, those related to the core, and those
related to the model thread, rather than having them all as part of this massive
test class. Would that be difficult?

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:874:
TEST_F(NonBlockingTypeProcessorCoreTest, NoCommitUntilFirstUpdate) {
Is this a valid sequence of events? It seems to me the datatype should not
attempt to commit anything until the type root node/first sync has been
completed.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:933: }
Will there also be update tests here as well?

https://codereview.chromium.org/299963002/diff/1/sync/engine/sync_thread_sync...
File sync/engine/sync_thread_sync_entity.cc (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/sync_thread_sync...
sync/engine/sync_thread_sync_entity.cc:189: // Commits are synchronous, so there
should there's no reason why the
nit: there should there's

https://codereview.chromium.org/299963002/diff/1/sync/engine/sync_thread_sync...
sync/engine/sync_thread_sync_entity.cc:241: specifics_.Clear();
why clear the specific here?

[rlarocque, 2014-05-29 20:54:51]

Some of your comments will require a bit more work to address.  In particular, I
need to:
- Add one or more update processing unit tests.
- Move the responsibility for not committing before the first update to the
NonBlockingTypeProcessor.

That's going to take a little while.  In the meantime, here's a bunch of small
fixes and responses to some of your comments.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_syn...
File sync/engine/non_blocking_sync_common.h (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_syn...
sync/engine/non_blocking_sync_common.h:17: static const int64
kUncommittedVersion = -1;
On 2014/05/28 23:56:15, Nicolas Zea wrote:
> Any reason this was changed from 0 to -1?

-1 is what the current sync engine uses.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
File sync/engine/non_blocking_type_commit_contribution.cc (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_commit_contribution.cc:44: bool serious_error =
false;
On 2014/05/28 23:56:15, Nicolas Zea wrote:
> How about unknown error instead, as that better reflects what we return.

Done.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_commit_contribution.cc:54: LOG(ERROR) << "Server
reports our commit message is invalid.";
On 2014/05/28 23:56:15, Nicolas Zea wrote:
> nit: remove "our", here and below

Done.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_commit_contribution.cc:60: LOG(ERROR) << "Server
reports conflict for our commit message.";
On 2014/05/28 23:56:15, Nicolas Zea wrote:
> Conflicts are valid responses, why treat this as an error?

I suppose you're right.  There are a few data types that could return a conflict
response.

Changed to warning, since it should be extremely rare.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_commit_contribution.cc:91: // This is where we let
the scheduler know about the failures.
On 2014/05/28 23:56:15, Nicolas Zea wrote:
> nit: remove "This is where we"

Done.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
File sync/engine/non_blocking_type_commit_contribution.h (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_commit_contribution.h:21:
NonBlockingTypeProcessorCore* parent,
On 2014/05/28 23:56:15, Nicolas Zea wrote:
> Parent is a pretty overloaded/ambiguous term. How about type_core or
something?

I went with processor_core_ to match the class name.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_commit_contribution.h:23: const
google::protobuf::RepeatedPtrField<sync_pb::SyncEntity>& entities,
On 2014/05/28 23:56:15, Nicolas Zea wrote:
> I wonder if it's worth passing this as vector of some wrapper struct that
> encapsulates an entity and a sequence number. It might be useful to have a
> tighter coupling of the two.

I'm on the fence about this.  We have to balance the advantages of more explicit
coupling with the disadvantages of adding yet another struct or class
definition.  I think this makes sense for now, but we should definitely
reconsider if there were three or four parallel vectors.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_commit_contribution.h:37:
NonBlockingTypeProcessorCore* parent_;
On 2014/05/28 23:56:15, Nicolas Zea wrote:
> make a const pointer?

Done.

It could even be a reference, since it's guaranteed to be non-NULL.  I kept it
as a pointer because I don't recall seeing many non-const references or
references that are members of a class in this code base, so I inferred that
there may be some convention that discourages it.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_commit_contribution.h:39: // The type-global
context inforamtion.
On 2014/05/28 23:56:15, Nicolas Zea wrote:
> nit: information

Done.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_commit_contribution.h:40: sync_pb::DataTypeContext
context_;
On 2014/05/28 23:56:15, Nicolas Zea wrote:
> make const?

Done.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
File sync/engine/non_blocking_type_processor_core.cc (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:13: #include
"sync/engine/non_blocking_type_processor.h"
On 2014/05/28 23:56:15, Nicolas Zea wrote:
> Why is NonBlockingTypeProcessor needed, if we have a
> NonBlockingTypeProcessorInterface?

Removed.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:44: *progress_marker =
data_type_state_.progress_marker;
On 2014/05/28 23:56:15, Nicolas Zea wrote:
> Why the difference between the context and progress marker (copyfrom vs
> assignment op)

I believe there's no significant difference between operator= and CopyFrom(). 
You're right that we should be consistent, though.  Chagned to CopyFrom() here.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:76:
DCHECK_EQ(data_type_state_.type_root_id, update_entity->id_string());
On 2014/05/28 23:56:15, Nicolas Zea wrote:
> nit: DCHECK(..empty() || ..type_root_id == ...)

I like it better as is.  As currently written, it will print an informative
error message if it fails.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:80: // More normal updates are
handled here.
On 2014/05/28 23:56:15, Nicolas Zea wrote:
> nit: remove "More"

Done.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:90:
entities_.insert(std::make_pair(client_tag_hash, entity));
On 2014/05/28 23:56:15, Nicolas Zea wrote:
> When do we remove from the entities map?

Never.  The map grows but never shrinks.  If this becomes a problem, we can take
steps to reduce the memory consumption.

The most expensive part of these entries, the specifics, are cleared when no
commits are pending.  So it's not as expensive as it might seem.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:111: // updates to the model
thread so it can do the rest.
On 2014/05/28 23:56:15, Nicolas Zea wrote:
> nit: you can just say "Forward these updates.."

Done.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:159: int64 sequence_number;
On 2014/05/28 23:56:15, Nicolas Zea wrote:
> initialize to 0 (or -1 or something invalid)

Done.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:179: DCHECK_EQ(type_,
GetModelTypeFromSpecifics(request.specifics));
On 2014/05/28 23:56:15, Nicolas Zea wrote:
> don't we always want the model type now?

True, but it doesn't have to be the caller (ie. Processor)'s responsibility. 
This class takes care of it prior to the commit, on line 264.

Either way would work, but I figured that adding the type just before the commit
leaves this class would be slightly less error-prone.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:247: // We'll get it in the
first update response.
On 2014/05/28 23:56:15, Nicolas Zea wrote:
> Where will be the logic to determine if a particular entity is ready to
commit?
> (i.e. that checks conflict, encryption, etc. state)

Conflicts are an event, not a state.  When a conflict happens, we cancel the
pending commit here and forward the update that triggered it to the UI thread.

Encryption is still TBD.  My guess is that items that can't be committed for
encryption reasons will be stored on the UI thread.  When we enter that
passphrase required state, we'll drop any pending commit entries here.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:257:
data_type_state_.next_client_id++;
On 2014/05/28 23:56:16, Nicolas Zea wrote:
> nit: can combine with previous line

Done.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
File sync/engine/non_blocking_type_processor_core.h (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.h:14: #include
"sync/engine/non_blocking_type_processor_interface.h"
On 2014/05/28 23:56:16, Nicolas Zea wrote:
> can forward declare

Done.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.h:55: DataTypeState initial_state,
On 2014/05/28 23:56:16, Nicolas Zea wrote:
> const ref?

Done.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.h:82: void
ProcessCommitResponses(const CommitResponseDataList& response_list);
On 2014/05/28 23:56:16, Nicolas Zea wrote:
> OnCommitResponse?

Done.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
File sync/engine/non_blocking_type_processor_core_unittest.cc (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:76: static const
std::string kTypeParentId;
On 2014/05/28 23:56:16, Nicolas Zea wrote:
> Why make this a static part of the test class? Why not just have it be a const
> variable in the anon namespace above?

Either way works.  I figured that it was tightly coupled to the test, so it made
sense to define it in the test framework.  Would there be some advantage to
moving it out of the class?

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:96: // Modifications on
the model thread that get sent to us.
On 2014/05/28 23:56:16, Nicolas Zea wrote:
> nit: using "us" is kind of ambiguous here, and elsewhere in this test file.
> Prefer explicitly saying "the core" or something similar

Done.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:101: void
TypeRootUpdateFromServer();
On 2014/05/28 23:56:16, Nicolas Zea wrote:
> possibly rename these Trigger*, to make it clearer that it's an action being
> taken, not an object

Done.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:107: // Callbacks from
the mock processor.  Called when the core_ tries to send
On 2014/05/28 23:56:16, Nicolas Zea wrote:
> nit: |core_|

Done.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:109: void
ModelThreadReceivesCommitResponse(
On 2014/05/28 23:56:16, Nicolas Zea wrote:
> nit: Receives -> Received seems clearer. Or perhaps
> OnModelThreadReceivedCommitResponse

Done.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:127: bool WillCommit();
On 2014/05/28 23:56:16, Nicolas Zea wrote:
> Given the number of helper methods here, perhaps it would be cleaner to break
> them into two separate helper classes, those related to the core, and those
> related to the model thread, rather than having them all as part of this
massive
> test class. Would that be difficult?

I don't think it would be difficult.  We could have separate classes to mock out
the server and model thread.

To some degree we already do have at least one separate class, the
MockNonBlockingTypeProcessor.  It should be possible to move more code into it.

The counter-argument is that if we split them up, then the amount of code in
this class is likely to increase.  The test framework will likely still have the
same methods as before, but with stub implementations.  ie. "void
NonBlockingTypeProcessorCoreTest::DoSuccessfulCommit() {
mock_server_.DoSuccessfulCommit(); }".  The real implementations would live on
elsewhere in this file.

I suppose we could mitigate those effects by moving the helper classes into
separate files, or have the test cases interact with them directly.  I don't
feel great about either of those alternatives.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:874:
TEST_F(NonBlockingTypeProcessorCoreTest, NoCommitUntilFirstUpdate) {
On 2014/05/28 23:56:16, Nicolas Zea wrote:
> Is this a valid sequence of events? It seems to me the datatype should not
> attempt to commit anything until the type root node/first sync has been
> completed.

I would define "datatype" as the code that is specific to a particular type, eg.
the bookmarks model.  The NonBlockingTypeProcessor is not included.

By that definition, the datatype doesn't know what sync's state is.  It
interacts with the storage+sync API but does not know whether those items are
being saved to disk, the cloud, or both.

I think the point you're making, though, is that the responsibility for not
committing items before the first update is received could lie with the
NonBlockingTypeProcessor, rather than the NonBlockingTypeProcessorCore.  Either
way should be viable, but I think your suggestion has a few advantages.

I'll try to re-implement it that way.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:933: }
On 2014/05/28 23:56:16, Nicolas Zea wrote:
> Will there also be update tests here as well?

Yep.  I was planning on adding a lot more tests, but decided to stop here since
the CL was getting pretty big.

I suppose I should add at least one update test, though.  Will fix.

https://codereview.chromium.org/299963002/diff/1/sync/engine/sync_thread_sync...
File sync/engine/sync_thread_sync_entity.cc (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/sync_thread_sync...
sync/engine/sync_thread_sync_entity.cc:189: // Commits are synchronous, so there
should there's no reason why the
On 2014/05/28 23:56:16, Nicolas Zea wrote:
> nit: there should there's

Done.

https://codereview.chromium.org/299963002/diff/1/sync/engine/sync_thread_sync...
sync/engine/sync_thread_sync_entity.cc:241: specifics_.Clear();
On 2014/05/28 23:56:16, Nicolas Zea wrote:
> why clear the specific here?

It's an optimization.  At this point we've given up on the commit, so we don't
need the specifics.  Clearing it might give back a bit of heap memory.

There's no proof that this is a useful optimization, so it is a bit premature. 
I can remove it if you're worried about it.

[rlarocque, 2014-05-30 17:05:56]

The latest uploads include some tests for update response handling in
NonBlockingTypeProcessorCore.  Those tests exposed some bugs in the handling of
non_unique_name, which are now fixed.

Also in these uploads is a set of changes to support an "initial_sync_done" flag
that prevents the NonBlockingTypeProcessor from requesting commits before the
sync engine is ready to commit.

PTAL.

[Nicolas Zea, 2014-06-02 20:27:16]

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
File sync/engine/non_blocking_type_commit_contribution.cc (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_commit_contribution.cc:60: LOG(ERROR) << "Server
reports conflict for our commit message.";
On 2014/05/29 20:54:52, rlarocque wrote:
> On 2014/05/28 23:56:15, Nicolas Zea wrote:
> > Conflicts are valid responses, why treat this as an error?
> 
> I suppose you're right.  There are a few data types that could return a
conflict
> response.
> 
> Changed to warning, since it should be extremely rare.

Why warning at all though? I could see DVLOG, but if it's a valid scenario, it
seems like it's just console spam? 

We'll be tracking conflict counts in about:sync either way right?

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
File sync/engine/non_blocking_type_processor_core.cc (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:90:
entities_.insert(std::make_pair(client_tag_hash, entity));
On 2014/05/29 20:54:52, rlarocque wrote:
> On 2014/05/28 23:56:15, Nicolas Zea wrote:
> > When do we remove from the entities map?
> 
> Never.  The map grows but never shrinks.  If this becomes a problem, we can
take
> steps to reduce the memory consumption.
> 
> The most expensive part of these entries, the specifics, are cleared when no
> commits are pending.  So it's not as expensive as it might seem.

Probably worth having a comment mentioning that we only actually need to track
those entities who have updates in flight, and that that's not there for now for
simplicity's sake.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
File sync/engine/non_blocking_type_processor_core_unittest.cc (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:76: static const
std::string kTypeParentId;
On 2014/05/29 20:54:52, rlarocque wrote:
> On 2014/05/28 23:56:16, Nicolas Zea wrote:
> > Why make this a static part of the test class? Why not just have it be a
const
> > variable in the anon namespace above?
> 
> Either way works.  I figured that it was tightly coupled to the test, so it
made
> sense to define it in the test framework.  Would there be some advantage to
> moving it out of the class?

I find it easier (and more common) to have all const values easily found at the
top of the file. As it is, you declare it here, and initialize it later,
requiring more looking around to get the actual value.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:127: bool WillCommit();
On 2014/05/29 20:54:52, rlarocque wrote:
> On 2014/05/28 23:56:16, Nicolas Zea wrote:
> > Given the number of helper methods here, perhaps it would be cleaner to
break
> > them into two separate helper classes, those related to the core, and those
> > related to the model thread, rather than having them all as part of this
> massive
> > test class. Would that be difficult?
> 
> I don't think it would be difficult.  We could have separate classes to mock
out
> the server and model thread.
> 
> To some degree we already do have at least one separate class, the
> MockNonBlockingTypeProcessor.  It should be possible to move more code into
it.
> 
> The counter-argument is that if we split them up, then the amount of code in
> this class is likely to increase.  The test framework will likely still have
the
> same methods as before, but with stub implementations.  ie. "void
> NonBlockingTypeProcessorCoreTest::DoSuccessfulCommit() {
> mock_server_.DoSuccessfulCommit(); }".  The real implementations would live on
> elsewhere in this file.
> 
> I suppose we could mitigate those effects by moving the helper classes into
> separate files, or have the test cases interact with them directly.  I don't
> feel great about either of those alternatives.  

I think the number of lines is less a concern than just the readability (up to
you if you want to have the classes in a separate file). Also note that you
don't need the stub implementations, you could just have an e.g. mock_server()
getter that exposes the helper classes.

https://codereview.chromium.org/299963002/diff/1/sync/engine/sync_thread_sync...
File sync/engine/sync_thread_sync_entity.cc (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/sync_thread_sync...
sync/engine/sync_thread_sync_entity.cc:241: specifics_.Clear();
On 2014/05/29 20:54:52, rlarocque wrote:
> On 2014/05/28 23:56:16, Nicolas Zea wrote:
> > why clear the specific here?
> 
> It's an optimization.  At this point we've given up on the commit, so we don't
> need the specifics.  Clearing it might give back a bit of heap memory.
> 
> There's no proof that this is a useful optimization, so it is a bit premature.

> I can remove it if you're worried about it.

Does the protobuf library actually free memory when cleared? I'm not sure. I
suppose it doesn't hurt to do this though. Maybe just comment about the purpose
of this.

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
File sync/engine/non_blocking_sync_common.cc (right):

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
sync/engine/non_blocking_sync_common.cc:15:
CommitRequestData::CommitRequestData() {
I just realized all the ints and bools for these structs should also be default
initialized

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
File sync/engine/non_blocking_sync_common.h (right):

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
sync/engine/non_blocking_sync_common.h:50: // model thread should not attempt to
commit any items until this flag is
nit: prefer referring to NonBlockingTypeProcessor directly, rather than just
model thread

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
File sync/engine/non_blocking_type_commit_contribution.cc (right):

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
sync/engine/non_blocking_type_commit_contribution.cc:1: #include
"sync/engine/non_blocking_type_commit_contribution.h"
Seems like the copyright message is missing (and in the header). I'm surprised
the presubmit didn't catch that?

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
File sync/engine/non_blocking_type_commit_contribution.h (right):

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
sync/engine/non_blocking_type_commit_contribution.h:21:
NonBlockingTypeProcessorCore* processor_core,
nit: processor_core should probably be the last param (as non-const methods will
be called on it)

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
sync/engine/non_blocking_type_commit_contribution.h:24: const std::vector<int64>
sequence_numbers);
const ref

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
File sync/engine/non_blocking_type_processor.cc (right):

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor.cc:191:
!data_type_state_.initial_sync_done && data_type_state.initial_sync_done;
is it valid for the new initial_sync_done bit to be false, and the old one to be
true? If not, why not just look at the new one? If so, a comment explaining why
might be useful.

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
File sync/engine/non_blocking_type_processor_core.h (right):

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor_core.h:93: // fetch is complete.
Nit: I find "access to enough state" confusing. How about just stating that
returns true if all the data type state is available, received on initial update
fetch completion.

https://codereview.chromium.org/299963002/diff/90001/sync/engine/sync_thread_...
File sync/engine/sync_thread_sync_entity.cc (right):

https://codereview.chromium.org/299963002/diff/90001/sync/engine/sync_thread_...
sync/engine/sync_thread_sync_entity.cc:131: NOTREACHED() << "Base version should
never decrease";
nit: Style guide tends to prefer not handling invalid scenarios like this.
Either it should NOTREACH, or it should return and error elsewhere.

[rlarocque, 2014-06-02 21:39:13]

Uploaded new patch and responded to some comments.  PTAL.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
File sync/engine/non_blocking_type_commit_contribution.cc (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_commit_contribution.cc:60: LOG(ERROR) << "Server
reports conflict for our commit message.";
On 2014/06/02 20:27:17, Nicolas Zea wrote:
> On 2014/05/29 20:54:52, rlarocque wrote:
> > On 2014/05/28 23:56:15, Nicolas Zea wrote:
> > > Conflicts are valid responses, why treat this as an error?
> > 
> > I suppose you're right.  There are a few data types that could return a
> conflict
> > response.
> > 
> > Changed to warning, since it should be extremely rare.
> 
> Why warning at all though? I could see DVLOG, but if it's a valid scenario, it
> seems like it's just console spam? 
> 
> We'll be tracking conflict counts in about:sync either way right?

Done.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
File sync/engine/non_blocking_type_processor_core.cc (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core.cc:90:
entities_.insert(std::make_pair(client_tag_hash, entity));
On 2014/06/02 20:27:17, Nicolas Zea wrote:
> On 2014/05/29 20:54:52, rlarocque wrote:
> > On 2014/05/28 23:56:15, Nicolas Zea wrote:
> > > When do we remove from the entities map?
> > 
> > Never.  The map grows but never shrinks.  If this becomes a problem, we can
> take
> > steps to reduce the memory consumption.
> > 
> > The most expensive part of these entries, the specifics, are cleared when no
> > commits are pending.  So it's not as expensive as it might seem.
> 
> Probably worth having a comment mentioning that we only actually need to track
> those entities who have updates in flight, and that that's not there for now
for
> simplicity's sake.

Added comment next to entities_'s definition noting that further optimizations
are possible.

I'm reluctant to call it a TODO, since it's pretty far down on my list of
priorities.  I doubt that the extra effort and inter-thread traffic would be
worth the bytes of memory we could save with this, though admittedly that is an
assumption that should be tested before we make any conclusions.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
File sync/engine/non_blocking_type_processor_core_unittest.cc (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:76: static const
std::string kTypeParentId;
On 2014/06/02 20:27:17, Nicolas Zea wrote:
> On 2014/05/29 20:54:52, rlarocque wrote:
> > On 2014/05/28 23:56:16, Nicolas Zea wrote:
> > > Why make this a static part of the test class? Why not just have it be a
> const
> > > variable in the anon namespace above?
> > 
> > Either way works.  I figured that it was tightly coupled to the test, so it
> made
> > sense to define it in the test framework.  Would there be some advantage to
> > moving it out of the class?
> 
> I find it easier (and more common) to have all const values easily found at
the
> top of the file. As it is, you declare it here, and initialize it later,
> requiring more looking around to get the actual value.

OK.  Moved it.

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:127: bool WillCommit();
On 2014/06/02 20:27:17, Nicolas Zea wrote:
> On 2014/05/29 20:54:52, rlarocque wrote:
> > On 2014/05/28 23:56:16, Nicolas Zea wrote:
> > > Given the number of helper methods here, perhaps it would be cleaner to
> break
> > > them into two separate helper classes, those related to the core, and
those
> > > related to the model thread, rather than having them all as part of this
> > massive
> > > test class. Would that be difficult?
> > 
> > I don't think it would be difficult.  We could have separate classes to mock
> out
> > the server and model thread.
> > 
> > To some degree we already do have at least one separate class, the
> > MockNonBlockingTypeProcessor.  It should be possible to move more code into
> it.
> > 
> > The counter-argument is that if we split them up, then the amount of code in
> > this class is likely to increase.  The test framework will likely still have
> the
> > same methods as before, but with stub implementations.  ie. "void
> > NonBlockingTypeProcessorCoreTest::DoSuccessfulCommit() {
> > mock_server_.DoSuccessfulCommit(); }".  The real implementations would live
on
> > elsewhere in this file.
> > 
> > I suppose we could mitigate those effects by moving the helper classes into
> > separate files, or have the test cases interact with them directly.  I don't
> > feel great about either of those alternatives.  
> 
> I think the number of lines is less a concern than just the readability (up to
> you if you want to have the classes in a separate file). Also note that you
> don't need the stub implementations, you could just have an e.g. mock_server()
> getter that exposes the helper classes.

True, I don't need the stubs.  But I've spent enough time refactoring unit tests
with implicit dependencies scattered all over the place that I've developed the
habit of moving behavior into the test framework wherever possible.  That way,
when (for example) the commit function gains another parameter, I'll only have
to modify it in one or two places, rather than in each and every test case. 
Using a mock_server() directly would cause the tests to depend on its arguments,
which would make them more brittle.

On the other hand, I'm starting to come around to the idea that these servers
will need to be split out into their own classes, if not their own files.  When
we start testing the encryption or error-handling functionality, we'll probably
need separate test harnesses.  Refactoring out the mock server will let us share
more code between those harnesses.

Refactoring out the classes will also help deal with some of the ambiguity in
the function parameters.  In most functions, it's no longer clear which ones
accept client tags and which ones accept client tag hashes.  The refactoring
should help isolate the client-tag-based lookups.

So... what to do in this CL?  I'm willing to implement it in this CL, but I
could also do it in a follow-up.  Probably after that gigantic rename follow-up
I've already promised.  How would you prefer to review it?  As part of this CL,
or separately?

https://codereview.chromium.org/299963002/diff/1/sync/engine/sync_thread_sync...
File sync/engine/sync_thread_sync_entity.cc (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/sync_thread_sync...
sync/engine/sync_thread_sync_entity.cc:241: specifics_.Clear();
On 2014/06/02 20:27:17, Nicolas Zea wrote:
> On 2014/05/29 20:54:52, rlarocque wrote:
> > On 2014/05/28 23:56:16, Nicolas Zea wrote:
> > > why clear the specific here?
> > 
> > It's an optimization.  At this point we've given up on the commit, so we
don't
> > need the specifics.  Clearing it might give back a bit of heap memory.
> > 
> > There's no proof that this is a useful optimization, so it is a bit
premature.
> 
> > I can remove it if you're worried about it.
> 
> Does the protobuf library actually free memory when cleared? I'm not sure. I
> suppose it doesn't hurt to do this though. Maybe just comment about the
purpose
> of this.

According to the documentation [1], protobufs can hang on to memory even after
they're cleared.  I would hope that they're smart enough to release the multi-kB
favicon specifics objects, but maybe not.  I suppose it can't hurt to try, and I
see no point in keeping around stale specifics.

I added a comment to explain this Clear() is not really necessary, but an
attempt at optimization.

If someone decides they actually care about this, they could always replace the
protobuf member with a scoped_ptr and call reset() here instead.

[1] https://developers.google.com/protocol-buffers/docs/cpptutorial, see section
"Optimization Tips"

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
File sync/engine/non_blocking_sync_common.cc (right):

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
sync/engine/non_blocking_sync_common.cc:15:
CommitRequestData::CommitRequestData() {
On 2014/06/02 20:27:17, Nicolas Zea wrote:
> I just realized all the ints and bools for these structs should also be
default
> initialized

I wasn't too concerned about it, but I suppose it can't hurt to give them
default values.  Done.

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
File sync/engine/non_blocking_sync_common.h (right):

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
sync/engine/non_blocking_sync_common.h:50: // model thread should not attempt to
commit any items until this flag is
On 2014/06/02 20:27:17, Nicolas Zea wrote:
> nit: prefer referring to NonBlockingTypeProcessor directly, rather than just
> model thread

Done.

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
File sync/engine/non_blocking_type_commit_contribution.cc (right):

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
sync/engine/non_blocking_type_commit_contribution.cc:1: #include
"sync/engine/non_blocking_type_commit_contribution.h"
On 2014/06/02 20:27:17, Nicolas Zea wrote:
> Seems like the copyright message is missing (and in the header). I'm surprised
> the presubmit didn't catch that?

Yeah, that is a bit odd.  Maybe it did report the issue but I ignored it?  I
don't recall seeing any warnings, but I could have missed them.

Added the headers.

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
File sync/engine/non_blocking_type_commit_contribution.h (right):

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
sync/engine/non_blocking_type_commit_contribution.h:21:
NonBlockingTypeProcessorCore* processor_core,
On 2014/06/02 20:27:17, Nicolas Zea wrote:
> nit: processor_core should probably be the last param (as non-const methods
will
> be called on it)

Done.

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
sync/engine/non_blocking_type_commit_contribution.h:24: const std::vector<int64>
sequence_numbers);
On 2014/06/02 20:27:17, Nicolas Zea wrote:
> const ref

Done.

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
File sync/engine/non_blocking_type_processor.cc (right):

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor.cc:191:
!data_type_state_.initial_sync_done && data_type_state.initial_sync_done;
On 2014/06/02 20:27:17, Nicolas Zea wrote:
> is it valid for the new initial_sync_done bit to be false, and the old one to
be
> true? If not, why not just look at the new one? If so, a comment explaining
why
> might be useful.

No, once the flag is set it should remain set.  (Not strictly true when data
types are enabled or disabled, but these objects will be deleted during that
flow so we don't need to worry about it here.)

I think we only want to FlushPendingCommits when the state changes.  If we only
ever look at the new state, then we would end up calling that method on every
update response.

Repeated calls to FlushPendingCommits should be harmless, but it would be  a
small waste of time to call it on every update response.

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
File sync/engine/non_blocking_type_processor_core.h (right):

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor_core.h:93: // fetch is complete.
On 2014/06/02 20:27:17, Nicolas Zea wrote:
> Nit: I find "access to enough state" confusing. How about just stating that
> returns true if all the data type state is available, received on initial
update
> fetch completion.

Rewrote comment.

https://codereview.chromium.org/299963002/diff/90001/sync/engine/sync_thread_...
File sync/engine/sync_thread_sync_entity.cc (right):

https://codereview.chromium.org/299963002/diff/90001/sync/engine/sync_thread_...
sync/engine/sync_thread_sync_entity.cc:131: NOTREACHED() << "Base version should
never decrease";
On 2014/06/02 20:27:17, Nicolas Zea wrote:
> nit: Style guide tends to prefer not handling invalid scenarios like this.
> Either it should NOTREACH, or it should return and error elsewhere.

With the way the code is currently written, this should be impossible.  If the
check fails, it's a logic error.  Therefore, I would argue against gracefully
handling it.

Would it be better to remove the return statement?

[Nicolas Zea, 2014-06-03 21:01:03]

LGTM with the final comments addressed

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
File sync/engine/non_blocking_type_processor.cc (right):

https://codereview.chromium.org/299963002/diff/90001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor.cc:191:
!data_type_state_.initial_sync_done && data_type_state.initial_sync_done;
On 2014/06/02 21:39:14, rlarocque wrote:
> On 2014/06/02 20:27:17, Nicolas Zea wrote:
> > is it valid for the new initial_sync_done bit to be false, and the old one
to
> be
> > true? If not, why not just look at the new one? If so, a comment explaining
> why
> > might be useful.
> 
> No, once the flag is set it should remain set.  (Not strictly true when data
> types are enabled or disabled, but these objects will be deleted during that
> flow so we don't need to worry about it here.)
> 
> I think we only want to FlushPendingCommits when the state changes.  If we
only
> ever look at the new state, then we would end up calling that method on every
> update response.
> 
> Repeated calls to FlushPendingCommits should be harmless, but it would be  a
> small waste of time to call it on every update response.

Ah, this is a bool denoting a change, I didn't realize that first time around.
SG

https://codereview.chromium.org/299963002/diff/90001/sync/engine/sync_thread_...
File sync/engine/sync_thread_sync_entity.cc (right):

https://codereview.chromium.org/299963002/diff/90001/sync/engine/sync_thread_...
sync/engine/sync_thread_sync_entity.cc:131: NOTREACHED() << "Base version should
never decrease";
On 2014/06/02 21:39:14, rlarocque wrote:
> On 2014/06/02 20:27:17, Nicolas Zea wrote:
> > nit: Style guide tends to prefer not handling invalid scenarios like this.
> > Either it should NOTREACH, or it should return and error elsewhere.
> 
> With the way the code is currently written, this should be impossible.  If the
> check fails, it's a logic error.  Therefore, I would argue against gracefully
> handling it.
> 
> Would it be better to remove the return statement?

I'd say just remove the return statement and just NOTREACH

https://codereview.chromium.org/299963002/diff/100001/sync/engine/non_blockin...
File sync/engine/non_blocking_sync_common.cc (right):

https://codereview.chromium.org/299963002/diff/100001/sync/engine/non_blockin...
sync/engine/non_blocking_sync_common.cc:26: response_version() {
0?

[Nicolas Zea, 2014-06-03 21:03:00]

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
File sync/engine/non_blocking_type_processor_core_unittest.cc (right):

https://codereview.chromium.org/299963002/diff/1/sync/engine/non_blocking_typ...
sync/engine/non_blocking_type_processor_core_unittest.cc:127: bool WillCommit();
On 2014/06/02 21:39:13, rlarocque wrote:
> On 2014/06/02 20:27:17, Nicolas Zea wrote:
> > On 2014/05/29 20:54:52, rlarocque wrote:
> > > On 2014/05/28 23:56:16, Nicolas Zea wrote:
> > > > Given the number of helper methods here, perhaps it would be cleaner to
> > break
> > > > them into two separate helper classes, those related to the core, and
> those
> > > > related to the model thread, rather than having them all as part of this
> > > massive
> > > > test class. Would that be difficult?
> > > 
> > > I don't think it would be difficult.  We could have separate classes to
mock
> > out
> > > the server and model thread.
> > > 
> > > To some degree we already do have at least one separate class, the
> > > MockNonBlockingTypeProcessor.  It should be possible to move more code
into
> > it.
> > > 
> > > The counter-argument is that if we split them up, then the amount of code
in
> > > this class is likely to increase.  The test framework will likely still
have
> > the
> > > same methods as before, but with stub implementations.  ie. "void
> > > NonBlockingTypeProcessorCoreTest::DoSuccessfulCommit() {
> > > mock_server_.DoSuccessfulCommit(); }".  The real implementations would
live
> on
> > > elsewhere in this file.
> > > 
> > > I suppose we could mitigate those effects by moving the helper classes
into
> > > separate files, or have the test cases interact with them directly.  I
don't
> > > feel great about either of those alternatives.  
> > 
> > I think the number of lines is less a concern than just the readability (up
to
> > you if you want to have the classes in a separate file). Also note that you
> > don't need the stub implementations, you could just have an e.g.
mock_server()
> > getter that exposes the helper classes.
> 
> True, I don't need the stubs.  But I've spent enough time refactoring unit
tests
> with implicit dependencies scattered all over the place that I've developed
the
> habit of moving behavior into the test framework wherever possible.  That way,
> when (for example) the commit function gains another parameter, I'll only have
> to modify it in one or two places, rather than in each and every test case. 
> Using a mock_server() directly would cause the tests to depend on its
arguments,
> which would make them more brittle.
> 
> On the other hand, I'm starting to come around to the idea that these servers
> will need to be split out into their own classes, if not their own files. 
When
> we start testing the encryption or error-handling functionality, we'll
probably
> need separate test harnesses.  Refactoring out the mock server will let us
share
> more code between those harnesses.
> 
> Refactoring out the classes will also help deal with some of the ambiguity in
> the function parameters.  In most functions, it's no longer clear which ones
> accept client tags and which ones accept client tag hashes.  The refactoring
> should help isolate the client-tag-based lookups.
> 
> So... what to do in this CL?  I'm willing to implement it in this CL, but I
> could also do it in a follow-up.  Probably after that gigantic rename
follow-up
> I've already promised.  How would you prefer to review it?  As part of this
CL,
> or separately?

Follow up CL seems fine for now (yes, this CL is massive already)

[rlarocque, 2014-06-03 21:14:33]

Uploaded patches to address comments and rebase.

https://codereview.chromium.org/299963002/diff/90001/sync/engine/sync_thread_...
File sync/engine/sync_thread_sync_entity.cc (right):

https://codereview.chromium.org/299963002/diff/90001/sync/engine/sync_thread_...
sync/engine/sync_thread_sync_entity.cc:131: NOTREACHED() << "Base version should
never decrease";
On 2014/06/03 21:01:03, Nicolas Zea wrote:
> On 2014/06/02 21:39:14, rlarocque wrote:
> > On 2014/06/02 20:27:17, Nicolas Zea wrote:
> > > nit: Style guide tends to prefer not handling invalid scenarios like this.
> > > Either it should NOTREACH, or it should return and error elsewhere.
> > 
> > With the way the code is currently written, this should be impossible.  If
the
> > check fails, it's a logic error.  Therefore, I would argue against
gracefully
> > handling it.
> > 
> > Would it be better to remove the return statement?
> 
> I'd say just remove the return statement and just NOTREACH

OK.  Changed to DHECKs, since that avoids the return and results in better debug
messages.

https://codereview.chromium.org/299963002/diff/100001/sync/engine/non_blockin...
File sync/engine/non_blocking_sync_common.cc (right):

https://codereview.chromium.org/299963002/diff/100001/sync/engine/non_blockin...
sync/engine/non_blocking_sync_common.cc:26: response_version() {
On 2014/06/03 21:01:03, Nicolas Zea wrote:
> 0?

Done.

I'm slightly annoyed that this compiled without warnings.  I know it's
technically valid, but the odds of doing this intentionally are about as high as
for something like "if (foo = 123)"...

[rlarocque, 2014-06-03 21:14:45]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-06-03 21:15:56]

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

[commit-bot: I haz the power, 2014-06-04 00:14:08]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-04 00:40:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-04 00:40:52]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)

[rlarocque, 2014-06-04 17:43:14]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-06-04 17:44:51]

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

[commit-bot: I haz the power, 2014-06-04 20:46:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[commit-bot: I haz the power, 2014-06-04 20:51:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-04 20:51:30]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[rlarocque, 2014-06-04 22:25:25]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-06-04 22:27:49]

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

[commit-bot: I haz the power, 2014-06-04 22:33:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[commit-bot: I haz the power, 2014-06-04 22:48:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-04 22:48:43]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)

[rlarocque, 2014-06-05 17:44:46]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-06-05 17:46:16]

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

[commit-bot: I haz the power, 2014-06-05 18:05:46]

Change committed as 275187