Add an autoconf field to printer objects.

Add an autoconf field to printer objects.

Printers can be configured without PPDs.  This will be represented by
the field 'autoconf.'  This adds the field to the printer object and the
PrinterSpecifics proto.

BUG=685676
Review-Url: https://codereview.chromium.org/2884863002
Cr-Commit-Position: refs/heads/master@{#474004}
Committed: https://chromium.googlesource.com/chromium/src/+/01de7ac6d3abe6fdd0f5c78c6fa9c5fa34b5e9ee

[skau, 2017-05-16 00:40:48]

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

[skau, 2017-05-16 00:40:49]

pavely@: Adding a field to the printer_specifics proto.
justincarlson@: Everything

[pavely, 2017-05-16 00:50:29]

lgtm % one comment.

https://codereview.chromium.org/2884863002/diff/1/components/sync/protocol/pr...
File components/sync/protocol/printer_specifics.proto (right):

https://codereview.chromium.org/2884863002/diff/1/components/sync/protocol/pr...
components/sync/protocol/printer_specifics.proto:30: optional bool autoconf = 5
[default = false];
Please also update VISIT_PROTO_FILEDS(PrinterPPDReference) in
proto_visitors.h:650.

[Carlson, 2017-05-16 16:15:27]

lgtm

https://codereview.chromium.org/2884863002/diff/1/chrome/browser/chromeos/pri...
File chrome/browser/chromeos/printing/specifics_translation.cc (right):

https://codereview.chromium.org/2884863002/diff/1/chrome/browser/chromeos/pri...
chrome/browser/chromeos/printing/specifics_translation.cc:32: if
(specifics.has_autoconf()) {
This conditional isn't useful, I think.  If you like it for orthogonality, ok
(though I'd argue that none of the conditionals in this function are useful)

https://codereview.chromium.org/2884863002/diff/1/chrome/browser/chromeos/pri...
chrome/browser/chromeos/printing/specifics_translation.cc:49:
specifics->set_autoconf(ref.autoconf);
Conversely, this probably *should* be wrapped in a conditional.  :)

https://codereview.chromium.org/2884863002/diff/1/chromeos/printing/printer_c...
File chromeos/printing/printer_configuration.h (right):

https://codereview.chromium.org/2884863002/diff/1/chromeos/printing/printer_c...
chromeos/printing/printer_configuration.h:40: // True if the printer should be
auto-configured.
Maybe add " and so doesn't need to have a PPD supplied" to the comment?

[skau, 2017-05-16 18:48:07]

Thanks for the review.  I made autoconf optional so we can merge it
intelligently.

https://codereview.chromium.org/2884863002/diff/1/chrome/browser/chromeos/pri...
File chrome/browser/chromeos/printing/specifics_translation.cc (right):

https://codereview.chromium.org/2884863002/diff/1/chrome/browser/chromeos/pri...
chrome/browser/chromeos/printing/specifics_translation.cc:32: if
(specifics.has_autoconf()) {
On 2017/05/16 16:15:27, Carlson wrote:
> This conditional isn't useful, I think.  If you like it for orthogonality, ok
> (though I'd argue that none of the conditionals in this function are useful)

Acknowledged.

https://codereview.chromium.org/2884863002/diff/1/chrome/browser/chromeos/pri...
chrome/browser/chromeos/printing/specifics_translation.cc:49:
specifics->set_autoconf(ref.autoconf);
On 2017/05/16 16:15:27, Carlson wrote:
> Conversely, this probably *should* be wrapped in a conditional.  :)

I've made autoconf optional in PpdRefernce so that'll have some meaning.

https://codereview.chromium.org/2884863002/diff/1/chromeos/printing/printer_c...
File chromeos/printing/printer_configuration.h (right):

https://codereview.chromium.org/2884863002/diff/1/chromeos/printing/printer_c...
chromeos/printing/printer_configuration.h:40: // True if the printer should be
auto-configured.
On 2017/05/16 16:15:27, Carlson wrote:
> Maybe add " and so doesn't need to have a PPD supplied" to the comment?

Done.

https://codereview.chromium.org/2884863002/diff/1/components/sync/protocol/pr...
File components/sync/protocol/printer_specifics.proto (right):

https://codereview.chromium.org/2884863002/diff/1/components/sync/protocol/pr...
components/sync/protocol/printer_specifics.proto:30: optional bool autoconf = 5
[default = false];
On 2017/05/16 00:50:28, pavely wrote:
> Please also update VISIT_PROTO_FILEDS(PrinterPPDReference) in
> proto_visitors.h:650.

Done.

[skau, 2017-05-16 18:48:10]

The CQ bit was checked by skau@chromium.org

[skau, 2017-05-16 18:48:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
justincarlson@chromium.org, pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2884863002/#ps20001
(title: "positive test")

[commit-bot: I haz the power, 2017-05-16 18:49:25]

CQ is trying da patch.

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

[skau, 2017-05-16 19:23:17]

The CQ bit was unchecked by skau@chromium.org

[skau, 2017-05-22 23:36:16]

justincarlson@: PTAL the fields in PpdReference are exclusive now.

[Carlson, 2017-05-22 23:46:07]

https://codereview.chromium.org/2884863002/diff/1/chrome/browser/chromeos/pri...
File chrome/browser/chromeos/printing/specifics_translation.cc (right):

https://codereview.chromium.org/2884863002/diff/1/chrome/browser/chromeos/pri...
chrome/browser/chromeos/printing/specifics_translation.cc:49:
specifics->set_autoconf(ref.autoconf);
On 2017/05/16 18:48:06, skau wrote:
> On 2017/05/16 16:15:27, Carlson wrote:
> > Conversely, this probably *should* be wrapped in a conditional.  :)
> 
> I've made autoconf optional in PpdRefernce so that'll have some meaning.

I don't understand what you mean by this.  Do you mean you're using the
existence of the field to have implicit semantics?

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

https://codereview.chromium.org/2884863002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/specifics_translation.cc:24: if
(specifics.has_autoconf() && specifics.autoconf()) {
The has_autoconf() here is not needed.

https://codereview.chromium.org/2884863002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/specifics_translation.cc:36: // |ref|.  If
|ref| is the default object, nothing will be changed in
Not sure what "default object" means here -- you mean it has no non-default
fields?

That seems like a slightly weird thing to guarantee, semantically.   But if it's
important, this comment should really be in the header.  (Also, the implicit
hierarchy that now exists should be documented in the header)

https://codereview.chromium.org/2884863002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/specifics_translation.cc:41:
specifics->set_autoconf(ref.autoconf);
You could write each of these as 
if (...) {
specifics->Clear();
specifics->set_some_field(some_value);
}

which might be slightly more future proof if more fields are added later?

https://codereview.chromium.org/2884863002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/specifics_translation_unittest.cc (right):

https://codereview.chromium.org/2884863002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/specifics_translation_unittest.cc:147: 
Comment for what this is testing for?  (Also, below.

[skau, 2017-05-23 01:00:22]

Added additional comments to the specifics_translation header.

https://codereview.chromium.org/2884863002/diff/1/chrome/browser/chromeos/pri...
File chrome/browser/chromeos/printing/specifics_translation.cc (right):

https://codereview.chromium.org/2884863002/diff/1/chrome/browser/chromeos/pri...
chrome/browser/chromeos/printing/specifics_translation.cc:49:
specifics->set_autoconf(ref.autoconf);
On 2017/05/22 23:46:07, Carlson wrote:
> On 2017/05/16 18:48:06, skau wrote:
> > On 2017/05/16 16:15:27, Carlson wrote:
> > > Conversely, this probably *should* be wrapped in a conditional.  :)
> > 
> > I've made autoconf optional in PpdRefernce so that'll have some meaning.
> 
> I don't understand what you mean by this.  Do you mean you're using the
> existence of the field to have implicit semantics?

Acknowledged.

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

https://codereview.chromium.org/2884863002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/specifics_translation.cc:24: if
(specifics.has_autoconf() && specifics.autoconf()) {
On 2017/05/22 23:46:07, Carlson wrote:
> The has_autoconf() here is not needed.

Done.

https://codereview.chromium.org/2884863002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/specifics_translation.cc:41:
specifics->set_autoconf(ref.autoconf);
On 2017/05/22 23:46:07, Carlson wrote:
> You could write each of these as 
> if (...) {
> specifics->Clear();
> specifics->set_some_field(some_value);
> }
> 
> which might be slightly more future proof if more fields are added later?

Done.

https://codereview.chromium.org/2884863002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/specifics_translation_unittest.cc (right):

https://codereview.chromium.org/2884863002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/specifics_translation_unittest.cc:147: 
On 2017/05/22 23:46:07, Carlson wrote:
> Comment for what this is testing for?  (Also, below.

Done.

[Carlson, 2017-05-23 17:03:00]

lgtm

https://codereview.chromium.org/2884863002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/specifics_translation_unittest.cc (right):

https://codereview.chromium.org/2884863002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/specifics_translation_unittest.cc:139: // Tests
that the autoconf value overwrites other PpdReference fields.
overrides is probably a little better word.

[skau, 2017-05-23 17:50:46]

Thanks.

https://codereview.chromium.org/2884863002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/specifics_translation_unittest.cc (right):

https://codereview.chromium.org/2884863002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/specifics_translation_unittest.cc:139: // Tests
that the autoconf value overwrites other PpdReference fields.
On 2017/05/23 17:03:00, Carlson wrote:
> overrides is probably a little better word.

Done.

[skau, 2017-05-23 17:50:50]

The CQ bit was checked by skau@chromium.org

[skau, 2017-05-23 17:50:51]

The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org,
justincarlson@chromium.org
Link to the patchset: https://codereview.chromium.org/2884863002/#ps80001
(title: "grammar")

[commit-bot: I haz the power, 2017-05-23 17:51:32]

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-05-23 18:57:01]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495561850651360,
"parent_rev": "8e36d829de1d5c51af637544b72a648af41dc288", "commit_rev":
"01de7ac6d3abe6fdd0f5c78c6fa9c5fa34b5e9ee"}

[commit-bot: I haz the power, 2017-05-23 18:57:27]

Description was changed from

==========
Add an autoconf field to printer objects.

Printers can be configured without PPDs.  This will be represented by
the field 'autoconf.'  This adds the field to the printer object and the
PrinterSpecifics proto.

BUG=685676
==========

to

==========
Add an autoconf field to printer objects.

Printers can be configured without PPDs.  This will be represented by
the field 'autoconf.'  This adds the field to the printer object and the
PrinterSpecifics proto.

BUG=685676

Review-Url: https://codereview.chromium.org/2884863002
Cr-Commit-Position: refs/heads/master@{#474004}
Committed:
https://chromium.googlesource.com/chromium/src/+/01de7ac6d3abe6fdd0f5c78c6fa9...
==========

[commit-bot: I haz the power, 2017-05-23 18:57:34]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/01de7ac6d3abe6fdd0f5c78c6fa9...