Closed Bug 1366823 Opened 2 years ago Closed 2 years ago

[devtools-addon] SessionStore.jsm uses devtools ScratchpadManager

Categories

(DevTools :: Scratchpad, defect, P3)

defect

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

Attachments

(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 http://searchfox.org/mozilla-central/rev/3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/browser/components/sessionstore/SessionStore.jsm#2775

- getSessionState at http://searchfox.org/mozilla-central/rev/3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/browser/components/sessionstore/SessionStore.jsm#3145

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 session_store.events.patch (obsolete) — Splinter Review
Attached patch session_store.events.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.

Thanks!
Flags: needinfo?(poirot.alex)
Comment on attachment 8870342 [details] [diff] [review]
session_store.check_installed.patch

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]
session_store.extend_shim.patch

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]
session_store.events.patch

About your question on notifyObserver and JS object, there is a way:
var listener = function (object){
  console.log(object.wrappedJSObject.foo);
}
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){
  console.log(JSON.parse(str).foo);
}
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;

https://reviewboard.mozilla.org/r/143152/#review147024

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

https://reviewboard.mozilla.org/r/143150/#review147532

Looks good, thanks!

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

s/Restored/Restore/

::: 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: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4a94a1906082ddb7b0ab6e25ad8b03f8cfd4f2b
Some oranges on try, but they don't seem related to the patch. Landing.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4d8182ea2319
add Scratchpad manager APIs to DevToolsShim;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/f39ea1fb1be5
Use DevToolsShim to call Scratchpad APIs from SessionStore.jsm;r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/4d8182ea2319
https://hg.mozilla.org/mozilla-central/rev/f39ea1fb1be5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.