Closed Bug 910646 Opened 8 years ago Closed 8 years ago

[Session Restore] Collect docShell capabilities from content script

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 27

People

(Reporter: ttaubert, Assigned: billm)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #804153 - Flags: review?(ttaubert)
Attached patch docshell-remote (obsolete) — Splinter Review
This patch collects the docshell capabilities from a content script when we're doing async collection.
Attachment #804154 - Flags: review?(ttaubert)
Attached patch docshell-remote v2 (obsolete) — Splinter Review
Fixed a bug. How would you suggest testing this code? I'm not even sure how to change the docshell properties.
Attachment #804154 - Attachment is obsolete: true
Attachment #804154 - Flags: review?(ttaubert)
Attachment #804924 - Flags: review?(ttaubert)
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]);

:)
Attachment #804153 - Flags: review?(ttaubert) → review+
Comment on attachment 804924 [details] [diff] [review]
docshell-remote v2

Review of attachment 804924 [details] [diff] [review]:
-----------------------------------------------------------------

\o/
Attachment #804924 - Flags: review?(ttaubert) → review+
(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?
Flags: needinfo?(wmccloskey)
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.
Attachment #804924 - Flags: review+
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+?
Attachment #804924 - Attachment is obsolete: true
Attachment #810710 - Flags: review+
Flags: needinfo?(wmccloskey)
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/mozilla-central/rev/8b905dced61b
https://hg.mozilla.org/mozilla-central/rev/3113b46272ac
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
You need to log in before you can comment on or make changes to this bug.