Print Preview: Fix compile errors in settings/ directory

Fix the print preview closure compiler errors from files in
chrome/browser/resources/print_preview/settings/*

After this CL, there are 2 warnings and 222 errors when
running the closure compiler.

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

Review-Url: https://codereview.chromium.org/2861713004
Cr-Commit-Position: refs/heads/master@{#469456}
Committed: https://chromium.googlesource.com/chromium/src/+/63120f6b07e2315545b80732bb94aa1930ae58cb

[rbpotter, 2017-05-03 21:57:11]

Description was changed from

==========
Fix most settings errors

Fix most of the print preview closure compiler errors from files in
chrome/browser/resources/print_preview/settings/*

BUG=717620
==========

to

==========
Fix most settings errors

Fix most of the print preview closure compiler errors from files in
chrome/browser/resources/print_preview/settings/*

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

[rbpotter, 2017-05-04 00:29:37]

Description was changed from

==========
Fix most settings errors

Fix most of the print preview closure compiler errors from files in
chrome/browser/resources/print_preview/settings/*

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

to

==========
Fix the print preview closure compiler errors from files in
chrome/browser/resources/print_preview/settings/*

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

[rbpotter, 2017-05-04 00:29:37]

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

[dpapad, 2017-05-04 01:20:04]

https://codereview.chromium.org/2861713004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/print_preview/data/ticket_items/margins_type.js
(right):

https://codereview.chromium.org/2861713004/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/ticket_items/margins_type.js:10:
print_preview.ticket_items.MarginsTypeValue = {
This relies on the fact that some other file has called
cr.define('print_preview.ticket_items',...) already.

Can we add a
cr.exportPath('print_preview.ticket_items');

to ensure that changing the order of the files being evaluated would not cause
this file to throw an error? See example where the same pattern is already used
https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/n....

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

https://codereview.chromium.org/2861713004/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/advanced_options_settings.js:64:
assert(this.getButton_()), 'click', function() {
Instead of asserting here, can we change getButton_() to return !HTMLElement and
assert there?

/**
 * @return {!HTMLElement}
 * @private
 */
getButton_: function() {
  return assert(this.getChildElement('.advanced-options-settings-button'));
},

Having said that, there is already wider TODO at
https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/c...
to make getCHildElement() return !HTMLElement, but probably better to do this
separately (if at all).

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

https://codereview.chromium.org/2861713004/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/advanced_settings/advanced_settings.js:73:
assert(this.getChildElement('.button-strip .cancel-button')),
Follow up from previous comment: if we make getChildElement() return
!HTMLElement, there would be no need to assert here, but that requires ensuring
that there are no valid use cases for having getChildElement() return null.

https://codereview.chromium.org/2861713004/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/advanced_settings/advanced_settings.js:199:
setIsVisible(assert(this.getChildElement('.search-box-area')), false);
Nit: Assertions are not free. Perhaps use a local var to avoid asserting twice

var searchBoxArea = assert(this.getChildElement('.search-box-area'));
...

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

