1273967 - Include DevTools command line handler via category entry

Status: RESOLVED FIXED

(DevTools :: General, enhancement)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1273967 - Include DevTools command line handler via category entry. r=ochameau

Review commit: https://reviewboard.mozilla.org/r/53602/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53602/

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 3

[Alexandre Poirot [:ochameau]]

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.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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 5

[Pulsebot]

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

Comment 6

[Alexandre Poirot [:ochameau]]

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!

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/fef64183cb93