Add Closure compiling instructions for site_list.

Add Closure compiling instructions for site_list.

BUG=543635
Committed: https://crrev.com/f80fa02e061de747625371414ea4c012fd565c10
Cr-Commit-Position: refs/heads/master@{#360803}

[Finnur, 2015-11-09 15:56:09]

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

[Finnur, 2015-11-09 15:56:09]

https://codereview.chromium.org/1428523006/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/1428523006/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:147: var setting =
sites[origin].setting;
Is this (and the change in onOriginTap_) the right way to address Closure errors
such as:

  ERROR - Property setting never defined on ?

Note: This change of mine is flagged as a JavaScript style guide violation: 

  line 146: Don't use wrapper types (i.e. new String() or @type {String})
      /** @type {{setting:Number}}*/
                          ^^^^^^

But I don't quite understand what that means and even though it points to the
JavaScript style guide, I can't find the reference for this...

Is this a false-positive?

[Dan Beam, 2015-11-09 19:28:56]

https://codereview.chromium.org/1428523006/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/1428523006/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:146: /** @type
{{setting:Number}}*/
this @type doesn't make sense here, but this might

/** @type {{setting: number}} */(sites[origin]).setting

https://codereview.chromium.org/1428523006/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:147: var setting =
sites[origin].setting;
On 2015/11/09 15:56:09, Finnur wrote:
> Is this (and the change in onOriginTap_) the right way to address Closure
errors
> such as:
> 
>   ERROR - Property setting never defined on ?
> 
> Note: This change of mine is flagged as a JavaScript style guide violation: 
> 
>   line 146: Don't use wrapper types (i.e. new String() or @type {String})
>       /** @type {{setting:Number}}*/
>                           ^^^^^^
> 
> But I don't quite understand what that means and even though it points to the
> JavaScript style guide, I can't find the reference for this...
> 
> Is this a false-positive?

Number -> number

https://codereview.chromium.org/1428523006/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:174: /** @type
{{model:Object}}*/
i don't understand what this is supposed to do?  are you trying to describe
|event|?

[Finnur, 2015-11-10 00:39:43]

Haven't updated the code. Just wanted to get a quick question in...

https://codereview.chromium.org/1428523006/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/1428523006/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:174: /** @type
{{model:Object}}*/
Um... I think so. Well, the error (as I recall, I'm not at my desk since it is
past midnight here :)) was that it complained that |item| on |?| was not
defined. But if I define model it stops complaining. I couldn't find much
documentation on these errors and how to handle them, so I modeled this approach
after other js code in the tree. Do you recommend a different approach?

[Dan Beam, 2015-11-10 05:58:58]

https://codereview.chromium.org/1428523006/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/1428523006/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:174: /** @type
{{model:Object}}*/
On 2015/11/10 00:39:43, Finnur wrote:
> Um... I think so. Well, the error (as I recall, I'm not at my desk since it is
> past midnight here :)) was that it complained that |item| on |?| was not
> defined. But if I define model it stops complaining. I couldn't find much
> documentation on these errors and how to handle them, so I modeled this
approach
> after other js code in the tree. Do you recommend a different approach?

have you actually been able to run the compiler?

[Finnur, 2015-11-10 11:58:50]

https://codereview.chromium.org/1428523006/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/1428523006/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:146: /** @type
{{setting:Number}}*/
It is modeled after other code in the tree and did take care of the closure
error, but your version also works (if I add enclosing parentheses) and is
clearer -- so I switched.

https://codereview.chromium.org/1428523006/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:147: var setting =
sites[origin].setting;
Bah. That's unfortunate (Polymer wanting |Number|, but Closure wanting
|number|). Anyway, that takes care of both problems (Closure and style guide
issue).

https://codereview.chromium.org/1428523006/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:174: /** @type
{{model:Object}}*/
> have you actually been able to run the compiler?

Yes, couldn't be doing this without it -- got the Closure compiler working after
switching to my Linux box (due to unrelated issue crbug.com/544889).


And now that I'm at my desk testing this I can properly describe what I'm trying
to do.

  this.selectedOrigin = event.model.item.url;

I'm obviously trying to get the origin that was selected from the list. With
only the assignment above (and no Closure hint) I get this:

  ERROR - Property model never defined on event

So, I'm trying to describe |model| -- or |event| if that describes |model| as
well.

Note: If I add this line above it, that error goes away and the Closure compiler
is happy (no further complaints):

  /** @type {{model:Object}}*/

