Closed Bug 1225108 Opened 4 years ago Closed 4 years ago

Introduce a special addon to ease working on devtools from Firefox

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

We revived devtools.loader.srcdir and Loader.jsm recently.
It allows to hot-reload a bunch of devtools resources, but not all.
Also, it is very specific to devtools and this workflow can't be applied to any other Firefox resources.

The big file moved :jryans did alllows us to simplify hot-reload feature by using an addon instead. We wouldn't bundled devtools as an addon.
Instead we would just register xul content, skin and locales via an addon chrome.manifest. There wouldn't be any magic done in Loader.jsm.
The addon is going to remap all root URL used by devtools to a local mozilla-central checkout transparently.
This would have the benefit to not introduce a different codepath/loader/whatever between running firefox from a local checkout versus released package.

The contribution workflow is done to be:
 * Install a Firefox Nightly
 * checkout mozilla-central with the same revision used to build this Nightly
 * Register the addon (that lives in m-c checkout) via Firefox UI, bug 1221141
 * Play with devtools
 * Hit a keystroke (CTRL+ALT+R?) to hot-reload devtools
 * Play with devtools with sources being hot-reloaded

 * If you reload firefox, devtools are automatically going to be using local checkout sources.
 * If you want to stop mapping to local sources, disable the addon.
Depends on: 1221141
Attached patch patch v1 (obsolete) — Splinter Review
A wip patch to expose how simple this story is.
This is just install.rdf and chrome.manifest files,
with minor tweaks to hot reload from the addon.
Attached patch patch v2 (obsolete) — Splinter Review
Rebased after the last themes URL change.
Attachment #8687966 - Attachment is obsolete: true
Attached patch patch v3 (obsolete) — Splinter Review
I think it is time to proceed.

Few things about this:
- I tweaked gDevTools to figure out when a toolbox is opened
  in order to reopen it if we are hitting the reload shortcut on a window with an opened toolbox
- The addon includes a omniscient key listener that can intercept key shortcut from any window,
  I would like to suggest moving this to about:debugging, so that we can have such trick
  to reload addons for all addon developers!

There is still various followups to ensure everything reload, like:
 - bug 1183280, get rid of all preprocessed file, who appear to break stuff with this addon
 - bug 1232290, which break the editor
 - bug 1188405 or similar, to allow reloading gDevTools itself
 - bug 1185460 to allow keeping the devtools addon installed even after firefox reboot
 - bug 1217472 or similar, to cleanup Loader.jsm to use only one codepath and stop supporting loader path

Note that I started writing a single page to guide contribution from nothing to check a patch in:
  https://public.etherpad-mozilla.org/p/devtools-contribution-workflow
  This includes the addon workflow that doesn't involve building firefox nor mach.
Attachment #8698460 - Flags: review?(jryans)
Attachment #8688495 - Attachment is obsolete: true
Also I plan to setup a small group to dogfood this new workflow and improve it based on feedback.
I think you, brian and patrick are good candidates as well as new hires.
But I need to checkin something first.
Comment on attachment 8698460 [details] [diff] [review]
patch v3

Review of attachment 8698460 [details] [diff] [review]:
-----------------------------------------------------------------

Well, I think the idea is interesting to explore.  But, it did not seem to work for me.  Hopefully we can figure out why!

