Julian Descottes [:jdescottes] Assignee
	Description • 7 years ago

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.

	Alexandre Poirot [:ochameau]
	Comment 1 • 7 years ago

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)

	Julian Descottes [:jdescottes] Assignee
	Comment 4 • 7 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

	Alexandre Poirot [:ochameau]
	Comment 5 • 7 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.

	Julian Descottes [:jdescottes] Assignee
	Comment 6 • 7 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.

	Alexandre Poirot [:ochameau]
	Comment 7 • 7 years ago

./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.

	Julian Descottes [:jdescottes] Assignee
	Comment 13 • 7 years ago

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 17 • 7 years ago

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).

	Alexandre Poirot [:ochameau]
	Comment 18 • 7 years ago

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

	Alexandre Poirot [:ochameau]
	Comment 19 • 7 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!

	Alexandre Poirot [:ochameau]
	Comment 20 • 7 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?

	Alexandre Poirot [:ochameau]
	Comment 21 • 7 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

	Julian Descottes [:jdescottes] Assignee
	Comment 25 • 7 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.

	Julian Descottes [:jdescottes] Assignee
	Comment 26 • 7 years ago

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?

	Julian Descottes [:jdescottes] Assignee
	Comment 27 • 7 years ago

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 31 • 7 years ago

Hopefully this one should be greenish: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76cb1b163e846321cb891f8e89ddaa39866f4d74

	Pulsebot
	Comment 32 • 7 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

	Ryan VanderMeulen [:RyanVM]
	Comment 33 • 7 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