Compile print_preview, part 5: reduce down to 104 errors

Compile print_preview, part 5: reduce down to 104 errors

R=alekseys@chromium.org
BUG=393873
TEST=GYP_GENERATORS=ninja gyp --depth . chrome/browser/resources/print_preview/compiled_resources.gyp && ninja -C out/Default

[Aleksey Shlyapnikov, 2014-09-24 17:54:43]

https://codereview.chromium.org/595153003/diff/1/chrome/browser/resources/pdf...
File chrome/browser/resources/pdf/pdf_scripting_api.js (right):

https://codereview.chromium.org/595153003/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/pdf_scripting_api.js:29: *         viewportHeight:
number}}
I think every field should be on its own line (just like the arguments, either
everything fits onto one line or to each its own line).

https://codereview.chromium.org/595153003/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/cloud_print_interface.js (right):

https://codereview.chromium.org/595153003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/cloud_print_interface.js:853:
CloudPrintRequest: CloudPrintRequest
Why are you exporting it? It's not used anywhere outside of this file.

https://codereview.chromium.org/595153003/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/preview_generator.js (right):

https://codereview.chromium.org/595153003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/preview_generator.js:62: * @type
{print_preview.ticket_items.MediaSize}
cp.cdd.MediaSizeTicketItem was a correct type, it is set to the value of this
ticket item, not to the ticket item itself.

[Vitaly Pavlenko, 2014-09-24 20:39:50]

cc: dbeam@ for CloudPrintRequest in cloud_print_interface.js:853 that's exported
only for closure compiler. Should we fix it in Compiler Pass?

https://codereview.chromium.org/595153003/diff/1/chrome/browser/resources/pdf...
File chrome/browser/resources/pdf/pdf_scripting_api.js (right):

https://codereview.chromium.org/595153003/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/pdf_scripting_api.js:29: *         viewportHeight:
number}}
On 2014/09/24 17:54:43, Aleksey Shlyapnikov wrote:
> I think every field should be on its own line (just like the arguments, either
> everything fits onto one line or to each its own line).

Done.

https://codereview.chromium.org/595153003/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/cloud_print_interface.js (right):

https://codereview.chromium.org/595153003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/cloud_print_interface.js:853:
CloudPrintRequest: CloudPrintRequest
On 2014/09/24 17:54:43, Aleksey Shlyapnikov wrote:
> Why are you exporting it? It's not used anywhere outside of this file.

Right now that's the only way to use the type cloudprint.CloudPrintRequest
inside this file inside JSDoc to make it visible for the compiler. It's ugly,
for sure. Is it acceptable to commit such a thing?

https://codereview.chromium.org/595153003/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/preview_generator.js (right):

https://codereview.chromium.org/595153003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/preview_generator.js:62: * @type
{print_preview.ticket_items.MediaSize}
On 2014/09/24 17:54:43, Aleksey Shlyapnikov wrote:
> cp.cdd.MediaSizeTicketItem was a correct type, it is set to the value of this
> ticket item, not to the ticket item itself.

Where is the type cp.cdd.MediaSizeTicketItem declared? I see no declaration in
the code base.

[Aleksey Shlyapnikov, 2014-09-24 21:13:18]

https://codereview.chromium.org/595153003/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/cloud_print_interface.js (right):

https://codereview.chromium.org/595153003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/cloud_print_interface.js:853:
CloudPrintRequest: CloudPrintRequest
On 2014/09/24 20:39:49, Vitaly Pavlenko wrote:
> On 2014/09/24 17:54:43, Aleksey Shlyapnikov wrote:
> > Why are you exporting it? It's not used anywhere outside of this file.
> 
> Right now that's the only way to use the type cloudprint.CloudPrintRequest
> inside this file inside JSDoc to make it visible for the compiler. It's ugly,
> for sure. Is it acceptable to commit such a thing?

If we have no workaround, then yes. Let's add a comment why we export it though.

https://codereview.chromium.org/595153003/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/preview_generator.js (right):

https://codereview.chromium.org/595153003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/preview_generator.js:62: * @type
{print_preview.ticket_items.MediaSize}
On 2014/09/24 20:39:50, Vitaly Pavlenko wrote:
> On 2014/09/24 17:54:43, Aleksey Shlyapnikov wrote:
> > cp.cdd.MediaSizeTicketItem was a correct type, it is set to the value of
this
> > ticket item, not to the ticket item itself.
> 
> Where is the type cp.cdd.MediaSizeTicketItem declared? I see no declaration in
> the code base.

Well, it is supposed to be generated from proto defs. Can you use your ValueType
here and move cp.cdd.MediaSizeTicketItem to comments?

[Vitaly Pavlenko, 2014-09-24 22:37:57]

