Introduce the field make_and_model for synced printers.

Introduce the field make_and_model for synced printers.

Parsing manufacturer and model consistently is difficult.  We don't need
the separate fields so store it as a single string.  This CL deprecates
the old fields in the proto and ensures that the new field is populated.
Future CLs will migrate old PrinterSpecifics objects and remove usages of
manufacturer() and model() on the Printer object.

BUG=730242
Review-Url: https://codereview.chromium.org/2956173002
Cr-Commit-Position: refs/heads/master@{#483209}
Committed: https://chromium.googlesource.com/chromium/src/+/6609d04f50475d06e8c34f2d1f293d07c2a92059

[skau, 2017-06-28 01:36:24]

skau@chromium.org changed reviewers:
+ justincarlson@chromium.org, skym@chromium.org

[skau, 2017-06-28 01:36:24]

Please review.  I'm going to start the migration with a dark launch.

skym@: //components/sync/*
justincarlson@: Everything

[skau, 2017-06-28 01:36:41]

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

[commit-bot: I haz the power, 2017-06-28 01:36:57]

Dry run: CQ is trying da patch.

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

[skau, 2017-06-28 01:37:22]

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

[commit-bot: I haz the power, 2017-06-28 01:37:54]

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, 2017-06-28 02:59:18]

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

[commit-bot: I haz the power, 2017-06-28 02:59:19]

Dry run: This issue passed the CQ dry run.

[Carlson, 2017-06-28 17:38:36]

lgtm

https://codereview.chromium.org/2956173002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/specifics_translation.cc (right):

https://codereview.chromium.org/2956173002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/specifics_translation.cc:54: std::string
MakeAndModel(base::StringPiece make, base::StringPiece model) {
Function comment.  Also, I'm not 100% sure this will do the right thing in all
places.  Do we have any printers that are, for instance, make: "Hewlett Packard"
but model "HP LaserJet 5 bajillion"?
 
If we do, it may be a small enough subset that we don't care.

https://codereview.chromium.org/2956173002/diff/20001/chromeos/printing/print...
File chromeos/printing/printer_configuration.h (right):

https://codereview.chromium.org/2956173002/diff/20001/chromeos/printing/print...
chromeos/printing/printer_configuration.h:138: std::string manufacturer_;
Do we want to mark these as deprecated?

https://codereview.chromium.org/2956173002/diff/20001/chromeos/printing/print...
File chromeos/printing/printer_translator.cc (right):

https://codereview.chromium.org/2956173002/diff/20001/chromeos/printing/print...
chromeos/printing/printer_translator.cc:71: make_and_model.append(model);
Nit:  I think it's a little clearer if this line is pulled into the if.

[skau, 2017-06-28 21:25:35]

PTAL

https://codereview.chromium.org/2956173002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/specifics_translation.cc (right):

https://codereview.chromium.org/2956173002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/specifics_translation.cc:54: std::string
MakeAndModel(base::StringPiece make, base::StringPiece model) {
On 2017/06/28 17:38:36, Carlson wrote:
> Function comment.  Also, I'm not 100% sure this will do the right thing in all
> places.  Do we have any printers that are, for instance, make: "Hewlett
Packard"
> but model "HP LaserJet 5 bajillion"?
>  
> If we do, it may be a small enough subset that we don't care.

I think it's a small enough subset.  Mostly, I'm trying to account for existing
data that has the effective_make_and_model string saved in the model field.

https://codereview.chromium.org/2956173002/diff/20001/chromeos/printing/print...
File chromeos/printing/printer_configuration.h (right):

https://codereview.chromium.org/2956173002/diff/20001/chromeos/printing/print...
chromeos/printing/printer_configuration.h:138: std::string manufacturer_;
On 2017/06/28 17:38:36, Carlson wrote:
> Do we want to mark these as deprecated?

Done.

https://codereview.chromium.org/2956173002/diff/20001/chromeos/printing/print...
File chromeos/printing/printer_translator.cc (right):

https://codereview.chromium.org/2956173002/diff/20001/chromeos/printing/print...
chromeos/printing/printer_translator.cc:71: make_and_model.append(model);
On 2017/06/28 17:38:36, Carlson wrote:
> Nit:  I think it's a little clearer if this line is pulled into the if.

But that wouldn't be equivalent.  If model is not empty and make_and_model is
empty, then make_and_model should be equal to model.  If I put it in the if,
then make_and_model would be empty in this case.

[skym, 2017-06-28 22:14:33]

sync lgtm

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

https://codereview.chromium.org/2956173002/diff/40001/components/sync/protoco...
components/sync/protocol/printer_specifics.proto:70: // for this is '<Make>
<Model>'.  This aligns with the typical formatting of
Not that it really matters, but you're not being consistent with capitalization
of <Make> or <make>, specifics_translation.cc uses lowercase.

[Carlson, 2017-06-28 22:22:04]

lgtm

https://codereview.chromium.org/2956173002/diff/20001/chromeos/printing/print...
File chromeos/printing/printer_translator.cc (right):

https://codereview.chromium.org/2956173002/diff/20001/chromeos/printing/print...
chromeos/printing/printer_translator.cc:71: make_and_model.append(model);
On 2017/06/28 21:25:35, skau wrote:
> On 2017/06/28 17:38:36, Carlson wrote:
> > Nit:  I think it's a little clearer if this line is pulled into the if.
> 
> But that wouldn't be equivalent.  If model is not empty and make_and_model is
> empty, then make_and_model should be equal to model.  If I put it in the if,
> then make_and_model would be empty in this case.

Indeed, I fail at logic today.  Alas!

Can I suggest that the if at 69 be slightly changed to

if (!manufacturer.empty() && !model.empty())

as I think the clearer parallelism might have made me fail less at basic logic
comprehension.  :)

[skau, 2017-06-28 22:41:06]

Thanks for the reviews.

https://codereview.chromium.org/2956173002/diff/20001/chromeos/printing/print...
File chromeos/printing/printer_translator.cc (right):

https://codereview.chromium.org/2956173002/diff/20001/chromeos/printing/print...
chromeos/printing/printer_translator.cc:71: make_and_model.append(model);
On 2017/06/28 22:22:04, Carlson wrote:
> On 2017/06/28 21:25:35, skau wrote:
> > On 2017/06/28 17:38:36, Carlson wrote:
> > > Nit:  I think it's a little clearer if this line is pulled into the if.
> > 
> > But that wouldn't be equivalent.  If model is not empty and make_and_model
is
> > empty, then make_and_model should be equal to model.  If I put it in the if,
> > then make_and_model would be empty in this case.
> 
> Indeed, I fail at logic today.  Alas!
> 
> Can I suggest that the if at 69 be slightly changed to
> 
> if (!manufacturer.empty() && !model.empty())
> 
> as I think the clearer parallelism might have made me fail less at basic logic
> comprehension.  :)

