DevToolsShim should differentiate between installed and initialized states

RESOLVED FIXED in Firefox 56

Status

DevTools
General
RESOLVED FIXED
11 months ago
12 days ago

People

(Reporter: jdescottes, Assigned: jdescottes)

Tracking

unspecified
Firefox 56

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

With Bug 1359855, devtools can be installed without being initialized/started. The shim relied on devtools-browser.js being loaded in order to "detect" devtools. This needs to change.

Small extract from a related comment:

(In reply to Julian Descottes [:jdescottes] from comment #87)
> 
> Last week we discussed about potential issues with the DevToolsShim. The
> Shim will now consider that DevTools are not installed until
> devtools-browser is loaded.
> One of the issues is the "inspect element" context menuitem, but other
> things will start breaking (addon stuff, restoring scratchpad sessions
> etc...).
> Our tests are hiding the issues since we init devtools on mochitest load.
> 
> My proposal would be to support two states in the DevToolsShim: installed
> and initialized. 
> Initialized would be set to true when gDevTools registers itself on the
> shim. Most methods in the shim should call Startup.initDevTools() if
> initialized is false.
> Regarding installed, it's a bit more complicated. Ideally we would rely on
> the AddonManager, but we can't do that until DevTools are a system addon. We
> could simply hardcode it to be always true until we land the devtools as
> system addon patches.

Additionally, :ochameau suggested the following strategy in order to detect the DevTools installation without having to rely on the AddonManager. We can check if the devtools resource path has already been registered, which means that devtools files are ready to be loaded (being addon or not does not make a difference here).

> Services.io.getProtocolHandler("resource").hasSubstitution("devtools")
Comment hidden (mozreview-request)
(Assignee)

Comment 2

11 months ago
Comment on attachment 8887882 [details]
Bug 1382173 - devtools shim support initialized and installed states;

On IRC I said I could land this without using your patches for the startup, but I actually need a way to call initDevTools, which should be exposed by the startup component. So maybe it would be better to submit this patch in your bug afterall?
Attachment #8887882 - Flags: review?(poirot.alex) → feedback?(poirot.alex)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

11 months ago
Here's another patch that integrates the DevToolsShim changes with the startup patches from Bug 1359855. I can confirm it fixes the scenarios requiring DevTools to start (tested inspect element and restore sessions).

Comment 6

11 months ago
mozreview-review
Comment on attachment 8887882 [details]
Bug 1382173 - devtools shim support initialized and installed states;

https://reviewboard.mozilla.org/r/158810/#review164122

Works great, with and without devtools-startup patches!

::: devtools/shim/DevToolsShim.jsm:182
(Diff revision 2)
>    getOpenedScratchpads: function () {
>      if (!this.isInstalled()) {
>        return [];
>      }
> +
> +    if (this.isInstalled && !this.isInitialized()) {

Here and after, re-checking for this.isInstalled is redundant with the if(!this.isInstalled() done just before.
So, the following should be enough:
  if (!this.isInitialized())

::: devtools/shim/DevToolsShim.jsm:200
(Diff revision 2)
>        return;
>      }
> +
> +    if (this.isInstalled && !this.isInitialized()) {
> +      this._initDevTools();
> +    }

We should double check that, with that, the tools aren't always initialized when session restore kicks in at startup.
"scratchpads" should be empty and we should not be calling restoreScratchpadSession:
http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#2759

::: devtools/shim/DevToolsShim.jsm:231
(Diff revision 2)
> +
>      return this.gDevTools.inspectNode(tab, selectors);
>    },
>  
> +  _initDevTools: function () {
> +    // Should call Startup.initDevTools.

You can do:
let { loader } = Cu.import("resource://devtools/shared/Loader.jsm", {});
loader.require("devtools/client/framework/devtools-browser");
And we would refactor that later.

Or call the existing "initDevTools" from devtools-startup.js:
http://searchfox.org/mozilla-central/source/devtools/client/devtools-startup.js#64-69
(you would need to do the wrappedJSObject trick to be able to call it)

::: devtools/shim/tests/unit/test_devtools_shim.js:203
(Diff revision 2)
>    DevToolsShim.unregister();
>    checkCalls(mock, "emit", 2, ["devtools-unregistered"]);
>  }
>  
>  function test_scratchpad_apis() {
> -  ok(!DevToolsShim.isInstalled(), "DevTools are not installed");
> +  mockDevToolsUninstalled();

Wouldn't it be useful to have one test before, dedicated to assert isInstalled behavior?
You are testing isInitialized a lot over all test, but no longer isInstalled.
Attachment #8887882 - Flags: review+
(Assignee)

Comment 7

11 months ago
mozreview-review-reply
Comment on attachment 8887882 [details]
Bug 1382173 - devtools shim support initialized and installed states;

https://reviewboard.mozilla.org/r/158810/#review164122

> Here and after, re-checking for this.isInstalled is redundant with the if(!this.isInstalled() done just before.
> So, the following should be enough:
>   if (!this.isInitialized())

I kept it because I felt it was more readable, but I agree it's redundant. Will remove.

> We should double check that, with that, the tools aren't always initialized when session restore kicks in at startup.
> "scratchpads" should be empty and we should not be calling restoreScratchpadSession:
> http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#2759

I checked and devtools are not initialized at startup with the new patches. restoreLastSession() is only called when the user requests for a session restore, right?

> You can do:
> let { loader } = Cu.import("resource://devtools/shared/Loader.jsm", {});
> loader.require("devtools/client/framework/devtools-browser");
> And we would refactor that later.
> 
> Or call the existing "initDevTools" from devtools-startup.js:
> http://searchfox.org/mozilla-central/source/devtools/client/devtools-startup.js#64-69
> (you would need to do the wrappedJSObject trick to be able to call it)

Option A works for me! I'll implement that and push for review.

> Wouldn't it be useful to have one test before, dedicated to assert isInstalled behavior?
> You are testing isInitialized a lot over all test, but no longer isInstalled.

Yes, will do that for final version of the patch!
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8887902 - Attachment is obsolete: true

Comment 10

11 months ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/134ac1090b4a
devtools shim support initialized and installed states;r=ochameau
https://hg.mozilla.org/mozilla-central/rev/134ac1090b4a
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Updated

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