[CUPS] Implement the UI handler for adding a new printer.

[CUPS] Implement the UI handler for adding a new printer.

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

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

[xdai1, 2016-09-14 04:03:02]

Patchset #2 (id:20001) has been deleted

[xdai1, 2016-09-14 04:03:23]

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

[commit-bot: I haz the power, 2016-09-14 04:03:48]

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-14 04:21:20]

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

[commit-bot: I haz the power, 2016-09-14 04:21:21]

Dry run: 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...)

[xdai1, 2016-09-14 05:34:09]

Patchset #3 (id:60001) has been deleted

[xdai1, 2016-09-16 21:47:38]

Patchset #4 (id:100001) has been deleted

[xdai1, 2016-09-16 23:38:18]

Patchset #4 (id:120001) has been deleted

[xdai1, 2016-09-16 23:39:49]

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

[xdai1, 2016-09-16 23:39:50]

michaelpg@, could you help review this CL please? Thanks!

[xdai1, 2016-09-16 23:40:01]

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

[commit-bot: I haz the power, 2016-09-16 23:40:16]

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-17 01:02:39]

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

[commit-bot: I haz the power, 2016-09-17 01:02:40]

Dry run: This issue passed the CQ dry run.

[xdai1, 2016-09-20 00:27:30]

On 2016/09/16 23:39:50, xdai1 wrote:
> michaelpg@, could you help review this CL please? Thanks!

kindly ping? I guess you might not see this one. Sorry about keeping throwing
CLs to you...

[michaelpg, 2016-09-20 20:58:42]

https://codereview.chromium.org/2333283004/diff/140001/chrome/app/settings_st...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2333283004/diff/140001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:834: <message
name="IDS_SETTINGS_PRINTING_CUPS_PRINTER_ADDED_PRINTER_MESSAGE" desc="The
message shown when a new printer is setup successfully.">
nit: "set up" (2 words)

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

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:266:
this.resetPrinterData_(this.selectedPrinter);
This doesn't notify Polymer of these changes. How does the dialog get updated?

A Polymer-y pattern might look like:
    this.selectedPrinter = this.getEmptyPrinter_();

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:373:
if (this.previousDialog_ == AddPrinterDialogs.MANUFACTURER)
I'm having trouble following the flow here. When would this fail without the
previous dialog being the manufacturer dialog? Why would that not be considered
a failed setup, if |success| is false?

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/printing_page/cups_printers.html (right):

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_printers.html:23: left:
150px;
you'll need to make this work in RTL, e.g.:

.message[dir=rtl] {
  left: 0;
  right: 150px;
}

