[CUPS] Implement the Webui handler for the printers auto discovery.

[CUPS] Implement the Webui handler for the printers auto discovery.

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

Committed: https://crrev.com/1ca5bbe347115d2ce3c3a6a26525d4bc4ca54223
Cr-Commit-Position: refs/heads/master@{#423265}

[xdai1, 2016-09-28 22:57:05]

Description was changed from

==========
[CUPS] Implement the Webui handler for the printers auto discovery.

BUG=631352

patch from issue 2344393002 at patchset 40001
(http://crrev.com/2344393002#ps40001)
==========

to

==========
[CUPS] Implement the Webui handler for the printers auto discovery.

BUG=631352

patch from issue 2344393002 at patchset 40001
(http://crrev.com/2344393002#ps40001)
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[xdai1, 2016-09-28 23:28:45]

Patchset #1 (id:1) has been deleted

[xdai1, 2016-09-28 23:28:53]

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

[commit-bot: I haz the power, 2016-09-28 23:29:08]

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-29 00:32:34]

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

[commit-bot: I haz the power, 2016-09-29 00:32:35]

Dry run: This issue passed the CQ dry run.

[xdai1, 2016-10-04 21:21:47]

Description was changed from

==========
[CUPS] Implement the Webui handler for the printers auto discovery.

BUG=631352

patch from issue 2344393002 at patchset 40001
(http://crrev.com/2344393002#ps40001)
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[CUPS] Implement the Webui handler for the printers auto discovery.

BUG=631352

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

[xdai1, 2016-10-04 21:26:50]

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

[xdai1, 2016-10-04 21:26:50]

michaelpg@, could you help review this CL please? Thanks! Please see the demo in
https://drive.google.com/corp/drive/u/0/folders/0B4R9nvLdzmC9d3o2WDNyNGRjejQ
about what it looks like.

[michaelpg, 2016-10-04 22:31:10]

overall lg, just some comments

https://codereview.chromium.org/2380753004/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html
(right):

https://codereview.chromium.org/2380753004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:37:
margin-left: 20px;
use -webkit-margin-start (-end) to work with RTL

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

https://codereview.chromium.org/2380753004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:31: *
is not showing.
@const {number}

https://codereview.chromium.org/2380753004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:33:
var printerListFullHeight = 350;
kPrinterListFullHeight or PRINTER_LIST_FULL_HEIGHT

https://codereview.chromium.org/2380753004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:60:
this.discovering_ = true;
Why not set the initial value to true, above?

https://codereview.chromium.org/2380753004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:80:
this.discoveredPrinters = this.discoveredPrinters.concat(printers);
instead of replacing the whole array (which might cause unnecessary work in
Polymer re-creating mostly the same DOM nodes), use this.push():

this.push.bind(this, 'discoveredPrinters').apply(printers);

see https://www.polymer-project.org/1.0/docs/api/Polymer.Base#method-push

https://codereview.chromium.org/2380753004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:88:
this.$.noPrinterMessage.hidden = false;
this.$.nPM.hidden = !!this.discoveredPrinters;

[xdai1, 2016-10-04 23:36:19]

michaelpg@, I've addressed your comments. Please take another look, thanks for
your review!

https://codereview.chromium.org/2380753004/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html
(right):

https://codereview.chromium.org/2380753004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:37:
margin-left: 20px;
On 2016/10/04 22:31:10, michaelpg wrote:
> use -webkit-margin-start (-end) to work with RTL

Done.

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

https://codereview.chromium.org/2380753004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:31: *
is not showing.
On 2016/10/04 22:31:10, michaelpg wrote:
> @const {number}

Done.

https://codereview.chromium.org/2380753004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:33:
var printerListFullHeight = 350;
On 2016/10/04 22:31:10, michaelpg wrote:
> kPrinterListFullHeight or PRINTER_LIST_FULL_HEIGHT

Done.

https://codereview.chromium.org/2380753004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:60:
this.discovering_ = true;
On 2016/10/04 22:31:10, michaelpg wrote:
> Why not set the initial value to true, above?

Done.

https://codereview.chromium.org/2380753004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:80:
this.discoveredPrinters = this.discoveredPrinters.concat(printers);
On 2016/10/04 22:31:10, michaelpg wrote:
> instead of replacing the whole array (which might cause unnecessary work in
> Polymer re-creating mostly the same DOM nodes), use this.push():
> 
> this.push.bind(this, 'discoveredPrinters').apply(printers);
> 
> see https://www.polymer-project.org/1.0/docs/api/Polymer.Base#method-push
> 

Seems like it could not push back a CupsPrinterInfo array directly to another
array. Thus push one by one.

https://codereview.chromium.org/2380753004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:88:
this.$.noPrinterMessage.hidden = false;
On 2016/10/04 22:31:10, michaelpg wrote:
> this.$.nPM.hidden = !!this.discoveredPrinters;

Done.

[xdai1, 2016-10-04 23:36:31]

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

[commit-bot: I haz the power, 2016-10-04 23:36:55]

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-10-05 00:37:09]

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

[commit-bot: I haz the power, 2016-10-05 00:37:11]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2016-10-05 19:40:16]

lgtm

[xdai1, 2016-10-05 19:53:10]

On 2016/10/05 19:40:16, michaelpg wrote:
> lgtm

Thanks for the review!

[xdai1, 2016-10-05 19:53:14]

The CQ bit was checked by xdai@chromium.org

[commit-bot: I haz the power, 2016-10-05 19:53:36]

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-10-05 20:00:37]

Description was changed from

==========
[CUPS] Implement the Webui handler for the printers auto discovery.

BUG=631352

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

to

==========
[CUPS] Implement the Webui handler for the printers auto discovery.

BUG=631352

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

[commit-bot: I haz the power, 2016-10-05 20:00:39]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-10-05 20:03:46]

Description was changed from

==========
[CUPS] Implement the Webui handler for the printers auto discovery.

BUG=631352

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

to

==========
[CUPS] Implement the Webui handler for the printers auto discovery.

BUG=631352

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/1ca5bbe347115d2ce3c3a6a26525d4bc4ca54223
Cr-Commit-Position: refs/heads/master@{#423265}
==========

[commit-bot: I haz the power, 2016-10-05 20:03:48]

Patchset 3 (id:??) landed as
https://crrev.com/1ca5bbe347115d2ce3c3a6a26525d4bc4ca54223
Cr-Commit-Position: refs/heads/master@{#423265}