1366823 - [devtools-addon] SessionStore.jsm uses devtools ScratchpadManager

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect, P3)

Description

[Julian Descottes [:jdescottes]]

(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).

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: session_store.check_installed.patch

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: session_store.extend_shim.patch

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: session_store.events.patch

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: session_store.events.patch

Comment 5

[Julian Descottes [:jdescottes]]

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!

Comment 6

[Alexandre Poirot [:ochameau]]

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.

Comment 7

[Alexandre Poirot [:ochameau]]

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.

Comment 8

[Alexandre Poirot [:ochameau]]

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.

Comment 9

[Julian Descottes [:jdescottes]]

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.

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1366823 - add Scratchpad manager APIs to DevToolsShim;

Review commit: https://reviewboard.mozilla.org/r/143150/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/143150/

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 1366823 - Use DevToolsShim to call Scratchpad APIs from SessionStore.jsm;

DevTools are moving out of mozilla-central and m-c code should only rely on the
DevToolsShim that will remain available in m-c after the migration.
This changeset updates SessionStore.jsm to call the DevToolsShim instead of the
scratchpadmanager.

The actual underlying implementation is the same when devtools are installed and
no-ops when they are not.

Review commit: https://reviewboard.mozilla.org/r/143152/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/143152/

Comment 12

[Julian Descottes [:jdescottes]]

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=c774ba72a4f6e6c919697550541a128b3b702c8f

Comment 13

[Mike de Boer [:mikedeboer]]

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!

Comment 14

[Alexandre Poirot [:ochameau]]

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.

Comment 15

[Julian Descottes [:jdescottes]]

Comment on attachment 8871679 [details]
Bug 1366823 - add Scratchpad manager APIs to DevToolsShim;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143150/diff/1-2/

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8871680 [details]
Bug 1366823 - Use DevToolsShim to call Scratchpad APIs from SessionStore.jsm;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143152/diff/1-2/

Comment 17

[Julian Descottes [:jdescottes]]

Thanks for the reviews! Previous try timed out on the decision task. New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4a94a1906082ddb7b0ab6e25ad8b03f8cfd4f2b

Comment 18

[Julian Descottes [:jdescottes]]

Some oranges on try, but they don't seem related to the patch. Landing.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4d8182ea2319
https://hg.mozilla.org/mozilla-central/rev/f39ea1fb1be5