Compile print_preview, part 2: reduce down to 260 errors

Compile print_preview, part 2: reduce down to 260 errors

R=dpapad@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/89e4fb7cebee5cdf71a43941b2e7010fe4462f66
Cr-Commit-Position: refs/heads/master@{#297101}

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

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

[Dan Beam, 2014-09-18 17:42:09]

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

[Dan Beam, 2014-09-18 17:42:09]

-dbeam, +dpapad

[Dan Beam, 2014-09-18 17:42:17]

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

[Vitaly Pavlenko, 2014-09-19 00:25:52]

vitalyp@chromium.org changed reviewers:
+ thestig@chromium.org
- dpapad@chromium.org

[Vitaly Pavlenko, 2014-09-19 00:26:07]

-dpapad, +thestig

[Lei Zhang, 2014-09-19 00:37:35]

thestig@chromium.org changed reviewers:
+ alekseys@chromium.org

[Lei Zhang, 2014-09-19 00:54:21]

lgtm unless Aleksey has any concerns.

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
chrome/browser/resources/print_preview/data/destination_store.js:274: return
!!this.cloudPrintInterface_ &&
Is there a preference between !!foo vs. foo != null ?

[Vitaly Pavlenko, 2014-09-19 00:57:59]

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
chrome/browser/resources/print_preview/data/destination_store.js:274: return
!!this.cloudPrintInterface_ &&
On 2014/09/19 00:54:20, Lei Zhang wrote:
> Is there a preference between !!foo vs. foo != null ?

I just insert the general !! cast in all cases. I don't know if it's a good idea
to do so here. Aleksey, what do you think?

[Aleksey Shlyapnikov, 2014-09-19 18:54:43]

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
File chrome/browser/resources/print_preview/component.js (right):

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
chrome/browser/resources/print_preview/component.js:157: * @return
{!HTMLElement} Element selected by the given query.
Why? This is a generic function and it's not guaranteed that the element we are
looking for is there.

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
File chrome/browser/resources/print_preview/data/app_state.js (right):

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
chrome/browser/resources/print_preview/data/app_state.js:220: };
What's the reason for this and other similar changes?

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
chrome/browser/resources/print_preview/data/destination_store.js:274: return
!!this.cloudPrintInterface_ &&
On 2014/09/19 00:57:59, Vitaly Pavlenko wrote:
> On 2014/09/19 00:54:20, Lei Zhang wrote:
> > Is there a preference between !!foo vs. foo != null ?
> 
> I just insert the general !! cast in all cases. I don't know if it's a good
idea
> to do so here. Aleksey, what do you think?

This is a pointer check, why would we convert it to boolean? Same semantics,
more code. Let's leave it as it was.

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
File chrome/browser/resources/print_preview/print_preview_utils.js (right):

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
chrome/browser/resources/print_preview/print_preview_utils.js:96: * @param
{?number} totalPageCount The total number of pages.
Maybe {number=} makes more sense here?

[Vitaly Pavlenko, 2014-09-19 20:33:09]

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
File chrome/browser/resources/print_preview/component.js (right):

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
chrome/browser/resources/print_preview/component.js:157: * @return
{!HTMLElement} Element selected by the given query.
On 2014/09/19 18:54:42, Aleksey Shlyapnikov wrote:
> Why? This is a generic function and it's not guaranteed that the element we
are
> looking for is there.

I tried to add this assert, compile the browser and play around with
print_preview. I haven't found any failures on this assert.

Do you think that instead I should add assert() around all calls to
getChildElement()?

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
File chrome/browser/resources/print_preview/data/app_state.js (right):

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
chrome/browser/resources/print_preview/data/app_state.js:220: };
On 2014/09/19 18:54:42, Aleksey Shlyapnikov wrote:
> What's the reason for this and other similar changes?

Right now Closure Compiler doesn't understand @typedefs and @enums if they are
placed inside function scope:
https://github.com/google/closure-compiler/issues/544

