Alexandre Poirot [:ochameau] Reporter
	Description • 7 years ago

Once DevTools are going to be shipped as an add-on, various code in Firefox is still going to depend on devtools, like WebExtension for example.
We still need some code in mozilla-central/Firefox to ease interacting with Devtools. This code should expose whatever API is required by Firefox codebase and react to the installation of the DevTools Add-on. It may also automatically install the add-on under some circonstance.

The main goal of this bug is to introduce this module and keep all tests green in the three following setup:
* with this module (and the related refactoring) on mozilla-central tip,
* same, but with DevTools being stripped from mozilla-central,
* on the incoming DevTools repo, with all the DevTools tests (xpcshell, mochitests), we may move some tests from m-c to devtools, like webextension tests related to devtools APIs.

Keeping mozilla-central tests green while devtools is being removed depends on some other bugs like bug 1356231, bug 1356231 and some other to come. In some cases we have to refactor m-c codebase to just stop depending on devtools.

This bug isn't related to the shim layer that will help web developers automatically install the add-on. Instead, it is focused on the low level shim that keep everything working when we do not have devtools and then install it.

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

Alex, I cleaned up my current patches and could use your feedback. The most important changesets here are the first two: 
- Bug 1356244 - create DevTools shim
- Bug 1356244 - call DevTools shim on devtools startup

I created a new package ("devtools-shim") so that we can still use "devtools" for addon resources and I register/unregister devtools in the constructor/destroy of client/framework/devtools. When I tested with the addon, I use to register/unregister from the addon's bootstrap startup() and shutdown() method. But the current approach allows to already start using this code without having to wait for devtools to be moved as an add on.

The 3 others are just there to show how the shim should be used by non-devtools code in mc (which is very similar to the previous DevTools.jsm anyway).

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

