Closed
Bug 1354331
Opened 7 years ago
Closed 7 years ago
Remove network.http.enablePerElementReferrer
Categories
(Core :: Networking: HTTP, enhancement)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: francois, Assigned: swapneshks, Mentored)
Details
(Keywords: good-first-bug, Whiteboard: [necko-would-take])
Attachments
(1 file, 1 obsolete file)
9.86 KB,
patch
|
valentin
:
review+
baku
:
review+
|
Details | Diff | Splinter Review |
That pref was used to gate the release of per-element referrer policies until the spec was changed to enable this delivery mechanism. The candidate recommendation of the Referrer Policy spec has it: https://www.w3.org/TR/referrer-policy/#referrer-policy-delivery-referrer-attribute and it has made its way into the HTML spec too: https://html.spec.whatwg.org/multipage/infrastructure.html#referrer-policy-attribute Given that we already have several prefs to trim the referrer (https://feeding.cloud.geek.nz/posts/tweaking-referrer-for-privacy-in-firefox/) and that we don't offer a toggle for document-level referrer policies, this per-element pref is not a meaningful user control mechanism.
Updated•7 years ago
|
Assignee | ||
Comment 1•7 years ago
|
||
Hi.. I'm a beginner and would like to take up this bug. I'm still catching up to things...
Flags: needinfo?(francois)
Reporter | ||
Comment 2•7 years ago
|
||
(In reply to swapneshks from comment #1) > Hi.. I'm a beginner and would like to take up this bug. > I'm still catching up to things... Feel free to give it a go. You can start by searching for the name of the pref in https://searchfox.org/ to find out where it's defined and where it's used. From there, you'll get a feel for what needs to change in order to remove it entirely.
Flags: needinfo?(francois)
Assignee | ||
Comment 3•7 years ago
|
||
So I had a look at the usages and found that the pref is being declared at https://searchfox.org/mozilla-central/source/modules/libpref/init/all.js#1548 I tried commenting out the above line, but I am not sure how to check whether the action took any effect. Also, regarding the pref usages, I have the following doubts: 1. Let's pick https://searchfox.org/mozilla-central/source/browser/base/content/browser.js#5821. Do I have to remove only the referrer condition or the entire block? 2. After removal of the pref usage, how do I test whether it took effect?
Flags: needinfo?(francois)
Comment 4•7 years ago
|
||
So these are all the instances of the pref: https://searchfox.org/mozilla-central/search?q=network.http.enablePerElementReferrer&path= (In reply to Swapnesh Kumar Sahoo [:swapneshks] from comment #3) > 1. Let's pick > https://searchfox.org/mozilla-central/source/browser/base/content/browser. > js#5821. Do I have to remove only the referrer condition or the entire block? All instances of the pref would become true. So if (Services.prefs.getBoolPref("network.http.enablePerElementReferrer") && linkNode) { would become if (linkNode) { Also, in webidl files, [SetterThrows, Pref="network.http.enablePerElementReferrer"] becomes [SetterThrows] > 2. After removal of the pref usage, how do I test whether it took effect? I don't think there is much to check. Just make all the changes, submit the patch, and we'll make sure all the tests are still passing.
Assignee: nobody → swapneshks
Flags: needinfo?(francois)
Assignee | ||
Comment 5•7 years ago
|
||
Do let me know if I went wrong anywhere/further changes to be made.
Attachment #8863260 -
Flags: review?(valentin.gosu)
Comment hidden (obsolete) |
Comment 7•7 years ago
|
||
Comment on attachment 8863260 [details] [diff] [review] Remove network.http.enablePerElementReferrer usages Review of attachment 8863260 [details] [diff] [review]: ----------------------------------------------------------------- You missed one instance at: http://searchfox.org/mozilla-central/source/browser/base/content/content.js#530
Attachment #8863260 -
Flags: review?(valentin.gosu)
Assignee | ||
Comment 8•7 years ago
|
||
Patch with changes added for earlier missed instance.
Attachment #8863261 -
Flags: review?(valentin.gosu)
Assignee | ||
Updated•7 years ago
|
Attachment #8863260 -
Attachment is obsolete: true
Comment 9•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b3ce802151165c85336a2e10410feb88e72d5c1
Comment 10•7 years ago
|
||
Comment on attachment 8863261 [details] [diff] [review] Remove network.http.enablePerElementReferrer usages Review of attachment 8863261 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thank you for your contribution!
Attachment #8863261 -
Flags: review?(valentin.gosu) → review+
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Valentin Gosu [:valentin] (Vacation until May 1st) from comment #10) > Comment on attachment 8863261 [details] [diff] [review] > Remove network.http.enablePerElementReferrer usages > > Review of attachment 8863261 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good. Thank you for your contribution! My pleasure! This is my first accepted patch... Feels great!
Comment 12•7 years ago
|
||
WebIDL changes need DOM peer review before they can land.
Keywords: checkin-needed
Updated•7 years ago
|
Attachment #8863261 -
Flags: review?(amarchesini)
Updated•7 years ago
|
Attachment #8863261 -
Flags: review?(amarchesini) → review+
Updated•7 years ago
|
Keywords: checkin-needed
Comment 13•7 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0041706d6e43 Remove network.http.enablePerElementReferrer usages. r=valentin r=baku
Keywords: checkin-needed
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0041706d6e43
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•