So I need to move them to a global scope to make the typechecking work.

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
chrome/browser/resources/print_preview/data/destination_store.js:274: return
!!this.cloudPrintInterface_ &&
On 2014/09/19 18:54:42, Aleksey Shlyapnikov wrote:
> On 2014/09/19 00:57:59, Vitaly Pavlenko wrote:
> > On 2014/09/19 00:54:20, Lei Zhang wrote:
> > > Is there a preference between !!foo vs. foo != null ?
> > 
> > I just insert the general !! cast in all cases. I don't know if it's a good
> idea
> > to do so here. Aleksey, what do you think?
> 
> This is a pointer check, why would we convert it to boolean? Same semantics,
> more code. Let's leave it as it was.

Otherwise Closure Compiler whines that we possibly return non-boolean from
function that is supposed to return boolean type.

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
File chrome/browser/resources/print_preview/print_preview_utils.js (right):

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
chrome/browser/resources/print_preview/print_preview_utils.js:96: * @param
{?number} totalPageCount The total number of pages.
On 2014/09/19 18:54:42, Aleksey Shlyapnikov wrote:
> Maybe {number=} makes more sense here?

You are right. Done.

[Aleksey Shlyapnikov, 2014-09-19 20:48:59]

lgtm

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
File chrome/browser/resources/print_preview/component.js (right):

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
chrome/browser/resources/print_preview/component.js:157: * @return
{!HTMLElement} Element selected by the given query.
On 2014/09/19 20:33:09, Vitaly Pavlenko wrote:
> On 2014/09/19 18:54:42, Aleksey Shlyapnikov wrote:
> > Why? This is a generic function and it's not guaranteed that the element we
> are
> > looking for is there.
> 
> I tried to add this assert, compile the browser and play around with
> print_preview. I haven't found any failures on this assert.
> 
> Do you think that instead I should add assert() around all calls to
> getChildElement()?

So with no assert and @return {HTMLElement} declaration Closure compiler
complains on every getChildElement's call site?

Can you leave a TODO here (on my name) to check all call sites and rename this
function to something like getRequiredChildElement?

[Vitaly Pavlenko, 2014-09-19 21:12:54]

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
File chrome/browser/resources/print_preview/component.js (right):

https://chromiumcodereview.appspot.com/575333002/diff/1/chrome/browser/resour...
chrome/browser/resources/print_preview/component.js:157: * @return
{!HTMLElement} Element selected by the given query.
On 2014/09/19 20:48:59, Aleksey Shlyapnikov wrote:
> On 2014/09/19 20:33:09, Vitaly Pavlenko wrote:
> > On 2014/09/19 18:54:42, Aleksey Shlyapnikov wrote:
> > > Why? This is a generic function and it's not guaranteed that the element
we
> > are
> > > looking for is there.
> > 
> > I tried to add this assert, compile the browser and play around with
> > print_preview. I haven't found any failures on this assert.
> > 
> > Do you think that instead I should add assert() around all calls to
> > getChildElement()?
> 
> So with no assert and @return {HTMLElement} declaration Closure compiler
> complains on every getChildElement's call site?
> 
> Can you leave a TODO here (on my name) to check all call sites and rename this
> function to something like getRequiredChildElement?

Yes, without this assert you get 40 errors like this:
##
/usr/local/google/home/vitalyp/src/chrome/browser/resources/print_preview/search/destination_search.js:441:
ERROR - actual parameter 1 of setIsVisible does not match formal parameter
## found   : (HTMLElement|null)
## required: HTMLElement
##       setIsVisible(this.getChildElement('.cloud-list'), loggedIn);
##                    ^

TODO is inserted.

[Vitaly Pavlenko, 2014-09-22 22:47:46]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-22 22:50:53]

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

[commit-bot: I haz the power, 2014-09-22 22:55:42]

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

[commit-bot: I haz the power, 2014-09-22 22:55:45]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/59230)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Vitaly Pavlenko, 2014-09-22 23:59:20]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-23 00:00:42]

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

[commit-bot: I haz the power, 2014-09-23 00:15:05]

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

[commit-bot: I haz the power, 2014-09-23 00:15:06]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Vitaly Pavlenko, 2014-09-23 00:27:09]

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

[Vitaly Pavlenko, 2014-09-23 00:27:31]

On 2014/09/23 00:15:06, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

dbeam@, please, review ui/webui/ part.

[Dan Beam, 2014-09-23 00:46:34]

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

https://codereview.chromium.org/575333002/diff/80001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/event_target.js:16: * @implements {EventTarget}
this is a confusing statement.  i assume this means that it implements
EventTarget provided by the browser, not itself?

https://codereview.chromium.org/575333002/diff/80001/ui/webui/resources/js/ev...
File ui/webui/resources/js/event_tracker.js (right):

https://codereview.chromium.org/575333002/diff/80001/ui/webui/resources/js/ev...
ui/webui/resources/js/event_tracker.js:43: * @param {!EventTarget} node The DOM
node to add a listener to.
shouldn't we rename to target everywhere?

[Vitaly Pavlenko, 2014-09-23 01:05:17]

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

https://codereview.chromium.org/575333002/diff/80001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/event_target.js:16: * @implements {EventTarget}
On 2014/09/23 00:46:34, Dan Beam wrote:
> this is a confusing statement.  i assume this means that it implements
> EventTarget provided by the browser, not itself?

Yes, all our constructors are visible to Closure compiler only by their
full-qualified names. Sometimes I need to add a constructor to the list of
exported entities just to use it inside JSDoc in the same file where it's
declared. So here it's definitely EventTarget coming from compiler externs.

https://codereview.chromium.org/575333002/diff/80001/ui/webui/resources/js/ev...
File ui/webui/resources/js/event_tracker.js (right):

https://codereview.chromium.org/575333002/diff/80001/ui/webui/resources/js/ev...
ui/webui/resources/js/event_tracker.js:43: * @param {!EventTarget} node The DOM
node to add a listener to.
On 2014/09/23 00:46:34, Dan Beam wrote:
> shouldn't we rename to target everywhere?

Done inside this function. Or you mean everywhere in the file?

[Dan Beam, 2014-09-23 01:42:28]

https://codereview.chromium.org/575333002/diff/100001/ui/webui/resources/js/e...
File ui/webui/resources/js/event_tracker.js (right):

https://codereview.chromium.org/575333002/diff/100001/ui/webui/resources/js/e...
ui/webui/resources/js/event_tracker.js:51: node: target,
does this property make much sense to you?

[Vitaly Pavlenko, 2014-09-23 19:30:12]

https://codereview.chromium.org/575333002/diff/100001/ui/webui/resources/js/e...
File ui/webui/resources/js/event_tracker.js (right):

https://codereview.chromium.org/575333002/diff/100001/ui/webui/resources/js/e...
ui/webui/resources/js/event_tracker.js:51: node: target,
On 2014/09/23 01:42:27, Dan Beam wrote:
> does this property make much sense to you?

Done.

[Dan Beam, 2014-09-23 20:19:59]

ui/webui lgtm

https://codereview.chromium.org/575333002/diff/120001/ui/webui/resources/js/e...
File ui/webui/resources/js/event_tracker.js (right):

https://codereview.chromium.org/575333002/diff/120001/ui/webui/resources/js/e...
ui/webui/resources/js/event_tracker.js:42: * Add an event listener - replacement
for Node.addEventListener.
EventTarget.addEventListener,
https://developer.mozilla.org/en-US/docs/Web/API/EventTarget.addEventListener

[Vitaly Pavlenko, 2014-09-23 22:26:02]

https://codereview.chromium.org/575333002/diff/120001/ui/webui/resources/js/e...
File ui/webui/resources/js/event_tracker.js (right):

https://codereview.chromium.org/575333002/diff/120001/ui/webui/resources/js/e...
ui/webui/resources/js/event_tracker.js:42: * Add an event listener - replacement
for Node.addEventListener.
On 2014/09/23 20:19:59, Dan Beam wrote:
> EventTarget.addEventListener,
> https://developer.mozilla.org/en-US/docs/Web/API/EventTarget.addEventListener

Done.

[Vitaly Pavlenko, 2014-09-23 22:26:14]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-23 22:26:40]

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

[commit-bot: I haz the power, 2014-09-23 23:49:40]

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

[commit-bot: I haz the power, 2014-09-23 23:49:41]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Vitaly Pavlenko, 2014-09-27 02:12:55]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-27 02:13:22]

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

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

Committed patchset #9 (id:160001) as f8aa505c51b67195082bc17138eb916343c31c2f

[commit-bot: I haz the power, 2014-09-27 04:05:25]

Patchset 9 (id:??) landed as
https://crrev.com/89e4fb7cebee5cdf71a43941b2e7010fe4462f66
Cr-Commit-Position: refs/heads/master@{#297101}