1044932 - Add JQuery support to visual events

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

This seems like a big job but if you consider that we can use something like these parsers to get the event information from the libraries it is fairly simple:
https://github.com/DataTables/VisualEvent/tree/master/js/parsers

1. JQuery - http://code.jquery.com/jquery-1.2.js
2. jQuery 1.3+ - http://code.jquery.com/jquery-1.11.1.js

Comment from [e-mail]:
I've tested with different versions of jQuery and it looks like this method works in 1.6+: 

    jQuery._data(elem).events

Live demo: http://jsfiddle.net/simevidas/2Uf9c/

I suggest implementing jQuery support first using the above method, i.e. iterate over all DOM elements and then perform something like this:

var events = jQuery._data(elem).events;
Object.keys( events ).forEach(function (type) {
    events[type].forEach(function (jHandler) {
        // type returns the event type
        // jHandler.handler references the original event handler
    });
});

Live demo: http://jsfiddle.net/simevidas/bNLjD/

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: visual-events-jquery-support-1044932.patch

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: visual-events-jquery-support-1044932.patch

This patch adds an API to add library support to visual events along with jQuery events and jQuery live events.

Most of the tests are very similar to one another but we have slightly different results due to the different jQuery versions.

I'll copy the comments because I don't want to type it all twice:
/**
 * An event parser must take the following form:
 *   {
 *     desc {String}: "jQuery events", // Short string explaining which library
 *                                        the parser works with.
 *     guid {String}: "168280ed-9d8e-42b9-a515-6858df295ae6", // Unique ID
 *     getListeners: function(node) { },     // Function that takes a node and
 *                                           // returns an array of eventInfo
 *                                           // objects (see below).
 *
 *     getHasListeners: function(node) { },  // Optional function that takes a
 *                                           // node and returns a boolean
 *                                           // indicating whether a node has
 *                                           // listeners attached.
 *
 *     normalizeHandler: function(fnDO) { }, // Optional function that takes a
 *                                           // Debugger.Object instance and
 *                                           // climbs the scope chain to get
 *                                           // the function that should be
 *                                           // displayed in the event bubble
 *                                           // see the following url for
 *                                           // details:
 *                                           //   https://developer.mozilla.org/
 *                                           //   docs/Tools/Debugger-API/
 *                                           //   Debugger.Object
 *   }
 *
 * An eventInfo object should take the following form (see getListeners):
 *   {
 *     type {String}:      "click",
 *     handler {Function}: event handler,
 *     tags {String}:      "jQuery,Live", // These tags will be displayed as
 *                                        // attributes in the events popup.
 *     hide: {
 *       dom0 {Boolean}:   true           // Hide or show fields.
 *     },
 *
 *     override: {                        // The following can be overridden:
 *       type: "click",
 *       origin: "http://www.mozilla.com",
 *       searchString: 'onclick="doSomething()"',
 *       DOM0: true,
 *       capturing: true
 *     }
 *   }
 */

Try:
https://tbpl.mozilla.org/?tree=Try&rev=f8b79a0f4494

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: visual-events-jquery-support-1044932.patch

Moved DOM event parser out into event parser array and optimised it a little.

We have concluded that creating a scope path is the only real way to get at the original function and that there is no magic bullet here. Exposing this discovery logic via an event parser is the only real way to handle this as we may sometimes want to see a bound or proxied function.

New try:
https://tbpl.mozilla.org/?tree=Try&rev=a22f1b0c6d26

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

In summary:
1. Library events are not necessarily real events. They often have a listener on the document that bubbles through all elements. When an element matches a selector they call a handler that then evaluates a bunch of functions and calls the appropriate one. This means that adding breakpoints when event listeners are added is pointless as only one may be added at the start of document load.
2. Proxied function often have a bunch of hanging variables assigned to event handlers. There is no magic way to know which event handler to use. Allowing a library author to define a path is the only realistic way to make this work... this is the method we are using.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: visual-events-jquery-support-1044932.patch

I have improved the heuristics to determine whether jQuery is loaded.

Joe has also requested that somebody else take a look over this so I will ask pbrosset for review.

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8474500 [details] [diff] [review]
visual-events-jquery-support-1044932.patch

Review of attachment 8474500 [details] [diff] [review]:
-----------------------------------------------------------------

I think the approach is fine, it opens up a nice way to support special event handling libraries. And having the code to support jQuery from the start is the right move.

I have some remarks about the code, mostly revolving around:
- removing/sharing lines of code
- making sure our registration/unregistration API is at the right place

::: browser/devtools/framework/gDevTools.jsm
@@ +151,5 @@
> +   * @param {Object} parser
> +   *        Each parser must contain the following properties:
> +   *        - id: Unique identifier for this parser (string) e.g. jQuery
> +   *        - parser: The actual function used to obtain the libraries event
> +   *                  handlers.

The longer comment you added in devtools/main.js should be here instead. This is the customizer-facing function where documentation is needed.

@@ +153,5 @@
> +   *        - id: Unique identifier for this parser (string) e.g. jQuery
> +   *        - parser: The actual function used to obtain the libraries event
> +   *                  handlers.
> +   */
> +  registerEventParser: function(parserObj) {

Is gDevTools supposed to become our add-on customization API entry point? Can you check with Honza and/or Gozala? It would be nice to prevent this object from becoming a dumping ground for all custom feature registration. But maybe there isn't a better way?

@@ +158,5 @@
> +    let parserDesc = parserObj.desc;
> +    let parserGUID = parserObj.guid;
> +
> +    if (!parserDesc || !parserGUID || this._eventParsers.has(parserGUID)) {
> +      throw new Error("Invalid parserObj.guid");

Make this error more self-explanatory. The word parserObj is too generic and it might be hard for an addon developer to relate this to the custom event parser.
Something like "Cannot register new event parser with unique ID parserObj.guid".
It would be nicer if the message was different if the ID already exists, logging out the description of the existing ID, so that the addon developer can understand.

@@ +175,5 @@
> +   * @param {String} parserGUID
> +   *        GUID of the event parser to unregister.
> +   * @param {boolean} isQuitApplication
> +   *        true to indicate that the call is due to app quit, so we should not
> +   *        cause a cascade of costly events

I don't understand this last parameter. Also it's not used right now, so you should remove it anyway.

@@ +182,5 @@
> +    if (!this._eventParsers.has(parserGUID)) {
> +      return;
> +    }
> +
> +    let {desc} = this._eventParsers.get(parserGUID);

This variable isn't used.

::: browser/devtools/main.js
@@ +363,5 @@
>  }
>  
> +/*******************************************************************************
> + * Register event parsers START
> + ******************************************************************************/

We usually don't have big comment separators like this in the codebase.
Using one of those usually means that whatever is in-between the start/end blocks should be in a separate module. This would help reduce the file size and better separate responsibilities.

Also, why is this done in main.js? main.js is pretty small so far and only cares about registering tools/panels. It would be great to keep it as such.

So moving this in a separate module is, I think, necessary AND calling and executing that module from the inspector-panel sounds like a better fit too. Why would you need to register custom event parsers when the inspector isn't loaded?

@@ +369,5 @@
> + * An event parser must take the following form:
> + *   {
> + *     desc {String}: "jQuery events", // Short string explaining which library
> + *                                        the parser works with.
> + *     guid {String}: "168280ed-9d8e-42b9-a515-6858df295ae6", // Unique ID

Do we really need 2 IDs in this API? Wouldn't 'guid' be enough? I guess library authors could pass values like guid:"jquery1.4".

@@ +374,5 @@
> + *     getListeners: function(node) { },     // Function that takes a node and
> + *                                           // returns an array of eventInfo
> + *                                           // objects (see below).
> + *
> + *     getHasListeners: function(node) { },  // Optional function that takes a

Why 'getHasListeners' and not simply 'hasListeners' ? I think this a better convention for boolean returning functions.
Also, can't we just deduce this by calling getListeners and checking for an empty array?

@@ +395,5 @@
> + * An eventInfo object should take the following form (see getListeners):
> + *   {
> + *     type {String}:      "click",
> + *     handler {Function}: event handler,
> + *     tags {String}:      "jQuery,Live", // These tags will be displayed as

If we make guid a simple ID string up to the lib author to define, we can use it here. Not instead of the tags though, next to them, I think tags are a good idea here.

@@ +398,5 @@
> + *     handler {Function}: event handler,
> + *     tags {String}:      "jQuery,Live", // These tags will be displayed as
> + *                                        // attributes in the events popup.
> + *     hide: {
> + *       dom0 {Boolean}:   true           // Hide or show fields.

You should list all fields that can be hidden or give a reference to the location in the code where they are listed.

@@ +417,5 @@
> +    guid: "168280ed-9d8e-42b9-a515-6858df295ae6",
> +    getListeners: function(node) {
> +      let global = node.ownerGlobal.wrappedJSObject;
> +      let jQuery = global.jQuery && global.jQuery.fn &&
> +                   global.jQuery.fn.jquery && global.jQuery;

Is checking for global.jQuery.fn.jquery really needed?
Also, checking twice for global.jQuery is a bit weird I'd say. I get that you're doing this to assign its value to the jQuery variable, but I think this code would be clearer for future maintainers as such:

let hasJQuery = global.jQuery && global.jQuery.fn && global.jQuery.fn.jquery;
let jQuery = global.jQuery;

if (!hasJQuery) {
  return;
}

...

@@ +484,5 @@
> +  },
> +  {
> +    desc: "jQuery live events",
> +    guid: "a0fcbdc9-d3bc-4f22-accd-a07f04b6bdf1",
> +    getHasListeners: function(node) {

There seems to be large copy/pasted blocks of code between this function and getListeners. Please put them in common in one function.
Or, as one my earlier comments suggests, just use one function: 'getListeners' and deduce if the element has listeners by checking if the returned array has length 0.

@@ +649,5 @@
> +      return false;
> +    },
> +    getListeners: function(node) {
> +      let handlers = [];
> +      let listeners = eventListenerService.getListenerInfoFor(node);

Why don't you need to check if node is <html> here like you did in getHasListeners?
Also getListeners and getHasListeners should share more code here too.

@@ +673,5 @@
> +    }
> +  }
> +];
> +
> +function handlerRegistered(handlers, eventInfo) {

Function should be named 'isHandlerRegistered' and should have a jsdoc comment to explain what it does.
Also, why is this needed, and only needed for jQuery 1.2+?

::: browser/devtools/markupview/test/browser.ini
@@ +36,5 @@
>  [browser_markupview_copy_image_data.js]
>  [browser_markupview_css_completion_style_attribute.js]
>  [browser_markupview_events.js]
>  skip-if = e10s # Bug 1040751 - CodeMirror editor.destroy() isn't e10s compatible
> +[browser_markupview_events_jquery_1.0.js]

Do we really need to test for all minor updates of this library? I'm guessing the way event handlers are managed in jQuery didn't change with each and every minor update!
I would be happy with just testing a few 1.x and 2.x, depending on which one refactored the way events are handled.

::: browser/devtools/markupview/test/browser_markupview_events_jquery_1.0.js
@@ +1,4 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +

Comments in this file go for all browser_markupview_events_jquery_x.x.js

At least 50% of the code of all these tests is copy/pasted, please share code. It especially easy for promiseNextTick and checkEventsForNode that are quite generic and separated already. Just give them self-explanatory names and add jsdoc comments and move them to head.js.

One option to reduce file size is to use a test "helper". The way to go is to move the common test runner part into a helper file (see helper_outerhtml_test_runner/js for instance) and leave in the test itself only the things that are different. You would usually leave those non common parts as test array.

You can load helper scripts with: loadHelperScript("helper_eventpopup_test_runner.js");

@@ +1,5 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";

Please add a one line comment just below "use strict"; that explains what the test is supposed to do.

@@ +3,5 @@
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +const TEST_URL = TEST_URL_ROOT + "doc_markup_events_jquery.html?lib_jquery_1.0.js";

Please define const TEST_LIB = "lib_jquery_1.0.js"; to avoid repeating this string in several places in the file.

@@ +10,5 @@
> +  let {inspector} = yield addTab(TEST_URL).then(openInspector);
> +
> +  yield inspector.markup.expandAll();
> +
> +  yield checkEventsForNode("html", [

If you were using a test runner helper as advised, your test data could be defined as an array like:

const TEST_DATA = [{
  selector: "html",
  handlers: [
    type: "load",
    filename: TEST_URL_ROOT + TEST_LIB,
    ...
  ]
}, {
  ...
}];

And your test code would just be something like: yield runEventPopupTests(TEST_DATA, inspector); (assuming you define a test runner helper function named runEventPopupTests in your new helper file).

@@ +17,5 @@
> +      filename: TEST_URL_ROOT + "lib_jquery_1.0.js",
> +      attributes: [
> +        "jQuery"
> +      ],
> +      handler: "// Handle when the DOM is ready\n" +

I think we could be smarter about testing the handler string here. The ability of the popup + codemirror to display the code isn't going to change depending on which version of jquery is being tested. I think you should add one simple test to the markupview test folder that only tests this once, and then just remove these checks in all of the browser_markupview_events_jquery_x.x.js files.
In fact, these tests files should only be checking whether the event popup shows the right number of events on the right node.

Moving generic functions to head.js, moving the common test running part to a helper, and removing/simplifying what is actually tested should help bring down the number of lines of code quite a lot.

::: toolkit/devtools/server/actors/inspector.js
@@ +289,5 @@
>      }
>    },
>  
>    /**
>     * Are event listeners that are listening on this node?

Are *there* event listeners that are listening on this node?

@@ +292,5 @@
>    /**
>     * Are event listeners that are listening on this node?
>     */
>    get _hasEventListeners() {
> +    // Other event listeners (provided by gDevTools.registerEventParser)

This comment is misleading. Why "other"?
I think it should explain that here, all event parsers defined via gDevTools.registerEventParser are being run to decide whether the node has any listeners. It should also give the reference to the location in the code where this is done.

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: visual-events-jquery-support-1044932.patch

Try:
https://tbpl.mozilla.org/?tree=Try&rev=d0ab5f98bbb7

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #7)
> Comment on attachment 8474500 [details] [diff] [review]
> visual-events-jquery-support-1044932.patch
> 
> Review of attachment 8474500 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the approach is fine, it opens up a nice way to support special
> event handling libraries. And having the code to support jQuery from the
> start is the right move.
> 
> I have some remarks about the code, mostly revolving around:
> - removing/sharing lines of code
> - making sure our registration/unregistration API is at the right place
> 
> ::: browser/devtools/framework/gDevTools.jsm
> @@ +151,5 @@
> > +   * @param {Object} parser
> > +   *        Each parser must contain the following properties:
> > +   *        - id: Unique identifier for this parser (string) e.g. jQuery
> > +   *        - parser: The actual function used to obtain the libraries event
> > +   *                  handlers.
> 
> The longer comment you added in devtools/main.js should be here instead.
> This is the customizer-facing function where documentation is needed.
> 

Moved

> @@ +153,5 @@
> > +   *        - id: Unique identifier for this parser (string) e.g. jQuery
> > +   *        - parser: The actual function used to obtain the libraries event
> > +   *                  handlers.
> > +   */
> > +  registerEventParser: function(parserObj) {
> 
> Is gDevTools supposed to become our add-on customization API entry point?
> Can you check with Honza and/or Gozala? It would be nice to prevent this
> object from becoming a dumping ground for all custom feature registration.
> But maybe there isn't a better way?
> 

We created gDevTools for exactly that purpose (to be the global extension point for our tools). That is why registerTools is there. Anyhow, Gozala and Honza have just landed the theme switching there too.

> @@ +158,5 @@
> > +    let parserDesc = parserObj.desc;
> > +    let parserGUID = parserObj.guid;
> > +
> > +    if (!parserDesc || !parserGUID || this._eventParsers.has(parserGUID)) {
> > +      throw new Error("Invalid parserObj.guid");
> 
> Make this error more self-explanatory. The word parserObj is too generic and
> it might be hard for an addon developer to relate this to the custom event
> parser.
> Something like "Cannot register new event parser with unique ID
> parserObj.guid".

Done

> It would be nicer if the message was different if the ID already exists,
> logging out the description of the existing ID, so that the addon developer
> can understand.
> 

Done

> @@ +175,5 @@
> > +   * @param {String} parserGUID
> > +   *        GUID of the event parser to unregister.
> > +   * @param {boolean} isQuitApplication
> > +   *        true to indicate that the call is due to app quit, so we should not
> > +   *        cause a cascade of costly events
> 
> I don't understand this last parameter. Also it's not used right now, so you
> should remove it anyway.
> 

Just copying unregisterTool although because we don't need an event it was pointless so I have removed it.

> @@ +182,5 @@
> > +    if (!this._eventParsers.has(parserGUID)) {
> > +      return;
> > +    }
> > +
> > +    let {desc} = this._eventParsers.get(parserGUID);
> 
> This variable isn't used.
> 

Removed

> ::: browser/devtools/main.js
> @@ +363,5 @@
> >  }
> >  
> > +/*******************************************************************************
> > + * Register event parsers START
> > + ******************************************************************************/
> 
> We usually don't have big comment separators like this in the codebase.
> Using one of those usually means that whatever is in-between the start/end
> blocks should be in a separate module. This would help reduce the file size
> and better separate responsibilities.
> 

Agreed, moved.

> Also, why is this done in main.js? main.js is pretty small so far and only
> cares about registering tools/panels. It would be great to keep it as such.
> 

And registering Themes... although I completely agree, we should probably break it up.

I have moved the events stuff into it's own module.

> So moving this in a separate module is, I think, necessary AND calling and
> executing that module from the inspector-panel sounds like a better fit too.
> Why would you need to register custom event parsers when the inspector isn't
> loaded?
> 

Event info and tooltips can be used from any tool so it doesn't make sense to only allow them to be used from the inspector panel e.g. GCLI commands, the debugger or custom tools may want to make use of them. We should probably consider moving the event into it's own actor though to make this clear.

> @@ +369,5 @@
> > + * An event parser must take the following form:
> > + *   {
> > + *     desc {String}: "jQuery events", // Short string explaining which library
> > + *                                        the parser works with.
> > + *     guid {String}: "168280ed-9d8e-42b9-a515-6858df295ae6", // Unique ID
> 
> Do we really need 2 IDs in this API? Wouldn't 'guid' be enough? I guess
> library authors could pass values like guid:"jquery1.4".
> 

Switched to only using an ID.

> @@ +374,5 @@
> > + *     getListeners: function(node) { },     // Function that takes a node and
> > + *                                           // returns an array of eventInfo
> > + *                                           // objects (see below).
> > + *
> > + *     getHasListeners: function(node) { },  // Optional function that takes a
> 
> Why 'getHasListeners' and not simply 'hasListeners' ? I think this a better
> convention for boolean returning functions.

I thought I had already changed that, done.

> Also, can't we just deduce this by calling getListeners and checking for an
> empty array?
> 

We could but that would be way, way, way less efficient.

Checking for listeners just adds the event bubble in the markup view if a node has one or more events. We loop till we find an event then bail out.

Getting the actual events is not done until the tooltip is opened and involves a lot of costly stuff like initializing the debugger, getting info for all events etc.

> @@ +395,5 @@
> > + * An eventInfo object should take the following form (see getListeners):
> > + *   {
> > + *     type {String}:      "click",
> > + *     handler {Function}: event handler,
> > + *     tags {String}:      "jQuery,Live", // These tags will be displayed as
> 
> If we make guid a simple ID string up to the lib author to define, we can
> use it here. Not instead of the tags though, next to them, I think tags are
> a good idea here.
> 

Switched to only using an ID.

> @@ +398,5 @@
> > + *     handler {Function}: event handler,
> > + *     tags {String}:      "jQuery,Live", // These tags will be displayed as
> > + *                                        // attributes in the events popup.
> > + *     hide: {
> > + *       dom0 {Boolean}:   true           // Hide or show fields.
> 
> You should list all fields that can be hidden or give a reference to the
> location in the code where they are listed.
> 

Done

> @@ +417,5 @@
> > +    guid: "168280ed-9d8e-42b9-a515-6858df295ae6",
> > +    getListeners: function(node) {
> > +      let global = node.ownerGlobal.wrappedJSObject;
> > +      let jQuery = global.jQuery && global.jQuery.fn &&
> > +                   global.jQuery.fn.jquery && global.jQuery;
> 
> Is checking for global.jQuery.fn.jquery really needed?
> Also, checking twice for global.jQuery is a bit weird I'd say. I get that
> you're doing this to assign its value to the jQuery variable, but I think
> this code would be clearer for future maintainers as such:
> 
> let hasJQuery = global.jQuery && global.jQuery.fn && global.jQuery.fn.jquery;
> let jQuery = global.jQuery;
> 
> if (!hasJQuery) {
>   return;
> }
> 
> ...
> 

Done

> @@ +484,5 @@
> > +  },
> > +  {
> > +    desc: "jQuery live events",
> > +    guid: "a0fcbdc9-d3bc-4f22-accd-a07f04b6bdf1",
> > +    getHasListeners: function(node) {
> 
> There seems to be large copy/pasted blocks of code between this function and
> getListeners. Please put them in common in one function.

Done

> Or, as one my earlier comments suggests, just use one function:
> 'getListeners' and deduce if the element has listeners by checking if the
> returned array has length 0.
> 

No, that would be a huge resource hog.

> @@ +649,5 @@
> > +      return false;
> > +    },
> > +    getListeners: function(node) {
> > +      let handlers = [];
> > +      let listeners = eventListenerService.getListenerInfoFor(node);
> 
> Why don't you need to check if node is <html> here like you did in
> getHasListeners?

I have added a comment explaining:
// The Node actor's getEventListenerInfo knows that when an html tag has
// been passed we need the window object so we don't need to account for
// event hoisting here as we did in hasListeners.

> Also getListeners and getHasListeners should share more code here too.
> 

Done

> @@ +673,5 @@
> > +    }
> > +  }
> > +];
> > +
> > +function handlerRegistered(handlers, eventInfo) {
> 
> Function should be named 'isHandlerRegistered' and should have a jsdoc
> comment to explain what it does.
> Also, why is this needed, and only needed for jQuery 1.2+?
> 

This function was historic and is no longer needed. Removed.

> ::: browser/devtools/markupview/test/browser.ini
> @@ +36,5 @@
> >  [browser_markupview_copy_image_data.js]
> >  [browser_markupview_css_completion_style_attribute.js]
> >  [browser_markupview_events.js]
> >  skip-if = e10s # Bug 1040751 - CodeMirror editor.destroy() isn't e10s compatible
> > +[browser_markupview_events_jquery_1.0.js]
> 
> Do we really need to test for all minor updates of this library? I'm
> guessing the way event handlers are managed in jQuery didn't change with
> each and every minor update!
> I would be happy with just testing a few 1.x and 2.x, depending on which one
> refactored the way events are handled.
> 

This was a very small sample of jQuery versions but I have now cut the list to
only include versions where there have been significant changes to the event and
proxy systems.

Removed:
1.5
1.8
1.9
1.1.0
1.1.1
2.0.0
2.1.0

> ::: browser/devtools/markupview/test/browser_markupview_events_jquery_1.0.js
> @@ +1,4 @@
> > +/* vim: set ts=2 et sw=2 tw=80: */
> > +/* Any copyright is dedicated to the Public Domain.
> > + http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> 
> Comments in this file go for all browser_markupview_events_jquery_x.x.js
> 
> At least 50% of the code of all these tests is copy/pasted, please share
> code. It especially easy for promiseNextTick and checkEventsForNode that are
> quite generic and separated already. Just give them self-explanatory names
> and add jsdoc comments and move them to head.js.
> 

promiseNextTick has been moved to head.js but I have moved checkEventsForNode
into a test helper.

> One option to reduce file size is to use a test "helper". The way to go is
> to move the common test runner part into a helper file (see
> helper_outerhtml_test_runner/js for instance) and leave in the test itself
> only the things that are different. You would usually leave those non common
> parts as test array.
> 
> You can load helper scripts with:
> loadHelperScript("helper_eventpopup_test_runner.js");
> 

Done

> @@ +1,5 @@
> > +/* vim: set ts=2 et sw=2 tw=80: */
> > +/* Any copyright is dedicated to the Public Domain.
> > + http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +"use strict";
> 
> Please add a one line comment just below "use strict"; that explains what
> the test is supposed to do.
> 

Done

> @@ +3,5 @@
> > + http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +"use strict";
> > +
> > +const TEST_URL = TEST_URL_ROOT + "doc_markup_events_jquery.html?lib_jquery_1.0.js";
> 
> Please define const TEST_LIB = "lib_jquery_1.0.js"; to avoid repeating this
> string in several places in the file.
> 

Done

> @@ +10,5 @@
> > +  let {inspector} = yield addTab(TEST_URL).then(openInspector);
> > +
> > +  yield inspector.markup.expandAll();
> > +
> > +  yield checkEventsForNode("html", [
> 
> If you were using a test runner helper as advised, your test data could be
> defined as an array like:
> 
> const TEST_DATA = [{
>   selector: "html",
>   handlers: [
>     type: "load",
>     filename: TEST_URL_ROOT + TEST_LIB,
>     ...
>   ]
> }, {
>   ...
> }];
> 
> And your test code would just be something like: yield
> runEventPopupTests(TEST_DATA, inspector); (assuming you define a test runner
> helper function named runEventPopupTests in your new helper file).
> 

Done

> @@ +17,5 @@
> > +      filename: TEST_URL_ROOT + "lib_jquery_1.0.js",
> > +      attributes: [
> > +        "jQuery"
> > +      ],
> > +      handler: "// Handle when the DOM is ready\n" +
> 
> I think we could be smarter about testing the handler string here. The
> ability of the popup + codemirror to display the code isn't going to change
> depending on which version of jquery is being tested. I think you should add
> one simple test to the markupview test folder that only tests this once, and
> then just remove these checks in all of the
> browser_markupview_events_jquery_x.x.js files.
> In fact, these tests files should only be checking whether the event popup
> shows the right number of events on the right node.
> 

The actual handler text does change between all of the jQuery versions that we test. Counting them does not test that unwrapping proxies is working... that is really what is being tested here.

> Moving generic functions to head.js, moving the common test running part to
> a helper, and removing/simplifying what is actually tested should help bring
> down the number of lines of code quite a lot.
> 

Agreed and done.

> ::: toolkit/devtools/server/actors/inspector.js
> @@ +289,5 @@
> >      }
> >    },
> >  
> >    /**
> >     * Are event listeners that are listening on this node?
> 
> Are *there* event listeners that are listening on this node?
> 

Fixed

> @@ +292,5 @@
> >    /**
> >     * Are event listeners that are listening on this node?
> >     */
> >    get _hasEventListeners() {
> > +    // Other event listeners (provided by gDevTools.registerEventParser)
> 
> This comment is misleading. Why "other"?

This was historic, removed.

> I think it should explain that here, all event parsers defined via
> gDevTools.registerEventParser are being run to decide whether the node has
> any listeners. It should also give the reference to the location in the code
> where this is done.

Done

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8479909 [details] [diff] [review]
visual-events-jquery-support-1044932.patch

Review of attachment 8479909 [details] [diff] [review]:
-----------------------------------------------------------------

Changes since last patch look great, thanks.

::: browser/devtools/markupview/test/browser_markupview_events_jquery_1.0.js
@@ +228,5 @@
> +];
> +
> +let test = asyncTest(function*() {
> +  yield runEventPopupTests();
> +});

let test = asyncTest(runEventPopupTests); should work I think.

::: browser/devtools/markupview/test/helper_jquery_events_test_runner.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

This file isn't only about testing jquery event bubbles, right? All types of event bubbles. So it shouldn't be named helper_jquery_events_test_runner.js

::: toolkit/devtools/event-parsers.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +

Can you add a comment here describing what this file contains?

::: toolkit/devtools/server/actors/inspector.js
@@ +365,5 @@
> +   *         The events array contains all event objects that we have gathered
> +   *         so far.
> +   * @param  {Debugger} dbg
> +   *         JSDebugger instance.
> +   * @param  {Object} eventInfo

The documentation for the eventInfo object is already in gDevTools, so you should simply add something like @see gDevTools ....

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: visual-events-jquery-support-1044932.patch

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #9)
> Comment on attachment 8479909 [details] [diff] [review]
> visual-events-jquery-support-1044932.patch
> 
> Review of attachment 8479909 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Changes since last patch look great, thanks.
> 
> ::: browser/devtools/markupview/test/browser_markupview_events_jquery_1.0.js
> @@ +228,5 @@
> > +];
> > +
> > +let test = asyncTest(function*() {
> > +  yield runEventPopupTests();
> > +});
> 
> let test = asyncTest(runEventPopupTests); should work I think.
> 

Of course, changed.

> ::: browser/devtools/markupview/test/helper_jquery_events_test_runner.js
> @@ +1,4 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> 
> This file isn't only about testing jquery event bubbles, right? All types of
> event bubbles. So it shouldn't be named helper_jquery_events_test_runner.js
> 

Renamed.

> ::: toolkit/devtools/event-parsers.js
> @@ +2,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +"use strict";
> > +
> 
> Can you add a comment here describing what this file contains?
> 

Done.

> ::: toolkit/devtools/server/actors/inspector.js
> @@ +365,5 @@
> > +   *         The events array contains all event objects that we have gathered
> > +   *         so far.
> > +   * @param  {Debugger} dbg
> > +   *         JSDebugger instance.
> > +   * @param  {Object} eventInfo
> 
> The documentation for the eventInfo object is already in gDevTools, so you
> should simply add something like @see gDevTools ....

Done

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8480519 [details] [diff] [review]
visual-events-jquery-support-1044932.patch

https://hg.mozilla.org/integration/fx-team/rev/9add1ec0251d

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9add1ec0251d