Add PrinterSpecifics to the sync protos.

Add PrinterSpecifics to the sync protos.

PrinterSpecifics will sync Printer device information for Chrome OS.
This just adds the proto.  Actual syncing will follow.

BUG=616862
TEST=Unit tests pass.

Committed: https://crrev.com/a66410d63369081eb6d14466d0e8b5fb1126da07
Cr-Commit-Position: refs/heads/master@{#418456}

[skau, 2016-09-01 22:46:48]

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

[commit-bot: I haz the power, 2016-09-01 22:47:22]

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-01 23:45:53]

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

[commit-bot: I haz the power, 2016-09-01 23:45:53]

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

[skau, 2016-09-02 19:42:41]

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

[skau, 2016-09-02 19:42:41]

PTAL.  I think this is a complete implementation to add a printer sync datatype.
 Even though I'm syncing a list, it doesn't seem like PrinterSpecifics should
have a repeated field.  Is that correct?

[skau, 2016-09-02 21:50:40]

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

[commit-bot: I haz the power, 2016-09-02 21:51:20]

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-02 22:02:50]

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

[commit-bot: I haz the power, 2016-09-02 22:02:51]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nicolas Zea, 2016-09-02 23:55:20]

Looks like this isn't compiling. I think you might need to update protocol.gypi
to include the new specifics.

[skau, 2016-09-06 20:08:56]

On 2016/09/02 23:55:20, Nicolas Zea wrote:
> Looks like this isn't compiling. I think you might need to update
protocol.gypi
> to include the new specifics.

It looks like somebody is cleaning up gn and protocol_sources.gni lists the
protobuffs now.  The tests pass locally so I'll see if it works after a rebase.

[skau, 2016-09-06 20:09:02]

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

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

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 21:18:44]

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

[commit-bot: I haz the power, 2016-09-06 21:18:45]

Dry run: This issue passed the CQ dry run.

[Nicolas Zea, 2016-09-07 00:18:05]

https://codereview.chromium.org/2306763002/diff/60001/components/sync/base/mo...
File components/sync/base/model_type.h (right):

https://codereview.chromium.org/2306763002/diff/60001/components/sync/base/mo...
components/sync/base/model_type.h:136: PRINTER,
nit: the enum is typically plural, so this should be PRINTERS

https://codereview.chromium.org/2306763002/diff/60001/components/sync/protoco...
File components/sync/protocol/printer_specifics.proto (right):

https://codereview.chromium.org/2306763002/diff/60001/components/sync/protoco...
components/sync/protocol/printer_specifics.proto:18: optional int32 id = 1
[default = 0];
Please add descriptions for each of these fields you're introducing. In
particular what format they're in, and what they're used for.

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