As I said, this is modelled after other code in the tree, so at least there is
precedent.

However, if I instead use the same approach as you suggested above:

  var model = /** @type {{model: Object}} */((event).model);
  this.selectedOrigin = model.item.url;

... then the error becomes:

  ERROR - Property item never defined on model

... and that error remains even if I define it like so:

  var model = /** @type {{model: Object}} */((event).model);
  var item = /** @type {{item: Object}} */((model).item);

I also suspect that if I manage to define |item|, then |url| will need to be
described.



> are you trying to describe |event|?

That was what I tried first. I searched through a sizable chunk of js code in
the tree, looking for "function(event" (without the double quotes) and could not
find any file that described an event in a way that worked with the above code.
A lot of functions didn't bother defining |event| in a generic way, some just
define it as Event, some as ChromeKeyboardEvent) but the end result for me was
all the same. It still complained about |model|.

[Dan Beam, 2015-11-11 07:01:40]

https://codereview.chromium.org/1428523006/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/1428523006/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:146: /** @type
{{setting:Number}}*/
On 2015/11/10 11:58:50, Finnur wrote:
> It is modeled after other code in the tree and did take care of the closure
> error, but your version also works (if I add enclosing parentheses) and is
> clearer -- so I switched.

i think it's just confusing the compiler into thinking this is an object with a
key but that could also have more keys (which it's just not braking about, but
does in stricter modes)

https://codereview.chromium.org/1428523006/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:147: var setting =
sites[origin].setting;
On 2015/11/10 11:58:50, Finnur wrote:
> Bah. That's unfortunate (Polymer wanting |Number|, but Closure wanting
> |number|). Anyway, that takes care of both problems (Closure and style guide
> issue).

Polymer({
  properties: {
    doUseWrappers: {
      type: Number,  // but not as jsdoc annotations denoting their @type
      // if this was typed with jsdoc, it'd be @type {number}
    },
  },
});

https://codereview.chromium.org/1428523006/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/1428523006/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:146: var setting =
/** @type {{setting: number}} */((sites[origin]).setting);
this is also casting incorrectly because you're saying

(sites[origin].setting) -> yields an object with a 'setting' key that has a
value of type number

this should probably be:

  var setting = /** @type {{setting: number}} */(origins[sites]);
  if (setting.setting == this.categorySubtype) {

https://codereview.chromium.org/1428523006/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:173: /** @type
{{model:Object}}*/
the current code means "this.selectedOrigin is an object with a key of 'model'
that has a value of Object or null".  basically: it's all wrong.

/** @param {Event} event */
onOriginTap_: function(event) {
  e = /** @type {{model: Object}} */(e);
  ...
},

is closer to right, but it doesn't actually describe the rest of the path
(.item.url).  is there a @typedef or how is this information generated?

[Finnur, 2015-11-11 13:40:07]

Patchset #3 (id:40001) has been deleted

[Finnur, 2015-11-11 13:41:23]

OK, this is a little closer to what I am after. PTAL.

[Dan Beam, 2015-11-11 23:18:39]

lgtm i suppose

https://codereview.chromium.org/1428523006/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/1428523006/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:170: * @param
{!{model: !{item: !{url: string}}}} event
this should probably be {Event}

https://codereview.chromium.org/1428523006/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:173: onOriginTap_:
function(event) {
and this should probably be

var model = /** @type {Object<Polymer.Base>} */(event.model);

https://github.com/Polymer/polymer/blob/f58d9faf54948c8df300eedd2379459e82ffc...

[Finnur, 2015-11-12 14:53:11]

Patchset #4 (id:80001) has been deleted

[Finnur, 2015-11-12 16:21:16]

I had explored the Polymer.Base angle before but couldn't get it to work. But if
that's the way to go, then I'm willing to explore it further.

Unfortunately, Closure doesn't want to recognize Polymer.Base, even if I add the
polymer.externs.js as an extern in compiled_resources.gyp (see CL).

Am I doing it wrong?

[Dan Beam, 2015-11-12 22:42:23]

On 2015/11/12 16:21:16, Finnur wrote:
> I had explored the Polymer.Base angle before but couldn't get it to work. But
if
> that's the way to go, then I'm willing to explore it further.
> 
> Unfortunately, Closure doesn't want to recognize Polymer.Base, even if I add
the
> polymer.externs.js as an extern in compiled_resources.gyp (see CL).
> 
> Am I doing it wrong?

you'd need to find where polymer.base is being defined and add that as a
dependency

[Finnur, 2015-11-13 10:32:00]

That's what I thought I was doing.

Polymer.base is defined here:
third_party/polymer/v1_0/components-chromium/polymer-externs/polymer.externs.js

As I see it, there are three ways to refer to it (in the context of this
discussion):

1) Add it as a 'depends' in the compiled_resources.gyp.
2) Add it as an 'extern' in the compiled_resources.gyp.
3) Add it as a 'link import' in the site_list.html file.
(And I tried various combinations of the above too).

