Closed Bug 1256153 Opened 4 years ago Closed 4 years ago

Port bug 1245184 for TB: nsICookieManager.remove now takes an extra argument

Categories

(Thunderbird :: Preferences, defect)

47 Branch
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 48.0

People

(Reporter: aleth, Assigned: aceman)

References

Details

Attachments

(1 file, 1 obsolete file)

15.08 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
This caused a chat core bug, there's likely usages of this in mail/ and mailnews/ too.
Blocks: 1255177
Flags: needinfo?(acelists)
Blocks: 1245184
No longer blocks: 1255177
Summary: Port bug 1255177 for TB: nsICookieManager.remove now takes an extra argument → Port bug 1245184 for TB: nsICookieManager.remove now takes an extra argument
Thanks for the notification.

Yes, the error does appear when trying to remove a cookie in the TB cookie manager in Preferences.
Assignee: nobody → acelists
Component: General → Preferences
Flags: needinfo?(acelists)
OS: Unspecified → All
Hardware: Unspecified → All
See Also: → 1255177
It seems I need to pull in bug 1199466 too to get all the needed pieces in the cookie manager.
Depends on: 1199466
See Also: → 1251368
Attached patch patch (obsolete) — Splinter Review
I've pulled in also the UserContextUI.jsm file so that the code and xul in our cookie editor is identical to Firefox. But I don't know if we need/want that feature. The SM guys in bug 1251368 also seem to be against it for now.
But they already have quite diverging code.
Attachment #8735458 - Flags: review?(mkmelin+mozilla)
Instantbird needs a patch too. It's imho just one change in twitter.js

>> Services.cookies.remove(cookie.host, cookie.name, cookie.path, false);

>> Services.cookies.remove(cookie.host, cookie.name, cookie.path, 
>>                         cookie.originAttributes, false);

Do it here or I can include it in the suite patch? But I am unable to test it.
I assume that is covered in bug 1255177 already.
Status: NEW → ASSIGNED
Comment on attachment 8735458 [details] [diff] [review]
patch

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

::: mail/components/preferences/cookies.js
@@ +560,5 @@
>        var noneSelected = this._bundle.getString("noCookieSelected");
>        properties = { name: noneSelected, value: noneSelected, host: noneSelected,
>                       path: noneSelected, expires: noneSelected,
> +                     isSecure: noneSelected, userContext: noneSelected };
> +      for (i = 0; i < ids.length; ++i) {

I think you should |var i| here

::: mailnews/base/util/UserContextUI.jsm
@@ +9,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm")
> +
> +XPCOMUtils.defineLazyGetter(this, "gBrowserBundle", function() {
> +  return Services.strings.createBundle("chrome://browser/locale/browser.properties");

You need to change this...
Attachment #8735458 - Flags: review?(mkmelin+mozilla) → review-
Comment on attachment 8735458 [details] [diff] [review]
patch

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

::: mailnews/base/util/UserContextUI.jsm
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +this.EXPORTED_SYMBOLS = ["UserContextUI"];

This looks like a copy of the file in browser/. How about moving the file to toolkit/ instead, so you can just include it? 

Forking it means it will have to be updated in the future every time the m-c version changes (and most of the time you'll notice that only too late, after something breaks).
So do we even want the file? Or work around it as we do not use the functionality? Seamonkey also does not want it.
(In reply to :aceman from comment #8)
> So do we even want the file? Or work around it as we do not use the
> functionality? Seamonkey also does not want it.

I know that SeaMonkey do not see a use for the functionality at this point. Also makes the patch a lot easier to write (see Bug 1251368).
Paenglab might know more about this UI.
Flags: needinfo?(richard.marti)
Yeah I don't think it's really useful for mail/feeds so let's not include it. Always using the "None" default seems fine.
Flags: needinfo?(richard.marti)
Attached patch patch v2Splinter Review
OK
Attachment #8735458 - Attachment is obsolete: true
Attachment #8736888 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8736888 [details] [diff] [review]
patch v2

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

LGTM, r=mkmelin
Attachment #8736888 - Flags: review?(mkmelin+mozilla) → review+
Thanks.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/a49d57503fb2a6c3e1cfee98ba76c4efca7b83b6
Bug 1256153 - migrate to the new 5-argument nsICookieManager.remove(). r=mkmelin
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 48.0
Comment on attachment 8736888 [details] [diff] [review]
patch v2

>+           !ChromeUtils.compareOriginAttributes(aCookieA.originAttributes,
>+                                                aCookieB.originAttributes);

>+              !ChromeUtils.compareOriginAttributes(item.originAttributes,
>+                                                   cookie.originAttributes)) {

Unfortunately !ChromeUtils.compareOriginAttributes got changed to ChromeUtils.isOriginAttributesEqual [sic] before the patch to bug 1245184 even landed... see bug 1245184 comment 17.
(Even after that, something other error is preventing me from removing cookies...)
Which version? The Services.cookies.remove parameters got shuffled around a few times.

See bug 1260399, bug 1259169 and bug 1263252 for SM.
You need to log in before you can comment on or make changes to this bug.