Closed Bug 1273967 Opened 4 years ago Closed 4 years ago

Include DevTools command line handler via category entry

Categories

(DevTools :: General, enhancement)

enhancement
Not set

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

Details

Attachments

(1 file)

Over in Positron[1], 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.

[1]: https://github.com/mozilla/positron/pull/42
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.
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[1].  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[2] 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.

[1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/commandlines/nsICommandLineHandler.idl#17
[2]:
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+
https://hg.mozilla.org/mozilla-central/rev/fef64183cb93
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.