Fix all remaining print preview closure compiler errors

Fix all remaining print preview closure compiler errors

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

Review-Url: https://codereview.chromium.org/2865633004
Cr-Commit-Position: refs/heads/master@{#470677}
Committed: https://chromium.googlesource.com/chromium/src/+/419bda2ce75c18c90e66fae1d03fdd1e5479ce04

[rbpotter, 2017-05-05 23:02:33]

Description was changed from

==========
Fix most remaining print preview closure compiler errors

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

to

==========
Fix most remaining print preview closure compiler errors

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

[rbpotter, 2017-05-08 18:48:55]

Description was changed from

==========
Fix most remaining print preview closure compiler errors

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

to

==========
Fix all remaining print preview closure compiler errors

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

[rbpotter, 2017-05-08 18:48:55]

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

[rbpotter, 2017-05-08 18:49:02]

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

[commit-bot: I haz the power, 2017-05-08 18:50:31]

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-08 20:40:10]

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/preview_generator.js (right):

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/preview_generator.js:105: * @type
{Array<Object<{from: number, to: number}>>}
Shouldn't this be as follows?
?Array<{from: number, to: number}>

(see
https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/p...).

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/previewarea/margin_control.js
(right):

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/previewarea/margin_control.js:330: if
(this.textbox_) {
The previous code assumed that this.textbox_ was always populated by the time
this function executes, and if not, an error would be thrown in the console,
probably from [1]. The new code would silently do nothing, instead of letting
the code blow up to surface the programmer's error.

Can we assert() instead of using the if (..) check, to keep previous and new
code equivalent?
 

[1]
https://cs.chromium.org/chromium/src/ui/webui/resources/js/event_tracker.js?l=57

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

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/print_preview.js:1354: var printPreview;
Can this be moved inside the anonymous function below?

var printPreview = new print_preview.PrintPreview();
printPreview.initialize();

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/search/destination_list_item.js
(right):

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/destination_list_item.js:322: *
@param {Event} e The event to handle.
!Event

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/destination_list_item.js:337: if
(e && hasKeyModifiers(e))
Can |e| be ever null? If not let's change the annotation to !Event, and remove
the extra condition.

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/search/destination_search.js
(right):

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/destination_search.js:558:
accountSelectEl.selectedIndex = this.userInfo_.activeUser ?
Is the logic here equivalent to previous logic?

Previous code would assign -1 to |selectedIndex| if |activeUser| or if
|activeUser| not defined. New code will assign 0 in the latter case, instead of
-1.

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/destination_search.js:818: if
(this.invitation_)
Can this ever be null/undefined here? Can we assert instead of adding the if
check?

[rbpotter, 2017-05-08 23:19:42]

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

[rbpotter, 2017-05-08 23:28:20]

Addressed comments. Making the printPreview variable local to
the anonymous function caused some additional compile errors
and broke the browser tests, so there are some new changes in
this patchset to fix those issues.

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/preview_generator.js (right):

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/preview_generator.js:105: * @type
{Array<Object<{from: number, to: number}>>}
On 2017/05/08 20:40:10, dpapad wrote:
> Shouldn't this be as follows?
> ?Array<{from: number, to: number}>
> 
> (see
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/p...).

Done.

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/previewarea/margin_control.js
(right):

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/previewarea/margin_control.js:330: if
(this.textbox_) {
On 2017/05/08 20:40:10, dpapad wrote:
> The previous code assumed that this.textbox_ was always populated by the time
> this function executes, and if not, an error would be thrown in the console,
> probably from [1]. The new code would silently do nothing, instead of letting
> the code blow up to surface the programmer's error.
> 
> Can we assert() instead of using the if (..) check, to keep previous and new
> code equivalent?
>  
> 
> [1]
>
https://cs.chromium.org/chromium/src/ui/webui/resources/js/event_tracker.js?l=57

Done.

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

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/print_preview.js:1354: var printPreview;
On 2017/05/08 20:40:10, dpapad wrote:
> Can this be moved inside the anonymous function below?
> 
> var printPreview = new print_preview.PrintPreview();
> printPreview.initialize();

Done. FYI, This triggered a bunch of new errors from some enums,
which is why there are some additional changes in the new
patchset. Also caused problems for the browser tests that were
relying on the global variable, so I added the boolean above and
check in this function to fix them. Let me know if this was not
a correct approach.

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/search/destination_list_item.js
(right):

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/destination_list_item.js:322: *
@param {Event} e The event to handle.
On 2017/05/08 20:40:10, dpapad wrote:
> !Event

This function has to have @param {Event} as the input, since it is assigned to
the onclick event of extensionIconEl, which requires  (function ((Event|null)):
?|null). See
https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/s...

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/destination_list_item.js:337: if
(e && hasKeyModifiers(e))
On 2017/05/08 20:40:10, dpapad wrote:
> Can |e| be ever null? If not let's change the annotation to !Event, and remove
> the extra condition.

Same as above for this function - assigned to onkeydown here:
https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/s...
Given that the assignment requires it accept a null argument I assume e can be
null, though not sure what would cause that.

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/search/destination_search.js
(right):

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/destination_search.js:558:
accountSelectEl.selectedIndex = this.userInfo_.activeUser ?
On 2017/05/08 20:40:10, dpapad wrote:
> Is the logic here equivalent to previous logic?
> 
> Previous code would assign -1 to |selectedIndex| if |activeUser| or if
> |activeUser| not defined. New code will assign 0 in the latter case, instead
of
> -1.

Done.

https://codereview.chromium.org/2865633004/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/destination_search.js:818: if
(this.invitation_)
On 2017/05/08 20:40:10, dpapad wrote:
> Can this ever be null/undefined here? Can we assert instead of adding the if
> check?

I don't think so, as the buttons that trigger it are hidden until
showInvitation_() is called and this.invitation_ is non-null. Changed
to an assert.

[commit-bot: I haz the power, 2017-05-08 23:35:40]

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-08 23:39:49]

https://codereview.chromium.org/2865633004/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/print_preview.js (right):

https://codereview.chromium.org/2865633004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview.js:89: print_preview.IsWebUITest = true;
We are adding a DOMContentLoaded handler here, and also one in print_preview
prod code, and we rely on the order that the two listeners execute (first one
sets a boolean, second one reads the boolean).

I think this is a bit fragile. Probably better to undo this change for now and
just leave the global "var printPriview" variable in the prod code and add a
comment that the reference is used by tests.

[dpapad, 2017-05-08 23:50:30]

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

https://codereview.chromium.org/2865633004/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/more_settings.js:61: ALL;
Can this fit in previous line?

[rbpotter, 2017-05-09 00:08:41]

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

[rbpotter, 2017-05-09 00:08:52]

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

https://codereview.chromium.org/2865633004/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/more_settings.js:61: ALL;
On 2017/05/08 23:50:30, dpapad wrote:
> Can this fit in previous line?

Done.

https://codereview.chromium.org/2865633004/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/print_preview.js (right):

https://codereview.chromium.org/2865633004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview.js:89: print_preview.IsWebUITest = true;
On 2017/05/08 23:39:48, dpapad wrote:
> We are adding a DOMContentLoaded handler here, and also one in print_preview
> prod code, and we rely on the order that the two listeners execute (first one
> sets a boolean, second one reads the boolean).
> 
> I think this is a bit fragile. Probably better to undo this change for now and
> just leave the global "var printPriview" variable in the prod code and add a
> comment that the reference is used by tests.

Done.

[commit-bot: I haz the power, 2017-05-09 00:18:59]

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-09 01:18:08]

