[devtools-addon] Register about:debugging dynamically

RESOLVED FIXED in Firefox 55

Status

enhancement
P3
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: jdescottes, Assigned: jdescottes)

Tracking

(Blocks 1 bug)

unspecified
Firefox 55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

about:debugging is registered statically in docshell/base/nsAboutRedirector.cpp and points to devtools/client/aboutdebugging/aboutdebugging.xhtml

We should keep the entry point valid even if DevTools are not installed. When they are not installed, the page should tell the user that DevTools are now an addon and can be installed (TBD in more details once we made some progress with the UI shim). And if DevTools are installed, the original about:debugging UI should be loaded.
Comment hidden (mozreview-request)
Assignee

Comment 4

2 years ago
(In reply to Alexandre Poirot [:ochameau] from comment #1)
> Note that if that can help, you may register about:debugging in
> JS/dynamically, like about:devtools-toolbox:
> http://searchfox.org/mozilla-central/source/devtools/client/framework/about-
> devtools-toolbox.js
> 
> (or like about:flyweb
>  http://searchfox.org/mozilla-central/source/mobile/android/extensions/
> flyweb/bootstrap.js#25
>  or like about:pocket
>  http://searchfox.org/mozilla-central/source/browser/extensions/pocket/
> content/AboutPocket.jsm#24)

Thanks! I actually did that in my first implementation on GitHub [1].

But I was thinking here I would keep a static registration since I still want a UI to be displayed when DevTools are not installed. That's the approach I am trying for now, feel free to have a look at the patches! I guess the other option would be to unregister the about:debugging shim when loading the devtools addon and register the real about:debugging, but I imagine the end result would be more complex than what I have right now. Might be wrong though!

[1] https://github.com/juliandescottes/ff-dt/commit/de3c796c7afae2c343c6d01450e031e54a9cbdec

Comment 5

2 years ago
mozreview-review
Comment on attachment 8865852 [details]
Bug 1363327 - simplify aboutdebugging landing page;

https://reviewboard.mozilla.org/r/137460/#review140630

::: devtools/shim/aboutdebugging/aboutdebugging-shim.js:28
(Diff revision 2)
> +  loadAboutDebugging() {
> +    const { BrowserLoader } = Components.utils.import("resource://devtools/client/shared/browser-loader.js", {});
> +    const { require } = BrowserLoader({
> +      baseURI: "resource://devtools/client/aboutdebugging/",
> +      window
> +    });
> +
> +    this.aboutDebugging = require("./initializer");
> +    this.aboutDebugging.init();
> +  },

I would not involve any devtools code from m-c.
Anything but the Shim.
So, instead of calling loadAboutDebugging,
I would call DevToolsShim.loadAboutDebugging(window);

But that's if we keep this solution overall.

I'm wondering if there is better opportunities with a JS implementation of about:debugging.
1) Implement it in JS like what you did here
https://github.com/juliandescottes/ff-dt/commit/de3c796c7afae2c343c6d01450e031e54a9cbdec#diff-1cb1ff33d172c8d7d313264b82274bfdR27
2) But instead of doing it from the addon, do it from from m-c. Then from that newChannel function:
newChannel: function (aURI, aLoadInfo) {
  let uri = DevToolsShim.isInstalled() ?
    "chrome://devtools/content/aboutdebugging/aboutdebugging.xhtml" :
    "chrome://devtools-shim/content/aboutdebugging/aboutdebugging.xhtml";
  let chan = Services.io.newChannelFromURIWithLoadInfo(uri, aLoadInfo);
3) It would be even better to retrieve the chrome://devtools/ url via the shim, from the addon.
Assignee

Comment 6

2 years ago
Thanks for the feedback. I was not sure about the lifecycle of the registration. If we can simply check for the addon in newChannel, then I agree it will be easier to implement this way. I'll prepare another patch following your suggestions. 

I'm not sure how we are supposed to generate the uuids used in the registration code (I copied the one on github from the about:devtools-toolbox registration code). But we can clarify that later on.
./mach uuid
if you want new id and I imagine you do if you switch from c++ to js implementation.
This is used to differenciate two xpcom. Sometimes you may have two implementing the same class name (contractId : @mozilla.org/network/protocol/about;1?what=debugging) So that you use ID to identify/get the right one.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
I don't think we should move this much more before the low-level + UI shim land, but this new series of patches follows your suggestions.

About:debugging is now registered using a manifest, and we are using the shim to know whether we should display the real about:debugging or a shim page. 

The shim page will need localized strings that will stay in mozilla-central. I think we will have localized strings in mozilla central for the UI shim too, so they will probably end up in the same place. 

When DevTools are registered, the about:debugging fake landing page now simply reloads the page. 

