Closed Bug 1351835 Opened 3 years ago Closed 3 years ago

Update PrivacyLevel.canSave() calls in SessionCookies.jsm

Categories

(Firefox :: Session Restore, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

Attachments

(1 file)

When bug 1235379 landed, we for some reason didn't touch SessionCookies.jsm. The PrivacyLevel.canSave() method has only a single `isHTTPS` parameter left, the `isPinned` is gone. We can simplify quite a bit of code that existed only to support the deferred privacy level stuff.
Here's a patch that cleans up some more stuff too.
Attachment #8852658 - Flags: review?(mdeboer)
Comment on attachment 8852658 [details] [diff] [review]
0001-Bug-1351835-Update-PrivacyLevel.canSave-calls-in-Ses.patch

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

Nice! I think your commit message could use some TLC, but other than that this is ready to go ;-)

::: browser/components/sessionstore/SessionStore.jsm
@@ +4318,5 @@
>      let cookieHosts = SessionCookies.getHostsForWindow(aTargetWinState);
>  
>      // By creating a regex we reduce overhead and there is only one loop pass
>      // through either array (cookieHosts and aWinState.cookies).
> +    let hosts = [...cookieHosts].join("|").replace(/\./g, "\\.");

Unrelated note: what I know about escaping strings for use in constructing a regex is this:

```js
str => str.replace(/([.*+?\^${}()|\[\]\/\\])/g, "\\$1");
```

I'm not sure what the odds are that any of these characters can be used in a hostname. Punycode?
Attachment #8852658 - Flags: review?(mdeboer) → review+
(In reply to Mike de Boer [:mikedeboer] from comment #2)
> >      // By creating a regex we reduce overhead and there is only one loop pass
> >      // through either array (cookieHosts and aWinState.cookies).
> > +    let hosts = [...cookieHosts].join("|").replace(/\./g, "\\.");
> 
> Unrelated note: what I know about escaping strings for use in constructing a
> regex is this:
> 
> ```js
> str => str.replace(/([.*+?\^${}()|\[\]\/\\])/g, "\\$1");
> ```
> 
> I'm not sure what the odds are that any of these characters can be used in a
> hostname. Punycode?

I honestly have no clue what the regex is used for, or if it's really necessary. I'd prefer to not touch it ;)
https://hg.mozilla.org/mozilla-central/rev/d08bdf680809
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.