LGTM

https://codereview.chromium.org/2865633004/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/print_preview.js (right):

https://codereview.chromium.org/2865633004/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview.js:122: 
Nit: Maybe revert changes in this file for now?

[commit-bot: I haz the power, 2017-05-09 01:26:12]

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

[commit-bot: I haz the power, 2017-05-09 01:26:13]

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_...)

[rbpotter, 2017-05-09 01:31:20]

Patchset #6 (id:100001) has been deleted

[rbpotter, 2017-05-09 01:35:23]

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

[commit-bot: I haz the power, 2017-05-09 01:36:08]

Dry run: CQ is trying da patch.

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

[rbpotter, 2017-05-09 01:36:36]

Also uncommented line in compiled_resources2

https://codereview.chromium.org/2865633004/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/print_preview.js (right):

https://codereview.chromium.org/2865633004/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview.js:122: 
On 2017/05/09 01:18:07, dpapad wrote:
> Nit: Maybe revert changes in this file for now?

Done.

[commit-bot: I haz the power, 2017-05-09 04:00:44]

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

[commit-bot: I haz the power, 2017-05-09 04:00:45]

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-05-09 17:04:50]

On 2017/05/09 at 01:36:36, rbpotter wrote:
> Also uncommented line in compiled_resources2
Thanks.