> 3) It would be even better to retrieve the chrome://devtools/ url via the shim, from the addon.

Agreed but I think that's something we can change later on.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8867209 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8867210 - Attachment is obsolete: true
I left out the last changeset which adds a shim landing page but I figure we could start reviewing the other patches already. 

One thing I don't like very much at the moment is that the manifest for registering about:debugging is in the shim folder. Since we're registering it for both the shim page and the real page, it could make sense to let it live somewhere else, but where? at the root of devtools? (otherwise devtools/shim/ is probably not that bad for the time being).
(In reply to Julian Descottes [:jdescottes] from comment #17)
> One thing I don't like very much at the moment is that the manifest for
> registering about:debugging is in the shim folder. Since we're registering
> it for both the shim page and the real page, it could make sense to let it
> live somewhere else, but where? at the root of devtools? (otherwise
> devtools/shim/ is probably not that bad for the time being).

It doesn't look bad to be. It is about the same than DevToolsShim.jsm.
You can think about /devtools/shim/ as "whatever stays on m-c" once devtools move to github.
Most of the files in /devtools/ should also go away, all but moz.build.

Comment 19

2 years ago
mozreview-review
Comment on attachment 8865852 [details]
Bug 1363327 - simplify aboutdebugging landing page;

https://reviewboard.mozilla.org/r/137460/#review145902

Wha! We were importing react for nothing?! I'm expecting this to speed up about:debugging load time!
Attachment #8865852 - Flags: review?(poirot.alex) → review+

Comment 20

2 years ago
mozreview-review
Comment on attachment 8867207 [details]
Bug 1363327 - register about:debugging dynamically using a manifest;

https://reviewboard.mozilla.org/r/138770/#review145880

::: devtools/shim/aboutdebugging-registration.js:10
(Diff revision 2)
> +"use strict";
> +
> +// Register the about:debugging URL, that allows to debug various targets such as addons,
> +// workers and tabs by launching a dedicated DevTools toolbox for the selected target.
> +// If DevTools are not installed, this about page will display a shim landing page
> +// encouraging the user download and install DevTools.

I think you miss "to" to be correct:
`the user +to+ download`

::: devtools/shim/aboutdebugging-registration.js:39
(Diff revision 2)
> +    chan.owner = Services.scriptSecurityManager.getSystemPrincipal();
> +    return chan;
> +  },
> +
> +  getURIFlags: function (uri) {
> +    return nsIAboutModule.ALLOW_SCRIPT || nsIAboutModule.ENABLE_INDEXED_DB;

Do we use/need indexedDB?
Attachment #8867207 - Flags: review?(poirot.alex) → review+

Comment 21

2 years ago
mozreview-review
Comment on attachment 8867208 [details]
Bug 1363327 - remove unused config variable MOZ_DEVTOOLS_ALL;

https://reviewboard.mozilla.org/r/138772/#review145904
Attachment #8867208 - Flags: review?(poirot.alex) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 25

2 years ago
mozreview-review-reply
Comment on attachment 8867207 [details]
Bug 1363327 - register about:debugging dynamically using a manifest;

https://reviewboard.mozilla.org/r/138770/#review145880

Thanks for the reviews, try https://treeherder.mozilla.org/#/jobs?repo=try&revision=96c3985d4b46f1107945177e77fbbcc37216a495

> I think you miss "to" to be correct:
> `the user +to+ download`

thanks!

> Do we use/need indexedDB?

I don't think so, I must have copied it from another registration.
Assignee

Updated

2 years ago
Summary: [devtools-addon] Add a shim landing page for about:debugging → [devtools-addon] Register about:debugging dynamically
Assignee

Updated

2 years ago
No longer depends on: 1356244
Getting TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | unreferenced file: chrome://devtools/content/aboutdebugging/aboutdebugging.xhtml 

Even though the file is referenced in https://hg.mozilla.org/try/file/137fba8b2e64/devtools/shim/aboutdebugging-registration.js#l22

Alex, do you know if I am missing something something here?
Flags: needinfo?(poirot.alex)
Other about:debugging tests are failing on try too. 
I guess I am probably missing an entry in package-manifest.in . Will look at that a bit later.
Flags: needinfo?(poirot.alex)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 32

2 years ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c72c2c5785e8
simplify aboutdebugging landing page;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/ccd1dc27a473
register about:debugging dynamically using a manifest;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/c735b95bef36
remove unused config variable MOZ_DEVTOOLS_ALL;r=ochameau

Comment 33

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c72c2c5785e8
https://hg.mozilla.org/mozilla-central/rev/ccd1dc27a473
https://hg.mozilla.org/mozilla-central/rev/c735b95bef36
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.