What looked most promising was 2) because that's exactly what
file_manager/gallery/js/compiled_resources.gyp does. But none of these seem to
get Closure to recognize Polymer.Base and 1) even adds a bunch of new Closure
errors to the mix.

[Dan Beam, 2015-11-13 15:32:54]

On 2015/11/13 10:32:00, Finnur wrote:
> That's what I thought I was doing.
> 
> Polymer.base is defined here:
>
third_party/polymer/v1_0/components-chromium/polymer-externs/polymer.externs.js
> 
> As I see it, there are three ways to refer to it (in the context of this
> discussion):
> 
> 1) Add it as a 'depends' in the compiled_resources.gyp.
> 2) Add it as an 'extern' in the compiled_resources.gyp.
> 3) Add it as a 'link import' in the site_list.html file.
> (And I tried various combinations of the above too).
> 
> What looked most promising was 2) because that's exactly what
> file_manager/gallery/js/compiled_resources.gyp does. But none of these seem to
> get Closure to recognize Polymer.Base and 1) even adds a bunch of new Closure
> errors to the mix.

externs are things that don't have a .js file.  things that are defined in js
would be dependencies.  this would be a dependency.  something like alert()
would be an extern (as it's provided by the browser).

[Finnur, 2015-11-13 16:59:42]

I do appreciate the terminology lesson, but I'm not sure how to apply it to the
problem at hand.

At first I thought you were implying that I should add:

   <script
src="chrome://resources/polymer/v1_0/components-chromium/polymer-externs/polymer.externs.js"

... alongside the other <script> include in the site_list.html, but that doesn't
see to help Closure find Polymer.Base... :/

Any hints?

[Finnur, 2015-11-20 11:01:00]

I don't want this to bitrot on a minor issue like this, so I'm checking in what
was approved and would be happy to follow-up, if needed.

[Finnur, 2015-11-20 11:01:05]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2015-11-20 11:01:05]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1428523006/#ps140001
(title: "Back to approved version")

[commit-bot: I haz the power, 2015-11-20 11:01:55]

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

[commit-bot: I haz the power, 2015-11-20 11:41:00]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-11-20 11:42:35]

Patchset 6 (id:??) landed as
https://crrev.com/f80fa02e061de747625371414ea4c012fd565c10
Cr-Commit-Position: refs/heads/master@{#360803}

[michaelpg, 2015-11-20 14:26:55]

On 2015/11/12 22:42:23, Dan Beam wrote:
> On 2015/11/12 16:21:16, Finnur wrote:
> > I had explored the Polymer.Base angle before but couldn't get it to work.
But
> if
> > that's the way to go, then I'm willing to explore it further.
> > 
> > Unfortunately, Closure doesn't want to recognize Polymer.Base, even if I add
> the
> > polymer.externs.js as an extern in compiled_resources.gyp (see CL).
> > 
> > Am I doing it wrong?
> 
> you'd need to find where polymer.base is being defined and add that as a
> dependency

:-\ I haven't had any luck getting polymer-micro-extracted.js to compile.

https://codereview.chromium.org/1428523006/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/1428523006/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:173: onOriginTap_:
function(event) {
On 2015/11/11 23:18:39, Dan Beam wrote:
> and this should probably be
> 
> var model = /** @type {Object<Polymer.Base>} */(event.model);

I think you mean (event['model']). if you @param event as an {Event}, model
isn't a valid property

> 
>
https://github.com/Polymer/polymer/blob/f58d9faf54948c8df300eedd2379459e82ffc...

The annotation you linked to doesn't even seem correct -- shouldn't it @return a
{Polymer.Base}, not an {Object<Polymer.Base>}?

Besides which, typing it as such isn't particularly helpful because we're
interested in the "item" property of this particular Polymer.Base, which is not
a property of Polymer.Base's in general.