1673932 - Use absolute paths in server/actors/highlighters.js register helper

Status: RESOLVED FIXED

(DevTools :: Inspector, task, P3)

Description

[Julian Descottes [:jdescottes]]

The register helper in devtools/server/actors/highlighters.js allows to register highlighter modules. Example:

register("TabbingOrderHighlighter", "tabbing-order");

Then, in the helper, it concatenates the provided string with "devtools/server/actors/highlighters/" to build the path to the module.

(https://searchfox.org/mozilla-central/rev/a147181ece866c1ecd176ac49f112785f960aac0/devtools/server/actors/highlighters.js#72 )

This makes it tricky to know if a module is used or not. We switched to full paths instead of relative paths in DevTools, I think we should update the helper to use full paths as well. (https://bugzilla.mozilla.org/show_bug.cgi?id=1596686)

Note: we might consider renaming it to something easier to search for as well (eg registerHighlighterModule)

Comment 1

[Cody Welsh]

Is the module referenced anywhere else? I couldn't find anything that used it, but just wanted to make sure. Otherwise, seems like a quick one. (Maybe we could even do all the register lines at the end from an array. Not sure if that's worth the extra syntax, though.)

Comment 2

[Julian Descottes [:jdescottes]]

Hi!

(In reply to Cody Welsh from comment #1)

Is the module referenced anywhere else? I couldn't find anything that used it, but just wanted to make sure.

Which module do you mean here? You can find the require calls for devtools/server/actors/highlighters.js: https://searchfox.org/mozilla-central/search?q=%22devtools%2Fserver%2Factors%2Fhighlighters%22&path=&case=false&regexp=false

If you meant "tabbing-order", the corresponding highlighter is currently still under review (I spotted this while doing the review actually).

Otherwise, seems like a quick one.

The register calls to update are at https://searchfox.org/mozilla-central/search?q=register%28%22&case=true&path=devtools%2Fserver
Let me know if you are interested in taking the bug :) Happy to assign it to you.

(Maybe we could even do all the register lines at the end from an array. Not sure if that's worth the extra syntax, though.)

Why not! Note that we have one (soon 2) calls outside of devtools/server/actors/highlighters.js, so we should still allow to call this from several places. And if we go for that, let's split in 2 changesets, would be easier to review.

Comment 3

[Cody Welsh]

If you meant "tabbing-order"

Yup. I thought perhaps it was just an example, but good to know, haha.

Let me know if you are interested in taking the bug :) Happy to assign it to you.

Sure thing! :) I'll go ahead and make all the changes with the last bit of info in mind, and report back with any struggles I may or may not encounter.

Comment 4

[Julian Descottes [:jdescottes]]

great, assigning the bug to you :) Thanks!

Comment 5

[Cody Welsh]

Thanks!

When trying to see if the tests still run, I'm getting the following result for those that reference highlighters.js even without making any changes:

devtools/server/tests/browser/browser_canvasframe_helper_04.js
  FAIL Uncaught exception - at chrome://mochitests/content/browser/devtools/server/tests/browser/browser_canvasframe_helper_04.js:76 - TypeError: target.addEventListener is not a function

Are these particular tests not functional right now? Just wondering what I should use to test my changes against :)

Comment 6

[Julian Descottes [:jdescottes]]

Sorry about that! This test is currently broken and normally disabled whenever "e10s/electrolysis" is enabled (which is basically always true these days)

https://searchfox.org/mozilla-central/rev/96e2c6e14998f38e419850d55d8a3d32a3fc244a/devtools/server/tests/browser/browser.ini#111-112

[browser_canvasframe_helper_04.js]
skip-if = e10s # Bug 1183605 - devtools/server/tests/browser/ tests are still disabled in E10S

But if you try to run a test explicitly, the "skip-if" rules are ignored and the test tries to run.

My advice here would be to simply build devtools, do a quick manual test of the inspector (try to highlight a node etc...), maybe run a few mochitests (eg tests under devtools/client/inspector/test/browser_inspector_highlighter*). And then push your change to our continuous integration server (try). If you have commit access level 1 (https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/), you should be able to push to try (mach try --preset devtools). If you don't have commit access level 1, you can push your patch to phabricator and someone will push it to try for you.

Comment 7

[Cody Welsh]

Alright, thanks for the info! Just wanted to make sure something wasn't wrong on my end.

I'll go ahead and run the inspector tests mentioned along with some manual testing. I don't have any access, so I'll have to push something to Phab first, like you mentioned.

Comment 8

[Cody Welsh]

Attachment: Bug 1673932 - [devtools] - Use absolute paths in highlighters register helper. r=jdescottes

Change the logic in the devtools/server/actors/highlighters.js file to
use absolute paths instead of constructing them from an internal helper
function. Also modify the calls to register at the bottom of the file
according to this change.

Comment 9

[Cody Welsh]

Attachment: Bug 1673932 - [devtools] - Combine `register` calls in highlighters. r=jdescottes

Combine the calls to the highlighter register() function in a loop.
One of several possible solutions here.

Depends on D96301

Comment 10

[Cody Welsh]

Made some commits and pushed the diffs to Phab. The diff relating to combining all the lines at the end has a bunch of possible solutions, but I put one of the more simple ones in it. Other options I can think of:

• Make the paths absolute everywhere else, but local within this specific file
• Keep the slightly weird syntax that results from longer line lengths with the full paths in register (linter expands them to 4 lines instead of 1 in just a few of them

I'm sure there are a bunch of other ways, too. Maybe the strange result mentioned is acceptable. :)

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/03301bdb32e6
[devtools] - Use absolute paths in highlighters register helper. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/ab99e22cab23
[devtools] - Combine `register` calls in highlighters.  r=jdescottes

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/03301bdb32e6
https://hg.mozilla.org/mozilla-central/rev/ab99e22cab23