[CUPS] Fix the Printer protocol string that displays in the UI.

[CUPS] Fix the Printer protocol string that displays in the UI.

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

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

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

Description was changed from

==========
[CUPS] Fix the Printer protocol string that displays in the UI.

BUG=626752
==========

to

==========
[CUPS] Fix the Printer protocol string that displays in the UI.

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

[xdai1, 2016-09-17 00:07:22]

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

[xdai1, 2016-09-17 00:07:23]

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

[michaelpg, 2016-09-17 02:19:06]

if the answer to both of these is "no" then LMK and I'll stamp it :-)

https://codereview.chromium.org/2341263003/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/printing_page/cups_printer_details_page.js
(right):

https://codereview.chromium.org/2341263003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/printing_page/cups_printer_details_page.js:58:
if (printerProtocol == 'ipp')
Optional: name the strings "printerProtocol_ipp", "printerProtocol_socket", etc.
and do a lookup:

    if (loadTimeData.valueExists('printerProtocol_' + printerProtocol))
      return loadTimeData.getString('printerProtocol_' + printerProtocol);
    return '';

Rationale: since the keys are basically generated using the printer's URL scheme
or w/e, there's not much value in customizing the names of the strings.

https://codereview.chromium.org/2341263003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/printing_page/cups_printer_details_page.js:72:
return '';
if the printer protocol should always be one of the above strings (or empty),
then return early if the string is '', and call assertNotReached at the end.

[xdai1, 2016-09-19 22:43:48]

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

https://codereview.chromium.org/2341263003/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/printing_page/cups_printer_details_page.js
(right):

https://codereview.chromium.org/2341263003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/printing_page/cups_printer_details_page.js:58:
if (printerProtocol == 'ipp')
On 2016/09/17 02:19:06, michaelpg wrote:
> Optional: name the strings "printerProtocol_ipp", "printerProtocol_socket",
etc.
> and do a lookup:
> 
>     if (loadTimeData.valueExists('printerProtocol_' + printerProtocol))
>       return loadTimeData.getString('printerProtocol_' + printerProtocol);
>     return '';
> 
> Rationale: since the keys are basically generated using the printer's URL
scheme
> or w/e, there's not much value in customizing the names of the strings.

Renaming it to "printerProtocol_ipp" seems break the naming convention in the
webui handler file... If you don't mind, I would prefer to keep it this way.

https://codereview.chromium.org/2341263003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/printing_page/cups_printer_details_page.js:72:
return '';
On 2016/09/17 02:19:06, michaelpg wrote:
> if the printer protocol should always be one of the above strings (or empty),
> then return early if the string is '', and call assertNotReached at the end.

Addressed! Yes, the protocol should only be one of the above strings.

[michaelpg, 2016-09-19 23:32:08]

lgtm

https://codereview.chromium.org/2341263003/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/printing_page/cups_printer_details_page.js
(right):

https://codereview.chromium.org/2341263003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/printing_page/cups_printer_details_page.js:58:
if (printerProtocol == 'ipp')
On 2016/09/19 22:43:48, xdai1 wrote:
> On 2016/09/17 02:19:06, michaelpg wrote:
> > Optional: name the strings "printerProtocol_ipp", "printerProtocol_socket",
> etc.
> > and do a lookup:
> > 
> >     if (loadTimeData.valueExists('printerProtocol_' + printerProtocol))
> >       return loadTimeData.getString('printerProtocol_' + printerProtocol);
> >     return '';
> > 
> > Rationale: since the keys are basically generated using the printer's URL
> scheme
> > or w/e, there's not much value in customizing the names of the strings.
> 
> Renaming it to "printerProtocol_ipp" seems break the naming convention in the
> webui handler file... If you don't mind, I would prefer to keep it this way.

That's fine, I just mentioned it as an option. (Straying from convention where
it makes sense is fine, and we don't have strict rules for string keys for
loadTimeData.)

[xdai1, 2016-09-19 23:54:07]

The CQ bit was checked by xdai@chromium.org

[commit-bot: I haz the power, 2016-09-19 23:54:33]

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-20 01:27:06]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-20 01:29:18]

Description was changed from

==========
[CUPS] Fix the Printer protocol string that displays in the UI.

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

to

==========
[CUPS] Fix the Printer protocol string that displays in the UI.

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

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

[commit-bot: I haz the power, 2016-09-20 01:29:19]

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