Add Closure compiling instructions for site_list.

chrome/browser/resources/settings/site_settings/site_list.js

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /**
  * @fileoverview
  * 'settings-site-list' shows a list of Allowed and Blocked sites for a given
  * category.
  *
  * Example:
@@ skipping 125 matching lines @@
   /**
    * Populate the sites list for display.
    * @private
    */
   populateList_: function() {
     var newList = [];
     var pref = this.getPref(
         this.computeCategoryExceptionsPrefName(this.category));
     var sites = pref.value;
     for (var origin in sites) {
-      if (sites[origin].setting == this.categorySubtype) {
+      /** @type {{setting:Number}}*/

[Dan Beam, 2015/11/09 19:28:55]

this @type doesn't make sense here, but this might

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

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

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.

[Dan Beam, 2015/11/11 07:01:39]

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)

+      var setting = sites[origin].setting;

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

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:55]

Number -> number

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

Bah. That's unfortunate (Polymer wanting |Number|, but Closure wanting
|number|). Anyway, that takes care of both problems (Closure and style guide
issue).

[Dan Beam, 2015/11/11 07:01:39]

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

+      if (setting == this.categorySubtype) {
         var tokens = origin.split(',');
         newList.push({url: tokens[0]});
       }
     }
 
     this.sites_ = newList;
   },
 
   /**
    * Setup the values to use for the action menu.
    * @private
    */
   setUpActionMenu_: function() {
     this.showAllowAction_ =
         this.categorySubtype == settings.DefaultValues.BLOCK;
     this.showBlockAction_ =
         this.categorySubtype == settings.DefaultValues.ALLOW &&
         this.category != settings.ContentSettingsTypes.FULLSCREEN;
   },
 
   /**
    * A handler for selecting a site (by clicking on the origin).
    * @private
    */
   onOriginTap_: function(event) {
+    /** @type {{model:Object}}*/

[Dan Beam, 2015/11/09 19:28:55]

i don't understand what this is supposed to do?  are you trying to describe
|event|?

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

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]

have you actually been able to run the compiler?

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

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.
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|.

     this.selectedOrigin = event.model.item.url;
   },
 
   /**
    * A handler for activating one of the menu action items.
    * @private
    */
   onActionMenuIronSelect_: function(event) {
     // TODO(finnur): Implement.
   },
 
   /**
    * Returns the appropriate header value for display.
-   * @param {array<string>} siteList The list of all sites to display for this
+   * @param {Array<string>} siteList The list of all sites to display for this
    *     category subtype.
    * @param {boolean} toggleState The state of the global toggle for this
    *     category.
    * @private
    */
   computeSiteListHeader_: function(siteList, toggleState) {
     if (this.categorySubtype == settings.DefaultValues.ALLOW) {
       return loadTimeData.getStringF(
           'titleAndCount',
           loadTimeData.getString(
@@ skipping 10 matching lines @@
   /**
    * Returns true if this widget is showing the allow list.
    * @private
    */
   isAllowList_: function() {
     return this.categorySubtype == settings.DefaultValues.ALLOW;
   },
 
   /**
    * Returns whether to show the site list.
-   * @param {array} siteList The list of all sites to display for this category
+   * @param {Array} siteList The list of all sites to display for this category
    *     subtype.
    * @param {boolean} toggleState The state of the global toggle for this
    *     category.
    * @private
    */
   showSiteList_: function(siteList, toggleState) {
     if (siteList.length == 0)
       return false;
     // The Block list is only shown when the category is set to Allow since it
     // is redundant to also list all the sites that are blocked.
     if (this.isAllowList_())
       return true;
 
     return toggleState;
   },
 
   /**
    * Returns the icon to use for a given site.
    * @param {string} url The url of the site to fetch the icon for.
    * @private
    */
   computeSiteIcon_: function(url) {
     // TODO(finnur): For now, we're returning a placeholder image for each site
     // but the actual favicon for each site will need to be returned.
     return 'communication:message';
   },
 });