Done.

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

https://codereview.chromium.org/2956173002/diff/40001/components/sync/protoco...
components/sync/protocol/printer_specifics.proto:70: // for this is '<Make>
<Model>'.  This aligns with the typical formatting of
On 2017/06/28 22:14:33, skym wrote:
> Not that it really matters, but you're not being consistent with
capitalization
> of <Make> or <make>, specifics_translation.cc uses lowercase.

Done.

[skau, 2017-06-28 22:41:09]

The CQ bit was checked by skau@chromium.org

[skau, 2017-06-28 22:41:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
justincarlson@chromium.org, skym@chromium.org
Link to the patchset: https://codereview.chromium.org/2956173002/#ps60001
(title: "done")

[commit-bot: I haz the power, 2017-06-28 22:41:50]

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, 2017-06-29 00:09:43]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498689669823380,
"parent_rev": "5dfbd3fa246a046c79a1d015f8c99040aa416624", "commit_rev":
"6609d04f50475d06e8c34f2d1f293d07c2a92059"}

[commit-bot: I haz the power, 2017-06-29 00:09:56]

Description was changed from

==========
Introduce the field make_and_model for synced printers.

Parsing manufacturer and model consistently is difficult.  We don't need
the separate fields so store it as a single string.  This CL deprecates
the old fields in the proto and ensures that the new field is populated.
Future CLs will migrate old PrinterSpecifics objects and remove usages of
manufacturer() and model() on the Printer object.

BUG=730242
==========

to

==========
Introduce the field make_and_model for synced printers.

Parsing manufacturer and model consistently is difficult.  We don't need
the separate fields so store it as a single string.  This CL deprecates
the old fields in the proto and ensures that the new field is populated.
Future CLs will migrate old PrinterSpecifics objects and remove usages of
manufacturer() and model() on the Printer object.

BUG=730242

Review-Url: https://codereview.chromium.org/2956173002
Cr-Commit-Position: refs/heads/master@{#483209}
Committed:
https://chromium.googlesource.com/chromium/src/+/6609d04f50475d06e8c34f2d1f29...
==========

[commit-bot: I haz the power, 2017-06-29 00:09:57]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/6609d04f50475d06e8c34f2d1f29...