(In reply to Julian Descottes [:jdescottes] from comment #6)
> I created a new package ("devtools-shim") so that we can still use
> "devtools" for addon resources and I register/unregister devtools in the
> constructor/destroy of client/framework/devtools.

I looked at your patches and it looks good overall.

But we have to talk about add-ons.
With this approach, we would basicaly force all add-ons using any devtools dependency to be refactored around the shim (an event to know when devtools are installed may help).
It is unfortunate as Firefox 56 still supports these addons.
This discussion would be really different if we target 57.

Did you gave some thought on keeping "devtools" package in m-c and use a new one for the rest?
At some point, most addons were only using gDevTools [1], but it was before the move of devtools from /browser to /devtools.
Since then, various addons switched to import Loader.jsm [2] and call require, which will be much harder to support.

I was wondering if you made some audit of addons on dxr? I'm not saying we have to keep transparent support for addons, but this is a good time to discuss about them.

[1]: https://dxr.mozilla.org/addons/search?q=gDevTools.jsm&redirect=false
[2]: https://dxr.mozilla.org/addons/search?q=Loader.jsm&redirect=false

> I use to register/unregister from the addon's bootstrap startup() and
> shutdown() method. But the current approach allows to already start using
> this code without having to wait for devtools to be moved as an add on.

That's a good option. We should keep bootstrap.js as short as possible.
It grew up over time, but ideally it would only contain the machinery to unload
and reload DevTools.
The DevTools inititialization should be done from this call:
  devtools.require("devtools/client/framework/devtools-browser")

(bootstrap.js isn't reloaded when you are using Ctrl+Alt+R shortcut,
 that's a reason why we should keep bootstrap.js the smaller)

	Alexandre Poirot [:ochameau] Reporter
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8860859 [details]
Bug 1356244 - call DevTools shim on devtools startup;

https://reviewboard.mozilla.org/r/132854/#review136076

::: devtools/client/framework/devtools.js:13
(Diff revision 1)
>  const Services = require("Services");
>  const promise = require("promise");
>  const defer = require("devtools/shared/defer");
>  
> +const { XPCOMUtils } = require("resource://gre/modules/XPCOMUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "DevToolsShim", "chrome://devtools-shim/content/DevToolsShim.jsm");

nit: loader.lazyImporter(this, "DevToolsShim", "chrome://...")

But note that making it lazy is useless as DevTools() constructor is called at end of this file, so it is going to be loaded anyway.

	Alexandre Poirot [:ochameau] Reporter
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8860860 [details]
Bug 1356244 - emit registered/unregistered events from DevTools Shim;

https://reviewboard.mozilla.org/r/132856/#review136102

I had in mind to move the context menu logic into devtools codebase, so that we can redefine its behavior from the add-on. But that's reasonable first step.

	Alexandre Poirot [:ochameau] Reporter
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8860862 [details]
Bug 1356244 - use DevToolsShim in addon-sdk

https://reviewboard.mozilla.org/r/132860/#review136090

::: addon-sdk/source/lib/dev/utils.js
(Diff revision 1)
>  
>  "use strict";
>  
>  const { Cu } = require("chrome");
> -const { gDevTools } = Cu.import("resource://devtools/client/framework/gDevTools.jsm", {});
> +const { DevToolsShim } = Cu.import("chrome://devtools-shim/content/DevToolsShim.jsm", {});
> -const { devtools } = Cu.import("resource://devtools/shared/Loader.jsm", {});

This module doesn't seem to be used anywhere in m-c.
And from addons [1], I see only two using it:
https://addons.mozilla.org/fr/firefox/addon/ng-inspect/ which isn't compatible with e10s.
And Firebug.next, which I think is discontinued.

It looks like the end of the shim file, "gDevToolsMethods" methods with the console.error call, is just for this module. And just for these two addons.

I imagine your main issue is the failing tests... but I'm wondering how much it is worth maintaining this module at all. May be we could open a followup to clean that up by FF57?

This is different for the other module, dev/toolbox is used in various addons [2].

[1] https://dxr.mozilla.org/addons/search?q=dev%2Futils&redirect=false
[2] https://dxr.mozilla.org/addons/search?q=gDevTools.jsm&redirect=false

	Patrick Brosset <:pbro>
	Comment 11 • 7 years ago

Triaging dt-addon blockers as P3/enhancement since we're not using priorities for this project (filter on CLIMBING SHOES).

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

I reviewed the current addons codebase. The full list of addons using DevTools APIs can be found at https://docs.google.com/a/mozilla.com/document/d/1a-gKbtNOrXhoaRGQJWIxTwaC8lYNyejSxTtUobmsOuA/edit?usp=sharing

I grouped the addons into several buckets:
- usage category
  - addons creating a new panel [13]
  - addons using devtools source editor [4]
  - addons using the browser console to log errors [7]
  - addons opening a node in the inspector [5]
  - addons using devtools require [4]
  - other usage [9]
- nightly audience [3]
- internal addons (damp, profiler, mozscreenshot ...) [5]
- deprecated addons [50+]

I don't think we should try to ensure compatibility for release 56. All those addons will still be working as long as devtools are already installed.

The shim already provides a way to know if devtools are installed. On top of that we could have a method that can start the installation of devtools. But I believe that given the timeframe the most realistic thing we can do here is to advertise that for these addons, the devtools should be installed and if they're not, users should install them and restart Firefox.

	Alexandre Poirot [:ochameau] Reporter
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8860858 [details]
Bug 1356244 - devtools-addon: create low-level DevTools shim for mozilla-central;

https://reviewboard.mozilla.org/r/132852/#review140572

It would be great to tweak a couple of existing tests to cover this codepath.
Tests like these ones:
http://searchfox.org/mozilla-central/source/devtools/client/framework/test/browser_devtools_api.js
http://searchfox.org/mozilla-central/source/devtools/client/framework/test/browser_toolbox_theme_registration.js
And one related to showToolbox and toolbox-created event (fork one if necessary).
It should be about using the shim instead of gDevTools.

Testing isInstalled and addon registration would be hard,
but we should at least test that things work when devtools are installed.
For addon registration testing, it may be easier to implement from github, with some tweaks to the test harness.

::: commit-message-e34f1:1
(Diff revision 2)
> +Bug 1356244 - devtools-addon: create low-level DevTools shim for mc;r=ochameau

s/mc/mozilla-central/

::: devtools/moz.build:18
(Diff revision 2)
>      ]
>  
>  DIRS += [
>      'server',
>      'shared',
> +    'shim',

In my WIP patch to remove devtools from mozilla-central, I plan to remove all folders but shared/heapsnapshot/ and server/ (C++ code).
https://github.com/ochameau/mozilla-central/commit/07734ebf966e2e070b320221094beaf7342056c0#diff-04ecb3e411b8bb9428bec874c13ca71dR7
May be it is better to put the shim files directly in /devtools/ folder?
Not a big deal, we can tweak that later.

::: devtools/shim/DevToolsShim.jsm:20
(Diff revision 2)
> +
> +/**
> + * DevToolsShim is a singleton that controls the Firefox Developer Tools.
> + *
> + * It is an instance of a DevTools class that holds a set of tools. It has the
> + * same lifetime as the browser.

I think we should elaborate. Mention the add-on.
Something like this:

Shim layer used to interact with DevTools Add-on.
This module has to be used by any code living in Firefox/mozilla-central to interact with DevTools.
It makes the interaction with DevTools transparent whether the Add-on is installed of not.
This module allows to (un)register tools and themes, open/close toolboxes and listen for DevTools internal events fired on gDevTools (devtools/client/framework/devtools).

Do not hesitate to reformulate/shuffle around, but I think it should mention all this.
We may also introduce a new README file (may be later, once we are ready to move to github) in m-c, explaining the overall add-on workflow.

::: devtools/shim/DevToolsShim.jsm:62
(Diff revision 2)
> +   * - on
> +   * - off
> +   * - registerTool
> +   * - unregisterTool
> +   * - registerTheme
> +   * - unregisterTheme

It would be great to specify the users of these API. (Especially given that you no longer provide the patches using all this code anymore)

This file is going to become our glue layer of compatibility, so it is important to know who is using what.
Also, I would keep it small. Exposing all possible gDevTools event may be hard to maintain. We should keep an explicit list of messages available. I see only toolbox-created/toolbox-destroyed listened from browser/components/extensions/ext-devtools.js. Do you think that's the only one important to support?
Once we reach FF57, I would suggest dropping symbols exposed only by the Addon-SDK like getToolbox (also used for DAMP, but I'm expecting it to disappear or be replaced by something new).
With these comments it should be easier to do such cleanup later.

::: devtools/shim/DevToolsShim.jsm:134
(Diff revision 2)
> +// The rest of the devtools API methods are no-ops if devtools are not installed.
> +let gDevToolsMethods = [
> +  "showToolbox",
> +  "closeToolbox",
> +  "getToolbox",
> +  "getTargetForTab",

Note that bug 1349896 is about to introduce a new "getTheme" API.

	Alexandre Poirot [:ochameau] Reporter
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8860859 [details]
Bug 1356244 - call DevTools shim on devtools startup;

https://reviewboard.mozilla.org/r/132854/#review140620

::: devtools/client/framework/devtools.js:51
(Diff revision 2)
>    // This is important step in initialization codepath where we are going to
>    // start registering all default tools and themes: create menuitems, keys, emit
>    // related events.
>    this.registerDefaults();
> +
> +  DevToolsShim.register(this);

This very important call shouldn't be so quiet and shy. Please comment.
"Register this new DevTools instance to Firefox. DevToolsShim is part of Firefox, integrating with all Firefox codebase and making the glue between code from mozilla-central and DevTools add-on on github".
(Again, do not hesitate to reformulate!)

	Alexandre Poirot [:ochameau] Reporter
	Comment 18 • 7 years ago
mozreview-review

Comment on attachment 8860860 [details]
Bug 1356244 - emit registered/unregistered events from DevTools Shim;

https://reviewboard.mozilla.org/r/132856/#review140622

Can't we use DevToolsShim.on("devtools-registered") and do this.gDevTools.emit("devtools-registered") from register?

I would rather introduce this patch with the one using this event.

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

Comment on attachment 8860858 [details]
Bug 1356244 - devtools-addon: create low-level DevTools shim for mozilla-central;

Not really up for re-review yet. Added a fairly complete xpcshell/unit test, but still need to add integration tests.

I removed all the noop methods used only by the utils of the addon-sdk. I also need to file a bug to update the addon-sdk tests that rely on devtools.

	Alexandre Poirot [:ochameau] Reporter
	Comment 23 • 7 years ago
mozreview-review

Comment on attachment 8860860 [details]
Bug 1356244 - emit registered/unregistered events from DevTools Shim;

https://reviewboard.mozilla.org/r/132856/#review142414

	Alexandre Poirot [:ochameau] Reporter
	Comment 28 • 7 years ago
mozreview-review

Comment on attachment 8860858 [details]
Bug 1356244 - devtools-addon: create low-level DevTools shim for mozilla-central;

https://reviewboard.mozilla.org/r/132852/#review143592

Looks good, thanks!

r+, assuming you get a green try.

::: devtools/shim/DevToolsShim.jsm:24
(Diff revision 3)
> + * The DevToolsShim is a part of the DevTools go faster project, which moves the Firefox
> + * DevTools outside of mozilla-central to an add-on. It aims to bridge the gap for
> + * existing mozilla-central code that still needs to interact with DevTools (such as
> + * web-extensions).
> + *
> + * DevToolsShim is a singleton that provides a set of helpers of interact with DevTools,

Did you meant "to interect" instead of "of interact" here?

	Alexandre Poirot [:ochameau] Reporter
	Comment 29 • 7 years ago
mozreview-review

Comment on attachment 8868555 [details]
Bug 1356244 - add integration test for DevToolsShim;

https://reviewboard.mozilla.org/r/140160/#review143596

This is great tests to have!

::: devtools/client/framework/test/browser_devtools_shim.js:11
(Diff revision 1)
> +"use strict";
> +
> +// Tests DevToolsShim API works as expected when DevTools are available.
> +
> +const TOOL_ID = "test-tool";
> +const CHROME_URL = "chrome://mochitests/content/browser/devtools/client/framework/test/";

This variable already exists from shared-head.js, but is called CHROME_URL_ROOT.

::: devtools/client/framework/test/browser_devtools_shim.js:23
(Diff revision 1)
> +
> +  yield testThemeRegistrationWithShim();
> +  yield testToolRegistrationWithShim();
> +
> +  gBrowser.removeCurrentTab();
> +  finish();

There is no need to call finish when using add_task

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

Thanks for the reviews! I'm still working on the try failures. My devtools shim jar and manifest were not actually packaged, which is why the try builds were failing. I'll ping you for review again once I'm green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8ce704d7220ac7c355d097f428596dd703dc112

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

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

	Julian Descottes [:jdescottes] Assignee
	Comment 40 • 7 years ago
mozreview-review

Comment on attachment 8860858 [details]
Bug 1356244 - devtools-addon: create low-level DevTools shim for mozilla-central;

https://reviewboard.mozilla.org/r/132852/#review143908

::: browser/installer/package-manifest.in:625
(Diff revision 6)
> +; [DevTools Shim Files]
> +@RESPATH@/browser/chrome/devtools-shim@JAREXT@
> +@RESPATH@/browser/chrome/devtools-shim.manifest
> +

Thanks for the reviews and comments Alex! The last patches should normally address all your comments.

In order for the shim files to be packaged, I had to update this file. I see in your other file you asked for a review from :gps. Should we do the same here?

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

(ni? for question in previous comment)

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

(In reply to Julian Descottes [:jdescottes] from comment #40)
> In order for the shim files to be packaged, I had to update this file. I see
> in your other file you asked for a review from :gps. Should we do the same
> here?

No that shouldn't be necessary, your changes are trivial here.

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

Try after rebase: https://treeherder.mozilla.org/#/jobs?repo=try&revision=70167d9cf6c4deb1c2d5fbb08f650847cd7a8d85

	Pulsebot
	Comment 48 • 7 years ago

Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a65577f40ebd
devtools-addon: create low-level DevTools shim for mozilla-central;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/e18063b2ed57
call DevTools shim on devtools startup;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/861f125ec68f
add integration test for DevToolsShim;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/d0aa9b4a0fef
emit registered/unregistered events from DevTools Shim;r=ochameau

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 49 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a65577f40ebd
https://hg.mozilla.org/mozilla-central/rev/e18063b2ed57
https://hg.mozilla.org/mozilla-central/rev/861f125ec68f
https://hg.mozilla.org/mozilla-central/rev/d0aa9b4a0fef