JSDoc for misc/ files

Such an enormous undertaking. My hat off to you, @nod_.

This is ready AFAICT. But the thing that it most needs, is feedback from a core committer, and likely from @jhodgdon. So, assigning to her.

Well, I took a look...

I'm not excited about committing this patch. It adds a lot of @param and @return lines with zero descriptive docs. I would really prefer to leave out the @param/@return entirely until we have descriptions, because having lines like this will encourage more of them to be done like this, and it's incomplete docs. I'm assuming that the same standards apply to JS docs as we have for PHP docs, that @param and @return require descriptions?

Some other comments:

1. +++ b/core/misc/active-link.js
   @@ -18,6 +18,8 @@
   +   * @type {Drupal~behavior}
   

   I took a look at the docs for the ~ and I still don't get this. Where is Drupal~behavior defined? I cannot find it anywhere.

2. +++ b/core/misc/ajax.js
   @@ -125,8 +162,8 @@
   +   * initialization of Drupal.Ajax objects and storing all created object in
   

   Nitpick: should be "... all created objects" although this was in the original incorrect as well, so possibly out of scope to fix here.

3. +++ b/core/misc/ajax.js
   @@ -125,8 +162,8 @@
   +   * the {@link Drupal.ajax.instances} array.
   

   You might add a big note on https://www.drupal.org/node/2183405 about this.

   In PHP docs, anything that can be turned into a link will be turned into a link automatically.

   It appears that in JSDoc, this is not true, and also the syntax of @link is quite different from the PHP docs we use in Drupal. So... I think if this is not true in JSDoc we probably need to have a standard that *everything* is made into links manually, which will be tedious. But otherwise it's hard to find what is being referred to.

4. +++ b/core/misc/ajax.js
   @@ -195,7 +232,7 @@
   +   * @type {Array.<Drupal.Ajax>}
   

   In the JSDoc standards doc it says to use "array" not "Array"?
   https://www.drupal.org/node/2183405

5. +++ b/core/misc/ajax.js
   @@ -300,6 +356,26 @@
   +    /**
   +     * Set the options for the ajaxSubmit function.
   +     *
   +     * @name Drupal.Ajax#options
   

   I'm a bit confused about this. It's a variable, so why is it's description starting with a verb as if it's a function that does something, rather than a variable that has a value?

6. +++ b/core/misc/ajax.js
   @@ -300,6 +356,26 @@
   +     * @prop {string} url
   +     * @prop {object} data
   +     * @prop {function} beforeSerialize
   +     * @prop {function} beforeSubmit
   +     * @prop {function} beforeSend
   +     * @prop {function} success
   +     * @prop {function} complete
   +     * @prop {string} dataType
   +     * @prop {object} accepts
   +     * @prop {string} accepts.json
   +     * @prop {string} type
   +     */
   

   Shouldn't all of these properties have documentation, and shouldn't their docs ideally be just before they're defined below rather than in the ajax.options doc block?

7. +++ b/core/misc/ajax.js
   @@ -409,6 +487,9 @@
   +   * @param {HTMLElement} element
   +   * @param {jQuery.Event} event
       */
   

   Shouldn't all @param lines have documentation about what the parameters are? The problem with committing this with partial docs is that people will start copy/pasting it. I would almost prefer to have nothing rather than setting a precident of having @param without docs of what it is.

8. +++ b/core/misc/ajax.js
   @@ -640,7 +740,13 @@
   +   * @param {string} [response.effect]
   +   * @param {string|number} [response.speed]
   +   *
   

   These parameters do not actually exist... they are properties of the response object parameter. Will this really work? We wouldn't document them like this in PHP at all. We'd document @param $response and then in the description have a list of what the properties are.

9. +++ b/core/misc/ajax.js
   @@ -640,7 +740,13 @@
   +   * @return {object}
   

   This tells me almost nothing, illustrating my inclination that adding @return or @param without docs is not a great idea.

So.. I ran out of time here... I'm not saying we should definitely not commit this patch, but I don't think it comes close to "this patch at least complies with our standards". I am only in favor of adding docs that are correct and are decent illustrations of how we should do docs. I'd rather not have docs than have docs that are bad examples... are they ever going to get fixed up? What's the plan here?

1. +++ b/core/misc/ajax.js
   @@ -1,9 +1,24 @@
   + *
   + * Ajax is a method of making a request via JavaScript while viewing an HTML
   + * page. The request returns an array of commands encoded in JSON, which is
   + * then executed to make any changes that are necessary to the page.
   

   I like this high level overview

