Closed Bug 1315639 Opened 8 years ago Closed 8 years ago

Event listener popup needs rewriting

Categories

(DevTools :: Inspector, defect, P1)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: miker, Assigned: miker)

References

Details

(Whiteboard: [btpp-fix-now])

Attachments

(1 file, 5 obsolete files)

Bug 1315639 - Event listener popup needs rewriting
58 bytes, text/x-review-board-request
pbro
: review+
Details 
There are multiple issues with the event bubbles. A few examples:
- There is no such thing as an event handler in JavaScript. Variable names should be changed to reflect this.
- The code is confusing and needs to be refactored.
- The code itself fails to get listener info in multiple situations.

Making a single change to the way that the event info is processed involves a large amount of changes to multiple tests. In order to avoid rewriting tests a large amount of times we should fix all changes in this bug and make all of the event bugs that it fixes dependencies.
Attachment #8808114 - Attachment description: Event listener popup needs rewriting → [WIP] Event listener popup needs rewriting
Basically rewritten the event processing code.

TODO:

  - Make document listeners available as we don't currently show them.
  - Try to find a way to get to DOM0 events... the target of the event would be ideal.
  - Add comments to make the event processing code easier to follow.
Attachment #8808616 - Attachment description: Event listener popup needs rewriting → [WIP] Event listener popup needs rewriting
Attachment #8808114 - Attachment is obsolete: true
Preparing for a rebase.
Attachment #8808616 - Attachment is obsolete: true
Attachment #8809073 - Attachment is obsolete: true
Changes:
  - Refactored all event bubble logic.
  - Added diffing system so that it is easy to see which line of code is not showing correctly in event bubble tests (saves a serious amount of time).
  - Fixed an issue that prevented clicks of the debugger icon from opening the file:line in the debugger.
  - Hide the debugger icon when a script is not available e.g. DOM0 events or the listener itself is native code i.e. addEventListener("click", array.sort).
  - Show native code listeners more clearly by including any parameter names.
  - Show listeners added to document and documentElement.
  - Reliably detect DOM0 events.
  - Tooltip no longer shrinks for short filenames.
  - No more empty popups.
  - We no longer use a regular expression to get the function source. This means that we may lose prefix strings e.g. funcname: function() { ... } will be missing funcname:. This is because there is no reliable way to get the information without causing a bunch of issues. The new method is much more robust and future proof.
TODO:

- Rebase this ready for review.
Attachment #8809080 - Attachment is obsolete: true
Changes:
  - Double-clicking an "Event Listeners" button (ev) shouldn't expand / collapse a node.
Attachment #8809382 - Attachment is obsolete: true
@gl: The patch is big but you don't need to worry about the tests, which take up the bulk of the patch. Fixing all these bugs in one go saves me from needing to rewrite all the tests for each bug.
Flags: needinfo?(gl)
Ok, will look into it in the morning
Flags: needinfo?(gl)
Comment on attachment 8809383 [details]
Bug 1315639 - Event listener popup needs rewriting

https://reviewboard.mozilla.org/r/91996/#review92246

::: devtools/client/inspector/markup/test/browser.ini:87
(Diff revision 1)
>  [browser_markup_dragdrop_reorder.js]
>  [browser_markup_dragdrop_tooltip.js]
>  [browser_markup_events1.js]
>  [browser_markup_events2.js]
>  [browser_markup_events3.js]
> +[browser_markup_events4.js]

The naming of these browser_markup_eventsX.js test don't quite conform with the naming structure. Can we rename all these browser_markup_eventsX.js test to follow the browser_markup_events_0X.js standard?

::: devtools/client/inspector/markup/test/browser_markup_events1.js:26
(Diff revision 1)
>          filename: TEST_URL,
>          attributes: [
>            "Bubbling",
>            "DOM0"
>          ],
> -        handler: "init();"
> +        handler: "function onload(event) {\n" +

Perhaps consider using multiline strings for all handler if possible, but not necessary to do this in this bug.

