Use MetadataChangeList and EntityChangeList in SharedModelTypeProcessor

Use MetadataChangeList and EntityChangeList in SharedModelTypeProcessor

BUG=569678, 569636
Committed: https://crrev.com/227d3214f2b02d201d695faa2f8ea2223b6b62bf
Cr-Commit-Position: refs/heads/master@{#369009}

[stanisc, 2016-01-06 23:45:41]

stanisc@chromium.org changed reviewers:
+ gangwu@chromium.org, maxbogue@chromium.org, pavely@chromium.org,
skym@chromium.org

[stanisc, 2016-01-06 23:45:42]

Please do a preliminary review of MetadataChangeList interface and its usage in
SharedModelTypeProcessor.

The work is still in progress (more unit test code is needed), but it would be
good to get a feedback earlier.

[skym, 2016-01-07 00:40:48]

All the changes in the processor look good to me, most of my comments are about
comments and tiny nits.

However, I'm worried about clearing metadata from the processor's cache and from
the storage. It seems to me to be really tricky to catch all instances of it,
and since 'cache' in the processor is authoritative, we can't ever purge things
we are not sure about.

It seems like we're probably going to have some cleanup routine on startup that
clears out obsolete metadata from durable storage, but we might want to have
some periodic job that clears out stale metadata in our cache as well. Or maybe
we catch it during our scan as we prepare a commit batch, I dunno.

https://codereview.chromium.org/1565503003/diff/1/sync/api/entity_change.h
File sync/api/entity_change.h (right):

https://codereview.chromium.org/1565503003/diff/1/sync/api/entity_change.h#ne...
sync/api/entity_change.h:16: class SYNC_EXPORT EntityChange {
Need a comment explaining the purpose of this class.

https://codereview.chromium.org/1565503003/diff/1/sync/api/entity_change.h#ne...
sync/api/entity_change.h:37: std::string client_key_;
I don't quite follow why these need to be mutable based off the other changes in
this CL.

https://codereview.chromium.org/1565503003/diff/1/sync/api/metadata_change_li...
File sync/api/metadata_change_list.h (right):

https://codereview.chromium.org/1565503003/diff/1/sync/api/metadata_change_li...
sync/api/metadata_change_list.h:25: // Requests DataTypeState to be updated in
the storage.
Should we be able to clear the DTS as well?

https://codereview.chromium.org/1565503003/diff/1/sync/api/metadata_change_li...
sync/api/metadata_change_list.h:32: // Please note that update requests for the
same entry are coalesced.
What does 'entry' mean? What does 'coalesced' mean?

Also remember that these comments are being written with the intention that a
service owner will write an implementation for them. They should be phrased as
such.

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
File sync/internal_api/shared_model_type_processor.cc (right):

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:210: // TODO(stanisc):
crbug/573333: Delete optimization.
Reword this sentence. It reads as if you're supposed to delete/remove some
optimization logic, which isn't what you intended.

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:211: // A local only
metadataentry that has never been committed to the worker
I'm a bit worried about this entire approach. Trying to know right here (and in
all the other locations) what things are safe to delete and what things are not
safe seems like a good way to end up with leaks. Both memory leaks at runtime,
and leaks in our persistent storage.

How are we going to even know if something has been sent to the worker?

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:266: // TODO(stanisc):
crbug/573333: Delete case.
Lots of crbugs in here, it looks like the prefered style is either
crbug.com/123456 or https://crbug.com/12345, see
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/whBCkcY8xtA

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:276:
service_->ApplySyncChanges(std::move(change_list), EntityChangeList());
This seems reasonable. Make sure you update model_type_service.h's comment
making this expectation explicit.

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:287: bool
got_new_encryption_requirements = data_type_state_.encryption_key_name !=
Can we move this variable down to where we actually use it? Or maybe just inline
the whole expression?

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:329: // TODO(stanisc):
crbug/561829: Avoid sending an update to the service
This same TODO and crbug should be on line ~181 as well, right?

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:385: // We may have new reasons
to commit by the time this function is done.
We do? Why's that exactly?

[pavely, 2016-01-07 20:51:46]

lgtm with couple comments.
More general though: we'll need unittests for SharedModelTypeProcessor that
trigger some actions and ensure that the right methods of MetadataChangeList are
called with the right parameters.

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
File sync/internal_api/shared_model_type_processor.cc (right):

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:283:
scoped_ptr<MetadataChangeList> change_list =
Variable names are confusing. Both of them have ChangeList in type name. Maybe
rename change_list to metadata_changes to contrast entity_changes.

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:386:
FlushPendingCommitRequests();
If Flush... is needed then it probably should be last thing in the function.

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:389: if
(!entity_changes.empty()) {
Could you DCHECK that if entity_changes are empty then change_list is empty as
well?

[stanisc, 2016-01-07 21:26:10]

On Thu, Jan 7, 2016 at 12:51 PM, <pavely@chromium.org> wrote:

> lgtm with couple comments.
> More general though: we'll need unittests for SharedModelTypeProcessor that
> trigger some actions and ensure that the right methods of
> MetadataChangeList are
> called with the right parameters.
>
>
Yes, that is coming. I am still working on the test side of the fix.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[skym, 2016-01-07 21:31:23]

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
File sync/internal_api/shared_model_type_processor.cc (right):

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:389: if
(!entity_changes.empty()) {
On 2016/01/07 20:51:46, pavely wrote:
> Could you DCHECK that if entity_changes are empty then change_list is empty as
> well?

Is this really true? Once we filter out via specifics hash (crbug.com/561829) I
think should be able to have empty entity changes and non-empty metadata
changes.

[stanisc, 2016-01-11 19:56:48]

I've addressed most of initial CR feedback and included modification to existing
test cases to pass MetadataChangeList to the processor and validate it
afterwards.

I think even more test code is needed in particular to test code paths that
process remote updates and commit confirmations (there is only one test that
currently). If it is OK with you I'd prefer to add more test code in a separate
change.

https://codereview.chromium.org/1565503003/diff/1/sync/api/entity_change.h
File sync/api/entity_change.h (right):

https://codereview.chromium.org/1565503003/diff/1/sync/api/entity_change.h#ne...
sync/api/entity_change.h:37: std::string client_key_;
On 2016/01/07 00:40:48, skym wrote:
> I don't quite follow why these need to be mutable based off the other changes
in
> this CL.

The compiler complains that copy constructor is deleted when these fields are
consts. The copy constructor is used when inserting EntityChange into
EntityChangeList.

https://codereview.chromium.org/1565503003/diff/1/sync/api/metadata_change_li...
File sync/api/metadata_change_list.h (right):

https://codereview.chromium.org/1565503003/diff/1/sync/api/metadata_change_li...
sync/api/metadata_change_list.h:25: // Requests DataTypeState to be updated in
the storage.
On 2016/01/07 00:40:48, skym wrote:
> Should we be able to clear the DTS as well?

I guess clearing DTS is equivalent to replacing it with a default one.

https://codereview.chromium.org/1565503003/diff/1/sync/api/metadata_change_li...
sync/api/metadata_change_list.h:32: // Please note that update requests for the
same entry are coalesced.
On 2016/01/07 00:40:48, skym wrote:
> What does 'entry' mean? What does 'coalesced' mean?
> 
> Also remember that these comments are being written with the intention that a
> service owner will write an implementation for them. They should be phrased as
> such.

What I tried to say that multiple updates with the same key would overwrite each
other. But I guess that ended up being confusing. I'll go ahead and just remove
this sentence.

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
File sync/internal_api/shared_model_type_processor.cc (right):

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:210: // TODO(stanisc):
crbug/573333: Delete optimization.
On 2016/01/07 00:40:48, skym wrote:
> Reword this sentence. It reads as if you're supposed to delete/remove some
> optimization logic, which isn't what you intended.

Done.

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:211: // A local only
metadataentry that has never been committed to the worker
On 2016/01/07 00:40:48, skym wrote:
> I'm a bit worried about this entire approach. Trying to know right here (and
in
> all the other locations) what things are safe to delete and what things are
not
> safe seems like a good way to end up with leaks. Both memory leaks at runtime,
> and leaks in our persistent storage.
> 
> How are we going to even know if something has been sent to the worker?

Good question. I guess an alternative approach is to always send all deletes to
the worker regardless of whether the server has seen an item or not. The worker
then will decide whether this item has a server ID or not. If the item doesn't
have server it could be deleted and commit confirmation generated for it right
away. I guess that should allow to have this kind of logic in commit
confirmation code only. I'll remove this comment.

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:266: // TODO(stanisc):
crbug/573333: Delete case.
On 2016/01/07 00:40:48, skym wrote:
> Lots of crbugs in here, it looks like the prefered style is either
> crbug.com/123456 or https://crbug.com/12345, see
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/whBCkcY8xtA

Done.

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:276:
service_->ApplySyncChanges(std::move(change_list), EntityChangeList());
On 2016/01/07 00:40:48, skym wrote:
> This seems reasonable. Make sure you update model_type_service.h's comment
> making this expectation explicit.

Done.

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:283:
scoped_ptr<MetadataChangeList> change_list =
On 2016/01/07 20:51:46, pavely wrote:
> Variable names are confusing. Both of them have ChangeList in type name. Maybe
> rename change_list to metadata_changes to contrast entity_changes.

OK, renamed to metadata_changes.

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:287: bool
got_new_encryption_requirements = data_type_state_.encryption_key_name !=
On 2016/01/07 00:40:48, skym wrote:
> Can we move this variable down to where we actually use it? Or maybe just
inline
> the whole expression?

This needs to be assigned before data_type_state_ is overwritten.

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:329: // TODO(stanisc):
crbug/561829: Avoid sending an update to the service
On 2016/01/07 00:40:48, skym wrote:
> This same TODO and crbug should be on line ~181 as well, right?

Done.

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:385: // We may have new reasons
to commit by the time this function is done.
On 2016/01/07 00:40:48, skym wrote:
> We do? Why's that exactly?

This comment was here before. This has something to do with the re-encryption
when the key changes.

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:386:
FlushPendingCommitRequests();
On 2016/01/07 20:51:46, pavely wrote:
> If Flush... is needed then it probably should be last thing in the function.

Done.

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:389: if
(!entity_changes.empty()) {
On 2016/01/07 21:31:23, skym wrote:
> On 2016/01/07 20:51:46, pavely wrote:
> > Could you DCHECK that if entity_changes are empty then change_list is empty
as
> > well?
> 
> Is this really true? Once we filter out via specifics hash (crbug.com/561829)
I
> think should be able to have empty entity changes and non-empty metadata
> changes.

I am not sure. I'll remove the condition for now. We can always optimize this
later.

[skym, 2016-01-11 20:45:40]

https://codereview.chromium.org/1565503003/diff/1/sync/api/entity_change.h
File sync/api/entity_change.h (right):

https://codereview.chromium.org/1565503003/diff/1/sync/api/entity_change.h#ne...
sync/api/entity_change.h:37: std::string client_key_;
On 2016/01/11 19:56:47, stanisc wrote:
> On 2016/01/07 00:40:48, skym wrote:
> > I don't quite follow why these need to be mutable based off the other
changes
> in
> > this CL.
> 
> The compiler complains that copy constructor is deleted when these fields are
> consts. The copy constructor is used when inserting EntityChange into
> EntityChangeList.

That makes me so sad. It should be completely possible for a copy constructor to
set all these fields correctly. Maybe we should define it? The assignment
operator on the other hand, won't work here regardless, right? And I think
std::vector::push_back requires the type the be assignable.

If we made this type movable, I think it might actually solve this problem. I
think Chromium style has recently been updated to allow explicit move
constructors http://chromium-cpp.appspot.com/ . Also if this works, you could
throw in DISALLOW_COPY_AND_ASSIGN as well!

There's also the emplace_back option (although not currently allowed, presumably
this is going to change in the near future). However, since we're using a static
method and private constructor, we'd have to write a custom allocator, yuck.

https://codereview.chromium.org/1565503003/diff/1/sync/api/metadata_change_li...
File sync/api/metadata_change_list.h (right):

https://codereview.chromium.org/1565503003/diff/1/sync/api/metadata_change_li...
sync/api/metadata_change_list.h:25: // Requests DataTypeState to be updated in
the storage.
On 2016/01/11 19:56:47, stanisc wrote:
> On 2016/01/07 00:40:48, skym wrote:
> > Should we be able to clear the DTS as well?
> 
> I guess clearing DTS is equivalent to replacing it with a default one.

I guess my problem here is that this interface is inconsistent. It seems wrong
to me that to clear the DTS you set it with default values. And to clear the
entity-metadata you call ClearMetadata.

I lean towards adding an explicit ClearDataTypeState. I would hate to be a data
type owner and check the passed in DTS during UpdateDataTypeState to see if all
values are defaulted and then if so delete some resource.

https://codereview.chromium.org/1565503003/diff/1/sync/api/metadata_change_li...
sync/api/metadata_change_list.h:32: // Please note that update requests for the
same entry are coalesced.
On 2016/01/11 19:56:47, stanisc wrote:
> On 2016/01/07 00:40:48, skym wrote:
> > What does 'entry' mean? What does 'coalesced' mean?
> > 
> > Also remember that these comments are being written with the intention that
a
> > service owner will write an implementation for them. They should be phrased
as
> > such.
> 
> What I tried to say that multiple updates with the same key would overwrite
each
> other. But I guess that ended up being confusing. I'll go ahead and just
remove
> this sentence.

I lean towards being explicit about how the implementer should behave with
repeated keys. It's fine if we don't want to require them to handle this case in
a specific way, but lets say that.

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
File sync/internal_api/shared_model_type_processor.cc (right):

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:211: // A local only
metadataentry that has never been committed to the worker
On 2016/01/11 19:56:47, stanisc wrote:
> On 2016/01/07 00:40:48, skym wrote:
> > I'm a bit worried about this entire approach. Trying to know right here (and
> in
> > all the other locations) what things are safe to delete and what things are
> not
> > safe seems like a good way to end up with leaks. Both memory leaks at
runtime,
> > and leaks in our persistent storage.
> > 
> > How are we going to even know if something has been sent to the worker?
> 
> Good question. I guess an alternative approach is to always send all deletes
to
> the worker regardless of whether the server has seen an item or not. The
worker
> then will decide whether this item has a server ID or not. If the item doesn't
> have server it could be deleted and commit confirmation generated for it right
> away. I guess that should allow to have this kind of logic in commit
> confirmation code only. I'll remove this comment.
> 

Oooh, interesting, I like that idea. Can you make sure we have a bug tracking
this idea?

[stanisc, 2016-01-12 19:32:11]

Added ClearDataTypeState method and responded to Sky's comments. PTAL!

https://codereview.chromium.org/1565503003/diff/1/sync/api/entity_change.h
File sync/api/entity_change.h (right):

https://codereview.chromium.org/1565503003/diff/1/sync/api/entity_change.h#ne...
sync/api/entity_change.h:37: std::string client_key_;
On 2016/01/11 20:45:40, skym wrote:
> On 2016/01/11 19:56:47, stanisc wrote:
> > On 2016/01/07 00:40:48, skym wrote:
> > > I don't quite follow why these need to be mutable based off the other
> changes
> > in
> > > this CL.
> > 
> > The compiler complains that copy constructor is deleted when these fields
are
> > consts. The copy constructor is used when inserting EntityChange into
> > EntityChangeList.
> 
> That makes me so sad. It should be completely possible for a copy constructor
to
> set all these fields correctly. Maybe we should define it? The assignment
> operator on the other hand, won't work here regardless, right? And I think
> std::vector::push_back requires the type the be assignable.
> 
> If we made this type movable, I think it might actually solve this problem. I
> think Chromium style has recently been updated to allow explicit move
> constructors http://chromium-cpp.appspot.com/ . Also if this works, you could
> throw in DISALLOW_COPY_AND_ASSIGN as well!
> 
> There's also the emplace_back option (although not currently allowed,
presumably
> this is going to change in the near future). However, since we're using a
static
> method and private constructor, we'd have to write a custom allocator, yuck.

I think these fields have to be assignable because when an element is inserted
into a vector the vector is first expanded to a larger size, which involves
construction of new elements at the end, then the new element is copied over. So
essentially existing elements are assigned new values. I am not sure if we need
to further complicate that now.

https://codereview.chromium.org/1565503003/diff/1/sync/api/metadata_change_li...
File sync/api/metadata_change_list.h (right):

https://codereview.chromium.org/1565503003/diff/1/sync/api/metadata_change_li...
sync/api/metadata_change_list.h:25: // Requests DataTypeState to be updated in
the storage.
On 2016/01/11 20:45:40, skym wrote:
> On 2016/01/11 19:56:47, stanisc wrote:
> > On 2016/01/07 00:40:48, skym wrote:
> > > Should we be able to clear the DTS as well?
> > 
> > I guess clearing DTS is equivalent to replacing it with a default one.
> 
> I guess my problem here is that this interface is inconsistent. It seems wrong
> to me that to clear the DTS you set it with default values. And to clear the
> entity-metadata you call ClearMetadata.
> 
> I lean towards adding an explicit ClearDataTypeState. I would hate to be a
data
> type owner and check the passed in DTS during UpdateDataTypeState to see if
all
> values are defaulted and then if so delete some resource.

OK, I've added ClearDataTypeState.

