1127238 - Remove the hard-coded list of highlighters

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Matteo Ferretti [:zer0] [:matteo]]

Currently we have the custom highlighters' list hard-coded in two different places (highlighter.js and root.js); we should make highlighters be able to register themselves, so that those two list won't be necessary anymore, make easier to maintain for us, and make possible for developers implement their own highlighters.

Comment 1

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: highlighter-list.patch

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8556983 [details] [diff] [review]
highlighter-list.patch

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

Cool! These changes look good to me.
As discussed in person today, I only have a few nits and I know you have a patch to correct those.
I have tested the patch locally both on the same desktop version and remotely connected to an older backend, everything worked as expected.
So, r=me with the discussed nit changes.
Let's push to try to see if everything's fine.

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: highlighter-list2.patch

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c722a3bc9add

Comment 5

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: highlighter-list3.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5842f3692b21

Comment 6

[Patrick Brosset <:pbro>]

Alright, great work Matteo, try is all green, patch is R+. You can either push the patch to fx-team if you can or add the checkin-needed keyword to the bug.

Comment 7

[Patrick Brosset <:pbro>]

CCing bgrins and miker: I think you should know about this change. With this patch, it'll be easier for addons to register new highlighters as the list of supported highlighters isn't hard-coded anymore. Highlighter classes register themselves instead.

Comment 8

[Matteo Ferretti [:zer0] [:matteo]]

I'll check with Joe about my permission to push on the repo! In the meantime, checkin-needed. :)

Comment 9

[Carsten Book [:Tomcat]]

Hi, this patch didn't apply cleanly:

adding 1127238 to series file
renamed 1127238 -> highlighter-list3.patch
applying highlighter-list3.patch
patching file toolkit/devtools/gcli/commands/highlight.js
Hunk #1 FAILED at 1
1 out of 1 hunks FAILED -- saving rejects to file toolkit/devtools/gcli/commands/highlight.js.rej

could you take a look, thanks!

Comment 10

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

Comment on attachment 8557095 [details] [diff] [review]
highlighter-list3.patch

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

I like this approach... awesome work!

Comment 11

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: highlighter-list4.patch

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/2dc360586545

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2dc360586545