The list of docShell capabilities (gDocShellCapabilities) should be collected from the sessionstore content script. gDocShellCapabilities itself should probably be moved to DocShellCapabilities.jsm or the like and offer (de)serialize() methods that we pass a docShell. The list of capabilities can be cached in the JSM as that doesn't change at runtime.

This patch moves the docshell capability collection into a separate module.

This patch collects the docshell capabilities from a content script when we're doing async collection.

Fixed a bug. How would you suggest testing this code? I'm not even sure how to change the docshell properties.

Comment on attachment 804153 [details] [diff] [review] docshell-capabilities Review of attachment 804153 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for attacking this! ::: browser/components/sessionstore/src/DocShellCapabilities.jsm @@ +7,5 @@ > +this.EXPORTED_SYMBOLS = ["DocShellCapabilities"]; > + > +const Cu = Components.utils; > +const Cc = Components.classes; > +const Ci = Components.interfaces; Nit: Those constants aren't used anywhere. @@ +22,5 @@ > + return DocShellCapabilitiesInternal.restore(docShell, disallow); > + }, > +}); > + > +let DocShellCapabilitiesInternal = { Nit: a short descriptive comment would be great. @@ +43,5 @@ > + let disallow = []; > + for (let cap of caps) { > + if (!docShell["allow" + cap]) > + disallow.push(cap); > + } return caps.filter(cap => !docShell["allow" + cap]); :)

Comment on attachment 804924 [details] [diff] [review] docshell-remote v2 Review of attachment 804924 [details] [diff] [review]: ----------------------------------------------------------------- \o/

(In reply to Bill McCloskey (:billm) from comment #3) > Fixed a bug. How would you suggest testing this code? I'm not even sure how > to change the docshell properties. We could use this as a template: http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/browser_capabilities.js Or change it to take async collection into account? Anyway this should probably be easier with bug 911115 landed.

Bill, what do you think about landing the patches here and including the docShell capability tests in bug 917277?

Comment on attachment 804924 [details] [diff] [review] docshell-remote v2 Review of attachment 804924 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/src/SessionStore.jsm @@ +4245,5 @@ > } > > + let disallow = yield Messenger.send(tab, "SessionStore:collectDocShellCapabilities"); > + if (disallow.length > 0) > + tabData.disallow = disallow.join(","); This needs to be updated. Please make sure to collect async data before _collectBaseTabData() is called, basically just like we do for sessionHistory/Storage.

Comment on attachment 804924 [details] [diff] [review] docshell-remote v2 Review of attachment 804924 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/content/content-sessionStore.js @@ +87,5 @@ > case "SessionStore:collectSessionStorage": > let storage = SessionStorage.serialize(docShell); > sendAsyncMessage(name, {id: id, data: storage}); > break; > + case "SessionStorage:collectDocShellCapabilities": This needs to be SessionStore:collectDocShellCapabilities. Oops.

Comment on attachment 804924 [details] [diff] [review] docshell-remote v2 Review of attachment 804924 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/content/content-sessionStore.js @@ +88,5 @@ > let storage = SessionStorage.serialize(docShell); > sendAsyncMessage(name, {id: id, data: storage}); > break; > + case "SessionStorage:collectDocShellCapabilities": > + let disallow = DocShellCapabilities.read(docShell); DocShellCapabilities is undefined, you need to import the JSM.

Sorry, I should have updated this a while ago. Is it okay to keep the r+?

The patches here should be ready to land. The entire stack of patches I have passes tryserver.

Comment on attachment 810710 [details] [diff] [review] docshell-remote v3 Review of attachment 810710 [details] [diff] [review]: ----------------------------------------------------------------- Yep, looks good to me!

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b905dced61b https://hg.mozilla.org/integration/mozilla-inbound/rev/3113b46272ac

https://hg.mozilla.org/mozilla-central/rev/8b905dced61b https://hg.mozilla.org/mozilla-central/rev/3113b46272ac