https://codereview.chromium.org/595153003/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/cloud_print_interface.js (right):

https://codereview.chromium.org/595153003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/cloud_print_interface.js:853:
CloudPrintRequest: CloudPrintRequest
On 2014/09/24 21:13:17, Aleksey Shlyapnikov wrote:
> On 2014/09/24 20:39:49, Vitaly Pavlenko wrote:
> > On 2014/09/24 17:54:43, Aleksey Shlyapnikov wrote:
> > > Why are you exporting it? It's not used anywhere outside of this file.
> > 
> > Right now that's the only way to use the type cloudprint.CloudPrintRequest
> > inside this file inside JSDoc to make it visible for the compiler. It's
ugly,
> > for sure. Is it acceptable to commit such a thing?
> 
> If we have no workaround, then yes. Let's add a comment why we export it
though.

Done.

https://codereview.chromium.org/595153003/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/preview_generator.js (right):

https://codereview.chromium.org/595153003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/preview_generator.js:62: * @type
{print_preview.ticket_items.MediaSize}
On 2014/09/24 21:13:18, Aleksey Shlyapnikov wrote:
> On 2014/09/24 20:39:50, Vitaly Pavlenko wrote:
> > On 2014/09/24 17:54:43, Aleksey Shlyapnikov wrote:
> > > cp.cdd.MediaSizeTicketItem was a correct type, it is set to the value of
> this
> > > ticket item, not to the ticket item itself.
> > 
> > Where is the type cp.cdd.MediaSizeTicketItem declared? I see no declaration
in
> > the code base.
> 
> Well, it is supposed to be generated from proto defs. Can you use your
ValueType
> here and move cp.cdd.MediaSizeTicketItem to comments?

Done.

[Aleksey Shlyapnikov, 2014-09-24 22:47:57]

lgtm

[Dan Beam, 2014-09-24 23:31:20]

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