::: devtools/client/inspector/markup/views/markup-container.js:676
(Diff revision 1)
>        focusable.focus();
>      }
>    },
>  
>    _onToggle: function (event) {
> +    if (event.target.hasAttribute("data-event")) {

Add a comment to explain why we bail out for an element with data-event.

::: devtools/client/shared/widgets/tooltip/EventTooltipHelper.js:14
(Diff revision 1)
>  const {LocalizationHelper} = require("devtools/shared/l10n");
>  const L10N = new LocalizationHelper("devtools/client/locales/inspector.properties");
>  
>  const Editor = require("devtools/client/sourceeditor/editor");
>  const beautify = require("devtools/shared/jsbeautify/beautify");
> +const viewSource = require("devtools/client/shared/view-source");

Move this above Editor to keep the require paths sorted alphabetically.
Comment on attachment 8809383 [details]
Bug 1315639 - Event listener popup needs rewriting

https://reviewboard.mozilla.org/r/91996/#review95470
Attachment #8809383 - Flags: review?(pbrosset)
Attachment #8809383 - Flags: review?(gl)
Comment on attachment 8809383 [details]
Bug 1315639 - Event listener popup needs rewriting

https://reviewboard.mozilla.org/r/91996/#review95466

Looking great! I have tested this a bit locally and it seems to work great. I've tested a few of the STRs of the dependent bugs and they seem to be fixed now.
Good work on this.

I do have a few comments that I'd like to see addressed before I give R+.
Also, getting a green try build might be hard, where are you on this?

::: devtools/client/inspector/markup/views/markup-container.js:676
(Diff revision 1)
>        focusable.focus();
>      }
>    },
>  
>    _onToggle: function (event) {
> +    if (event.target.hasAttribute("data-event")) {

If it's a data attribute, you might as well use `dataSet` instead:
`if (event.target.dataSet.event)`

::: devtools/server/actors/inspector.js:613
(Diff revision 1)
>      let eventObj = {
> -      type: typeof override.type !== "undefined" ? override.type : type,
> -      handler: functionSource.trim(),
> -      origin: typeof override.origin !== "undefined" ?
> -                     override.origin : origin,
> -      searchString: typeof override.searchString !== "undefined" ?
> +      type: override.type || type,
> +      handler: override.handler || functionSource.trim(),
> +      origin: override.origin || origin,
> +      tags: override.tags || tags,
> +      dom0: typeof override.dom0 !== "undefined" ? override.dom0 : dom0,

This event object is sent to the front-end, right? So if you change `DOM0` to `dom0`, then you're making a backward incompatible change that will break when connecting to old servers.
For such a simple case change, I don't think it's worth going through the trouble, on the front-end, to have a `if/else` logic depending on what the server returns, so I suggest just keeping this as `DOM0`.

::: devtools/shared/diff.js:1
(Diff revision 1)
> +/**

I'm not comfortable adding this to devtools/shared.
We don't currently need to diff anything apart from inside your tests. And when we'll need to do it, we might want to consider other diff libraries perhaps. I'm not saying that this one doesn't get the job done, but there might be perf implications that we would need to spend time thinking about then.
So, let's not lose time on this now and only keep this for your tests. So let's move it next to helper_events_test_runner.js, inside a test folder.

::: devtools/shared/diff.js:5
(Diff revision 1)
> +/**
> + * This diff utility is taken from:
> + * https://github.com/Slava/diff.js
> + *
> + * The MIT License (MIT)

Do we need a license review and also add this to about:license ?

::: devtools/shared/diff.js:286
(Diff revision 1)
> +    if (equals(item, arr[i]))
> +      return i;
> +  return -1;
> +};
> +
> +function textDiff(text1, text2) {

Code after this line is code that you have added to the file. You should probably add a comment here (and at the top of the file) explaining to people who might want to update from upstream that they need to avoid overriding this part.

::: devtools/shared/diff.js:290
(Diff revision 1)
> +
> +function textDiff(text1, text2) {
> +  return diff(text1.split("\n"), text2.split("\n"));
> +}
> +
> +function testDiff(text1, text2, msg, ok) {

Please add some jsdoc comment for this function.

::: devtools/shared/diff.js:298
(Diff revision 1)
> +  if (text1 === text2) {
> +    ok(true, msg);
> +    return;
> +  }
> +
> +  let result = textDiff(text1, text2);

Not sure if textDiff is a very useful function to export (or even to define for that matter). You could just do the split on `\n` here instead. `testDiff` is the only function you really need.
Attachment #8809383 - Flags: review?(pbrosset) → review-
Blocks: 1083896
No longer depends on: 1083896
Comment on attachment 8809383 [details]
Bug 1315639 - Event listener popup needs rewriting

https://reviewboard.mozilla.org/r/91996/#review92246

> The naming of these browser_markup_eventsX.js test don't quite conform with the naming structure. Can we rename all these browser_markup_eventsX.js test to follow the browser_markup_events_0X.js standard?

Done... also renamed accompanying html files.

> Perhaps consider using multiline strings for all handler if possible, but not necessary to do this in this bug.

The reason I didn't use multiline strings is that then things don't line up properly e.g.:

```
        handler: `function onload(event) {
  init();
}`
```

Even aligning them can be ugly:
        handler:
```
`function onload(event) {
  init();
}`
```
Comment on attachment 8809383 [details]
Bug 1315639 - Event listener popup needs rewriting

https://reviewboard.mozilla.org/r/91996/#review95466

Green try will be no problem... it was green before addressing these comments.

> If it's a data attribute, you might as well use `dataSet` instead:
> `if (event.target.dataSet.event)`

dataset should be lower case but done.
Comment on attachment 8809383 [details]
Bug 1315639 - Event listener popup needs rewriting

https://reviewboard.mozilla.org/r/91996/#review95538

Thanks for addressing my comments.
Attachment #8809383 - Flags: review?(pbrosset) → review+
Comment on attachment 8809383 [details]
Bug 1315639 - Event listener popup needs rewriting

Gerv, Mike changed about:license here. I don't know if a review is necessary, but pinging you to be on the safe side.
Attachment #8809383 - Flags: review?(gerv)
Gerv, I changed about:license here to add diff.js (MIT License). I don't think a review is necessary, but pinging you to be on the safe side.
Flags: needinfo?(gerv)
Comment on attachment 8809383 [details]
Bug 1315639 - Event listener popup needs rewriting

https://reviewboard.mozilla.org/r/91996/#review95698

::: toolkit/content/license.html:2857
(Diff revision 3)
> +<pre>
> +This diff utility is taken from:
> +https://github.com/Slava/diff.js
> +
> +The MIT License (MIT)
> +

Remove the 5 lines above this comment, please. Otherwise fine.
Attachment #8809383 - Flags: review+
Flags: needinfo?(gerv)
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4802b32ee10b
Event listener popup needs rewriting r=gerv,pbro
https://hg.mozilla.org/mozilla-central/rev/4802b32ee10b
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: