[Sync] USS: Add EntityChange.

[Sync] USS: Add EntityChange.

BUG=536895, 543405
Committed: https://crrev.com/f09d94201b089399747476e4bab4b81a176ba675
Cr-Commit-Position: refs/heads/master@{#361153}

[maxbogue, 2015-11-20 17:05:05]

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

[maxbogue, 2015-11-20 17:05:05]

USS folks, PTAL!

[stanisc, 2015-11-20 19:01:49]

lgtm with a couple of comments.

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

https://codereview.chromium.org/1458013003/diff/40001/sync/api/entity_change....
sync/api/entity_change.h:23: static EntityChange CreateAdd(std::string
client_tag, EntityDataPtr data);
I am not sure we need these 3 functions as a part of the API. They would make
sense if EntityChange was used both ways but with the current design services
aren't going to ever construct EntityChange.

I am still open to changing the design. The advantage of using this type of
construction on the service side is that it would allow to pass the data without
copying it. 
Let's keep these functions here but file a tracking bug to finalize the API for
submitting local changes once we start making progress with DeviceInfo service
implementation.

https://codereview.chromium.org/1458013003/diff/40001/sync/api/entity_change....
sync/api/entity_change.h:31: EntityDataPtr data() const { return data_; }
This should be const EntityData&

[maxbogue, 2015-11-20 19:53:25]

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

https://codereview.chromium.org/1458013003/diff/40001/sync/api/entity_change....
sync/api/entity_change.h:23: static EntityChange CreateAdd(std::string
client_tag, EntityDataPtr data);
On 2015/11/20 19:01:49, stanisc wrote:
> I am not sure we need these 3 functions as a part of the API. They would make
> sense if EntityChange was used both ways but with the current design services
> aren't going to ever construct EntityChange.
> 
> I am still open to changing the design. The advantage of using this type of
> construction on the service side is that it would allow to pass the data
without
> copying it. 
> Let's keep these functions here but file a tracking bug to finalize the API
for
> submitting local changes once we start making progress with DeviceInfo service
> implementation.

Filed http://crbug.com/559276.

https://codereview.chromium.org/1458013003/diff/40001/sync/api/entity_change....
sync/api/entity_change.h:31: EntityDataPtr data() const { return data_; }
On 2015/11/20 19:01:49, stanisc wrote:
> This should be const EntityData& 

Done.

[pavely, 2015-11-21 00:59:48]

lgtm % one change.

https://codereview.chromium.org/1458013003/diff/80001/sync/BUILD.gn
File sync/BUILD.gn (right):

https://codereview.chromium.org/1458013003/diff/80001/sync/BUILD.gn#newcode29
sync/BUILD.gn:29: "api/entity_change.cc",
There should be corresponding gyp file change.

[maxbogue, 2015-11-23 16:30:00]

I've prefixed the ChangeType enums with ACTION_ because apparently windows is
ridiculous enough to have a DELETE macro, which means I can't have an enum value
called that. Thanks to Stan for helping me figure that out!

[maxbogue, 2015-11-23 16:30:05]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2015-11-23 16:30:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
stanisc@chromium.org, pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/1458013003/#ps100001
(title: "Prefix with ACTION_ and add gyp.")

[commit-bot: I haz the power, 2015-11-23 16:30:34]

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

[maxbogue, 2015-11-23 16:52:25]

The CQ bit was unchecked by maxbogue@chromium.org

[skym, 2015-11-23 16:57:16]

lgtm

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

https://codereview.chromium.org/1458013003/diff/100001/sync/api/entity_change...
sync/api/entity_change.h:29: std::string client_tag() const { return
client_tag_; }
My understanding was that we were trying to use client_key for this field going
forward.

https://codereview.chromium.org/1458013003/diff/100001/sync/api/entity_change...
sync/api/entity_change.h:41: typedef std::vector<EntityChange> EntityChangeList;
Should you have #include <vector> for this?

https://codereview.chromium.org/1458013003/diff/100001/sync/api/model_type_se...
File sync/api/model_type_service.h (right):

https://codereview.chromium.org/1458013003/diff/100001/sync/api/model_type_se...
sync/api/model_type_service.h:53: // TODO(skym): The change type should be in
here somehow.
This TODO can be removed.

[maxbogue, 2015-11-23 17:00:46]

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

https://codereview.chromium.org/1458013003/diff/100001/sync/api/entity_change...
sync/api/entity_change.h:29: std::string client_tag() const { return
client_tag_; }
On 2015/11/23 16:57:16, skym wrote:
> My understanding was that we were trying to use client_key for this field
going
> forward.

Done.

https://codereview.chromium.org/1458013003/diff/100001/sync/api/entity_change...
sync/api/entity_change.h:41: typedef std::vector<EntityChange> EntityChangeList;
On 2015/11/23 16:57:16, skym wrote:
> Should you have #include <vector> for this?

Done.

https://codereview.chromium.org/1458013003/diff/100001/sync/api/model_type_se...
File sync/api/model_type_service.h (right):

https://codereview.chromium.org/1458013003/diff/100001/sync/api/model_type_se...
sync/api/model_type_service.h:53: // TODO(skym): The change type should be in
here somehow.
On 2015/11/23 16:57:16, skym wrote:
> This TODO can be removed.

Done.

[maxbogue, 2015-11-23 17:00:55]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2015-11-23 17:00:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
stanisc@chromium.org, skym@chromium.org, pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/1458013003/#ps120001
(title: "s/client_tag/client_key/ etc")

[commit-bot: I haz the power, 2015-11-23 17:02:09]

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

[commit-bot: I haz the power, 2015-11-23 18:09:23]

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

[commit-bot: I haz the power, 2015-11-23 18:09:23]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[maxbogue, 2015-11-23 18:17:10]

The CQ bit was checked by maxbogue@chromium.org

[commit-bot: I haz the power, 2015-11-23 18:17:28]

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

[commit-bot: I haz the power, 2015-11-23 19:27:06]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-23 19:28:00]

Patchset 7 (id:??) landed as
https://crrev.com/f09d94201b089399747476e4bab4b81a176ba675
Cr-Commit-Position: refs/heads/master@{#361153}