https://codereview.chromium.org/1565503003/diff/1/sync/api/metadata_change_li...
sync/api/metadata_change_list.h:32: // Please note that update requests for the
same entry are coalesced.
On 2016/01/11 20:45:40, skym wrote:
> On 2016/01/11 19:56:47, stanisc wrote:
> > On 2016/01/07 00:40:48, skym wrote:
> > > What does 'entry' mean? What does 'coalesced' mean?
> > > 
> > > Also remember that these comments are being written with the intention
that
> a
> > > service owner will write an implementation for them. They should be
phrased
> as
> > > such.
> > 
> > What I tried to say that multiple updates with the same key would overwrite
> each
> > other. But I guess that ended up being confusing. I'll go ahead and just
> remove
> > this sentence.
> 
> I lean towards being explicit about how the implementer should behave with
> repeated keys. It's fine if we don't want to require them to handle this case
in
> a specific way, but lets say that.

Done.

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
File sync/internal_api/shared_model_type_processor.cc (right):

https://codereview.chromium.org/1565503003/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor.cc:211: // A local only
metadataentry that has never been committed to the worker
On 2016/01/11 20:45:40, skym wrote:
> On 2016/01/11 19:56:47, stanisc wrote:
> > On 2016/01/07 00:40:48, skym wrote:
> > > I'm a bit worried about this entire approach. Trying to know right here
(and
> > in
> > > all the other locations) what things are safe to delete and what things
are
> > not
> > > safe seems like a good way to end up with leaks. Both memory leaks at
> runtime,
> > > and leaks in our persistent storage.
> > > 
> > > How are we going to even know if something has been sent to the worker?
> > 
> > Good question. I guess an alternative approach is to always send all deletes
> to
> > the worker regardless of whether the server has seen an item or not. The
> worker
> > then will decide whether this item has a server ID or not. If the item
doesn't
> > have server it could be deleted and commit confirmation generated for it
right
> > away. I guess that should allow to have this kind of logic in commit
> > confirmation code only. I'll remove this comment.
> > 
> 
> Oooh, interesting, I like that idea. Can you make sure we have a bug tracking
> this idea?

