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

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect)

Description

[aleth [:aleth]]

This caused a chat core bug, there's likely usages of this in mail/ and mailnews/ too.

Comment 1

[:aceman]

Thanks for the notification.

Yes, the error does appear when trying to remove a cookie in the TB cookie manager in Preferences.

Comment 2

[:aceman]

It seems I need to pull in bug 1199466 too to get all the needed pieces in the cookie manager.

Comment 3

[:aceman]

Attachment: patch

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.

Comment 4

[Frank-Rainer Grahl (:frg)]

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.

Comment 5

[:aceman]

I assume that is covered in bug 1255177 already.

Comment 6

[Magnus Melin [:mkmelin]]

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...

Comment 7

[aleth [:aleth]]

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).

Comment 8

[:aceman]

So do we even want the file? Or work around it as we do not use the functionality? Seamonkey also does not want it.

Comment 9

[Ian Neal]

(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).

Comment 10

[aleth [:aleth]]

Paenglab might know more about this UI.

Comment 11

[Magnus Melin [:mkmelin]]

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.

Comment 12

[:aceman]

Attachment: patch v2

OK

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 8736888 [details] [diff] [review]
patch v2

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

LGTM, r=mkmelin

Comment 14

[:aceman]

Thanks.

Comment 15

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/a49d57503fb2a6c3e1cfee98ba76c4efca7b83b6
Bug 1256153 - migrate to the new 5-argument nsICookieManager.remove(). r=mkmelin

Comment 16

[[e-mail]]

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.

Comment 17

[[e-mail]]

(Even after that, something other error is preventing me from removing cookies...)

Comment 18

[Frank-Rainer Grahl (:frg)]

Which version? The Services.cookies.remove parameters got shuffled around a few times.

See bug 1260399, bug 1259169 and bug 1263252 for SM.

Comment 19

[Frank-Rainer Grahl (:frg)]

Oops looks like bug 1259169 was never ported to TB 50+

https://hg.mozilla.org/releases/comm-esr60/annotate/dd958ef605d132d08a063f29606737ffb3453e68/mail/components/preferences/cookies.js#l720