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

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

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

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.

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.

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

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.

OK

Comment on attachment 8736888 [details] [diff] [review] patch v2 Review of attachment 8736888 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, r=mkmelin

Thanks.

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

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.

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