This is still tracked by 573333. I've added comments to the bug.

[skym, 2016-01-12 19:38:54]

lgtm

https://codereview.chromium.org/1565503003/diff/1/sync/api/entity_change.h
File sync/api/entity_change.h (right):

https://codereview.chromium.org/1565503003/diff/1/sync/api/entity_change.h#ne...
sync/api/entity_change.h:37: std::string client_key_;
On 2016/01/12 19:32:11, stanisc wrote:
> On 2016/01/11 20:45:40, skym wrote:
> > On 2016/01/11 19:56:47, stanisc wrote:
> > > On 2016/01/07 00:40:48, skym wrote:
> > > > I don't quite follow why these need to be mutable based off the other
> > changes
> > > in
> > > > this CL.
> > > 
> > > The compiler complains that copy constructor is deleted when these fields
> are
> > > consts. The copy constructor is used when inserting EntityChange into
> > > EntityChangeList.
> > 
> > That makes me so sad. It should be completely possible for a copy
constructor
> to
> > set all these fields correctly. Maybe we should define it? The assignment
> > operator on the other hand, won't work here regardless, right? And I think
> > std::vector::push_back requires the type the be assignable.
> > 
> > If we made this type movable, I think it might actually solve this problem.
I
> > think Chromium style has recently been updated to allow explicit move
> > constructors http://chromium-cpp.appspot.com/ . Also if this works, you
could
> > throw in DISALLOW_COPY_AND_ASSIGN as well!
> > 
> > There's also the emplace_back option (although not currently allowed,
> presumably
> > this is going to change in the near future). However, since we're using a
> static
> > method and private constructor, we'd have to write a custom allocator, yuck.
> 
> I think these fields have to be assignable because when an element is inserted
> into a vector the vector is first expanded to a larger size, which involves
> construction of new elements at the end, then the new element is copied over.
So
> essentially existing elements are assigned new values. I am not sure if we
need
> to further complicate that now.