you can test with a language like Arabic

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/resourc...
File
chrome/browser/resources/settings/printing_page/cups_printers_browser_proxy.js
(right):

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_printers_browser_proxy.js:50:
* @param {string} printerId
add param

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

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:189:
CallJavascriptFunction("cr.webUIListenerCallback",
nit: indent this and above consistently?

[xdai1, 2016-09-21 17:40:20]

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

https://codereview.chromium.org/2333283004/diff/140001/chrome/app/settings_st...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2333283004/diff/140001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:834: <message
name="IDS_SETTINGS_PRINTING_CUPS_PRINTER_ADDED_PRINTER_MESSAGE" desc="The
message shown when a new printer is setup successfully.">
On 2016/09/20 20:58:42, michaelpg wrote:
> nit: "set up" (2 words)

Done.

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

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:266:
this.resetPrinterData_(this.selectedPrinter);
On 2016/09/20 20:58:42, michaelpg wrote:
> This doesn't notify Polymer of these changes. How does the dialog get updated?
> 
> A Polymer-y pattern might look like:
>     this.selectedPrinter = this.getEmptyPrinter_();

Actually it works. For example, this.resetPrinterData_(this.newPrinter) sets
this.newPrinter.printerName to an empty string, the element in the html file
that binds to newPrinter.printerName gets notified and updates itself
immediately, right?

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:373:
if (this.previousDialog_ == AddPrinterDialogs.MANUFACTURER)
On 2016/09/20 20:58:42, michaelpg wrote:
> I'm having trouble following the flow here. When would this fail without the
> previous dialog being the manufacturer dialog? Why would that not be
considered
> a failed setup, if |success| is false?

This is the flow
https://folio.googleplex.com/chrome-ux/mocks/390-CrOS-settings-printing#%2FCU....
For manually setup flow, it's only considered as "failed" in the path:
04->04a->05->05a->05b. So on 04a, user clicks "Add" button to set up a printer,
wait for true/false response from the backend, if the backend returns false, go
to 05 and make the user specifies the maker/model and ppd file and clicks "Add"
button to retry to set up the printer, if it fails again, show 05b. So
|this.previousDialog_| could be AddPrinterDialogs.MANUALLY or
AddPrinterDialogs.MANUFACTURER, only the latter one is considered a real
failure.

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/printing_page/cups_printers.html (right):

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_printers.html:23: left:
150px;
On 2016/09/20 20:58:42, michaelpg wrote:
> you'll need to make this work in RTL, e.g.:
> 
> .message[dir=rtl] {
>   left: 0;
>   right: 150px;
> }
> 
> you can test with a language like Arabic

I could not test the exact string since it hasn't been translated so I used a
similar string and tested it. It looks fine to me (I made a screenshot here:
https://drive.google.com/corp/drive/u/0/folders/0B4R9nvLdzmC9d3o2WDNyNGRjejQ See
the back rectangle). Let me know if it looks good to you.

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/resourc...
File
chrome/browser/resources/settings/printing_page/cups_printers_browser_proxy.js
(right):

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_printers_browser_proxy.js:50:
* @param {string} printerId
On 2016/09/20 20:58:42, michaelpg wrote:
> add param

Done.

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

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:189:
CallJavascriptFunction("cr.webUIListenerCallback",
On 2016/09/20 20:58:42, michaelpg wrote:
> nit: indent this and above consistently?

This is what "git cl format" gives me...

[michaelpg, 2016-09-21 19:13:23]

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

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:266:
this.resetPrinterData_(this.selectedPrinter);
On 2016/09/21 17:40:19, xdai1 wrote:
> On 2016/09/20 20:58:42, michaelpg wrote:
> > This doesn't notify Polymer of these changes. How does the dialog get
updated?
> > 
> > A Polymer-y pattern might look like:
> >     this.selectedPrinter = this.getEmptyPrinter_();
> 
> Actually it works. For example, this.resetPrinterData_(this.newPrinter) sets
> this.newPrinter.printerName to an empty string, the element in the html file
> that binds to newPrinter.printerName gets notified and updates itself
> immediately, right?

No: https://jsfiddle.net/jdysdxey/

See https://www.polymer-project.org/1.0/docs/devguide/model-data#notify-path

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:373:
if (this.previousDialog_ == AddPrinterDialogs.MANUFACTURER)
On 2016/09/21 17:40:19, xdai1 wrote:
> On 2016/09/20 20:58:42, michaelpg wrote:
> > I'm having trouble following the flow here. When would this fail without the
> > previous dialog being the manufacturer dialog? Why would that not be
> considered
> > a failed setup, if |success| is false?
> 
> This is the flow
>
https://folio.googleplex.com/chrome-ux/mocks/390-CrOS-settings-printing#%2FCU....
> For manually setup flow, it's only considered as "failed" in the path:
> 04->04a->05->05a->05b. So on 04a, user clicks "Add" button to set up a
printer,
> wait for true/false response from the backend, if the backend returns false,
go
> to 05 and make the user specifies the maker/model and ppd file and clicks
"Add"
> button to retry to set up the printer, if it fails again, show 05b. So
> |this.previousDialog_| could be AddPrinterDialogs.MANUALLY or
> AddPrinterDialogs.MANUFACTURER, only the latter one is considered a real
> failure. 

Is this just because you haven't implemented the 04b error message yet?

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/printing_page/cups_printers.html (right):

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_printers.html:23: left:
150px;
On 2016/09/21 17:40:19, xdai1 wrote:
> On 2016/09/20 20:58:42, michaelpg wrote:
> > you'll need to make this work in RTL, e.g.:
> > 
> > .message[dir=rtl] {
> >   left: 0;
> >   right: 150px;
> > }
> > 
> > you can test with a language like Arabic
> 
> I could not test the exact string since it hasn't been translated so I used a
> similar string and tested it. It looks fine to me (I made a screenshot here:
> https://drive.google.com/corp/drive/u/0/folders/0B4R9nvLdzmC9d3o2WDNyNGRjejQ
See
> the back rectangle). Let me know if it looks good to you.

So the rectangle is centered horizontally at any width (when you resize the
page)?

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

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:189:
CallJavascriptFunction("cr.webUIListenerCallback",
On 2016/09/21 17:40:20, xdai1 wrote:
> On 2016/09/20 20:58:42, michaelpg wrote:
> > nit: indent this and above consistently?
> 
> This is what "git cl format" gives me...

Acknowledged.

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

https://codereview.chromium.org/2333283004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:88:
printerPPDPath: '',
duplicate

[xdai1, 2016-09-21 23:16:16]

michealpg@, I've addressed your comments. Please take another look. thanks!

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

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:266:
this.resetPrinterData_(this.selectedPrinter);
On 2016/09/21 19:13:23, michaelpg wrote:
> On 2016/09/21 17:40:19, xdai1 wrote:
> > On 2016/09/20 20:58:42, michaelpg wrote:
> > > This doesn't notify Polymer of these changes. How does the dialog get
> updated?
> > > 
> > > A Polymer-y pattern might look like:
> > >     this.selectedPrinter = this.getEmptyPrinter_();
> > 
> > Actually it works. For example, this.resetPrinterData_(this.newPrinter) sets
> > this.newPrinter.printerName to an empty string, the element in the html file
> > that binds to newPrinter.printerName gets notified and updates itself
> > immediately, right?
> 
> No: https://jsfiddle.net/jdysdxey/
> 
> See https://www.polymer-project.org/1.0/docs/devguide/model-data#notify-path

Thanks! You're right. But then I'm confused why it does work in my case. Could
it because we only reset the printer data each time we reopen the dialog? I
modified it as you suggested.

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:373:
if (this.previousDialog_ == AddPrinterDialogs.MANUFACTURER)
On 2016/09/21 19:13:23, michaelpg wrote:
> On 2016/09/21 17:40:19, xdai1 wrote:
> > On 2016/09/20 20:58:42, michaelpg wrote:
> > > I'm having trouble following the flow here. When would this fail without
the
> > > previous dialog being the manufacturer dialog? Why would that not be
> > considered
> > > a failed setup, if |success| is false?
> > 
> > This is the flow
> >
>
https://folio.googleplex.com/chrome-ux/mocks/390-CrOS-settings-printing#%2FCU....
> > For manually setup flow, it's only considered as "failed" in the path:
> > 04->04a->05->05a->05b. So on 04a, user clicks "Add" button to set up a
> printer,
> > wait for true/false response from the backend, if the backend returns false,
> go
> > to 05 and make the user specifies the maker/model and ppd file and clicks
> "Add"
> > button to retry to set up the printer, if it fails again, show 05b. So
> > |this.previousDialog_| could be AddPrinterDialogs.MANUALLY or
> > AddPrinterDialogs.MANUFACTURER, only the latter one is considered a real
> > failure. 
> 
> Is this just because you haven't implemented the 04b error message yet?

Actually based on the discussion I had with the PM (weifangsun@) and the backend
engineer (skau@), for now we never show img04b directly. We will always fallback
to ask the user for more info(img05) and re-try and then show the error message
if failed. I'll follow up with the PM to see if we still need the flow from
img04a->img04b.

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/printing_page/cups_printers.html (right):

https://codereview.chromium.org/2333283004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_printers.html:23: left:
150px;
On 2016/09/21 19:13:23, michaelpg wrote:
> On 2016/09/21 17:40:19, xdai1 wrote:
> > On 2016/09/20 20:58:42, michaelpg wrote:
> > > you'll need to make this work in RTL, e.g.:
> > > 
> > > .message[dir=rtl] {
> > >   left: 0;
> > >   right: 150px;
> > > }
> > > 
> > > you can test with a language like Arabic
> > 
> > I could not test the exact string since it hasn't been translated so I used
a
> > similar string and tested it. It looks fine to me (I made a screenshot here:
> > https://drive.google.com/corp/drive/u/0/folders/0B4R9nvLdzmC9d3o2WDNyNGRjejQ
> See
> > the back rectangle). Let me know if it looks good to you.
> 
> So the rectangle is centered horizontally at any width (when you resize the
> page)?

No, it's not... It's always centered horizontally against the full width of the
Cups print subpage.

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

https://codereview.chromium.org/2333283004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:88:
printerPPDPath: '',
On 2016/09/21 19:13:23, michaelpg wrote:
> duplicate

Removed it.

[xdai1, 2016-09-22 18:22:11]

michaelpg@, please take another look, thanks!

https://codereview.chromium.org/2333283004/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/printing_page/cups_printers.html (right):

https://codereview.chromium.org/2333283004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/printing_page/cups_printers.html:51: <div
id="message">
Modified it as you suggested. Now the rectangle is centered horizontally at any
width. Thanks!

[michaelpg, 2016-09-22 20:10:38]

lgtm

[xdai1, 2016-09-22 20:17:47]

On 2016/09/22 20:10:38, michaelpg wrote:
> lgtm

Thanks for the review!

[xdai1, 2016-09-22 20:17:51]

The CQ bit was checked by xdai@chromium.org

[commit-bot: I haz the power, 2016-09-22 20:18:34]

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-22 21:30:48]

Committed patchset #8 (id:220001)

[commit-bot: I haz the power, 2016-09-22 21:33:20]

Description was changed from

==========
[CUPS] Implement the UI handler for adding a new printer.

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

to

==========
[CUPS] Implement the UI handler for adding a new printer.

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

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

[commit-bot: I haz the power, 2016-09-22 21:33:21]

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