::: devtools/bootstrap.js
@@ +10,5 @@
> +
> +// Helper to listen to a key on all windows
> +function MultiWindowKeyListener({ key, ctrlKey, altKey, callback }) {
> +  let keyListener = function (event) {
> +    if (event.ctrlKey == !!ctrlKey &&

I wasn't able to press a key combo that actually met all these rules at once.  I watched in Browser Toolbox, but `callback` was never called.

I assume I am supposed to press Ctrl-Alt-R?

@@ +71,5 @@
> +  let reloadToolbox = false;
> +  if (isBrowser && top.gDevToolsBrowser.hasToolboxOpened) {
> +    reloadToolbox = top.gDevToolsBrowser.hasToolboxOpened(top);
> +  }
> +  dump("Reload devtools.  (reload-toolbox:"+reloadToolbox+")\n");

Nit: devtools -> DevTools

@@ +83,5 @@
> +}
> +
> +let listener;
> +function startup() {
> +  dump("Devtools addon started.\n");

Nit: Devtools -> DevTools

::: devtools/install.rdf
@@ +8,5 @@
> +<RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
> +     xmlns:em="http://www.mozilla.org/2004/em-rdf#">
> +  <Description about="urn:mozilla:install-manifest"
> +               em:id="devtools@mozilla.org"
> +               em:name="Devtools"

Well, this should at the very least be "DevTools".  But, it might be better as a longer name.  "Developer Tools (local version)"?  Or some other phrase that means "not the regular tools"?  It seems worthwhile to signify a special mode, as opposed to the built-in tools.
Attachment #8698460 - Flags: review?(jryans) → feedback+
Comment on attachment 8698460 [details] [diff] [review]
patch v3

Review of attachment 8698460 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/install.rdf
@@ +8,5 @@
> +<RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
> +     xmlns:em="http://www.mozilla.org/2004/em-rdf#">
> +  <Description about="urn:mozilla:install-manifest"
> +               em:id="devtools@mozilla.org"
> +               em:name="Devtools"

You should also add the `em:description` field with a longer description of what is going on.
What's the reasoning for this living in an addon as opposed to something built in but that's preffed off by maybe the chrome debugging pref or a new one?
Attached patch patch v4 (obsolete) — Splinter Review
That was an issue specific to Mac.
event.key was the "Registered" character instead of just "r"...
It should work better with using keyCode.
Attachment #8700564 - Flags: review?(jryans)
Attachment #8698460 - Attachment is obsolete: true
(In reply to Brian Grinstead [:bgrins] from comment #7)
> What's the reasoning for this living in an addon as opposed to something
> built in but that's preffed off by maybe the chrome debugging pref or a new
> one?

The reasoning was that this work wasn't really specific to devtools.
We end up using a set of tricks that would benefit to other teams working on Firefox frontend as well as addon developers.

* The loader trick, with the srcdir pref was really specific to devtools. It was a hack. Hard to maintain and very often broken as it includes very different codepaths. I would like to get rid of this completely as soon as the addon workflow is ready.
* Instead, we are using a chrome.manifest file to redirect all devtools URIs (chrome://devtools/xxx, resource://devtools/) to the local checkout. So that we don't have different codepaths in the loader. There is just this chrome.manifest file that redirects everything. Then we just have to ensure destroying the old loader and spawn a new one.
* We could have done something in devtools to register this chrome.manifest, but we would have been working around the fact that working on addons sucks...
* It would have been unfair to all addon developers who are using the same feature but have to do many tricks to get their code working in Firefox. So instead I'm trying to improve the workflow for everyone.
* With this chrome.manifest, devtools looks exactly like pdf.js and shumway. We could all reuse the same workflow to work on Firefox features. And anytime we improve something, all teams win. I hope system addons are going to also follow this track.
* One example is the magic shortcut. I would like to include this in about:debugging to allow doing this for any add-on.
* So far, I'm not trying to package devtools as an addon, but that work would certainly help us doing it if we want to go that way (DevTools as system add-on??)
Something about the fonts or styles used forces CodeMirror to use serif fonts instead of monospace.
Comment on attachment 8700564 [details] [diff] [review]
patch v4

Review of attachment 8700564 [details] [diff] [review]:
-----------------------------------------------------------------

Check attachment 8701245 [details], there seems to be some kind of font issue.  Have you seen this?  If you don't want to solve it now, we should at least have a follow up bug.

I think the double reload issue should be resolved before landing, it seems to impede actually using this much at all.

Anyway, the reloading does work now, at least the first time!

::: devtools/bootstrap.js
@@ +69,5 @@
> +  let top = getTopLevelWindow(event.view)
> +  let isBrowser = top.location.href.includes("/browser.xul") && top.gDevToolsBrowser;
> +  let reloadToolbox = false;
> +  if (isBrowser && top.gDevToolsBrowser.hasToolboxOpened) {
> +    reloadToolbox = top.gDevToolsBrowser.hasToolboxOpened(top);

Something about this seems to fail if you reload twice in a row:

1. Open toolbox
2. Press Ctrl-Alt-R
3. Toolbox reloads, log says "Reload DevTools.  (reload-toolbox:true)"
4. Press Ctrl-Alt-R again
5. Toolbox disappears, log says "Reload DevTools.  (reload-toolbox:false)"

::: devtools/install.rdf
@@ +8,5 @@
> +<RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
> +     xmlns:em="http://www.mozilla.org/2004/em-rdf#">
> +  <Description about="urn:mozilla:install-manifest"
> +               em:id="devtools@mozilla.org"
> +               em:name="Developer Tools (local version)"

Please add a longer-form em:description field as well.
Attachment #8700564 - Flags: review?(jryans) → feedback+
Comment on attachment 8702536 [details] [diff] [review]
Introduce an addon to easily work on devtools codebase. r=jryans

Review of attachment 8702536 [details] [diff] [review]:
-----------------------------------------------------------------

- with the long description
- fixed the double reload. I had to tweak hasToolboxOpened. There is some state mixup between the multiple loader instances living around gDevTools singleton.
  (hopefully that is going to be fixed by bug 1188405)
- the font issue is related to bug 1183280, I'll try to move forward on that.

Waiting for Jan 4 to toggle to r? flag ;)
Attachment #8700564 - Attachment is obsolete: true
Attachment #8702536 - Flags: review?(jryans)
Comment on attachment 8702536 [details] [diff] [review]
Introduce an addon to easily work on devtools codebase. r=jryans

Review of attachment 8702536 [details] [diff] [review]:
-----------------------------------------------------------------

Great, double reload does seem fixed here as well.  This seems ready to land so you can iterate further with some more testers.
Attachment #8702536 - Flags: review?(jryans) → review+
https://hg.mozilla.org/integration/fx-team/rev/1850e2691a107ed4cd99cae0950fe8af0ce48433
Bug 1225108 - Introduce an addon to easily work on devtools codebase. r=jryans
Depends on: 1236905
https://hg.mozilla.org/mozilla-central/rev/1850e2691a10
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Depends on: 1237208
Depends on: 1237606
Depends on: 1247270
Depends on: 1241131
Depends on: 1241050
Depends on: 1248609
Depends on: 1257532
Depends on: 1258546
Depends on: 1314608
Depends on: 1314609
Depends on: 1349509
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.