Creating protobuf for reading list sync

Creating protobuf for reading list sync

Protobuf contains a single item.

Committed: https://crrev.com/113b244cda221257c2a0a0583a1f63cca76028f8
Cr-Commit-Position: refs/heads/master@{#419440}

[Olivier, 2016-08-12 09:23:55]

olivierrobin@chromium.org changed reviewers:
+ jif@chromium.org, lod@chromium.org, stkhapugin@chromium.org

[Olivier, 2016-08-12 09:23:56]

Do we need more fields?

[jif-google, 2016-08-12 09:46:43]

On 2016/08/12 09:23:56, Olivier Robin wrote:
> Do we need more fields?

1/ Do we really need the "UNKNOWN" ReadingListEntryStatus?
2/ The only other field I can think of that could _possibly_ be needed later is
"device type", but it can always be added later, right?
3/ FMI how are conflicts involving deletions and updates solved?

[lody, 2016-08-12 09:47:55]

Should there be ordering information?

https://codereview.chromium.org/2241583003/diff/1/components/sync/protocol/re...
File components/sync/protocol/reading_list_specifics.proto (right):

https://codereview.chromium.org/2241583003/diff/1/components/sync/protocol/re...
components/sync/protocol/reading_list_specifics.proto:22: required int64
creation_time_us = 4;
What's the "us" stand for?

[stkhapugin, 2016-08-12 09:53:12]

How do you plan tracking seen/unseen?

https://codereview.chromium.org/2241583003/diff/1/components/sync/protocol/re...
File components/sync/protocol/reading_list_specifics.proto (right):

https://codereview.chromium.org/2241583003/diff/1/components/sync/protocol/re...
components/sync/protocol/reading_list_specifics.proto:27: UNKNOWN = 0;
Do we need this? We have no code for such state.

[Olivier, 2016-08-12 09:58:13]

1/ Do we really need the "UNKNOWN" ReadingListEntryStatus?
Removed

2/ The only other field I can think of that could _possibly_ be needed later is
"device type", but it can always be added later, right?
I think we can add fields. Removing is harder.

3/ FMI how are conflicts involving deletions and updates solved?
Not here. When you receive data, you must merge.


Should there be ordering information?
Ordering is done by update_timestamp. This allows to update a single item. 

How do you plan tracking seen/unseen?
I can add an UNSEEN status.
Can an item be UNSEEN/READ (seems incompatible to me).

https://codereview.chromium.org/2241583003/diff/1/components/sync/protocol/re...
File components/sync/protocol/reading_list_specifics.proto (right):

https://codereview.chromium.org/2241583003/diff/1/components/sync/protocol/re...
components/sync/protocol/reading_list_specifics.proto:22: required int64
creation_time_us = 4;
On 2016/08/12 09:47:55, lody wrote:
> What's the "us" stand for?

micro seconds.

https://codereview.chromium.org/2241583003/diff/1/components/sync/protocol/re...
components/sync/protocol/reading_list_specifics.proto:27: UNKNOWN = 0;
On 2016/08/12 09:53:12, stkhapugin wrote:
> Do we need this? We have no code for such state. 

Removed

[lody, 2016-08-12 11:19:23]

lgtm

[stkhapugin, 2016-08-12 11:25:39]

https://codereview.chromium.org/2241583003/diff/1/components/sync/protocol/re...
File components/sync/protocol/reading_list_specifics.proto (right):

https://codereview.chromium.org/2241583003/diff/1/components/sync/protocol/re...
components/sync/protocol/reading_list_specifics.proto:22: required int64
creation_time_us = 4;
On 2016/08/12 09:58:13, Olivier Robin wrote:
> On 2016/08/12 09:47:55, lody wrote:
> > What's the "us" stand for?
> 
> micro seconds.

Is this a universal naming? I've never seen it before.

[Olivier, 2016-08-12 11:42:22]

https://codereview.chromium.org/2241583003/diff/1/components/sync/protocol/re...
File components/sync/protocol/reading_list_specifics.proto (right):

https://codereview.chromium.org/2241583003/diff/1/components/sync/protocol/re...
components/sync/protocol/reading_list_specifics.proto:22: required int64
creation_time_us = 4;
On 2016/08/12 11:25:39, stkhapugin wrote:
> On 2016/08/12 09:58:13, Olivier Robin wrote:
> > On 2016/08/12 09:47:55, lody wrote:
> > > What's the "us" stand for?
> > 
> > micro seconds.
> 
> Is this a universal naming? I've never seen it before. 

Universal, I guess not. Very common yes, especially in context where ascii is
needed.

https://www.google.com/?gfe_rd=cr&ei=7LWtV6yIJJPu8weAoJvoAw&gws_rd=cr&fg=1#q=...

[Olivier, 2016-08-12 15:30:43]

olivierrobin@chromium.org changed reviewers:
+ zea@chromium.org

[Olivier, 2016-08-17 14:19:09]

ping zea.
Could you take a look and tell me if something is missing?

[Nicolas Zea, 2016-08-17 19:12:09]

Mostly LG, but could you give us more info about how the reading list type would
work? (see email thread)

https://codereview.chromium.org/2241583003/diff/20001/components/sync/protoco...
File components/sync/protocol/reading_list_specifics.proto (right):

https://codereview.chromium.org/2241583003/diff/20001/components/sync/protoco...
components/sync/protocol/reading_list_specifics.proto:14: // Properties of
Reading list items..
nit: remove extra newline

[Nicolas Zea, 2016-08-17 19:12:32]

https://codereview.chromium.org/2241583003/diff/20001/components/sync/protoco...
File components/sync/protocol/reading_list_specifics.proto (right):

https://codereview.chromium.org/2241583003/diff/20001/components/sync/protoco...
components/sync/protocol/reading_list_specifics.proto:14: // Properties of
Reading list items..
On 2016/08/17 19:12:09, Nicolas Zea wrote:
> nit: remove extra newline

By newline, I mean period :)

