Closed Bug 1382173 Opened 7 years ago Closed 7 years ago

DevToolsShim should differentiate between installed and initialized states

Categories

(DevTools :: General, enhancement)

Product:
DevTools
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1382173 - devtools shim support initialized and installed states;
59 bytes, text/x-review-board-request
ochameau
: review+
Details 
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 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)
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 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+
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!
Attachment #8887902 - Attachment is obsolete: true
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
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: