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

RESOLVED FIXED in Firefox 57

Status

P3
normal
RESOLVED FIXED
a year ago
3 months ago

People

(Reporter: jdescottes, Assigned: jdescottes)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 57
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

a year ago
mozreview-review
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 15

a year ago
mozreview-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 16

a year ago
mozreview-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 17

a year ago
mozreview-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 18

a year ago
mozreview-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 19

a year ago
mozreview-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 20

a year ago
mozreview-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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 25

a year ago
mozreview-review-reply
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 28

a year ago
mozreview-review
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 29

a year ago
mozreview-review
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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)

Comment 42

a year ago
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)

Comment 44

a year ago
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

Comment 45

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e2e779f54e10
https://hg.mozilla.org/mozilla-central/rev/7a0133d5ee1d
https://hg.mozilla.org/mozilla-central/rev/d8b6d2584aec
https://hg.mozilla.org/mozilla-central/rev/49ff7e926ea7
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Flags: needinfo?(jdescottes)
Depends on: 1392531

Updated

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