Typecheck some of ui/webui/resources/js/ with Closure compiler.

Typecheck some of ui/webui/resources/js/ with Closure compiler.

Also adds:

  assertNotReached("message");

as a rough equivalent of

  NOTREACHED() << "message";

in Chromium and:

  ASSERT_NOT_REACHED();

in blink.

R=arv@chromium.org
BUG=393873
TEST=gyp --depth . ui/webui/resources/js/compiled_resources.gyp ui/webui/resources/js/chromeos/compiled_resources.gyp && ninja -C out/Default/

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287403

[Dan Beam, 2014-07-24 04:09:49]

+arv@ for review, +vitalyp@ fyi

[arv (Not doing code reviews), 2014-07-24 16:44:24]

Nice work. A few issues...

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/a...
File ui/webui/resources/js/assert.js (right):

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/a...
ui/webui/resources/js/assert.js:30: function notReached(opt_message) {
How about assertNotReached instead?

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/c...
File ui/webui/resources/js/chromeos/ui_account_tweaks.js (right):

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/c...
ui/webui/resources/js/chromeos/ui_account_tweaks.js:112: * @param {!Node} node
Root node of DOM subtree that should be disabled.
!Element was more correct. Here is a case where the type system is doing us a
disservice.

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/c...
ui/webui/resources/js/chromeos/ui_account_tweaks.js:142:
node.classList.add(sessionType + '-disabled');
Node does not have classList property. It is a property of Element so this
should cause a type error.

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/c...
ui/webui/resources/js/chromeos/ui_account_tweaks.js:144: if
(/^(INPUT|SELECT|BUTTON)$/.test(node.nodeName))
If you are refactoring this a switch would be the cleanest (and fastest).

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/c...
ui/webui/resources/js/chromeos/ui_account_tweaks.js:145: node.disabled = true;
Node does not have a disabled property. Is the type checker not working or are
the externs just wrong?

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr.js (right):

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/c...
ui/webui/resources/js/cr.js:15: var cr = (function() {
There was a reason someone changed this... maybe something related to
snapshotting?

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/c...
ui/webui/resources/js/cr.js:100: * @return {Function} The getter for the
property.
Function is less precise than function() : *

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/c...
ui/webui/resources/js/cr.js:132: * @param {Function=} opt_setHook A function to
run after the property is set,
Same here. Function is less precise.

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/i...
File ui/webui/resources/js/i18n_process.js (right):

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_process.js:6: i18nTemplate.process(document,
window.templateData);
Why is this change needed?

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/l...
File ui/webui/resources/js/load_time_data.js (right):

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/l...
ui/webui/resources/js/load_time_data.js:16: //
https://github.com/google/closure-compiler/issues/544 is fixed.
This is not the right bug number

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/l...
File ui/webui/resources/js/local_strings.js (right):

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/l...
ui/webui/resources/js/local_strings.js:12: * @param {Object=} opt_templateData
Optional object containing translated
No need to include "Optional" in the description since the type and code tells
you that it is optional already.

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/t...
File ui/webui/resources/js/template_data_externs.js (right):

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/t...
ui/webui/resources/js/template_data_externs.js:11: window.templateData;
can this be?

var templateData;

[Dan Beam, 2014-07-25 01:52:33]

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/a...
File ui/webui/resources/js/assert.js (right):

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/a...
ui/webui/resources/js/assert.js:30: function notReached(opt_message) {
On 2014/07/24 16:44:24, arv wrote:
> How about assertNotReached instead?

hmmm, blink uses ASSERT_NOT_REACHED() while chrome uses NOTREACHED.

"assertNotReached" is clearer than "notReached" (arguably), but
"assertNotReached" is longer than "assert(false, " or "notReached".

whatever, "assertNotReached" it is.

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/c...
File ui/webui/resources/js/chromeos/ui_account_tweaks.js (right):

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/c...
ui/webui/resources/js/chromeos/ui_account_tweaks.js:112: * @param {!Node} node
Root node of DOM subtree that should be disabled.
On 2014/07/24 16:44:24, arv wrote:
> !Element was more correct. Here is a case where the type system is doing us a
> disservice.

see what you think of updated changes

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/c...
ui/webui/resources/js/chromeos/ui_account_tweaks.js:142:
node.classList.add(sessionType + '-disabled');
On 2014/07/24 16:44:24, arv wrote:
> Node does not have classList property. It is a property of Element so this
> should cause a type error.

that's what i thought as well, but it didn't.  i think it's just those externs
where someone threw all the properties they could find onto object or
something... either way, it's now !Element.

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/c...
ui/webui/resources/js/chromeos/ui_account_tweaks.js:144: if
(/^(INPUT|SELECT|BUTTON)$/.test(node.nodeName))
On 2014/07/24 16:44:24, arv wrote:
> If you are refactoring this a switch would be the cleanest (and fastest).

reverted

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/c...
ui/webui/resources/js/chromeos/ui_account_tweaks.js:145: node.disabled = true;
On 2014/07/24 16:44:24, arv wrote:
> Node does not have a disabled property. Is the type checker not working or are
> the externs just wrong?

externs

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr.js (right):

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/c...
ui/webui/resources/js/cr.js:15: var cr = (function() {
On 2014/07/24 16:44:24, arv wrote:
> There was a reason someone changed this... maybe something related to
> snapshotting?

yeah, i don't think that materialized.

we can find a fix that works for both if it's required in the future.

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/c...
ui/webui/resources/js/cr.js:100: * @return {Function} The getter for the
property.
On 2014/07/24 16:44:24, arv wrote:
> Function is less precise than function() : *

Done.

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/c...
ui/webui/resources/js/cr.js:132: * @param {Function=} opt_setHook A function to
run after the property is set,
On 2014/07/24 16:44:24, arv wrote:
> Same here. Function is less precise.

Done.

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/i...
File ui/webui/resources/js/i18n_process.js (right):

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_process.js:6: i18nTemplate.process(document,
window.templateData);
On 2014/07/24 16:44:24, arv wrote:
> Why is this change needed?

less potential reference errors

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/l...
File ui/webui/resources/js/load_time_data.js (right):

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/l...
ui/webui/resources/js/load_time_data.js:16: //
https://github.com/google/closure-compiler/issues/544 is fixed.
On 2014/07/24 16:44:24, arv wrote:
> This is not the right bug number

how so?  we have to export the type here so other classes that depend on this
file understand it.

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/l...
File ui/webui/resources/js/local_strings.js (right):

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/l...
ui/webui/resources/js/local_strings.js:12: * @param {Object=} opt_templateData
Optional object containing translated
On 2014/07/24 16:44:24, arv wrote:
> No need to include "Optional" in the description since the type and code tells
> you that it is optional already.

Done.

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/t...
File ui/webui/resources/js/template_data_externs.js (right):

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/t...
ui/webui/resources/js/template_data_externs.js:11: window.templateData;
On 2014/07/24 16:44:24, arv wrote:
> can this be?
> 
> var templateData;

it could've but I felt using `window.templateData` everywhere else was safer
because it wouldn't cause a reference error if it's not defined (just
`undefined`).  is this desired?

[arv (Not doing code reviews), 2014-07-25 18:27:13]

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/l...
File ui/webui/resources/js/load_time_data.js (right):

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/l...
ui/webui/resources/js/load_time_data.js:16: //
https://github.com/google/closure-compiler/issues/544 is fixed.
On 2014/07/25 01:52:33, Dan Beam wrote:
> On 2014/07/24 16:44:24, arv wrote:
> > This is not the right bug number
> 
> how so?  we have to export the type here so other classes that depend on this
> file understand it.

I see. Other code uses {LoadTimeData} then this makes sense.

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/t...
File ui/webui/resources/js/template_data_externs.js (right):

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/t...
ui/webui/resources/js/template_data_externs.js:11: window.templateData;
On 2014/07/25 01:52:33, Dan Beam wrote:
> On 2014/07/24 16:44:24, arv wrote:
> > can this be?
> > 
> > var templateData;
> 
> it could've but I felt using `window.templateData` everywhere else was safer
> because it wouldn't cause a reference error if it's not defined (just
> `undefined`).  is this desired?

It seems like a ReferenceError might be able to catch errors in the code.

https://codereview.chromium.org/405743002/diff/240001/ui/webui/resources/js/c...
File ui/webui/resources/js/chromeos/ui_account_tweaks.js (right):

https://codereview.chromium.org/405743002/diff/240001/ui/webui/resources/js/c...
ui/webui/resources/js/chromeos/ui_account_tweaks.js:128: if (!(node instanceof
Element)) {
this is always false

[Dan Beam, 2014-07-25 21:58:18]

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/t...
File ui/webui/resources/js/template_data_externs.js (right):

https://codereview.chromium.org/405743002/diff/180001/ui/webui/resources/js/t...
ui/webui/resources/js/template_data_externs.js:11: window.templateData;
On 2014/07/25 18:27:13, arv wrote:
> On 2014/07/25 01:52:33, Dan Beam wrote:
> > On 2014/07/24 16:44:24, arv wrote:
> > > can this be?
> > > 
> > > var templateData;
> > 
> > it could've but I felt using `window.templateData` everywhere else was safer
> > because it wouldn't cause a reference error if it's not defined (just
> > `undefined`).  is this desired?
> 
> It seems like a ReferenceError might be able to catch errors in the code.

very well, var templateData it is! :)

https://codereview.chromium.org/405743002/diff/240001/ui/webui/resources/js/c...
File ui/webui/resources/js/chromeos/ui_account_tweaks.js (right):

https://codereview.chromium.org/405743002/diff/240001/ui/webui/resources/js/c...
ui/webui/resources/js/chromeos/ui_account_tweaks.js:128: if (!(node instanceof
Element)) {
On 2014/07/25 18:27:13, arv wrote:
> this is always false

whoops, removed and changed back to cast

[arv (Not doing code reviews), 2014-07-25 22:04:18]

LGTM

[Dan Beam, 2014-07-25 23:07:09]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2014-07-25 23:08:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dbeam@chromium.org/405743002/260001

[Dan Beam, 2014-07-26 02:09:30]

The CQ bit was unchecked by dbeam@chromium.org

[Dan Beam, 2014-07-26 02:11:08]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2014-07-26 05:00:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dbeam@chromium.org/405743002/260001

[commit-bot: I haz the power, 2014-07-26 11:11:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-07-26 12:19:29]

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

[commit-bot: I haz the power, 2014-07-26 12:19:30]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)

[Dan Beam, 2014-07-29 03:49:20]

+darin@ for compiled_resources.json (would you prefer this be somewhere else?)

[Dan Beam, 2014-07-30 01:42:33]

On 2014/07/29 03:49:20, Dan Beam wrote:
> +darin@ for compiled_resources.json (would you prefer this be somewhere else?)

ping darin@

FYI: vitalyp@ has renamed

  compiled_resources.json => compiled.resources[1]

and is switching from

  JSON => Python object literal parsing

here: https://chromiumcodereview.appspot.com/410333002/

We're doing this because we:
 - # Can comment in the code.
 - can use trailing commas
 - require less json parsing modules
 - to better match .gclient/.gyp/.gypi

[darin (slow to review), 2014-07-30 20:49:27]

Why does it make sense for compiled_resources.json to live in the root
directory? It mostly just references files under ui/. Shouldn't it live there?
The only other dependency is on third_party/. Seems fine for ui/ to express
dependencies on third_party/.

[Dan Beam, 2014-07-30 21:15:37]

On 2014/07/30 20:49:27, darin wrote:
> Why does it make sense for compiled_resources.json to live in the root
> directory? It mostly just references files under ui/. Shouldn't it live there?
> The only other dependency is on third_party/. Seems fine for ui/ to express
> dependencies on third_party/.

  chrome/
    browser/
    test/data/webui/
  remoting/
  ui/keyboard/

will be referenced very soon.  chrome/browser/ is already in progress here:
https://chromiumcodereview.appspot.com/418663002/

[darin (slow to review), 2014-07-30 21:52:47]

Shouldn't each of those directories have its own build rules?


On Wed, Jul 30, 2014 at 2:15 PM, <dbeam@chromium.org> wrote:

> On 2014/07/30 20:49:27, darin wrote:
>
>> Why does it make sense for compiled_resources.json to live in the root
>> directory? It mostly just references files under ui/. Shouldn't it live
>> there?
>> The only other dependency is on third_party/. Seems fine for ui/ to
>> express
>> dependencies on third_party/.
>>
>
>   chrome/
>     browser/
>     test/data/webui/
>   remoting/
>   ui/keyboard/
>
> will be referenced very soon.  chrome/browser/ is already in progress here:
> https://chromiumcodereview.appspot.com/418663002/
>
> https://chromiumcodereview.appspot.com/405743002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dan Beam, 2014-07-30 22:17:16]

On 2014/07/30 21:52:47, darin wrote:
> Shouldn't each of those directories have its own build rules?

thinking about it more, we should probably just use GYP if possible.

> 
> 
> On Wed, Jul 30, 2014 at 2:15 PM, <mailto:dbeam@chromium.org> wrote:
> 
> > On 2014/07/30 20:49:27, darin wrote:
> >
> >> Why does it make sense for compiled_resources.json to live in the root
> >> directory? It mostly just references files under ui/. Shouldn't it live
> >> there?
> >> The only other dependency is on third_party/. Seems fine for ui/ to
> >> express
> >> dependencies on third_party/.
> >>
> >
> >   chrome/
> >     browser/
> >     test/data/webui/
> >   remoting/
> >   ui/keyboard/
> >
> > will be referenced very soon.  chrome/browser/ is already in progress here:
> > https://chromiumcodereview.appspot.com/418663002/
> >
> > https://chromiumcodereview.appspot.com/405743002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Dan Beam, 2014-08-01 00:38:52]

moved to gyp

[Dan Beam, 2014-08-02 00:43:25]

ping darin@

[Dan Beam, 2014-08-02 01:43:47]

i've split off the build aspect of this CL to:
https://codereview.chromium.org/436843006/

CQ'ing just the JS fixes

[Dan Beam, 2014-08-02 01:45:15]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2014-08-02 01:46:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dbeam@chromium.org/405743002/540001

[Dan Beam, 2014-08-02 03:09:29]

The CQ bit was unchecked by dbeam@chromium.org

[Dan Beam, 2014-08-02 05:01:50]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2014-08-02 05:03:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dbeam@chromium.org/405743002/560001

[commit-bot: I haz the power, 2014-08-02 10:23:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-02 11:02:13]

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

[commit-bot: I haz the power, 2014-08-02 11:02:14]

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

[Dan Beam, 2014-08-04 18:02:01]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2014-08-04 18:03:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dbeam@chromium.org/405743002/580001

[commit-bot: I haz the power, 2014-08-04 23:45:25]

Change committed as 287403