[jif, 2016-08-18 11:32:27]

lgtm

[Olivier, 2016-08-18 14:59:52]

https://codereview.chromium.org/2241583003/diff/20001/components/sync/protoco...
File components/sync/protocol/reading_list_specifics.proto (right):

https://codereview.chromium.org/2241583003/diff/20001/components/sync/protoco...
components/sync/protocol/reading_list_specifics.proto:14: // Properties of
Reading list items..
On 2016/08/17 19:12:09, Nicolas Zea wrote:
> nit: remove extra newline

Done.

[Nicolas Zea, 2016-08-22 18:23:51]

lgtm

[Nicolas Zea, 2016-08-22 18:24:13]

oops, forgot one thing: please update proto_value_conversions.cc with the new
reading list info

[Patrick Noland, 2016-08-29 17:01:04]

pnoland@chromium.org changed reviewers:
+ pnoland@chromium.org

[Patrick Noland, 2016-08-29 17:01:05]

https://codereview.chromium.org/2241583003/diff/40001/components/sync/syncabl...
File components/sync/syncable/model_type.cc (right):

https://codereview.chromium.org/2241583003/diff/40001/components/sync/syncabl...
components/sync/syncable/model_type.cc:142: {READING_LIST, "READING_LIST",
"reading_list", "Reading List",
Apologies for the driveby, but this struct literal should be in the same
position in the array as the READING_LIST entry is in the ModelType enum.

[Olivier, 2016-09-06 14:06:07]

The CQ bit was checked by olivierrobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 14:06:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Olivier, 2016-09-06 14:06:59]

Hi zea,

I think I fixed all the feedback.
I ended up making more changes. Can you take another quick look?
Thanks

https://codereview.chromium.org/2241583003/diff/40001/components/sync/syncabl...
File components/sync/syncable/model_type.cc (right):

https://codereview.chromium.org/2241583003/diff/40001/components/sync/syncabl...
components/sync/syncable/model_type.cc:142: {READING_LIST, "READING_LIST",
"reading_list", "Reading List",
On 2016/08/29 17:01:05, Patrick Noland wrote:
> Apologies for the driveby, but this struct literal should be in the same
> position in the array as the READING_LIST entry is in the ModelType enum.

Done. Thanks

[Olivier, 2016-09-06 14:07:21]

olivierrobin@chromium.org changed reviewers:
+ rkaplow@chromium.org

[Olivier, 2016-09-06 14:07:22]

+rkaplow as histograms.xml owner

[commit-bot: I haz the power, 2016-09-06 15:13:14]

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

[commit-bot: I haz the power, 2016-09-06 15:13:15]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Olivier, 2016-09-06 15:40:06]