https://codereview.chromium.org/2306763002/diff/60001/components/sync/syncabl...
components/sync/syncable/model_type.cc:136: {PRINTER, "PRINTER", "printer",
"Printer",
Given the other change to make the enum be PRINTERS, the second two strings here
should be changed to "printers" and "Printers" respectively.

[skau, 2016-09-09 19:02:15]

Comments addressed.  PTAL.

https://codereview.chromium.org/2306763002/diff/60001/components/sync/base/mo...
File components/sync/base/model_type.h (right):

https://codereview.chromium.org/2306763002/diff/60001/components/sync/base/mo...
components/sync/base/model_type.h:136: PRINTER,
On 2016/09/07 00:18:04, Nicolas Zea wrote:
> nit: the enum is typically plural, so this should be PRINTERS

Done.

https://codereview.chromium.org/2306763002/diff/60001/components/sync/protoco...
File components/sync/protocol/printer_specifics.proto (right):

https://codereview.chromium.org/2306763002/diff/60001/components/sync/protoco...
components/sync/protocol/printer_specifics.proto:18: optional int32 id = 1
[default = 0];
On 2016/09/07 00:18:04, Nicolas Zea wrote:
> Please add descriptions for each of these fields you're introducing. In
> particular what format they're in, and what they're used for.

Done.

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

https://codereview.chromium.org/2306763002/diff/60001/components/sync/syncabl...
components/sync/syncable/model_type.cc:136: {PRINTER, "PRINTER", "printer",
"Printer",
On 2016/09/07 00:18:04, Nicolas Zea wrote:
> Given the other change to make the enum be PRINTERS, the second two strings
here
> should be changed to "printers" and "Printers" respectively.

Done.

[Nicolas Zea, 2016-09-13 00:14:06]

https://codereview.chromium.org/2306763002/diff/80001/components/sync/protoco...
File components/sync/protocol/sync.proto (right):

https://codereview.chromium.org/2306763002/diff/80001/components/sync/protoco...
components/sync/protocol/sync.proto:140: optional PrintersSpecifics printers =
410745;
I should have been more clear before. This field name should remain printer.
Similarly, the specifics (and the file name) should be PrinterSpecifics.

The enum is all that I meant you should change to PRINTERS previously, as it
describes the set of all entities of that type. The individual entities still
represent a single "printer".

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

https://codereview.chromium.org/2306763002/diff/80001/components/sync/syncabl...
components/sync/syncable/model_type.cc:136: {PRINTERS, "PRINTERS", "printers",
"Printers",
This should be:
{PRINTERS, "PRINTER", "printers", "Printers",

[skau, 2016-09-13 21:02:44]

Thanks for pointing out the issue in model_type.cc.  It should be fixed now.

https://codereview.chromium.org/2306763002/diff/80001/components/sync/protoco...
File components/sync/protocol/sync.proto (right):

https://codereview.chromium.org/2306763002/diff/80001/components/sync/protoco...
components/sync/protocol/sync.proto:140: optional PrintersSpecifics printers =
410745;
On 2016/09/13 00:14:06, Nicolas Zea wrote:
> I should have been more clear before. This field name should remain printer.
> Similarly, the specifics (and the file name) should be PrinterSpecifics.
> 
> The enum is all that I meant you should change to PRINTERS previously, as it
> describes the set of all entities of that type. The individual entities still
> represent a single "printer".

Okay.  Do you know where this presubmit warning comes from?

Message:
Error at lines 135-136 in model_type.cc: notification type "PRINTERS" does not
match proto message name defined in sync.proto: "printer"

I didn't realize that it was flagging the UMA label and not the enum name.

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

https://codereview.chromium.org/2306763002/diff/80001/components/sync/syncabl...
components/sync/syncable/model_type.cc:136: {PRINTERS, "PRINTERS", "printers",
"Printers",
On 2016/09/13 00:14:06, Nicolas Zea wrote:
> This should be:
> {PRINTERS, "PRINTER", "printers", "Printers",

Done.

[Nicolas Zea, 2016-09-13 21:11:57]

On 2016/09/13 21:02:44, skau wrote:
> Thanks for pointing out the issue in model_type.cc.  It should be fixed now.
> 
>
https://codereview.chromium.org/2306763002/diff/80001/components/sync/protoco...
> File components/sync/protocol/sync.proto (right):
> 
>
https://codereview.chromium.org/2306763002/diff/80001/components/sync/protoco...
> components/sync/protocol/sync.proto:140: optional PrintersSpecifics printers =
> 410745;
> On 2016/09/13 00:14:06, Nicolas Zea wrote:
> > I should have been more clear before. This field name should remain printer.
> > Similarly, the specifics (and the file name) should be PrinterSpecifics.
> > 
> > The enum is all that I meant you should change to PRINTERS previously, as it
> > describes the set of all entities of that type. The individual entities
still
> > represent a single "printer".
> 
> Okay.  Do you know where this presubmit warning comes from?
> 
> Message:
> Error at lines 135-136 in model_type.cc: notification type "PRINTERS" does not
> match proto message name defined in sync.proto: "printer"
> 
> I didn't realize that it was flagging the UMA label and not the enum name.
> 

That comes from
https://cs.chromium.org/chromium/src/components/sync/PRESUBMIT.py. Specifically,
the lowercase version of the first string in model_type.cc ("PRINTER") needs to
match the field name in sync.proto ("printer").

LGTM

[skau, 2016-09-13 22:34:24]

The CQ bit was checked by skau@chromium.org

[commit-bot: I haz the power, 2016-09-13 22:34:55]

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-14 00:36:02]

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

[commit-bot: I haz the power, 2016-09-14 00:36:03]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[skau, 2016-09-14 01:35:42]

The CQ bit was checked by skau@chromium.org

[commit-bot: I haz the power, 2016-09-14 01:36:31]

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-14 02:12:26]

Committed patchset #6 (id:100001)

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

Description was changed from

==========
Add PrinterSpecifics to the sync protos.

PrinterSpecifics will sync Printer device information for Chrome OS.
This just adds the proto.  Actual syncing will follow.

BUG=616862
TEST=Unit tests pass.
==========

to

==========
Add PrinterSpecifics to the sync protos.

PrinterSpecifics will sync Printer device information for Chrome OS.
This just adds the proto.  Actual syncing will follow.

BUG=616862
TEST=Unit tests pass.

Committed: https://crrev.com/a66410d63369081eb6d14466d0e8b5fb1126da07
Cr-Commit-Position: refs/heads/master@{#418456}
==========

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

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