[Dan Beam, 2014-09-24 23:31:21]

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/print_preview/cloud_print_interface.js (right):

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
chrome/browser/resources/print_preview/cloud_print_interface.js:399: if
(account) {
:(

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
chrome/browser/resources/print_preview/cloud_print_interface.js:845: * @type
{string}
:(

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/print_preview/common/overlay.js (right):

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
chrome/browser/resources/print_preview/common/overlay.js:15: };
nit: remove ;

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/print_preview/print_preview_animations.js (right):

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
chrome/browser/resources/print_preview/print_preview_animations.js:78: *
@suppress {uselessCode}
i think this is supposed to be suspicious code, no?

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
chrome/browser/resources/print_preview/print_preview_animations.js:78: *
@suppress {uselessCode}
nit: just add a

  /** @supress {suspiciousCode} */
  el.offsetHeight;  // Should force an update of the computed style.

2\s + // comment

is a closure-style thing and this requires less lines and less of a change

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

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
chrome/browser/resources/print_preview/settings/other_options_settings.js:325:
(this.duplexTicketItem_.getValue());
nit: !!

[Aleksey Shlyapnikov, 2014-09-24 23:52:30]

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/print_preview/cloud_print_interface.js (right):

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
chrome/browser/resources/print_preview/cloud_print_interface.js:399: if
(account) {
On 2014/09/24 23:31:21, Dan Beam wrote:
> :(

Assert protects from null and undefined, this condition checks for empty string.

[Vitaly Pavlenko, 2014-09-24 23:57:07]

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/print_preview/cloud_print_interface.js (right):

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
chrome/browser/resources/print_preview/cloud_print_interface.js:399: if
(account) {
On 2014/09/24 23:31:21, Dan Beam wrote:
> :(

Done.

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
chrome/browser/resources/print_preview/cloud_print_interface.js:845: * @type
{string}
On 2014/09/24 23:31:20, Dan Beam wrote:
> :(

Done.

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/print_preview/common/overlay.js (right):

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
chrome/browser/resources/print_preview/common/overlay.js:15: };
On 2014/09/24 23:31:21, Dan Beam wrote:
> nit: remove ;

Done.

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/print_preview/print_preview_animations.js (right):

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
chrome/browser/resources/print_preview/print_preview_animations.js:78: *
@suppress {uselessCode}
On 2014/09/24 23:31:21, Dan Beam wrote:
> i think this is supposed to be suspicious code, no?

Either way works.

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
chrome/browser/resources/print_preview/print_preview_animations.js:78: *
@suppress {uselessCode}
On 2014/09/24 23:31:21, Dan Beam wrote:
> nit: just add a
> 
>   /** @supress {suspiciousCode} */
>   el.offsetHeight;  // Should force an update of the computed style.
> 
> 2\s + // comment
> 
> is a closure-style thing and this requires less lines and less of a change

Done.

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

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
chrome/browser/resources/print_preview/settings/other_options_settings.js:325:
(this.duplexTicketItem_.getValue());
On 2014/09/24 23:31:21, Dan Beam wrote:
> nit: !!

Done.

[Vitaly Pavlenko, 2014-09-24 23:58:45]

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/print_preview/cloud_print_interface.js (right):

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
chrome/browser/resources/print_preview/cloud_print_interface.js:399: if
(account) {
On 2014/09/24 23:52:30, Aleksey Shlyapnikov wrote:
> On 2014/09/24 23:31:21, Dan Beam wrote:
> > :(
> 
> Assert protects from null and undefined, this condition checks for empty
string.

I think that our assert also throws on an empty string:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...

[Aleksey Shlyapnikov, 2014-09-25 00:16:37]

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/print_preview/cloud_print_interface.js (right):

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
chrome/browser/resources/print_preview/cloud_print_interface.js:399: if
(account) {
On 2014/09/24 23:58:45, Vitaly Pavlenko wrote:
> On 2014/09/24 23:52:30, Aleksey Shlyapnikov wrote:
> > On 2014/09/24 23:31:21, Dan Beam wrote:
> > > :(
> > 
> > Assert protects from null and undefined, this condition checks for empty
> string.
> 
> I think that our assert also throws on an empty string:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...

Then you'll break this code. Please remove the assert.

[Vitaly Pavlenko, 2014-09-25 00:21:32]

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/print_preview/cloud_print_interface.js (right):

https://codereview.chromium.org/595153003/diff/20001/chrome/browser/resources...
chrome/browser/resources/print_preview/cloud_print_interface.js:399: if
(account) {
On 2014/09/25 00:16:36, Aleksey Shlyapnikov wrote:
> On 2014/09/24 23:58:45, Vitaly Pavlenko wrote:
> > On 2014/09/24 23:52:30, Aleksey Shlyapnikov wrote:
> > > On 2014/09/24 23:31:21, Dan Beam wrote:
> > > > :(
> > > 
> > > Assert protects from null and undefined, this condition checks for empty
> > string.
> > 
> > I think that our assert also throws on an empty string:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...
> 
> Then you'll break this code. Please remove the assert.

Done:
https://codereview.chromium.org/595153003/diff/60001/chrome/browser/resources...

[Dan Beam, 2014-09-29 22:13:41]

https://codereview.chromium.org/595153003/diff/80001/chrome/browser/resources...
File chrome/browser/resources/print_preview/cloud_print_interface.js (right):

https://codereview.chromium.org/595153003/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/cloud_print_interface.js:391: var
xsrfToken = account ? this.xsrfTokens_[account] : undefined;
nit: unless there's an account named "undefined", this.xsrfTokens_[account]
should work fine...

https://codereview.chromium.org/595153003/diff/80001/chrome/browser/resources...
File chrome/browser/resources/print_preview/preview_generator.js (right):

https://codereview.chromium.org/595153003/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/preview_generator.js:63: * defs.
why don't we have @externs for protos?

[Vitaly Buka (NO REVIEWS), 2014-10-04 05:54:14]

On 2014/09/29 22:13:41, Dan Beam wrote:
>
https://codereview.chromium.org/595153003/diff/80001/chrome/browser/resources...
> File chrome/browser/resources/print_preview/cloud_print_interface.js (right):
> 
>
https://codereview.chromium.org/595153003/diff/80001/chrome/browser/resources...
> chrome/browser/resources/print_preview/cloud_print_interface.js:391: var
> xsrfToken = account ? this.xsrfTokens_[account] : undefined;
> nit: unless there's an account named "undefined", this.xsrfTokens_[account]
> should work fine...
> 
>
https://codereview.chromium.org/595153003/diff/80001/chrome/browser/resources...
> File chrome/browser/resources/print_preview/preview_generator.js (right):
> 
>
https://codereview.chromium.org/595153003/diff/80001/chrome/browser/resources...
> chrome/browser/resources/print_preview/preview_generator.js:63: * defs.
> why don't we have @externs for protos?

Vitaly, we can try to finish this with Aleksey after his vacation.

[Dan Beam, 2016-12-06 04:11:15]

Description was changed from

==========
Compile print_preview, part 5: reduce down to 104 errors

R=alekseys@chromium.org
BUG=393873
TEST=GYP_GENERATORS=ninja gyp --depth .
chrome/browser/resources/print_preview/compiled_resources.gyp && ninja -C
out/Default
==========

to

==========
Compile print_preview, part 5: reduce down to 104 errors

R=alekseys@chromium.org
BUG=393873
TEST=GYP_GENERATORS=ninja gyp --depth .
chrome/browser/resources/print_preview/compiled_resources.gyp && ninja -C
out/Default
==========