Compile print_preview, part 3: reduce down to 185 errors

Compile print_preview, part 3: reduce down to 185 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

Committed: https://crrev.com/46e7f9bc4f4bd3ae4d473876db35dc5c225e25a6
Cr-Commit-Position: refs/heads/master@{#297592}

[Vitaly Pavlenko, 2014-09-22 17:23:37]

On 2014/09/19 20:59:18, Vitaly Pavlenko wrote:

ping alekseys@

[Aleksey Shlyapnikov, 2014-09-22 22:00:40]

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/data/app_state.js (right):

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/data/app_state.js:15: * @type
{Object.<string,print_preview.ValueType>}
Space after comma.

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/data/capabilities_holder.js (right):

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/data/capabilities_holder.js:15: * @type
{?print_preview.Cdd}
I thought objects are nullable by default, why do you need '?' here?

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/data/capabilities_holder.js:28: * @param
{?print_preview.Cdd} capabilities New instance to put into the
Why did you replace '!' with '?'? The idea was that you can only set it to valid
capabilities.

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/data/destination.js (right):

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/data/destination.js:101: * @private
{?print_preview.Cdd}
Same here, it's an object, right?

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/data/print_ticket_store.js (right):

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/data/print_ticket_store.js:362: /** @type
{!Object.<string,string>} */(this.appState_.getField(
Space after comma.

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

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:1164:
!this.appState_.isGcpPromoDismissed &&
It should be:
    !this.destinationStore_.isLocalDestinationSearchInProgress &&	
    !this.destinationStore_.isCloudDestinationSearchInProgress &&

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/search/destination_list_item.js
(right):

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/search/destination_list_item.js:40:
this.query_ = opt_query;
Can we have it this way:
@type {RegExp}
and
this.query_ = opt_query || null;

[Vitaly Pavlenko, 2014-09-22 22:33:11]

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/data/app_state.js (right):

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/data/app_state.js:15: * @type
{Object.<string,print_preview.ValueType>}
On 2014/09/22 22:00:40, Aleksey Shlyapnikov wrote:
> Space after comma.

Done.

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/data/capabilities_holder.js (right):

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/data/capabilities_holder.js:15: * @type
{?print_preview.Cdd}
On 2014/09/22 22:00:40, Aleksey Shlyapnikov wrote:
> I thought objects are nullable by default, why do you need '?' here?

Well, objects are, but typedefed objects aren't:
https://gist.github.com/vpavlenko/8845b2b4c06a2e3a22ba

tbreisacher@ from the compiler team thinks it's intentional and personally
recommends to put "?" or "!" every time.

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/data/capabilities_holder.js:28: * @param
{?print_preview.Cdd} capabilities New instance to put into the
On 2014/09/22 22:00:40, Aleksey Shlyapnikov wrote:
> Why did you replace '!' with '?'? The idea was that you can only set it to
valid
> capabilities.

Sorry about that, reverted.

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/data/destination.js (right):

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/data/destination.js:101: * @private
{?print_preview.Cdd}
On 2014/09/22 22:00:40, Aleksey Shlyapnikov wrote:
> Same here, it's an object, right?

Same answer.

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/data/print_ticket_store.js (right):

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/data/print_ticket_store.js:362: /** @type
{!Object.<string,string>} */(this.appState_.getField(
On 2014/09/22 22:00:40, Aleksey Shlyapnikov wrote:
> Space after comma.

Done.

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

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:1164:
!this.appState_.isGcpPromoDismissed &&
On 2014/09/22 22:00:40, Aleksey Shlyapnikov wrote:
> It should be:
>     !this.destinationStore_.isLocalDestinationSearchInProgress &&	
>     !this.destinationStore_.isCloudDestinationSearchInProgress &&

Done.

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/search/destination_list_item.js
(right):

https://codereview.chromium.org/588713002/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/search/destination_list_item.js:40:
this.query_ = opt_query;
On 2014/09/22 22:00:40, Aleksey Shlyapnikov wrote:
> Can we have it this way:
> @type {RegExp}
> and
> this.query_ = opt_query || null;

Done, with {?RegExp}.

[Aleksey Shlyapnikov, 2014-09-22 23:25:13]

lgtm

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

https://codereview.chromium.org/588713002/diff/40001/chrome/browser/resources...
chrome/browser/resources/print_preview/search/destination_list_item.js:37: *
@type {?RegExp}
RegExp is an object and it's not typedefed, right? Why ? then?

[Vitaly Pavlenko, 2014-09-23 00:11:30]

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

https://codereview.chromium.org/588713002/diff/40001/chrome/browser/resources...
chrome/browser/resources/print_preview/search/destination_list_item.js:37: *
@type {?RegExp}
On 2014/09/22 23:25:13, Aleksey Shlyapnikov wrote:
> RegExp is an object and it's not typedefed, right? Why ? then?

Oh, I was confused with @typedef stuff. You're right, we don't need it. Removed.

[Vitaly Pavlenko, 2014-09-27 04:47:01]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-27 04:47:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/588713002/80001

[commit-bot: I haz the power, 2014-09-27 04:47:58]

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

[commit-bot: I haz the power, 2014-09-27 04:47:58]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Vitaly Pavlenko, 2014-09-27 05:03:49]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-27 05:03:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/588713002/80001

[commit-bot: I haz the power, 2014-09-27 05:03:58]

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

[commit-bot: I haz the power, 2014-09-27 05:03:59]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Dan Beam, 2014-09-29 18:41:59]

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

[Dan Beam, 2014-09-29 18:42:01]

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

https://codereview.chromium.org/588713002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/component.js:159: *     something like
getRequiredChildElement.
either remove this ^ or figure out how to address querySelector() returning null

https://codereview.chromium.org/588713002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/print_preview/data/app_state.js (right):

https://codereview.chromium.org/588713002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/data/app_state.js:15: * @type
{Object.<string, print_preview.ValueType>}
are you sure this is used?  the templatized object types?

https://codereview.chromium.org/588713002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/data/app_state.js:84: * @return
{?print_preview.Destination.Origin.<string>} Origin of the
whhhhat?

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

https://codereview.chromium.org/588713002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/print_preview.js:868: <if
expr="toolkit_views">
arguably these <if>s should be 0-indented, e.g.

<if expr="toolkit_views">
        // On the toolkit_views environment, ESC key is handled by C++-side 
</if>

https://codereview.chromium.org/588713002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/print_preview.js:1004: event.settings =
nit: var settings (try not to re-assign to arguments or argument properties,
they can have special properties, even if it's identity assignment)

https://codereview.chromium.org/588713002/diff/80001/chrome/browser/resources...
File
chrome/browser/resources/print_preview/settings/advanced_settings/advanced_settings_item.js
(left):

https://codereview.chromium.org/588713002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/settings/advanced_settings/advanced_settings_item.js:258:
selectEl.add(item);
how did this work?

https://codereview.chromium.org/588713002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/print_preview/settings/media_size_settings.js
(right):

https://codereview.chromium.org/588713002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/settings/media_size_settings.js:85: /**
@type {function(string, boolean=): string} */
what is the point of this cast?

https://codereview.chromium.org/588713002/diff/80001/ui/webui/resources/js/cr...
File ui/webui/resources/js/cr/event_target.js (right):

https://codereview.chromium.org/588713002/diff/80001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/event_target.js:22: 
nit: remove \n here

https://codereview.chromium.org/588713002/diff/80001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/event_target.js:26: * @param
{EventListener|function(!Event):(boolean|undefined)} handler The
nit: typedef

https://codereview.chromium.org/588713002/diff/80001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/event_target.js:26: * @param
{EventListener|function(!Event):(boolean|undefined)} handler The
why boolean|undefined rather than just *?  a method that returns * can still be
compared to === false

[Vitaly Pavlenko, 2014-10-01 00:33:38]

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

https://codereview.chromium.org/588713002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/component.js:159: *     something like
getRequiredChildElement.
On 2014/09/29 18:42:00, Dan Beam wrote:
> either remove this ^ or figure out how to address querySelector() returning
null

Removed the assert for now.

https://codereview.chromium.org/588713002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/print_preview/data/app_state.js (right):

https://codereview.chromium.org/588713002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/data/app_state.js:15: * @type
{Object.<string, print_preview.ValueType>}
On 2014/09/29 18:42:00, Dan Beam wrote:
> are you sure this is used?  the templatized object types?

Removed the template parameters, as here they are trivial: <string, ?>

https://codereview.chromium.org/588713002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/data/app_state.js:84: * @return
{?print_preview.Destination.Origin.<string>} Origin of the
On 2014/09/29 18:42:00, Dan Beam wrote:
> whhhhat?

It means "the string that is a key in enum":

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

Or you asked something else?

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

https://codereview.chromium.org/588713002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/print_preview.js:868: <if
expr="toolkit_views">
On 2014/09/29 18:42:00, Dan Beam wrote:
> arguably these <if>s should be 0-indented, e.g.
> 
> <if expr="toolkit_views">
>         // On the toolkit_views environment, ESC key is handled by C++-side 
> </if>

Done.

https://codereview.chromium.org/588713002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/print_preview.js:1004: event.settings =
On 2014/09/29 18:42:00, Dan Beam wrote:
> nit: var settings (try not to re-assign to arguments or argument properties,
> they can have special properties, even if it's identity assignment)

Done.

https://codereview.chromium.org/588713002/diff/80001/chrome/browser/resources...
File
chrome/browser/resources/print_preview/settings/advanced_settings/advanced_settings_item.js
(left):

https://codereview.chromium.org/588713002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/settings/advanced_settings/advanced_settings_item.js:258:
selectEl.add(item);
On 2014/09/29 18:42:00, Dan Beam wrote:
> how did this work?

As I understand, HTMLSelectElement.prototype.add() with 1 argument equals to
Node.appendChild(). MDN doesn't tell it cleanly, but WebKit source seems to be:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/588713002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/print_preview/settings/media_size_settings.js
(right):

https://codereview.chromium.org/588713002/diff/80001/chrome/browser/resources...
chrome/browser/resources/print_preview/settings/media_size_settings.js:85: /**
@type {function(string, boolean=): string} */
On 2014/09/29 18:42:00, Dan Beam wrote:
> what is the point of this cast?

Otherwise at line 93 we get the following error:

##
/usr/local/google/home/vitalyp/src/chrome/browser/resources/print_preview/settings/media_size_settings.js:93:
ERROR - Function getLocaleToCompare: called with 1 argument(s). Function
requires at least 2 argument(s) and no more than 2 argument(s).
##                 if (getLocaleToCompare(items[i].locale) == locale)
##                     ^

https://codereview.chromium.org/588713002/diff/80001/ui/webui/resources/js/cr...
File ui/webui/resources/js/cr/event_target.js (right):

https://codereview.chromium.org/588713002/diff/80001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/event_target.js:22: 
On 2014/09/29 18:42:00, Dan Beam wrote:
> nit: remove \n here

Done.

https://codereview.chromium.org/588713002/diff/80001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/event_target.js:26: * @param
{EventListener|function(!Event):(boolean|undefined)} handler The
On 2014/09/29 18:42:00, Dan Beam wrote:
> nit: typedef

Done.

https://codereview.chromium.org/588713002/diff/80001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/event_target.js:26: * @param
{EventListener|function(!Event):(boolean|undefined)} handler The
On 2014/09/29 18:42:01, Dan Beam wrote:
> why boolean|undefined rather than just *?  a method that returns * can still
be
> compared to === false

Done.

[Dan Beam, 2014-10-01 00:37:45]

lgtm

[Vitaly Pavlenko, 2014-10-01 00:42:49]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-10-01 00:45:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/588713002/100001

[Vitaly Pavlenko, 2014-10-01 02:22:35]

The CQ bit was unchecked by vitalyp@chromium.org

[Vitaly Pavlenko, 2014-10-01 02:51:17]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-10-01 02:51:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/588713002/120001

[commit-bot: I haz the power, 2014-10-01 03:44:24]

Committed patchset #7 (id:120001) as cd56704315846959d0c90d72e4f37fa2a27ff4d2

[commit-bot: I haz the power, 2014-10-01 03:48:26]

Patchset 7 (id:??) landed as
https://crrev.com/46e7f9bc4f4bd3ae4d473876db35dc5c225e25a6
Cr-Commit-Position: refs/heads/master@{#297592}