Closed Bug 1374735 Opened 3 years ago Closed 3 years ago

[dt-addon] remaining dependencies between m-c and devtools

Categories

(DevTools :: General, defect, P3)

defect

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

Attachments

(4 files, 2 obsolete files)

We misseda few mozilla-central -> devtools dependencies. After facing an issue with XPIProvider.jsm, I did a new query to find remaining dependencies and got the following list:

http://searchfox.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm
    "resource://devtools/shared/Loader.jsm");

http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionCommon.jsm
    "resource://devtools/shared/Loader.jsm");

http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-browser-content.js
    "resource://devtools/shared/Loader.jsm");

http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-c-toolkit.js
    require("devtools/client/framework/devtools-browser");
    let hudservice = require("devtools/client/webconsole/hudservice");

http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-management.js
    "resource://devtools/shared/event-emitter.js");

http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-devtools-inspectedWindow.js
    } = require("devtools/shared/fronts/webextension-inspected-window");

http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-devtools.js
    const {TabTarget} = require("devtools/client/framework/target");

http://searchfox.org/mozilla-central/source/browser/tools/mozscreenshots/mozscreenshots/extension/configurations/DevTools.jsm
    Cu.import("resource://devtools/client/framework/gDevTools.jsm");
    let { devtools } = Cu.import("resource://devtools/shared/Loader.jsm", {});

http://searchfox.org/mozilla-central/source/browser/base/content/test/siteIdentity/test_no_mcb_on_http_site_font.css
    src: url(http://example.com/browser/devtools/client/fontinspector/test/browser_font.woff);

http://searchfox.org/mozilla-central/source/dom/media/webvtt/vtt.jsm
    const { require } = Cu.import("resource://devtools/shared/Loader.jsm", {});

http://searchfox.org/mozilla-central/source/mobile/android/chrome/content/RemoteDebugger.js
    let { require } = Cu.import("resource://devtools/shared/Loader.jsm", {});
    let { DebuggerServer } = require("devtools/server/main");
    *        devtools/shared/security/auth.js.

http://searchfox.org/mozilla-central/source/mobile/android/components/extensions/ext-browserAction.js
    "resource://devtools/shared/event-emitter.js");

http://searchfox.org/mozilla-central/source/mobile/android/installer/allowed-dupes.mn
    modules/devtools/Console.jsm

http://searchfox.org/mozilla-central/source/mobile/android/modules/dbg-browser-actors.js
    const { RootActor } = require("devtools/server/actors/root");
    const { DebuggerServer } = require("devtools/server/main");
    require("devtools/server/actors/webbrowser");
Comment on attachment 8880513 [details]
Bug 1374735 - use toolkit eventemitter in extensions/ext-management.js;

https://reviewboard.mozilla.org/r/151836/#review161674
Attachment #8880513 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8880515 [details]
Bug 1374735 - use DevToolsShim to create TabTarget in ext-devtools;

https://reviewboard.mozilla.org/r/151840/#review161676

::: browser/components/extensions/ext-devtools.js:57
(Diff revision 2)
>    if (!context.devToolsToolbox.target.isLocalTab) {
>      throw new Error("Unexpected target type: only local tabs are currently supported.");
>    }
>  
> -  const {TabTarget} = require("devtools/client/framework/target");
> -
> +  const tab = context.devToolsToolbox.target.tab;
> +  context.devToolsTarget = DevToolsShim.getTargetForTab(tab);

https://dxr.mozilla.org/mozilla-central/source/devtools/shim/DevToolsShim.jsm#221

getTargetForTab is marked to go away with addon-sdk, seems like that should be changed.
Attachment #8880515 - Flags: review?(mixedpuppy) → review-
Comment on attachment 8880517 [details]
Bug 1374735 - use DevToolsShim to open browser console from extensions;

https://reviewboard.mozilla.org/r/151844/#review162300

r=me for the devtools-browser changes.  Can you please request review for the extensions change from someone who can better review that?
Attachment #8880517 - Flags: review?(bgrinstead) → review+
Comment on attachment 8880516 [details]
Bug 1374735 - use DevToolsShim to retrieve the WebExtensionInspectedWindowFront;

https://reviewboard.mozilla.org/r/151842/#review162796

This relates to bug 1222047, it should be easier to create an actor/front out of toolbox or target object. Here the codebase already has access to the target object.
I imagine that's a reasonable way to address that until we simplify this...