> 
>
https://codereview.chromium.org/2865633004/diff/80001/chrome/test/data/webui/...
> File chrome/test/data/webui/print_preview.js (right):
> 
>
https://codereview.chromium.org/2865633004/diff/80001/chrome/test/data/webui/...
> chrome/test/data/webui/print_preview.js:122: 
> On 2017/05/09 01:18:07, dpapad wrote:
> > Nit: Maybe revert changes in this file for now?
> 
> Done.

Still LGTM.

[rbpotter, 2017-05-09 20:38:27]

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

[rbpotter, 2017-05-09 20:40:52]

On 2017/05/09 17:04:50, dpapad wrote:
> On 2017/05/09 at 01:36:36, rbpotter wrote:
> > Also uncommented line in compiled_resources2
> Thanks.
> 
> > 
> >
>
https://codereview.chromium.org/2865633004/diff/80001/chrome/test/data/webui/...
> > File chrome/test/data/webui/print_preview.js (right):
> > 
> >
>
https://codereview.chromium.org/2865633004/diff/80001/chrome/test/data/webui/...
> > chrome/test/data/webui/print_preview.js:122: 
> > On 2017/05/09 01:18:07, dpapad wrote:
> > > Nit: Maybe revert changes in this file for now?
> > 
> > Done.
> 
> Still LGTM.

+dbeam for third_party/closure_compiler/compiled_resources2.gyp

[Dan Beam, 2017-05-09 21:25:48]

ultimately the third_party/closure_compiler stuff lgtm (as you're just
uncommenting a file) but had other comments

thanks generally for getting print_preview working!

https://codereview.chromium.org/2865633004/diff/140001/chrome/browser/resourc...
File
chrome/browser/resources/print_preview/previewarea/margin_control_container.js
(right):

https://codereview.chromium.org/2865633004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/print_preview/previewarea/margin_control_container.js:371:
* controls.
nit: @param

https://codereview.chromium.org/2865633004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/previewarea/preview_area.js (right):

https://codereview.chromium.org/2865633004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/print_preview/previewarea/preview_area.js:5: //
<include ../../pdf/pdf_scripting_api.js>
shouldn't this be

<include src="../../pdf/pdf_scripting_api.js">

?

https://codereview.chromium.org/2865633004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/search/destination_list_item.js
(right):

https://codereview.chromium.org/2865633004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/print_preview/search/destination_list_item.js:294: if
(e && !hasKeyModifiers(e)) {
is this to avoid requiring !KeyboardEvent as the type?

https://codereview.chromium.org/2865633004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/search/destination_search.js
(right):

https://codereview.chromium.org/2865633004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/print_preview/search/destination_search.js:681:
destination.id);
nit: indent

[rbpotter, 2017-05-09 22:29:03]

Patchset #8 (id:160001) has been deleted

[rbpotter, 2017-05-09 22:31:39]

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

[rbpotter, 2017-05-09 22:32:14]

https://codereview.chromium.org/2865633004/diff/140001/chrome/browser/resourc...
File
chrome/browser/resources/print_preview/previewarea/margin_control_container.js
(right):

https://codereview.chromium.org/2865633004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/print_preview/previewarea/margin_control_container.js:371:
* controls.
On 2017/05/09 21:25:47, Dan Beam wrote:
> nit: @param 

Done.

https://codereview.chromium.org/2865633004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/previewarea/preview_area.js (right):

https://codereview.chromium.org/2865633004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/print_preview/previewarea/preview_area.js:5: //
<include ../../pdf/pdf_scripting_api.js>
On 2017/05/09 21:25:47, Dan Beam wrote:
> shouldn't this be
> 
> <include src="../../pdf/pdf_scripting_api.js">
> 
> ?

This was actually left in by mistake and is not needed.
Have removed it.

https://codereview.chromium.org/2865633004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/search/destination_list_item.js
(right):

https://codereview.chromium.org/2865633004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/print_preview/search/destination_list_item.js:294: if
(e && !hasKeyModifiers(e)) {
On 2017/05/09 21:25:47, Dan Beam wrote:
> is this to avoid requiring !KeyboardEvent as the type?

Yes, changed to !KeyboardEvent.

https://codereview.chromium.org/2865633004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/search/destination_search.js
(right):

https://codereview.chromium.org/2865633004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/print_preview/search/destination_search.js:681:
destination.id);
On 2017/05/09 21:25:48, Dan Beam wrote:
> nit: indent

