Cloud Print: Add human readable names for standard media sizes.

Cloud Print: Add human readable names for standard media sizes.

Also sort the media sizes in the same order as the Google Cloud Print
website.

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

Committed: https://crrev.com/19e26d404c3aab3a3b33c3a360b02bbf825743ac
Cr-Commit-Position: refs/heads/master@{#426311}

[Lei Zhang, 2016-10-19 06:57:08]

Description was changed from

==========
Cloud Print: Add human readable names for standard media sizes.

Also sort the media sizes in the same order as the Google Cloud Print
website.

BUG=653019
==========

to

==========
Cloud Print: Add human readable names for standard media sizes.

Also sort the media sizes in the same order as the Google Cloud Print
website.

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

[Lei Zhang, 2016-10-19 06:57:17]

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

[Lei Zhang, 2016-10-19 07:21:21]

thestig@chromium.org changed reviewers:
+ dpapad@chromium.org

[Lei Zhang, 2016-10-19 07:21:22]

dpapad: PTAL. Let me know if I'm doing silly things in JS.
paolof: FYI. Comments/suggestions welcome.

https://codereview.chromium.org/2436603002/diff/1/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://codereview.chromium.org/2436603002/diff/1/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/data/destination_store.js:343: var
mediaDisplayNames = {
We can always add more later. The additions are sufficient for the printer I'm
testing with.

https://codereview.chromium.org/2436603002/diff/1/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/data/destination_store.js:393: // For the
standard sizes, separate into categories, as seen in the Cloud
I have not seen the code for the Cloud Print website, but based on observations,
this is what I believe happens.

[Lei Zhang, 2016-10-19 07:41:54]

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

[commit-bot: I haz the power, 2016-10-19 09:32:35]

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-19 13:03:00]

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

[commit-bot: I haz the power, 2016-10-19 13:03:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dpapad, 2016-10-19 18:30:17]

https://codereview.chromium.org/2436603002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://codereview.chromium.org/2436603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:373: * @param
{string} a Media to compare.
If |a|, |b| are of type string, they can't have 'custom_display_name_localized'
member var. The type must be something else here.

https://codereview.chromium.org/2436603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:396: if
(mediaSize) {
Nit (optional): Instead of placing the main body of this function in an if()
{...}, consider reversing as follows. IMO is more readable this way (the
tradeoff is repeating the return statement, vs having most of the code indented
by extra 2 spaces.

if (!mediaSize)
  return capabilities;

// remaining logic here.
.....
return capabilities;

https://codereview.chromium.org/2436603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:443: combined =
combined.concat(categoryStandardNA);
Array#concat() is creating a new array every time. You could save some copies by
using the 1st array as the starting point and concat all other arrays onto that
one, as follows,

var combined = categoryStandardNA;
combined.push(...categoryStandardCN);
combined.push(...categoryStandardISO);
combined.push(...categoryStandardJP);
combined.push(...categoryStandardMisc);
combined.push(...categoryCustom);
mediaSize.option = combined;

"..." is a new ES6 spread operator, which basically distributes an array as
positional arguments.

[dpapad, 2016-10-19 18:57:50]

https://codereview.chromium.org/2436603002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://codereview.chromium.org/2436603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:381: if (nameA
> nameB)
Nit: This can be compressed as follows.

return nameA == nameB ? 0 : (nameA > nameB ? 1 : -1);

[Lei Zhang, 2016-10-19 18:58:28]

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

[commit-bot: I haz the power, 2016-10-19 18:58:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2016-10-19 18:59:28]

https://codereview.chromium.org/2436603002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://codereview.chromium.org/2436603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:373: * @param
{string} a Media to compare.
On 2016/10/19 18:30:17, dpapad wrote:
> If |a|, |b| are of type string, they can't have
'custom_display_name_localized'
> member var. The type must be something else here.

"!Object" it is.

https://codereview.chromium.org/2436603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:396: if
(mediaSize) {
On 2016/10/19 18:30:17, dpapad wrote:
> Nit (optional): Instead of placing the main body of this function in an if()
> {...}, consider reversing as follows. IMO is more readable this way (the
> tradeoff is repeating the return statement, vs having most of the code
indented
> by extra 2 spaces.
> 
> if (!mediaSize)
>   return capabilities;
> 
> // remaining logic here.
> .....
> return capabilities;

Sure. We can do early returns. This was copy + pasted from another function.

https://codereview.chromium.org/2436603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:443: combined =
combined.concat(categoryStandardNA);
On 2016/10/19 18:30:17, dpapad wrote:
> Array#concat() is creating a new array every time. You could save some copies
by
> using the 1st array as the starting point and concat all other arrays onto
that
> one, as follows,
> 
> var combined = categoryStandardNA;
> combined.push(...categoryStandardCN);
> combined.push(...categoryStandardISO);
> combined.push(...categoryStandardJP);
> combined.push(...categoryStandardMisc);
> combined.push(...categoryCustom);
> mediaSize.option = combined;
> 
> "..." is a new ES6 spread operator, which basically distributes an array as
> positional arguments.

Done.

[Lei Zhang, 2016-10-19 19:03:17]

BTW, this is not exactly the same as the cloud print website. In some cases, the
website changes "$size" to "$vendor_id - $size" but I suppose this is sufficient
for now.

[dpapad, 2016-10-19 19:12:31]

LGTM

https://codereview.chromium.org/2436603002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://codereview.chromium.org/2436603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:381: if (nameA
> nameB)
On 2016/10/19 at 18:57:50, dpapad wrote:
> Nit: This can be compressed as follows.
> 
> return nameA == nameB ? 0 : (nameA > nameB ? 1 : -1);

^^

https://codereview.chromium.org/2436603002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://codereview.chromium.org/2436603002/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:446:
combined.push(...categoryStandardCN);
Nit (optional): Just realized this. You can also compress even more as follows

combined.push(...categoryStandardCN, ...categoryStandardISO,
...categoryStandardJP, ...etc);

[dpapad, 2016-10-19 19:15:30]

https://codereview.chromium.org/2436603002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://codereview.chromium.org/2436603002/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:446:
combined.push(...categoryStandardCN);
On 2016/10/19 at 19:12:31, dpapad wrote:
> Nit (optional): Just realized this. You can also compress even more as follows
> 
> combined.push(...categoryStandardCN, ...categoryStandardISO,
...categoryStandardJP, ...etc);

Just FYI, you can also do the following, eliminating the need to call push()
(starting to like the spread operator even more ;>).

var combined = [...categoryStandardNA, ...categoryStandardCN, ...etc];

[Lei Zhang, 2016-10-19 19:17:27]

https://codereview.chromium.org/2436603002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://codereview.chromium.org/2436603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:381: if (nameA
> nameB)
On 2016/10/19 18:57:50, dpapad wrote:
> Nit: This can be compressed as follows.
> 
> return nameA == nameB ? 0 : (nameA > nameB ? 1 : -1);

Woo, ternary operators.

https://codereview.chromium.org/2436603002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://codereview.chromium.org/2436603002/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:446:
combined.push(...categoryStandardCN);
On 2016/10/19 19:12:31, dpapad wrote:
> Nit (optional): Just realized this. You can also compress even more as follows
> 
> combined.push(...categoryStandardCN, ...categoryStandardISO,
> ...categoryStandardJP, ...etc);

Done.

[Lei Zhang, 2016-10-19 19:17:33]

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

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

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-19 21:19:56]

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

[commit-bot: I haz the power, 2016-10-19 21:19:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Lei Zhang, 2016-10-19 21:25:38]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2016-10-19 21:25:38]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/2436603002/#ps60001
(title: "Address comments x2")

[commit-bot: I haz the power, 2016-10-19 21:26:39]

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-19 22:40:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-21 13:12:23]

Description was changed from

==========
Cloud Print: Add human readable names for standard media sizes.

Also sort the media sizes in the same order as the Google Cloud Print
website.

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

to

==========
Cloud Print: Add human readable names for standard media sizes.

Also sort the media sizes in the same order as the Google Cloud Print
website.

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

Committed: https://crrev.com/19e26d404c3aab3a3b33c3a360b02bbf825743ac
Cr-Commit-Position: refs/heads/master@{#426311}
==========

[commit-bot: I haz the power, 2016-10-21 13:12:24]

Patchset 4 (id:??) landed as
https://crrev.com/19e26d404c3aab3a3b33c3a360b02bbf825743ac
Cr-Commit-Position: refs/heads/master@{#426311}