1503654 - Port bug 1259169 nsICookieManager::remove() should be back-compatible for 1 or 2 releases and fix comparision

Status: RESOLVED FIXED

(Thunderbird :: Preferences, task)

Description

[Frank-Rainer Grahl (:frg)]

+++ This bug was initially created as a clone of Bug #1256153 +++

Bug 1259169 changed the parameter order for the cookies remove call. As neil pointed out in Bug 1256153 comment 16 the name of the comparision method also changed.

Comment 1

[Frank-Rainer Grahl (:frg)]

Attachment: 1503654-cookies-upto63.patch

Patch up to 63.

Comment 2

[Frank-Rainer Grahl (:frg)]

Attachment: 1503654-cookies-cc.patch

TB 64+ only needs the comparision fixed because bug 1260399 reverted the parameter order.

Comment 3

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9021601 [details] [diff] [review]
1503654-cookies-upto63.patch

[Approval Request Comment]
Regression caused by (bug #): bug 1259169
User impact if declined: cookies can not be deleted.
Testing completed (on c-c, etc.): not yet
Risk to taking this patch (and alternatives if risky): trivial

Patch is untested. Please check it out.

Comment 4

[Jorg K (CEST = GMT+2)]

Comment on attachment 9021601 [details] [diff] [review]
1503654-cookies-upto63.patch

OK. So we note that in TB 60, bool comes before origin attributes.

Our chat friends changed that but forgot the rest of the product:
https://hg.mozilla.org/releases/comm-beta/rev/8f10f52f195829c856757ebb094fe9fbabe639eb#l1.18

Lent me do the checkin, I need to fix the commit message a bit.

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 9021603 [details] [diff] [review]
1503654-cookies-cc.patch

(In reply to Frank-Rainer Grahl (:frg) from comment #2)
> TB 64+ only needs the comparision fixed because bug 1260399 reverted the
> parameter order.
Where do you see that? I don't think so, see:
https://searchfox.org/mozilla-central/search?q=Services.cookies.remove&case=false&regexp=false&path=
Are you trying to confuse me?

I think the patch should be identical for all versions >= 60.

Comment 6

[Frank-Rainer Grahl (:frg)]

> Are you trying to confuse me?

Yes.

> I think the patch should be identical for all versions >= 60

I am sorry you are right. I thought this was reverted because Bug 1260399 explicitly stated "Revert bug 1259169" but I see all it did was made the optional parameter mandatory:

>                in boolean       aBlocked,
> -              [optional] in jsval aOriginAttributes);
> +              in jsval         aOriginAttributes);

Sorry for the confusion. I looked at mozilla-central but don't ask me why I overlooked this. Too many tasks... I will upload a new patch for cc. Needs a minor rebase.

Comment 7

[Frank-Rainer Grahl (:frg)]

Attachment: 1503654-cookies-cc.patch

This should do it for cc.

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 9021687 [details] [diff] [review]
1503654-cookies-cc.patch

Yes, this should do the trick. No need for checkin-needed, I handle it in the morning.

In the commit comment you say:
Bug 1503654 - Correct comparision and parameter order for cookie removal.
Port Bug 1259169 and fix port of Bug 1245184.

I think this should be:
Bug 1503654 - Correct comparision and parameter order for cookie removal.
Port Bug  1245184 (comparison function) and bug 1259169 (parameter order).

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/89c98293e4af
Correct comparision (from bug 1245184) and parameter order (from bug 1259169) for cookie removal. r=jorgk

Comment 10

[Jorg K (CEST = GMT+2)]

TB 60.3.1/TB 60.4 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/c9dd6a36ae6244864117ed63f48bbb6331a0d6bc

No one noticed the problem through the entire 52 ESR cycle :-( - I pushed it now to see whether the ESR also suffers from bug 1503837.

Comment 11

[Jorg K (CEST = GMT+2)]

Beta (TB 64 beta 2):
https://hg.mozilla.org/releases/comm-beta/rev/8338dbce84a93ad65edec1b462b9cd83d7018bf3