Closed Bug 1366823 Opened 6 years ago Closed 6 years ago

[devtools-addon] SessionStore.jsm uses devtools ScratchpadManager


(DevTools Graveyard :: Scratchpad, defect, P3)



(firefox55 fixed)

Firefox 55
Tracking Status
firefox55 --- fixed


(Reporter: jdescottes, Assigned: jdescottes)




(2 files, 4 obsolete files)

(extracted from Bug 1361054, where we moved the scratchpad session test to the devtools test suite)

As devtools are moving out of mozilla-central, the scratchpad-manager module might not always be available.

SessionStore.jsm is calling ScratchpadManager in 2 spots:

- restoreSession at

- getSessionState at

The first path is only reached if there are scratchpad sessions saved in the current state. 
The second path is only reached if the scratchpad-manager module is loaded (using Cu.isModuleLoaded("resource://[...]scratchpad-manager.jsm")).

In theory this should keep working even if devtools are not available, but we could explicitly rely on the shim in order to check if devtools are available or not (Bug 1356244).
See Also: → 1361054
Blocks: dt-addon
Attached patch session_store.extend_shim.patch (obsolete) — Splinter Review
Attached patch (obsolete) — Splinter Review
Attached patch (obsolete) — Splinter Review
Attachment #8870345 - Attachment is obsolete: true
Alex, I'm not sure which approach we should take here, can you give me your feedback on the different approaches I tried here?

By increasing order of complexity:
- attachment 8870342 [details] [diff] [review] : simply protect the calls to ScratchpadManager behind a check for DevToolsShim.isInstalled()
- attachment 8870343 [details] [diff] [review] : add new APIs in DevTools shim for Scratchpad (avoids having mc depending on non-shim code, but seems very specific just for the shim)
- attachment 8870346 [details] [diff] [review] : use events. That's what I initially wanted to do but it proved challenging for two reasons: we need to pass objects back and forth between SessionStore and Scratchpad, so I don't think I can simply use Services.obs here and used EventEmitter. Plus the event should be listened to even if scratchpad is not loaded so I needed to create another module that I initialize. I feel like just having events could be good, but the current solution I have with them feels to complex.

Flags: needinfo?(poirot.alex)
Comment on attachment 8870342 [details] [diff] [review]

We would still depend on resource://devtools/client/scratchpad/scratchpad-manager.jsm which is bad. We may easily want to change all that (resource and chrome URLs) in the add-on. Keeping these include path will make that hard.
Flags: needinfo?(poirot.alex)
Attachment #8870342 - Flags: feedback-
Comment on attachment 8870343 [details] [diff] [review]

I feel you will find it too scratchpad specific, but I would even suggest handing over to gDevTools, to really let the add-on control everything:
getOpenedScratchpads() {
  if (!this.isInstalled()) 
    return []
  return this.gDevTools.getOpenedScratchpads()
And similar for restoreScratchpadSession.

I could live with that option.
Attachment #8870343 - Flags: feedback+
Comment on attachment 8870346 [details] [diff] [review]

About your question on notifyObserver and JS object, there is a way:
var listener = function (object){
Services.obs.addObserver(listener, "event", false); Services.obs.notifyObservers({wrappedJSObject: {foo : "bar"}}, "event", null);

One another way is to stringify the object:
var listener = function (a, b, str){
Services.obs.addObserver(listener, "event", false); Services.obs.notifyObservers(null, "event", JSON.stringify({foo:"bar"}));

You will find these two patterns in m-c here and there.

Having said that, I imagine it is fine using the event emitter.
This patch looks good to me too, but it should be more up to the maintainer of this code to see the value of such abstraction.
Instead of being "session-store-scratchpad-restore" and "session-store-scratchpad-save", these two events can be "session-store-restore"/"session-store-restore" and be used for other purposes. But it isn't clear if that would benefit SessionRestore codebase...

About the additional module scratchpad-session-observer.js, you may also decide to inline this code into devtools/client/framework/devtools-browser.js. This module already listen to various browser event like this.
Thanks for the detailed answers, especially about the possibility of defining a custom subject for the observer.
I will go with the second option here, I think it's a good compromise.
Attachment #8870342 - Attachment is obsolete: true
Attachment #8870343 - Attachment is obsolete: true
Attachment #8870346 - Attachment is obsolete: true
Comment on attachment 8871680 [details]
Bug 1366823 - Use DevToolsShim to call Scratchpad APIs from SessionStore.jsm;

LGTM, thanks!
Attachment #8871680 - Flags: review?(mdeboer) → review+
Comment on attachment 8871679 [details]
Bug 1366823 - add Scratchpad manager APIs to DevToolsShim;

Looks good, thanks!

::: devtools/client/framework/devtools.js:413
(Diff revision 1)
> +    }
> +    return ScratchpadManager.getSessionState();
> +  },
> +
> +  /**
> +   * Restored the provided array of scratchpad window states.


::: devtools/client/framework/devtools.js:566
(Diff revision 1)
>      gDevTools.unregisterDefaults();
>      removeThemeObserver(this._onThemeChanged);
> -    // Notify the DevToolsShim that DevTools are no longer available, particularly if the
> -    // destroy was caused by disabling/removing the DevTools add-on.
> +    // Do not unregister devtools from the DevToolsShim if the destroy is caused by an
> +    // application shutdown. SessionStore for instance might need to save devtools state

I would mention Scractchpad in this sentence, as there is some related code in this module.
Attachment #8871679 - Flags: review?(poirot.alex) → review+
Thanks for the reviews! Previous try timed out on the decision task. New try:
Some oranges on try, but they don't seem related to the patch. Landing.
Pushed by
add Scratchpad manager APIs to DevToolsShim;r=ochameau
Use DevToolsShim to call Scratchpad APIs from SessionStore.jsm;r=mikedeboer
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.