https://codereview.chromium.org/2861713004/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/advanced_settings/advanced_settings_item.js:139:
return /** @type {HTMLSelectElement} */(
How about casting to !HTMLSelectElement here, and change the @return too, and
avoid having to assert() when using text_? Similar for select_ above.

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

https://codereview.chromium.org/2861713004/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/copies_settings.js:88:
this.inputField_ = this.getChildElement('input.user-value');
Nit: Maybe
this.inputField_ = assert(this.getChildElement('input.user-value'));
and avoid the need for a second var pointing to the same object?

[rbpotter, 2017-05-04 01:43:07]

https://codereview.chromium.org/2861713004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/print_preview/data/ticket_items/margins_type.js
(right):

https://codereview.chromium.org/2861713004/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/ticket_items/margins_type.js:10:
print_preview.ticket_items.MarginsTypeValue = {
On 2017/05/04 01:20:03, dpapad wrote:
> This relies on the fact that some other file has called
> cr.define('print_preview.ticket_items',...) already.
> 
> Can we add a
> cr.exportPath('print_preview.ticket_items');
> 
> to ensure that changing the order of the files being evaluated would not cause
> this file to throw an error? See example where the same pattern is already
used
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/n....

Done.

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

https://codereview.chromium.org/2861713004/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/advanced_options_settings.js:64:
assert(this.getButton_()), 'click', function() {
On 2017/05/04 01:20:04, dpapad wrote:
> Instead of asserting here, can we change getButton_() to return !HTMLElement
and
> assert there?
> 
> /**
>  * @return {!HTMLElement}
>  * @private
>  */
> getButton_: function() {
>   return assert(this.getChildElement('.advanced-options-settings-button'));
> },
> 
> Having said that, there is already wider TODO at
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/c...
> to make getCHildElement() return !HTMLElement, but probably better to do this
> separately (if at all).

Done.

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

https://codereview.chromium.org/2861713004/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/advanced_settings/advanced_settings.js:73:
assert(this.getChildElement('.button-strip .cancel-button')),
On 2017/05/04 01:20:04, dpapad wrote:
> Follow up from previous comment: if we make getChildElement() return
> !HTMLElement, there would be no need to assert here, but that requires
ensuring
> that there are no valid use cases for having getChildElement() return null.

Acknowledged - will investigate getChildElement and upload a follow up
CL if it is possible to make it return !HTMLElement.

https://codereview.chromium.org/2861713004/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/advanced_settings/advanced_settings.js:199:
setIsVisible(assert(this.getChildElement('.search-box-area')), false);
On 2017/05/04 01:20:04, dpapad wrote:
> Nit: Assertions are not free. Perhaps use a local var to avoid asserting twice
> 
> var searchBoxArea = assert(this.getChildElement('.search-box-area'));
> ...

Done.

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

https://codereview.chromium.org/2861713004/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/advanced_settings/advanced_settings_item.js:139:
return /** @type {HTMLSelectElement} */(
On 2017/05/04 01:20:04, dpapad wrote:
> How about casting to !HTMLSelectElement here, and change the @return too, and
> avoid having to assert() when using text_? Similar for select_ above.

Done.

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

https://codereview.chromium.org/2861713004/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/copies_settings.js:88:
this.inputField_ = this.getChildElement('input.user-value');
On 2017/05/04 01:20:04, dpapad wrote:
> Nit: Maybe
> this.inputField_ = assert(this.getChildElement('input.user-value'));
> and avoid the need for a second var pointing to the same object?

Done.

[rbpotter, 2017-05-04 01:43:15]

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

[commit-bot: I haz the power, 2017-05-04 01:43:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-05-04 02:21:31]

LGTM. Can you mention the number of errors remaining after this CL in the
description? Unfortunately there is no count displayed, so what I do is to
forward the output to a file and then

./third_party/closure_compiler/run_compiler print_preview > errors.txt
cat errors.txt | grep ERROR | wc -l

[commit-bot: I haz the power, 2017-05-04 03:34:29]

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

[commit-bot: I haz the power, 2017-05-04 03:34:32]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2017-05-04 20:33:39]

Description was changed from

==========
Fix the print preview closure compiler errors from files in
chrome/browser/resources/print_preview/settings/*

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

to

==========
Fix the print preview closure compiler errors from files in
chrome/browser/resources/print_preview/settings/*

After this CL, there are 2 warnings and 222 errors remaining.

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

[rbpotter, 2017-05-04 20:35:58]

Description was changed from

==========
Fix the print preview closure compiler errors from files in
chrome/browser/resources/print_preview/settings/*

After this CL, there are 2 warnings and 222 errors remaining.

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

to

==========
Fix the print preview closure compiler errors from files in
chrome/browser/resources/print_preview/settings/*

After this CL, there are 2 warnings and 222 errors when
running the closure compiler.

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

[rbpotter, 2017-05-04 20:36:39]

On 2017/05/04 02:21:31, dpapad wrote:
> LGTM. Can you mention the number of errors remaining after this CL in the
> description? Unfortunately there is no count displayed, so what I do is to
> forward the output to a file and then
> 
> ./third_party/closure_compiler/run_compiler print_preview > errors.txt
> cat errors.txt | grep ERROR | wc -l

Thanks! Updated the description. 222 errors and 2 warnings left.

[rbpotter, 2017-05-04 20:37:03]

The CQ bit was checked by rbpotter@chromium.org

[commit-bot: I haz the power, 2017-05-04 20:37: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, 2017-05-04 20:44:27]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493930223021420,
"parent_rev": "29b751a9ca7189238f4ad3703fd9db726698af91", "commit_rev":
"63120f6b07e2315545b80732bb94aa1930ae58cb"}

[commit-bot: I haz the power, 2017-05-04 20:44:48]

Description was changed from

==========
Fix the print preview closure compiler errors from files in
chrome/browser/resources/print_preview/settings/*

After this CL, there are 2 warnings and 222 errors when
running the closure compiler.

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

to

==========
Fix the print preview closure compiler errors from files in
chrome/browser/resources/print_preview/settings/*

After this CL, there are 2 warnings and 222 errors when
running the closure compiler.

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

Review-Url: https://codereview.chromium.org/2861713004
Cr-Commit-Position: refs/heads/master@{#469456}
Committed:
https://chromium.googlesource.com/chromium/src/+/63120f6b07e2315545b80732bb94...
==========

[commit-bot: I haz the power, 2017-05-04 20:44:49]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/63120f6b07e2315545b80732bb94...