2. +++ b/core/misc/ajax.js
   @@ -195,7 +233,7 @@
   -   * @type {Array}
   +   * @type {Array.<Drupal.Ajax>}
   

   Nice that this is possible!

3. +++ b/core/misc/ajax.js
   @@ -300,6 +357,26 @@
   +     * @prop {string} url
   +     * @prop {object} data
   

   Cool stuff!

4. +++ b/core/misc/ajax.js
   @@ -607,6 +705,9 @@
   +   * @param {Array.<Drupal.AjaxCommands~commandDefinition>} response
   

   Oh I see its defined somewhere else in that patch.

5. +++ b/core/misc/ajax.js
   @@ -768,6 +913,12 @@
   +     * @param {Drupal.Ajax} [ajax]
   ...
        remove: function (ajax, response, status) {
          var settings = response.settings || ajax.settings || drupalSettings;
   

   in remove ajax seems to still be used?

6. +++ b/core/misc/dropbutton/dropbutton.js
   @@ -67,18 +93,31 @@
   +
              /**
               * Adds a timeout to close the dropdown on mouseleave.
   +           *
   +           * @ignore
               */
              'mouseleave.dropbutton': $.proxy(this.hoverOut, this),
   ...
              /**
               * Clears timeout when mouseout of the dropdown.
   +           *
   +           * @ignore
               */
   ...
              /**
               * Similar to mouseleave/mouseenter, but for keyboard navigation.
   +           *
   +           * @ignore
               */
   ...
   +          /**
   +           * @ignore
   +           */
   

   I'd had expected that you can use just "//" for those cases? My naive thinking is that "//" is always ignored

7. +++ b/core/misc/machine-name.js
   @@ -163,17 +174,19 @@
   -     * @return
   +     * @return {jQuery}
         *   The transliterated source string.
   

   I was confused by the type used here, but well indeed, this seems to be the case, its the result of a $.get() call, so everything is fine.

8. +++ b/core/misc/states.js
   @@ -48,13 +57,18 @@
   +   * @param {object} args
   +   *   Object with the following keys (all of which are required)
   +   * @param {jQuery} args.element
   +   *   A jQuery object of the dependent element
   +   * @param {Drupal.states.State} args.state
   +   *   A State object describing the state that is dependent
   +   * @param {object} args.constraints
   +   *   An object with dependency specifications. Lists all elements that this
   +   *   element depends on. It can be nested and can contain
   +   *   arbitrary AND and OR clauses.
       */
      states.Dependent = function (args) {
   

   I guess its not worth to add its own type here?

9. +++ b/core/misc/tabledrag.js
   @@ -38,39 +53,124 @@
   +    /**
   +     *
   +     * @type {bool}
   +     */
   +    this.changed = false;
   +    // Whether anything in the entire table has changed.
   +    /**
   +     *
   +     * @type {number}
   +     */
   +    this.maxDepth = 0;
   +    // Maximum amount of allowed parenting.
   

   Those bits are a bit odd. Won't it be helpful to have that part of the documentation?

10. +++ b/core/misc/tabledrag.js
    @@ -1100,15 +1302,16 @@
    +   * @param {HTMLElement|null} prevRow
    ...
    +   * @param {HTMLElement|null} nextRow
    

    In other bits of the code we seemed to have used ?HTMLElement to make things optional. I guess both things are possible?

Alright

Looks good enough! Committed to 8.0.x. Whew! That's the last of the big ones. ;) Thanks nod_ and reviewers!

Followups/questions:

a)

+  $.extend(Drupal.theme, /** @lends Drupal.theme */{

I'm not sure exactly what these lines do with @lends (but they're common in all 3 patches) - minor suggestion, I think they'd be easier to scan/read by humans if there was a space between the */ and the {

Thoughts?

b)
Also don't use JS in docs, write out JavaScript:

+++ b/core/misc/drupal.js
@@ -1,7 +1,40 @@
 /**
- * Base framework for Drupal-specific JavaScript, behaviors, and settings.
+ * @file
+ * Defines the Drupal JS API.
  */

c) In drupal.js, there are a number of doc blocks that don't seem to have any code after them. Is that right? Seems like the docs should go near the code they're documenting?

+/**
+ * A jQuery object.
+ *
+ * @typedef {object} jQuery
+ *
+ * @prop {number} length=0
+ */
+
+/**
+ * Variable generated by Drupal with all the configuration created from PHP.
+ *
+ * @global
+ *
+ * @var {object} drupalSettings
+ */
+
+/**
...

Cool. Let's just proceed as you have been doing it then. If there's a good reason, there's a good reason. ;)