Done.

[commit-bot: I haz the power, 2017-05-09 22:33:44]

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, 2017-05-09 23:36:32]

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

[commit-bot: I haz the power, 2017-05-09 23:36:33]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Dan Beam, 2017-05-10 00:00:14]

still lgtm

https://codereview.chromium.org/2865633004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/pdf/pdf_scripting_api.js (right):

https://codereview.chromium.org/2865633004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/pdf/pdf_scripting_api.js:75: */ (event.data);
nit: this is prettier, imo

/**
 * @type {{
 *   pageX: number,
 *   pageY: number,
 *   pageWidth: number,
 *   viewportWidth: number,
 *   viewportHeight: number
 * }}
 */
var viewportData = event.data;

https://codereview.chromium.org/2865633004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/search/destination_list_item.js
(right):

https://codereview.chromium.org/2865633004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/print_preview/search/destination_list_item.js:337: if
(e && hasKeyModifiers(e))
same nit regarding !Event vs e && (just to type filter out null)

[rbpotter, 2017-05-10 01:44:30]

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

[commit-bot: I haz the power, 2017-05-10 01:45:35]

Dry run: CQ is trying da patch.

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

[rbpotter, 2017-05-10 01:56:23]

https://codereview.chromium.org/2865633004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/pdf/pdf_scripting_api.js (right):

https://codereview.chromium.org/2865633004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/pdf/pdf_scripting_api.js:75: */ (event.data);
On 2017/05/10 00:00:14, Dan Beam wrote:
> nit: this is prettier, imo
> 
> /**
>  * @type {{
>  *   pageX: number,
>  *   pageY: number,
>  *   pageWidth: number,
>  *   viewportWidth: number,
>  *   viewportHeight: number
>  * }}
>  */
> var viewportData = event.data;

Done.

https://codereview.chromium.org/2865633004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/search/destination_list_item.js
(right):

https://codereview.chromium.org/2865633004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/print_preview/search/destination_list_item.js:337: if
(e && hasKeyModifiers(e))
On 2017/05/10 00:00:14, Dan Beam wrote:
> same nit regarding !Event vs e && (just to type filter out null)

Does not work here as this gets assigned to extensionIconEl.keydown,
which requires (function ((Event|null)): ?|null).

Changed to an assert instead, let me know if it should be something
different.

[dpapad, 2017-05-10 02:54:12]

https://codereview.chromium.org/2865633004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/search/destination_list_item.js
(right):

https://codereview.chromium.org/2865633004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/print_preview/search/destination_list_item.js:337: if
(e && hasKeyModifiers(e))
On 2017/05/10 at 01:56:23, rbpotter wrote:
> On 2017/05/10 00:00:14, Dan Beam wrote:
> > same nit regarding !Event vs e && (just to type filter out null)
> 
> Does not work here as this gets assigned to extensionIconEl.keydown,
> which requires (function ((Event|null)): ?|null).
> 
> Changed to an assert instead, let me know if it should be something
> different.

Nit(optional): It seems that onkeydown,onclick are erroneously annotated with
Event instead of !Event, see [1]. Perhaps changing the code to use
addEventListener('click', ...) instead would allow you to use @param{!Event}
here and remove the assertion.

[1]
https://github.com/google/closure-compiler/blob/02ee72c0f8248ffe6be9f9efd591f...
[2]
https://github.com/google/closure-compiler/blob/8ac08c03cd695b84f8a79ac3a1338...

[dpapad, 2017-05-10 02:59:20]

https://codereview.chromium.org/2865633004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/search/destination_list_item.js
(right):

https://codereview.chromium.org/2865633004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/print_preview/search/destination_list_item.js:337: if
(e && hasKeyModifiers(e))
> Nit(optional): It seems that onkeydown,onclick are erroneously annotated with
Event instead of !Event, see [1]. Perhaps changing the code to use
addEventListener('click', ...) instead would allow you to use @param{!Event}
here and remove the assertion.
> 
> [1]
https://github.com/google/closure-compiler/blob/02ee72c0f8248ffe6be9f9efd591f...
> [2]
https://github.com/google/closure-compiler/blob/8ac08c03cd695b84f8a79ac3a1338...

Actually you should probably not follow my previous suggestion, because calling
addEventListener() and using onclick, onkeydown are not equivalent here.

