Closed Bug 1273967 Opened 4 years ago Closed 4 years ago
Tools command line handler via category entry
MozReview Request: Bug 1273967 - Include DevTools command line handler via category entry. r=ochameau
58 bytes, text/x-review-board-request
Over in Positron, we need to adjust the DevTools command line handler so that it is explicitly installed with its own category entry. The current approach today _replaces_ the CLH for toolkit's jsconsole. However, it relies on special load ordering in Firefox which end up loading the DevTools one later, so it takes effect. We should explicitly enable the DevTools CLH instead to get the behavior we want. One side effect of the approach here is that two help strings are printed for --jsconsole, but that seems okay. : https://github.com/mozilla/positron/pull/42
Review commit: https://reviewboard.mozilla.org/r/53602/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53602/
Attachment #8753956 - Flags: review?(poirot.alex)
Haven't had time to look before shutting down. I don't get why you had to rename b-jsconsole to t-jsconsole? Why do you also stop overriding the contract name: @mozilla.org/toolkit/console-clh;1 and use a new one? Was that useless? Couldn't you just, and only add a category name: a-devtools? So that the patch would only be: devtools/client/devtools-startup.manifest: +category command-line-handler a-devtools @mozilla.org/toolkit/console-clh;1 r+ if that's just that. Otherwise these is something I'm missing. I'll be back on tuesday, feel free to redirect, or I'll look into this when I'm back.
4 years ago
Severity: normal → enhancement
(In reply to Alexandre Poirot [:ochameau] from comment #3) > Haven't had time to look before shutting down. > > I don't get why you had to rename b-jsconsole to t-jsconsole? I changed the category entry for the toolkit handler to better match ordering suggested by nsICommandLineHandler.idl. It says: By convention, handler with ordinary priority should begin with "m". So, my aim was to treat the DevTools handler as "ordinary" and start it with m. However, we also need the DevTools handler to process args before (or "have higher priority") than the toolkit handler, so the DevTools one needs to sort before it. I noticed the other handler in /toolkit uses a letter near the end ("y"), and in general it makes sense to me for toolkit handlers to run near the end of command line processing. So, I chose the letter "t" for toolkit, which also happens to be pretty late in the alphabet. > Why do you also stop overriding the contract name: > @mozilla.org/toolkit/console-clh;1 and use a new one? > Was that useless? > > Couldn't you just, and only add a category name: a-devtools? > So that the patch would only be: > devtools/client/devtools-startup.manifest: > +category command-line-handler a-devtools > @mozilla.org/toolkit/console-clh;1 > r+ if that's just that. Otherwise these is something I'm missing. For Positron, adding just a category entry like you propose does not help because the contract @mozilla.org/toolkit/console-clh;1 is still occupied by the toolkit handler. With your change, the toolkit handler runs twice and the DevTools handler is ignored. So, for Positron at least, it appears the DevTools handler needs a unique contract. : https://dxr.mozilla.org/mozilla-central/source/toolkit/components/commandlines/nsICommandLineHandler.idl#17 :
Comment on attachment 8753956 [details] MozReview Request: Bug 1273967 - Include DevTools command line handler via category entry. r=ochameau https://reviewboard.mozilla.org/r/53602/#review51718 Ok. Makes sense with the additional explainations. Thanks!
Attachment #8753956 - Flags: review?(poirot.alex) → review+
You need to log in before you can comment on or make changes to this bug.