Print Preview: Simplify other_options_settings javascript code.

Print Preview: Simplify other_options_settings javascript code.

There is a lot of repeated code in other_options_settings.js. Simplify
the code and make it easier to add additional options if needed in
future.

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

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

[rbpotter, 2016-12-15 01:44:08]

Description was changed from

==========
Simplify other_options_settings javascript code.

There is a lot of repeated code in other_options_settings.js. Simplify
the code and make it easier to add additional options if needed in
future.

BUG=None
==========

to

==========
Simplify other_options_settings javascript code.

There is a lot of repeated code in other_options_settings.js. Simplify
the code and make it easier to add additional options if needed in
future.

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

[rbpotter, 2016-12-15 02:30:11]

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

[commit-bot: I haz the power, 2016-12-15 02:30:16]

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

[rbpotter, 2016-12-15 02:30:20]

Description was changed from

==========
Simplify other_options_settings javascript code.

There is a lot of repeated code in other_options_settings.js. Simplify
the code and make it easier to add additional options if needed in
future.

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

to

==========
Simplify other_options_settings javascript code.

There is a lot of repeated code in other_options_settings.js. Simplify
the code and make it easier to add additional options if needed in
future.

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

[rbpotter, 2016-12-15 02:30:20]

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

[rbpotter, 2016-12-15 02:30:35]

Other options settings changes in separate CL

[commit-bot: I haz the power, 2016-12-15 02:36:24]

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

[commit-bot: I haz the power, 2016-12-15 02:36:25]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[dpapad, 2016-12-15 18:40:40]

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/print_preview/settings/other_options_settings.js
(right):

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:8:
function CheckboxTicketItemElement(ticketItem, collapsible, className) {
@constructor

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:11: *
@type {!print_preview.ticket_items.TicketItem}
@private {!print_preview.ticket_items.TicketItem}
Same below.

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:35: *
container_.
Nit: Let's add the following to the comment.
"Populated when decorate() is called."
Similarly for |container_| above.

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:49: *
@return {print_preview.ticket_items.TicketItem} The ticket item for this
!print_preview.ticket_items.TicketItem

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:64:
this.className_);
If className identifies uniquely this element, can we turn it into an ID instead
of a CSS class name?

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:76: if
(this.checkbox_.checked != null)
Is it possible for |checked| to be null? It seems to me that it always has a
boolean value, see https://jsfiddle.net/ykhd7fs3/.

Did you mean the following?

if (this.checkbox_ != null)
  this.ticketItem_.updateValue(this.checkbox_.checked);

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:82: *
@param {print_preview.SettingsSection.OtherOptionsSettings} The
!print_preview.SettingsSection.OtherOptionsSettings

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:128: *
@type {Array<CheckboxTicketItemElement>} checkbox ticket item elements
@private {!Array<!CheckboxTicketItemElement>}

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:165:
for (var i = 0; i < this.elements_.length - 1; i++)
Is this skipping the last element on purpose? If yes, let's add a comment
explaining why. If not, let's just use forEach() which is much easier than
regular for (no need to keep track of a counter).