The CQ bit was checked by olivierrobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 15:40:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 16:22:43]

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

[commit-bot: I haz the power, 2016-09-06 16:22:44]

Dry run: Exceeded global retry quota

[Olivier, 2016-09-06 16:56:23]

The CQ bit was checked by olivierrobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 16:56:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Olivier, 2016-09-06 17:11:39]

components unittests are stull failing with 
[libprotobuf FATAL
../../third_party/protobuf/src/google/protobuf/message_lite.cc:286] CHECK
failed: IsInitialized(): Can't serialize message of type
"sync_pb.EntitySpecifics" because it is missing required fields: (cannot
determine missing fields for lite message)

I don't know what I am still missing.
zea, do you know if I missed something?

[Olivier, 2016-09-06 17:16:33]

On 2016/09/06 17:11:39, Olivier Robin wrote:
> components unittests are stull failing with 
> [libprotobuf FATAL
> ../../third_party/protobuf/src/google/protobuf/message_lite.cc:286] CHECK
> failed: IsInitialized(): Can't serialize message of type
> "sync_pb.EntitySpecifics" because it is missing required fields: (cannot
> determine missing fields for lite message)
> 
> I don't know what I am still missing.
> zea, do you know if I missed something?

Message is 
Server did not create the top-level Autofill node. We might be running against
an out-of-date server.
Do I need to change something server side?

[Nicolas Zea, 2016-09-06 17:27:42]

You don't need to update the real server, but you might need to update the
python server (used in integration tests): chromiumsync.py

[Olivier, 2016-09-07 08:53:35]

The CQ bit was checked by olivierrobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 08:53:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Olivier, 2016-09-07 09:53:35]

The problem was that there were required fields in the protobuf.
Apparently no other protobuf has required field, so I removed mine (it also
seems to be current policy to have only optional or repeated fields).

[commit-bot: I haz the power, 2016-09-07 10:06:34]

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

[commit-bot: I haz the power, 2016-09-07 10:06:34]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[rkaplow, 2016-09-07 13:28:30]

lgtm

[Olivier, 2016-09-07 13:54:08]

The CQ bit was checked by olivierrobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 13:54:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 15:03:11]

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

[commit-bot: I haz the power, 2016-09-07 15:03:12]

Dry run: This issue passed the CQ dry run.

[Olivier, 2016-09-19 07:51:50]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2016-09-19 07:51:51]

The patchset sent to the CQ was uploaded after l-g-t-m from lod@chromium.org,
jif@chromium.org, zea@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2241583003/#ps160001
(title: "Fix last bots")

[commit-bot: I haz the power, 2016-09-19 07:51:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 07:54:26]

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

[commit-bot: I haz the power, 2016-09-19 07:54:27]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Olivier, 2016-09-19 10:58:37]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2016-09-19 10:58:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, jif@chromium.org, lod@chromium.org, zea@chromium.org
Link to the patchset: https://codereview.chromium.org/2241583003/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-19 10:58:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 12:18:18]

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

[commit-bot: I haz the power, 2016-09-19 12:18:19]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Olivier, 2016-09-19 12:19:53]

The CQ bit was checked by olivierrobin@chromium.org

[commit-bot: I haz the power, 2016-09-19 12:20:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 12:52:30]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-19 12:54:48]

Description was changed from

==========
Creating protobuf for reading list sync

Protobuf contains a single item.
==========

to

==========
Creating protobuf for reading list sync

Protobuf contains a single item.

Committed: https://crrev.com/113b244cda221257c2a0a0583a1f63cca76028f8
Cr-Commit-Position: refs/heads/master@{#419440}
==========

[commit-bot: I haz the power, 2016-09-19 12:54:49]

Patchset 10 (id:??) landed as
https://crrev.com/113b244cda221257c2a0a0583a1f63cca76028f8
Cr-Commit-Position: refs/heads/master@{#419440}