Closed Bug 863227 Opened 10 years ago Closed 10 years ago

List of docShell capabilities to (re)store should not be static

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 23

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

Attachments

(1 file)

I came across the CAPABILITIES constant introduced by bug 493467. I don't like that this says "XXX keep these in sync with all the attributes starting with "allow" in /docshell/base/nsIDocShell.idl". This is just a potential source for future errors because no one that remembers this is even around anymore.

I think we should just lazily collect all nsIDocShell.allow* properties and use that to (re)store docShell capabilities.
I took the test browser_493467.js and renamed it to browser_capabilities.js. That was the only test covering it and I seized the chance to rewrite it a little and actually ensure that all capabilities are allowed again once a tab gets re-used.
Attachment #739021 - Flags: review?(dteller)
Attachment #739021 - Attachment description: lazily retrieve list of nsIDocShell.allow* propeties to (re)store docShell capabilities → lazily retrieve list of nsIDocShell.allow* properties to (re)store docShell capabilities
Comment on attachment 739021 [details] [diff] [review]
lazily retrieve list of nsIDocShell.allow* properties to (re)store docShell capabilities

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +92,5 @@
> +  let capabilities = [];
> +
> +  return docShell => {
> +    if (!capabilities.length) {
> +      for (let key of Object.keys(docShell)) {

Out of curiosity: why not |forEach|? :)

@@ +100,5 @@
> +    }
> +
> +    return capabilities;
> +  };
> +});

That looks a little convoluted. What is the point of making this a lazy getter if its evaluation is already lazy?

@@ +3111,5 @@
>                                                       aIdMap, aDocIdentMap), true);
>      }
>  
>      // make sure to reset the capabilities and attributes, in case this tab gets reused
> +    let disallow = new Set(tabData.disallow ? tabData.disallow.split(",") : []);

Do you actually need this |[]|?

@@ +3113,5 @@
>  
>      // make sure to reset the capabilities and attributes, in case this tab gets reused
> +    let disallow = new Set(tabData.disallow ? tabData.disallow.split(",") : []);
> +    for (let cap of gDocShellCapabilities(browser.docShell))
> +      browser.docShell["allow" + cap] = !disallow.has(cap);

Nice one.

::: browser/components/sessionstore/test/browser_493467.js
@@ +29,5 @@
> +  // Check that we correctly save disallowed features.
> +  let disallowedState = JSON.parse(ss.getTabState(tab));
> +  let disallow = new Set(disallowedState.disallow.split(","));
> +  ok(disallow.has("Images"), "images not allowed");
> +  ok(disallow.has("MetaRedirects"), "meta redirects not allowed");

Perhaps we should rather check whether |disallow| contains exactly the allow* flags set to |false|?

@@ +31,5 @@
> +  let disallow = new Set(disallowedState.disallow.split(","));
> +  ok(disallow.has("Images"), "images not allowed");
> +  ok(disallow.has("MetaRedirects"), "meta redirects not allowed");
> +
> +  // Restore another tab in the current one.

In the current what? Tab in a tab?

@@ +47,5 @@
> +  // Check that we correctly restored features as disabled.
> +  state = JSON.parse(ss.getTabState(tab));
> +  disallow = new Set(state.disallow.split(","));
> +  ok(disallow.has("Images"), "images not allowed anymore");
> +  ok(disallow.has("MetaRedirects"), "meta redirects not allowed anymore");

As above, perhaps we should rather check whether |disallow| contains exactly the allow* flags set to |false|.
Attachment #739021 - Flags: review?(dteller) → review+
(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> > +  return docShell => {
> > +    if (!capabilities.length) {
> > +      for (let key of Object.keys(docShell)) {
> 
> Out of curiosity: why not |forEach|? :)

Ok, let's do it the functional way :)

> That looks a little convoluted. What is the point of making this a lazy
> getter if its evaluation is already lazy?

Yeah, sorry that's nonsense.

> >      // make sure to reset the capabilities and attributes, in case this tab gets reused
> > +    let disallow = new Set(tabData.disallow ? tabData.disallow.split(",") : []);
> 
> Do you actually need this |[]|?

Yeah, we need a fallback to initialize the Set() if nothing is disallowed. "".split() returns [""] so that's unfortunately a useless entry.

> > +  // Check that we correctly save disallowed features.
> > +  let disallowedState = JSON.parse(ss.getTabState(tab));
> > +  let disallow = new Set(disallowedState.disallow.split(","));
> > +  ok(disallow.has("Images"), "images not allowed");
> > +  ok(disallow.has("MetaRedirects"), "meta redirects not allowed");
> 
> Perhaps we should rather check whether |disallow| contains exactly the
> allow* flags set to |false|?

Sure, good idea.

> @@ +31,5 @@
> > +  let disallow = new Set(disallowedState.disallow.split(","));
> > +  ok(disallow.has("Images"), "images not allowed");
> > +  ok(disallow.has("MetaRedirects"), "meta redirects not allowed");
> > +
> > +  // Restore another tab in the current one.
> 
> In the current what? Tab in a tab?

Will clarify.
(In reply to Tim Taubert [:ttaubert] from comment #3)
> > Do you actually need this |[]|?
> 
> Yeah, we need a fallback to initialize the Set() if nothing is disallowed.
> "".split() returns [""] so that's unfortunately a useless entry.

Wouldn't |undefined| do the trick?
(In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> (In reply to Tim Taubert [:ttaubert] from comment #3)
> > > Do you actually need this |[]|?
> > 
> > Yeah, we need a fallback to initialize the Set() if nothing is disallowed.
> > "".split() returns [""] so that's unfortunately a useless entry.
> 
> Wouldn't |undefined| do the trick?

Hmm right. So we could do:

let disallow = new Set(tabData.disallow && tabData.disallow.split(","));
https://hg.mozilla.org/mozilla-central/rev/39ddfeaa09dc
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Depends on: 882575
You need to log in before you can comment on or make changes to this bug.