Closed
Bug 1127238
Opened 9 years ago
Closed 9 years ago
Remove the hard-coded list of highlighters
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 38
People
(Reporter: zer0, Assigned: zer0)
References
Details
Attachments
(1 file, 3 obsolete files)
19.03 KB,
patch
|
zer0
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → zer0
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8556983 -
Flags: review?(pbrosset)
Comment 2•9 years ago
|
||
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.
Attachment #8556983 -
Flags: review?(pbrosset) → review+
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8557068 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8556983 -
Attachment is obsolete: true
Assignee | ||
Comment 4•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c722a3bc9add
Assignee | ||
Comment 5•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5842f3692b21
Attachment #8557068 -
Attachment is obsolete: true
Attachment #8557095 -
Flags: review+
Comment 6•9 years ago
|
||
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.
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 7•9 years ago
|
||
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.
Assignee | ||
Comment 8•9 years ago
|
||
I'll check with Joe about my permission to push on the repo! In the meantime, checkin-needed. :)
Keywords: checkin-needed
Comment 9•9 years ago
|
||
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!
Flags: needinfo?(zer0)
Keywords: checkin-needed
Comment on attachment 8557095 [details] [diff] [review] highlighter-list3.patch Review of attachment 8557095 [details] [diff] [review]: ----------------------------------------------------------------- I like this approach... awesome work!
Attachment #8557095 -
Flags: review+ → feedback+
Attachment #8557095 -
Flags: review+
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8557095 -
Attachment is obsolete: true
Flags: needinfo?(zer0)
Attachment #8558474 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/2dc360586545
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2dc360586545
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Depends on: 1132255
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•