this.elements_.forEach(function(element) {
  element.checkbox.disabled = !isEnabled;
});

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:172:
for (var i = 0; i < this.elements_.length; i++) {
Nit: Using forEach makes it more readable I think

this.elements_.forEach(function(element) {
  this.tracker.add(
      element.checkbox,
      'click',
      element.onCheckboxClick.bind(element));
   ...
}, this);

[rbpotter, 2016-12-15 20:09:30]

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

[commit-bot: I haz the power, 2016-12-15 20:10:27]

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

[rbpotter, 2016-12-15 20:12:10]

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

[rbpotter, 2016-12-15 20:12:17]

Patchset #3 (id:40001) has been deleted

[rbpotter, 2016-12-15 20:12:38]

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/print_preview/settings/other_options_settings.js
(right):

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:8:
function CheckboxTicketItemElement(ticketItem, collapsible, className) {
On 2016/12/15 18:40:40, dpapad wrote:
> @constructor

Done.

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:11: *
@type {!print_preview.ticket_items.TicketItem}
On 2016/12/15 18:40:40, dpapad wrote:
> @private {!print_preview.ticket_items.TicketItem}
> Same below.

Done.

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:35: *
container_.
On 2016/12/15 18:40:40, dpapad wrote:
> Nit: Let's add the following to the comment.
> "Populated when decorate() is called."
> Similarly for |container_| above.

Done.

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:49: *
@return {print_preview.ticket_items.TicketItem} The ticket item for this
On 2016/12/15 18:40:40, dpapad wrote:
> !print_preview.ticket_items.TicketItem

Done.

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:64:
this.className_);
On 2016/12/15 18:40:40, dpapad wrote:
> If className identifies uniquely this element, can we turn it into an ID
instead
> of a CSS class name?

Done.

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:76: if
(this.checkbox_.checked != null)
On 2016/12/15 18:40:40, dpapad wrote:
> Is it possible for |checked| to be null? It seems to me that it always has a
> boolean value, see https://jsfiddle.net/ykhd7fs3/.
> 
> Did you mean the following?
> 
> if (this.checkbox_ != null)
>   this.ticketItem_.updateValue(this.checkbox_.checked);

It can't; this is leftover from when I was trying to figure out which ticket
item was getting updated with a null value (it was custom margins) while looking
into the preview requests test. Have removed the check as there are many things
that will fail if checkbox is null.

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:82: *
@param {print_preview.SettingsSection.OtherOptionsSettings} The
On 2016/12/15 18:40:40, dpapad wrote:
> !print_preview.SettingsSection.OtherOptionsSettings

Done.

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:128: *
@type {Array<CheckboxTicketItemElement>} checkbox ticket item elements
On 2016/12/15 18:40:40, dpapad wrote:
> @private {!Array<!CheckboxTicketItemElement>}

Done.

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:165:
for (var i = 0; i < this.elements_.length - 1; i++)
On 2016/12/15 18:40:40, dpapad wrote:
> Is this skipping the last element on purpose? If yes, let's add a comment
> explaining why. If not, let's just use forEach() which is much easier than
> regular for (no need to keep track of a counter).
> 
> this.elements_.forEach(function(element) {
>   element.checkbox.disabled = !isEnabled;
> });

Done.

https://codereview.chromium.org/2576233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:172:
for (var i = 0; i < this.elements_.length; i++) {
On 2016/12/15 18:40:40, dpapad wrote:
> Nit: Using forEach makes it more readable I think
> 
> this.elements_.forEach(function(element) {
>   this.tracker.add(
>       element.checkbox,
>       'click',
>       element.onCheckboxClick.bind(element));
>    ...
> }, this);

Done.

[commit-bot: I haz the power, 2016-12-15 20:13:41]

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

[dpapad, 2016-12-15 20:32:59]

Mostly nits.

Reading the CL description, is not obvious that it refers to Print preview. Can
you add a prefix as follows?

Print preview: Simplify ......

https://codereview.chromium.org/2576233003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/print_preview/settings/other_options_settings.js
(right):

https://codereview.chromium.org/2576233003/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:13: *
@param {string} The CSS id selector string for the container element for
s/selector string//
(See related following comment too).

Also all @param annotations here are missing the name of the parameter. The
valid syntax is

@param {!print_preview.ticket_items.TicketItem} ticketItem The ticket item ....

https://codereview.chromium.org/2576233003/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:73:
this.cssId_);
Technically the "#" prefix is not part of the ID. Can you remove it from lines
141-149, and simply add it here when calling querySelector?

this.container_ = otherOptionsSettings.getElement().querySelector('#' +
this.cssId_);

But also,
There is not really a need to pass |otherOptionsSettings| as a parameter, is it?
You could simply get the container elements by ID as follows

document.getElementById(this.cssId_);  // Note that for this to work cssId_
should not include the '#' prefix. So how about the following?

this.container_ = document.getElementById(this.cssId_);
this.checkbox_ = this.container_.querySelector('.checkbox');

https://codereview.chromium.org/2576233003/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:173:
/* Skip selection only, which must always be the last element, as this
Nit:
Skip |ticket_items.SelectionOnly|, which must always be .....

[commit-bot: I haz the power, 2016-12-15 21:09:46]

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

[commit-bot: I haz the power, 2016-12-15 21:09:48]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2016-12-15 23:28:10]

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

[rbpotter, 2016-12-15 23:28:17]

https://codereview.chromium.org/2576233003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/print_preview/settings/other_options_settings.js
(right):

https://codereview.chromium.org/2576233003/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:13: *
@param {string} The CSS id selector string for the container element for
On 2016/12/15 20:32:59, dpapad wrote:
> s/selector string//
> (See related following comment too).
> 
> Also all @param annotations here are missing the name of the parameter. The
> valid syntax is
> 
> @param {!print_preview.ticket_items.TicketItem} ticketItem The ticket item
....

Done.

https://codereview.chromium.org/2576233003/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:73:
this.cssId_);
On 2016/12/15 20:32:59, dpapad wrote:
> Technically the "#" prefix is not part of the ID. Can you remove it from lines
> 141-149, and simply add it here when calling querySelector?
> 
> this.container_ = otherOptionsSettings.getElement().querySelector('#' +
> this.cssId_);
> 
> But also,
> There is not really a need to pass |otherOptionsSettings| as a parameter, is
it?
> You could simply get the container elements by ID as follows
> 
> document.getElementById(this.cssId_);  // Note that for this to work cssId_
> should not include the '#' prefix. So how about the following?
> 
> this.container_ = document.getElementById(this.cssId_);
> this.checkbox_ = this.container_.querySelector('.checkbox');

Done.

https://codereview.chromium.org/2576233003/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:173:
/* Skip selection only, which must always be the last element, as this
On 2016/12/15 20:32:59, dpapad wrote:
> Nit:
> Skip |ticket_items.SelectionOnly|, which must always be .....

Done.

[commit-bot: I haz the power, 2016-12-15 23:28:43]

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

[rbpotter, 2016-12-15 23:29:21]

Description was changed from

==========
Simplify other_options_settings javascript code.

There is a lot of repeated code in other_options_settings.js. Simplify
the code and make it easier to add additional options if needed in
future.

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

to

==========
Print Preview: Simplify other_options_settings javascript code.

There is a lot of repeated code in other_options_settings.js. Simplify
the code and make it easier to add additional options if needed in
future.

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

[dpapad, 2016-12-16 00:53:48]

lgtm

https://codereview.chromium.org/2576233003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/print_preview/settings/other_options_settings.js
(right):

https://codereview.chromium.org/2576233003/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:72:
this.container_ = document.getElementById(this.cssId_);
-2 indent

[rbpotter, 2016-12-16 01:19:37]

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

[rbpotter, 2016-12-16 01:19:49]

https://codereview.chromium.org/2576233003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/print_preview/settings/other_options_settings.js
(right):

https://codereview.chromium.org/2576233003/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/other_options_settings.js:72:
this.container_ = document.getElementById(this.cssId_);
On 2016/12/16 00:53:48, dpapad wrote:
> -2 indent

Done.

[commit-bot: I haz the power, 2016-12-16 01:20:29]

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-12-16 06:02:05]

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

[commit-bot: I haz the power, 2016-12-16 06:02:06]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2016-12-16 18:05:47]

The CQ bit was checked by rbpotter@chromium.org

[rbpotter, 2016-12-16 18:05:47]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2576233003/#ps100001
(title: "fix indent")

[commit-bot: I haz the power, 2016-12-16 18:06:29]

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-12-16 18:17:26]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481911547417820,
"parent_rev": "fa2ca2fc40a8e79ebdb7da67abb8d3587e32a699", "commit_rev":
"9e99610ccdca8b778993a04eff4ccc1d9da30352"}

[commit-bot: I haz the power, 2016-12-16 18:17:58]

Description was changed from

==========
Print Preview: Simplify other_options_settings javascript code.

There is a lot of repeated code in other_options_settings.js. Simplify
the code and make it easier to add additional options if needed in
future.

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

to

==========
Print Preview: Simplify other_options_settings javascript code.

There is a lot of repeated code in other_options_settings.js. Simplify
the code and make it easier to add additional options if needed in
future.

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

Review-Url: https://codereview.chromium.org/2576233003
==========

[commit-bot: I haz the power, 2016-12-16 18:17:59]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-12-16 18:22:38]

Description was changed from

==========
Print Preview: Simplify other_options_settings javascript code.

There is a lot of repeated code in other_options_settings.js. Simplify
the code and make it easier to add additional options if needed in
future.

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

Review-Url: https://codereview.chromium.org/2576233003
==========

to

==========
Print Preview: Simplify other_options_settings javascript code.

There is a lot of repeated code in other_options_settings.js. Simplify
the code and make it easier to add additional options if needed in
future.

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

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

[commit-bot: I haz the power, 2016-12-16 18:22:39]

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