Noissue - Update JSDoc to mark optionals in lib/filterClasses.js

lib/filterClasses.js

Index: lib/filterClasses.js
===================================================================
--- a/lib/filterClasses.js
+++ b/lib/filterClasses.js
@@ -369,17 +369,17 @@
       this._lastHit = value;
       FilterNotifier.triggerListeners("filter.lastHit", this, value, oldValue);
     }
     return this._lastHit;
   },
 
   /**
    * String that the domains property should be generated from
-   * @type {string}
+   * @type {?string}
    */
   domainSource: null,
 
   /**
    * Separator character used in domainSource property, must be
    * overridden by subclasses
    * @type {string}
    */
@@ -466,23 +466,23 @@
     }
 
     Object.defineProperty(this, "domains", {value: domains, enumerable: true});
     return this.domains;
   },
 
   /**
    * Array containing public keys of websites that this filter should apply to
-   * @type {string[]}
+   * @type {?string[]}
    */
   sitekeys: null,
 
   /**
    * Checks whether this filter is active on a domain.
-   * @param {string} docDomain domain name of the document that loads the URL
+   * @param {string} [docDomain] domain name of the document that loads the URL
    * @param {string} [sitekey] public key provided by the document
    * @return {boolean} true in case of the filter being active
    */
   isActiveOnDomain(docDomain, sitekey)
   {
     // Sitekeys are case-sensitive so we shouldn't convert them to
     // upper-case to avoid false positives here. Instead we need to
     // change the way filter options are parsed.
@@ -681,29 +681,28 @@
    * Defines whether the filter should distinguish between lower and
    * upper case letters
    * @type {boolean}
    */
   matchCase: false,
   /**
    * Defines whether the filter should apply to third-party or
    * first-party content only. Can be null (apply to all content).
-   * @type {boolean}
+   * @type {?boolean}
    */
   thirdParty: null,
 
   /**
    * String that the sitekey property should be generated from
-   * @type {string}
+   * @type {?string}
    */
   sitekeySource: null,
 
   /**
-   * Array containing public keys of websites that this filter should apply to
-   * @type {string[]}
+   * @see ActiveFilter.sitekeys

[Manish Jethani, 2018/05/09 16:56:08]

This may seem unrelated but I wanted to specify that the value can be null, but
ESLint wouldn't allow me, then I realized the documentation can just be
inherited.

    */
   get sitekeys()
   {
     // Despite this property being cached, the getter is called
     // several times on Safari, due to WebKit bug 132872
     let prop = Object.getOwnPropertyDescriptor(this, "sitekeys");
     if (prop)
       return prop.value;
@@ -721,20 +720,20 @@
     );
     return this.sitekeys;
   },
 
   /**
    * Tests whether the URL matches this filter
    * @param {string} location URL to be tested
    * @param {number} typeMask bitmask of content / request types to match
-   * @param {string} docDomain domain name of the document that loads the URL
-   * @param {boolean} thirdParty should be true if the URL is a third-party
-   *                             request
-   * @param {string} sitekey public key provided by the document
+   * @param {string} [docDomain] domain name of the document that loads the URL
+   * @param {boolean} [thirdParty] should be true if the URL is a third-party
+   *                               request
+   * @param {string} [sitekey] public key provided by the document
    * @return {boolean} true in case of a match
    */
   matches(location, typeMask, docDomain, thirdParty, sitekey)
   {
     if (this.contentType & typeMask &&
         (this.thirdParty == null || this.thirdParty == thirdParty) &&
         this.isActiveOnDomain(docDomain, sitekey) && this.regexp.test(location))
     {
@@ -880,22 +879,22 @@
                                         RegExpFilter.typeMap.POPUP |
                                         RegExpFilter.typeMap.GENERICHIDE |
                                         RegExpFilter.typeMap.GENERICBLOCK);
 
 /**
  * Class for blocking filters
  * @param {string} text see Filter()
  * @param {string} regexpSource see RegExpFilter()
- * @param {number} contentType see RegExpFilter()
- * @param {boolean} matchCase see RegExpFilter()
- * @param {string} domains see RegExpFilter()
- * @param {boolean} thirdParty see RegExpFilter()
- * @param {string} sitekeys see RegExpFilter()
- * @param {boolean} collapse
+ * @param {number} [contentType] see RegExpFilter()
+ * @param {boolean} [matchCase] see RegExpFilter()
+ * @param {string} [domains] see RegExpFilter()
+ * @param {boolean} [thirdParty] see RegExpFilter()
+ * @param {string} [sitekeys] see RegExpFilter()
+ * @param {boolean} [collapse]
  *   defines whether the filter should collapse blocked content, can be null
  * @param {string} [csp]
  *   Content Security Policy to inject when the filter matches
  * @constructor
  * @augments RegExpFilter
  */
 function BlockingFilter(text, regexpSource, contentType, matchCase, domains,
                         thirdParty, sitekeys, collapse, csp)
@@ -909,36 +908,36 @@
 exports.BlockingFilter = BlockingFilter;
 
 BlockingFilter.prototype = extend(RegExpFilter, {
   type: "blocking",
 
   /**
    * Defines whether the filter should collapse blocked content.
    * Can be null (use the global preference).
-   * @type {boolean}
+   * @type {?boolean}
    */
   collapse: null,
 
   /**
    * Content Security Policy to inject for matching requests.
-   * @type {string}
+   * @type {?string}
    */
   csp: null
 });
 
 /**
  * Class for whitelist filters
  * @param {string} text see Filter()
  * @param {string} regexpSource see RegExpFilter()
- * @param {number} contentType see RegExpFilter()
- * @param {boolean} matchCase see RegExpFilter()
- * @param {string} domains see RegExpFilter()
- * @param {boolean} thirdParty see RegExpFilter()
- * @param {string} sitekeys see RegExpFilter()
+ * @param {number} [contentType] see RegExpFilter()
+ * @param {boolean} [matchCase] see RegExpFilter()
+ * @param {string} [domains] see RegExpFilter()
+ * @param {boolean} [thirdParty] see RegExpFilter()
+ * @param {string} [sitekeys] see RegExpFilter()
  * @constructor
  * @augments RegExpFilter
  */
 function WhitelistFilter(text, regexpSource, contentType, matchCase, domains,
                          thirdParty, sitekeys)
 {
   RegExpFilter.call(this, text, regexpSource, contentType, matchCase, domains,
                     thirdParty, sitekeys);
@@ -983,33 +982,33 @@
   /**
    * @see ActiveFilter.ignoreTrailingDot
    */
   ignoreTrailingDot: false,
 
   /**
    * Host names or domains the filter should be restricted to (can be null for
    * no restriction)
-   * @type {string}
+   * @type {?string}
    */
   selectorDomains: null,
   /**
    * CSS selector for the HTML elements that should be hidden
    * @type {string}

[Manish Jethani, 2018/05/09 16:56:08]

Note that selector will always be non-null.

[kzar, 2018/05/10 11:06:25]

I guess this is technically incorrect for the abstract ElemHideBase class, since
in a few lines you can see it's null.

[Manish Jethani, 2018/05/11 03:20:57]

So ElemHideBase is an abstract class, we don't create instances of it. Assuming
that this documentation here is going to be inherited by the derived classes,
then it is true that selector will never be null.

I don't know if we're actually generating the HTML documentation out of this,
and I'm pretty sure there would be a lot of things to fix (which hopefully we
will do once we switch to ES2015 classes), but for now at least in the source
code I hope that the JSDoc comments reflect how things are.

[Manish Jethani, 2018/05/11 03:26:52]

By the way there's a @abstract tag in JSDoc [1], but I think we might be
overdoing it if we use it on the code in its current form. Since we're switching
to ES2015 classes anyway, I think it would be more useful to mark an entire
class as abstract (which would mean all members are "abstract" too).

[1]: http://usejsdoc.org/tags-abstract.html

    */
   selector: null
 });
 
 /**
  * Creates an element hiding filter from a pre-parsed text representation
  *
  * @param {string} text         same as in Filter()
- * @param {string?} domains
+ * @param {string} [domains]
  *   domains part of the text representation
- * @param {string?} type
+ * @param {string} [type]
 *    rule type, either empty or @ (exception) or ? (emulation rule)
  * @param {string} selector     raw CSS selector
  * @return {ElemHideFilter|ElemHideException|
  *          ElemHideEmulationFilter|InvalidFilter}
  */
 ElemHideBase.fromText = function(text, domains, type, selector)
 {
   // We don't allow ElemHide filters which have any empty domains.
@@ -1032,17 +1031,17 @@
   }
 
   return new ElemHideFilter(text, domains, selector);
 };
 
 /**
  * Class for element hiding filters
  * @param {string} text see Filter()
- * @param {string} domains  see ElemHideBase()
+ * @param {string} [domains]  see ElemHideBase()
  * @param {string} selector see ElemHideBase()
  * @constructor
  * @augments ElemHideBase
  */
 function ElemHideFilter(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
 }
@@ -1050,17 +1049,17 @@
 
 ElemHideFilter.prototype = extend(ElemHideBase, {
   type: "elemhide"
 });
 
 /**
  * Class for element hiding exceptions
  * @param {string} text see Filter()
- * @param {string} domains  see ElemHideBase()
+ * @param {string} [domains]  see ElemHideBase()
  * @param {string} selector see ElemHideBase()
  * @constructor
  * @augments ElemHideBase
  */
 function ElemHideException(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
 }