::: devtools/client/framework/devtools.js:581
(Diff revision 2)
>    initBrowserToolboxProcessForAddon: function (addonID) {
>      BrowserToolboxProcess.init({ addonID });
>    },
>  
>    /**
> +   * Compatibility layer for web-extensions. Used fromt the DevToolsShim by

s/Used fromt the DevToolsShim by/Used by/
Attachment #8880516 - Flags: review?(poirot.alex) → review+
Comment on attachment 8880514 [details]
Bug 1374735 - stop requiring devtools loader if not used;

https://reviewboard.mozilla.org/r/151838/#review163022
Attachment #8880514 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8880518 [details]
Bug 1374735 - stop exposing devtools require to extensions ext-* files;

https://reviewboard.mozilla.org/r/151850/#review163024
Attachment #8880518 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8880517 [details]
Bug 1374735 - use DevToolsShim to open browser console from extensions;

https://reviewboard.mozilla.org/r/151844/#review163026

r=me, assuming the tests still pass. Will this keep working if it's triggered while the devtools extension isn't installed, though?
Attachment #8880517 - Flags: review+
Comment on attachment 8880515 [details]
Bug 1374735 - use DevToolsShim to create TabTarget in ext-devtools;

https://reviewboard.mozilla.org/r/151840/#review161676

> https://dxr.mozilla.org/mozilla-central/source/devtools/shim/DevToolsShim.jsm#221
> 
> getTargetForTab is marked to go away with addon-sdk, seems like that should be changed.

Thanks for catching that! I moved it to the "webextensions" related methods. Is this semantically correct?
(In reply to Kris Maglione [:kmag] from comment #20)
> Comment on attachment 8880517 [details]
> Bug 1374735 - use event to open browser console from extensions;
> 
> https://reviewboard.mozilla.org/r/151844/#review163026
> 
> r=me, assuming the tests still pass. Will this keep working if it's
> triggered while the devtools extension isn't installed, though?

If devtools are not installed, the event will not be processed so this will be a no-op.
For now, DevTools are always considered to be installed (ie. all channels will have it built in at least until 57).

After it becomes optional, we have several options if we feel like a no-op is not acceptable here:
- 1: trigger the download & installation of the DevTools addon in the background
- 2: open the devtools addon install page
- 3: notify the user differently (dump? Services.prompt.alert?)

Options 1 and 2 need to wait until the bugs for installing devtools are fixed (Bug 1361080 & Bug 1378397).
Option 3 probably can be implemented right away behind a `if (DevToolsShim.isInstalled()) {...}`
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8880516 [details]
Bug 1374735 - use DevToolsShim to retrieve the WebExtensionInspectedWindowFront;

https://reviewboard.mozilla.org/r/151842/#review163480
Attachment #8880516 - Flags: review?(lgreco) → review+
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment on attachment 8880515 [details]
Bug 1374735 - use DevToolsShim to create TabTarget in ext-devtools;

https://reviewboard.mozilla.org/r/151840/#review163682
Attachment #8880515 - Flags: review?(mixedpuppy) → review+
try https://treeherder.mozilla.org/#/jobs?repo=try&revision=0920a6d19a138dd08744b7aa33e307afb4db5cdb

Slightly changed the approach for https://reviewboard.mozilla.org/r/151844/diff/4#index_header (Bug 1374735 - use event to open browser console from extensions;) because of Bug 1359855. We are now using the DevTools shim rather than an event to communicate with devtools. This way if devtools is installed, but not initialized, the shim will take care of the initialization.
Blocks: 1369801
Attachment #8880513 - Attachment is obsolete: true
Attachment #8880514 - Attachment is obsolete: true
Just rebased on latest central. The two first changesets became redundant with other patches that landed before. Pushed to try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f9d0166c9bb0e35a0e0a4cd4908105f675e8370

Clearing the ni?
Flags: needinfo?(kmaglione+bmo)
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/22baf4e67730
use DevToolsShim to create TabTarget in ext-devtools;r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/b7435cd66ce3
use DevToolsShim to retrieve the WebExtensionInspectedWindowFront;r=ochameau,rpl
https://hg.mozilla.org/integration/autoland/rev/da075933f7bc
use DevToolsShim to open browser console from extensions;r=bgrins,kmag
https://hg.mozilla.org/integration/autoland/rev/50a36fb7c7f9
stop exposing devtools require to extensions ext-* files;r=kmag
Backed out for test_ext_i18n.js failures.
https://hg.mozilla.org/integration/autoland/rev/8eeea4adcdd14613d4c4ba7c26edb8aabbe5985d

https://treeherder.mozilla.org/logviewer.html#?job_id=124174019&repo=autoland

TEST-UNEXPECTED-FAIL | xpcshell-e10s.ini:toolkit/components/extensions/test/xpcshell/test_ext_i18n.js | test_i18n - [test_i18n : 175] Extension left running at test shutdown - "running" == "unloaded"
Flags: needinfo?(jdescottes)
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e2e779f54e10
use DevToolsShim to create TabTarget in ext-devtools;r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/7a0133d5ee1d
use DevToolsShim to retrieve the WebExtensionInspectedWindowFront;r=ochameau,rpl
https://hg.mozilla.org/integration/autoland/rev/d8b6d2584aec
use DevToolsShim to open browser console from extensions;r=bgrins,kmag
https://hg.mozilla.org/integration/autoland/rev/49ff7e926ea7
stop exposing devtools require to extensions ext-* files;r=kmag CLOSED TREE
Flags: needinfo?(jdescottes)
Depends on: 1392531
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.