You're right, in order for EntityChange to be assignable, these fields must be
assignable. But one of the points I was trying to make was I think EntityChange
only needs to be either assignable or movable to work with std::vector. You make
a good point though, no need to deal with this now.

[stanisc, 2016-01-12 22:13:55]

The CQ bit was checked by stanisc@chromium.org

[stanisc, 2016-01-12 22:13:55]

The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/1565503003/#ps60001
(title: "Added ClearDataTypeState")

[commit-bot: I haz the power, 2016-01-12 22:14:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1565503003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1565503003/60001

[commit-bot: I haz the power, 2016-01-12 22:20:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-12 22:21:48]

Description was changed from

==========
Use MetadataChangeList and EntityChangeList in SharedModelTypeProcessor

BUG=569678,569636
==========

to

==========
Use MetadataChangeList and EntityChangeList in SharedModelTypeProcessor

BUG=569678,569636

Committed: https://crrev.com/227d3214f2b02d201d695faa2f8ea2223b6b62bf
Cr-Commit-Position: refs/heads/master@{#369009}
==========

[commit-bot: I haz the power, 2016-01-12 22:21:49]

Patchset 4 (id:??) landed as
https://crrev.com/227d3214f2b02d201d695faa2f8ea2223b6b62bf
Cr-Commit-Position: refs/heads/master@{#369009}