addEventListener() adds a new listener every time it is called, whereas onclick
= ..., replaces the existing listener. The enclosing code [1] seems to be
executed multiple times, so you don't want to add multiple listeners every time
is called.

[1]
https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/s...

[Dan Beam, 2017-05-10 03:18:35]

https://codereview.chromium.org/2865633004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/search/destination_list_item.js
(right):

https://codereview.chromium.org/2865633004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/print_preview/search/destination_list_item.js:337: if
(e && hasKeyModifiers(e))
On 2017/05/10 02:59:20, dpapad wrote:
> > Nit(optional): It seems that onkeydown,onclick are erroneously annotated
with
> Event instead of !Event, see [1]. Perhaps changing the code to use
> addEventListener('click', ...) instead would allow you to use @param{!Event}
> here and remove the assertion.
> > 
> > [1]
>
https://github.com/google/closure-compiler/blob/02ee72c0f8248ffe6be9f9efd591f...
> > [2]
>
https://github.com/google/closure-compiler/blob/8ac08c03cd695b84f8a79ac3a1338...
> 
> Actually you should probably not follow my previous suggestion, because
calling
> addEventListener() and using onclick, onkeydown are not equivalent here.
> 
> addEventListener() adds a new listener every time it is called, whereas
onclick
> = ..., replaces the existing listener. The enclosing code [1] seems to be
> executed multiple times, so you don't want to add multiple listeners every
time
> is called.
> 
> [1]
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/s...

this works for me 
http://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClos...

[rbpotter, 2017-05-10 03:36:58]

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

[commit-bot: I haz the power, 2017-05-10 03:37:43]

Dry run: CQ is trying da patch.

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

[rbpotter, 2017-05-10 03:42:11]

https://codereview.chromium.org/2865633004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/search/destination_list_item.js
(right):

https://codereview.chromium.org/2865633004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/print_preview/search/destination_list_item.js:337: if
(e && hasKeyModifiers(e))
On 2017/05/10 03:18:35, Dan Beam wrote:
> On 2017/05/10 02:59:20, dpapad wrote:
> > > Nit(optional): It seems that onkeydown,onclick are erroneously annotated
> with
> > Event instead of !Event, see [1]. Perhaps changing the code to use
> > addEventListener('click', ...) instead would allow you to use @param{!Event}
> > here and remove the assertion.
> > > 
> > > [1]
> >
>
https://github.com/google/closure-compiler/blob/02ee72c0f8248ffe6be9f9efd591f...
> > > [2]
> >
>
https://github.com/google/closure-compiler/blob/8ac08c03cd695b84f8a79ac3a1338...
> > 
> > Actually you should probably not follow my previous suggestion, because
> calling
> > addEventListener() and using onclick, onkeydown are not equivalent here.
> > 
> > addEventListener() adds a new listener every time it is called, whereas
> onclick
> > = ..., replaces the existing listener. The enclosing code [1] seems to be
> > executed multiple times, so you don't want to add multiple listeners every
> time
> > is called.
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/s...
> 
> this works for me 
>
http://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClos...

Done. Should I change it to !KeyboardEvent if we are going to cast the
function anyway? (was KeyboardEvent before this CL)

[commit-bot: I haz the power, 2017-05-10 07:21:41]

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

[commit-bot: I haz the power, 2017-05-10 07:21:42]

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_...)

[rbpotter, 2017-05-10 19:36:17]

The CQ bit was checked by rbpotter@chromium.org

[rbpotter, 2017-05-10 19:36:17]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org,
dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2865633004/#ps220001
(title: "Fix onkeydown function")

[commit-bot: I haz the power, 2017-05-10 19:38:28]

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-10 19:51:12]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1494444977256640,
"parent_rev": "6aa839b75ec10cf23591c1dfc2f9b4b910102a8c", "commit_rev":
"419bda2ce75c18c90e66fae1d03fdd1e5479ce04"}

[commit-bot: I haz the power, 2017-05-10 19:51:23]

Description was changed from

==========
Fix all remaining print preview closure compiler errors

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

to

==========
Fix all remaining print preview closure compiler errors

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

Review-Url: https://codereview.chromium.org/2865633004
Cr-Commit-Position: refs/heads/master@{#470677}
Committed:
https://chromium.googlesource.com/chromium/src/+/419bda2ce75c18c90e66fae1d03f...
==========

[commit-bot: I haz the power, 2017-05-10 19:51:25]

Committed patchset #10 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/419bda2ce75c18c90e66fae1d03f...