If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Remove alwaysAcceptSessionCookies pref

RESOLVED FIXED in Firefox 47

Status

()

Core
Networking: Cookies
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mak, Assigned: malayaleecoder, Mentored)

Tracking

({dev-doc-needed})

unspecified
mozilla47
dev-doc-needed
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [good first bug][lang=cpp])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Keywords: dev-doc-needed
Blocks: 1235199
No longer blocks: 1235199
See Also: → bug 1235199
(Assignee)

Comment 1

2 years ago
Is this bug fixed? Else I would like to work on it!
(Reporter)

Comment 2

2 years ago
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
Assignee: nobody → malayaleecoder
(Assignee)

Comment 3

2 years ago
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
(Assignee)

Comment 4

2 years ago
Created attachment 8710863 [details] [diff] [review]
Bug1234875.diff

Fix-Version_1
Attachment #8710863 - Flags: review?(mak77)
(Assignee)

Comment 5

2 years ago
@marco I have submitted my first patch. Please suggest me some improvements.
(Reporter)

Comment 6

2 years ago
Comment on attachment 8710863 [details] [diff] [review]
Bug1234875.diff

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

Please, also remove mCookiesAlwaysAcceptSession from nsCookiePermission.h
Attachment #8710863 - Flags: review?(mak77)
(Reporter)

Comment 7

2 years ago
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
(Assignee)

Comment 8

2 years ago
Created attachment 8711129 [details] [diff] [review]
Bug1234875-v2.diff

Bug #1234875 - Version 2
Attachment #8710863 - Attachment is obsolete: true
Attachment #8711129 - Flags: review?(mak77)
(Assignee)

Comment 9

2 years ago
@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!
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 10

2 years ago
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 <someuser@somedomain.com>

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+.
Attachment #8711129 - Flags: review?(mak77) → review+
(Assignee)

Comment 11

2 years ago
Created attachment 8712112 [details] [diff] [review]
Bug1234875-3.diff

Final patch
(Assignee)

Comment 12

2 years ago
Thank You Marco for correcting me! I have submitted the final patch.
(Reporter)

Updated

2 years ago
Attachment #8711129 - Attachment is obsolete: true

Comment 13

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/c79c550405c8
(Reporter)

Comment 14

2 years ago
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.
(Assignee)

Comment 15

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c79c550405c8
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.