1234875 - Remove alwaysAcceptSessionCookies pref

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

This pref is unused and especially after bug 606655 it makes a lot of sense to just remove it. Indeed mCookiesAlwaysAcceptSession is now only assigned to, but never used.

Comment 1

[Vishnu (:malayaleecoder)]

Is this bug fixed? Else I would like to work on it!

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

It is not fixed yet, so feel free to work on it.
Do you already have a working build environment?

The scope here is to remove mCookiesAlwaysAcceptSession and any supporting code:
http://mxr.mozilla.org/mozilla-central/search?string=CookiesAlwaysAcceptSession

plus remove network.cookie.alwaysAcceptSessionCookies from all.js
http://mxr.mozilla.org/mozilla-central/search?string=network.cookie.alwaysAcceptSessionCookies

Comment 3

[Vishnu (:malayaleecoder)]

I have got the working environment now. Will fix it as soon as possible and will ping you back!

(In reply to Marco Bonardo [::mak] from comment #2)
> It is not fixed yet, so feel free to work on it.
> Do you already have a working build environment?
> 
> The scope here is to remove mCookiesAlwaysAcceptSession and any supporting
> code:
> http://mxr.mozilla.org/mozilla-central/
> search?string=CookiesAlwaysAcceptSession
> 
> plus remove network.cookie.alwaysAcceptSessionCookies from all.js
> http://mxr.mozilla.org/mozilla-central/search?string=network.cookie.
> alwaysAcceptSessionCookies

Comment 4

[Vishnu (:malayaleecoder)]

Attachment: Bug1234875.diff

Fix-Version_1

Comment 5

[Vishnu (:malayaleecoder)]

@marco I have submitted my first patch. Please suggest me some improvements.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8710863 [details] [diff] [review]
Bug1234875.diff

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

Please, also remove mCookiesAlwaysAcceptSession from nsCookiePermission.h

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Also, you should add author and a commit message to the patch.

See https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 8

[Vishnu (:malayaleecoder)]

Attachment: Bug1234875-v2.diff

Bug #1234875 - Version 2

Comment 9

[Vishnu (:malayaleecoder)]

@Marco, Very sorry, I had forgotten about nsCookiePermission.h completely. I have added a new patch with author and commit message. Please have a look at it!

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8711129 [details] [diff] [review]
Bug1234875-v2.diff

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

Thank you, from a code pooint of view this now looks good.
But the patch is still missing author information and the commit message is not in the expected format.

Notice that setting -U in mercurial.ini qnew defaults works only for newly created queue patches.
To properly assign an author to an existing qdiff you should do an hg qref -U (provided you properly set username in Mercurial.ini)
In the final diff file you should see something like:
# HG changeset patch
# User someuser <[e-mail]>

Also please check again the commit message conventions we use:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions

Please attach an updated diff with the missing data, you don't need a further r+.

Comment 11

[Vishnu (:malayaleecoder)]

Attachment: Bug1234875-3.diff

Final patch

Comment 12

[Vishnu (:malayaleecoder)]

Thank You Marco for correcting me! I have submitted the final patch.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c79c550405c8

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Thanks. The author was not correct yet, did you set it manually in the commit message? For the next patch please go through the documentation and try to figure how to do it properly, you really need to set Mercurial.ini correctly or use hg qref -U or hg qref -u "user <user@mail>" when necessary.

I corrected those minor details and pushed the patch.

Comment 15

[Vishnu (:malayaleecoder)]

Yes, I did set it in the commit message. I am still having problems configuring my mercurial. Will fix that ASAP. Thank you very much, @Marco, for getting me started into Bugzilla.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c79c550405c8