Query printers for autoconf info during setup.

Query printers for autoconf info during setup.

Use an IPP query to establish if we can perform automatic
configuaration of the printer by setting the autoconf
field. This is determined by support for IPP 2.0+ and one
of the two PDLs PDF or PWG-Raster.

BUG=685676
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2915703002
Cr-Commit-Position: refs/heads/master@{#479243}
Committed: https://chromium.googlesource.com/chromium/src/+/8d580f5a9aecf7a5682382b9a2eb57df0eb2cd98

[skau, 2017-05-31 00:08:04]

Description was changed from

==========
Query printers for autoconf info during setup.

Use an IPP query to establish if we can perform automatic
configuaration of the printer by setting the autoconf
field. This is determined by support for IPP 2.0+ and one
of the two PDLs PDF or PWG-Raster.

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

to

==========
Query printers for autoconf info during setup.

Use an IPP query to establish if we can perform automatic
configuaration of the printer by setting the autoconf
field. This is determined by support for IPP 2.0+ and one
of the two PDLs PDF or PWG-Raster.

BUG=685676
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[skau, 2017-05-31 01:12:32]

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

[skau, 2017-05-31 01:12:32]

FYI: This integrates autoconf into the setup flow.

[Carlson, 2017-05-31 16:58:53]

Mostly I think this looks ok, some nits.

At a high level, I'd advise paying more attention to leaving signposts for
future readers of the code; it may be clear to *you* what you're doing right
now, but it's definitely not straightforward for readers new to the code.

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js
(right):

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:52:
autoconf: false,
Dunno why the printerXXX naming convention is used here, but is there a reason
to break it?

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:183:
return;
nit:  
if () {
  x
} else {
  y
}

seems slightly more elegant to me than

if () {
  x;
  return;
}
y;
 

you do the latter in a few places

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:186:
// Open configuring dialog.
Generally, better to comment why you're doing stuff like this than just comment
what's happening.

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:195:
infoFailed_: function(rejected) {
Why is rejected not used?

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right):

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:218: // Run
the failure callback.
I'm confused about when you do and don't run the failure callback.  For the
NOTREACHED conditions above, if you reach them, what's the effect?  We'll hang
forever waiting for the promise?

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:233:
printer_address + "/" + printer_queue;
I don't understand why you assemble a uri and then parse it back out again.  Is
it so you can pull out the port?  Could use some commenting

Also, do you need to be paranoid about badly-formed printer_address input making
things like parsed.path.len -1?

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h (right):

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h:52: void
HandleGetPrinterInfo(const base::ListValue* args);
Function comment

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h:104: void
OnPrinterInfo(const std::string& callback_id,
Function comment please.  When is this called, what is it expected to do?

[skau, 2017-06-01 21:50:28]

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

[skau, 2017-06-01 21:50:29]

Sorry about the lack of comments.  I've been staring at this code for too long. 
Hopefully it's grokable now.

xdai@: Please take a look.  Let me know if you think getPrinterInfo should be
moved in cups_add_printer_dialog.js.

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js
(right):

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:52:
autoconf: false,
On 2017/05/31 16:58:52, Carlson wrote:
> Dunno why the printerXXX naming convention is used here, but is there a reason
> to break it?

No reason either way AFAICT.  I've changed it.

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:183:
return;
On 2017/05/31 16:58:52, Carlson wrote:
> nit:  
> if () {
>   x
> } else {
>   y
> }
> 
> seems slightly more elegant to me than
> 
> if () {
>   x;
>   return;
> }
> y;
>  
> 
> you do the latter in a few places

Done.

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:186:
// Open configuring dialog.
On 2017/05/31 16:58:52, Carlson wrote:
> Generally, better to comment why you're doing stuff like this than just
comment
> what's happening.

Done.

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:195:
infoFailed_: function(rejected) {
On 2017/05/31 16:58:52, Carlson wrote:
> Why is rejected not used?

There's nothing useful in it at the moment.  But the contract for Promise
indicates that there should be an object passed to onRejection.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right):

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:218: // Run
the failure callback.
On 2017/05/31 16:58:52, Carlson wrote:
> I'm confused about when you do and don't run the failure callback.  For the
> NOTREACHED conditions above, if you reach them, what's the effect?  We'll hang
> forever waiting for the promise?

We run the failure callback for expected failures.  The NOTREACHED() sections
indicate places where there's been a violation of expected calling conditions.

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:233:
printer_address + "/" + printer_queue;
On 2017/05/31 16:58:52, Carlson wrote:
> I don't understand why you assemble a uri and then parse it back out again. 
Is
> it so you can pull out the port?  Could use some commenting
> 
> Also, do you need to be paranoid about badly-formed printer_address input
making
> things like parsed.path.len -1?

It is just for the port so the whole uri doesn't need to be constructed then
deconstructed.  Lengths won't be negative for any of the Components as
ParseStandardURL will attempt to extract all components.  len could be 0, but it
won't be -1.

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h (right):

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h:52: void
HandleGetPrinterInfo(const base::ListValue* args);
On 2017/05/31 16:58:52, Carlson wrote:
> Function comment

Done.

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h:104: void
OnPrinterInfo(const std::string& callback_id,
On 2017/05/31 16:58:52, Carlson wrote:
> Function comment please.  When is this called, what is it expected to do?

Done.

[xdai1, 2017-06-05 17:55:55]

lgtm with nit

https://codereview.chromium.org/2915703002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js
(right):

https://codereview.chromium.org/2915703002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:203:
if (this.newPrinter.printerProtocol == 'ipp' ||
Do you need to move line 216 - 218 to here?

[skau, 2017-06-07 23:55:39]

Sorry for the delay.  I've been able to revisit this.  I've moved the query to
the configuring screen but I'm working through a bug where the dialog can't be
dismissed.

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right):

https://codereview.chromium.org/2915703002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:233:
printer_address + "/" + printer_queue;
On 2017/06/01 21:50:29, skau wrote:
> On 2017/05/31 16:58:52, Carlson wrote:
> > I don't understand why you assemble a uri and then parse it back out again. 
> Is
> > it so you can pull out the port?  Could use some commenting
> > 
> > Also, do you need to be paranoid about badly-formed printer_address input
> making
> > things like parsed.path.len -1?
> 
> It is just for the port so the whole uri doesn't need to be constructed then
> deconstructed.  Lengths won't be negative for any of the Components as
> ParseStandardURL will attempt to extract all components.  len could be 0, but
it
> won't be -1.

So, I was wrong.  If there's no scheme (aka protocol) the parsing falls over.

https://codereview.chromium.org/2915703002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js
(right):

https://codereview.chromium.org/2915703002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:203:
if (this.newPrinter.printerProtocol == 'ipp' ||
On 2017/06/05 17:55:55, xdai1 wrote:
> Do you need to move line 216 - 218 to here?

I've moved this query to the configuring screen instead so we get the same
behavior now.

[skau, 2017-06-08 05:00:51]

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

[skau, 2017-06-08 05:00:51]

stevenjb@: settings OWNER

PTAL.  The dependent CL with QueryIppPrinter should be committed tomorrow.

[stevenjb, 2017-06-08 15:58:28]

https://codereview.chromium.org/2915703002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js
(right):

https://codereview.chromium.org/2915703002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:178:
// Strip a leading backslashes.  It is expected that this results in an
s/a leading backslashes/the leading backslash/

Also, is this being done for UI presentation, or for providing input to the
model (C++ code)? If the latter, we should really have the model just handle it.

https://codereview.chromium.org/2915703002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:481:
getPrinterInfo(this.newPrinter).
Does getPrinterInfo ever return an error (e.g. if it can't find the printer)? If
so, we need to check for that and handle it in onPrinterFound_() (or in a
wrapper function).

https://codereview.chromium.org/2915703002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:486:
this.fire('open-manufacturer-model-dialog');
Looks like we do these two things in several places, maybe add a helper method.

https://codereview.chromium.org/2915703002/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right):

https://codereview.chromium.org/2915703002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:265: port =
631;
These constants should be defined at the top of the file in a local namespace,
e.g. kIppSchemePort (or better yet, use already defined constants if available).

https://codereview.chromium.org/2915703002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:270:
NOTREACHED() << "Unrecognized protocol.  Port was not set.";
nit: No typewriter spacing in strings either :)

https://codereview.chromium.org/2915703002/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h (right):

https://codereview.chromium.org/2915703002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h:56: // Handles
the callback for HandleGetPrinterInfo.  |callback_id| is the
nit: We generally avoid using two spaces between sentences in comments.

[skau, 2017-06-08 22:11:26]

Thanks for the review.  PTAL.

https://codereview.chromium.org/2915703002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js
(right):

https://codereview.chromium.org/2915703002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:178:
// Strip a leading backslashes.  It is expected that this results in an
On 2017/06/08 15:58:28, stevenjb wrote:
> s/a leading backslashes/the leading backslash/
> 
> Also, is this being done for UI presentation, or for providing input to the
> model (C++ code)? If the latter, we should really have the model just handle
it.

It's not for UI presentation.  It's been moved into the model.

https://codereview.chromium.org/2915703002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:481:
getPrinterInfo(this.newPrinter).
On 2017/06/08 15:58:27, stevenjb wrote:
> Does getPrinterInfo ever return an error (e.g. if it can't find the printer)?
If
> so, we need to check for that and handle it in onPrinterFound_() (or in a
> wrapper function).

No.  If there's an error in getPrinterInfo, we reject the promise and infoFailed
gets called.

https://codereview.chromium.org/2915703002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:486:
this.fire('open-manufacturer-model-dialog');
On 2017/06/08 15:58:27, stevenjb wrote:
> Looks like we do these two things in several places, maybe add a helper
method.

Done.

https://codereview.chromium.org/2915703002/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right):

https://codereview.chromium.org/2915703002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:265: port =
631;
On 2017/06/08 15:58:28, stevenjb wrote:
> These constants should be defined at the top of the file in a local namespace,
> e.g. kIppSchemePort (or better yet, use already defined constants if
available).

Done.

https://codereview.chromium.org/2915703002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:270:
NOTREACHED() << "Unrecognized protocol.  Port was not set.";
On 2017/06/08 15:58:28, stevenjb wrote:
> nit: No typewriter spacing in strings either :)

Done.

https://codereview.chromium.org/2915703002/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h (right):

https://codereview.chromium.org/2915703002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h:56: // Handles
the callback for HandleGetPrinterInfo.  |callback_id| is the
On 2017/06/08 15:58:28, stevenjb wrote:
> nit: We generally avoid using two spaces between sentences in comments.

Done.

[stevenjb, 2017-06-09 21:00:06]

lgtm

[skau, 2017-06-12 18:23:28]

The CQ bit was checked by skau@chromium.org

[skau, 2017-06-12 18:23:28]

The patchset sent to the CQ was uploaded after l-g-t-m from xdai@chromium.org
Link to the patchset: https://codereview.chromium.org/2915703002/#ps200001
(title: "address comments from stevenjb@")

[commit-bot: I haz the power, 2017-06-12 18:23:59]

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-12 18:42:43]

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

[commit-bot: I haz the power, 2017-06-12 18:42:44]

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

[skau, 2017-06-12 21:05:53]

The CQ bit was checked by skau@chromium.org

[skau, 2017-06-12 21:05:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, xdai@chromium.org
Link to the patchset: https://codereview.chromium.org/2915703002/#ps220001
(title: "boolean")

[commit-bot: I haz the power, 2017-06-12 21:06:23]

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-12 21:29:06]

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

[commit-bot: I haz the power, 2017-06-12 21:29:07]

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

[skau, 2017-06-12 23:54:49]

The CQ bit was checked by skau@chromium.org

[skau, 2017-06-12 23:54:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, xdai@chromium.org
Link to the patchset: https://codereview.chromium.org/2915703002/#ps240001
(title: "star matches")

[commit-bot: I haz the power, 2017-06-12 23:55:18]

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-13 01:27:50]

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

[commit-bot: I haz the power, 2017-06-13 01:27:51]

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_...)

[skau, 2017-06-13 23:49:45]

The CQ bit was checked by skau@chromium.org

[skau, 2017-06-13 23:49:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, xdai@chromium.org
Link to the patchset: https://codereview.chromium.org/2915703002/#ps260001
(title: "fix test")

[commit-bot: I haz the power, 2017-06-13 23:50:11]

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-14 01:32:00]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1497397785205060,
"parent_rev": "2cd6f2dd305dee2674898b6c423ec3035330cc11", "commit_rev":
"8d580f5a9aecf7a5682382b9a2eb57df0eb2cd98"}

[commit-bot: I haz the power, 2017-06-14 01:32:19]

Description was changed from

==========
Query printers for autoconf info during setup.

Use an IPP query to establish if we can perform automatic
configuaration of the printer by setting the autoconf
field. This is determined by support for IPP 2.0+ and one
of the two PDLs PDF or PWG-Raster.

BUG=685676
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Query printers for autoconf info during setup.

Use an IPP query to establish if we can perform automatic
configuaration of the printer by setting the autoconf
field. This is determined by support for IPP 2.0+ and one
of the two PDLs PDF or PWG-Raster.

BUG=685676
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2915703002
Cr-Commit-Position: refs/heads/master@{#479243}
Committed:
https://chromium.googlesource.com/chromium/src/+/8d580f5a9aecf7a5682382b9a2eb...
==========

[commit-bot: I haz the power, 2017-06-14 01:32:23]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/8d580f5a9aecf7a5682382b9a2eb...