Implement referrer policy delivery by header #12441

aravind-pg · 2016-07-14T05:00:38Z

Adds a new Option<ReferrerPolicy> field to Document and sets it appropriately in ScriptThread::load if a Referrer-Policy header is present.

r? @jdm

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #11860
There are tests for these changes

This change is

highfive · 2016-07-14T05:00:42Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/document.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/domparser.rs, components/script/script_thread.rs, components/script/dom/domimplementation.rs, components/script/parse/html.rs, components/script/dom/node.rs, components/script/dom/xmldocument.rs

jdm · 2016-07-14T14:17:15Z

Great work!
-S-awaiting-review +S-needs-code-changes

Reviewed 2 of 2 files at r1, 8 of 8 files at r2, 26 of 26 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

components/script/script_thread.rs, line 1713 [r2] (raw file):

        };

        use hyper::header::ReferrerPolicy as ReferrerPolicyHeader;

Let's add this with the other imports at the top of the file.

components/script/script_thread.rs, line 1715 [r2] (raw file):

        use hyper::header::ReferrerPolicy as ReferrerPolicyHeader;
        let referrer_policy = if let Some(headers) = metadata.headers {
            match headers.get::<ReferrerPolicyHeader>() {

headers.get::<ReferrerPolicyHeader>().map(|h| match *h {
    ReferrerPolicyHeader::NoReferrer => ReferrerPolicy::NoReferrer,
    //etc
})

components/script/dom/document.rs, line 1662 [r2] (raw file):

        // Once other delivery methods are implemented, make the unset case really
        // unset (i.e. None).
        let referrer_policy = if referrer_policy.is_some() {

let referrer_policy = referrer_policy.or(Some(ReferrerPolicy::NoReferrer));

Comments from Reviewable

aravind-pg · 2016-07-14T15:16:16Z

Review status: 34 of 36 files reviewed at latest revision, 3 unresolved discussions.

components/script/script_thread.rs, line 1713 [r2] (raw file):

Previously, jdm (Josh Matthews) wrote…

Let's add this with the other imports at the top of the file.

Done. Do you think it yould nice to add a `use ReferrerPolicy::*;` locally to make things more readable? Bit noisy at present.

components/script/script_thread.rs, line 1715 [r2] (raw file):

Previously, jdm (Josh Matthews) wrote…

headers.get::<ReferrerPolicyHeader>().map(|h| match *h {
    ReferrerPolicyHeader::NoReferrer => ReferrerPolicy::NoReferrer,
    //etc
})

Great suggestion, done.

components/script/dom/document.rs, line 1662 [r2] (raw file):

Previously, jdm (Josh Matthews) wrote…

let referrer_policy = referrer_policy.or(Some(ReferrerPolicy::NoReferrer));

Nice, done.

Comments from Reviewable

aravind-pg · 2016-07-14T15:27:31Z

Incidentally, is there any interest in renaming NoRefWhenDowngrade NoReferrerWhenDowngrade? Right it now it sticks out a bit as being somewhat inconsistent with the other variants.

jdm · 2016-07-14T15:55:24Z

I agree, renaming that variant would make it more consistent.
-S-awaiting-review +S-needs-code-changes

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

components/script/script_thread.rs, line 1713 [r2] (raw file):

Previously, aravind-pg (Aravind Gollakota) wrote…

Done. Do you think it yould nice to add a use ReferrerPolicy::*; locally to make things more readable? Bit noisy at present.

I could go either way. I think it's fine as-is.

components/script/script_thread.rs, line 1725 [r4] (raw file):

                    ReferrerPolicy::Origin,
                ReferrerPolicyHeader::OriginWhenCrossOrigin =>
                    ReferrerPolicyHeader::OriginWhenCrossOrigin,

This doesn't look like it compiles :)

Comments from Reviewable

aravind-pg · 2016-07-14T16:05:45Z

Whoops! Will fix later today

On 14 Jul 2016 8:55 a.m., "Josh Matthews" notifications@github.com wrote:

I agree, renaming that variant would make it more consistent.

-S-awaiting-review +S-needs-code-changes

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved

discussions.

components/script/script_thread.rs, line 1713 [r2]
https://reviewable.io:443/reviews/servo/servo/12441#-KMdjNEormtv4e_XqDVT:-KMe6C9XIv89MIcx-Xut:1982670286
(raw file

servo/components/script/script_thread.rs

Line 1713 in 5de7db2

use hyper::header::ReferrerPolicy as ReferrerPolicyHeader;

):
Previously, aravind-pg (Aravind Gollakota) wrote…

Done. Do you think it yould nice to add a use ReferrerPolicy::*; locally
to make things more readable? Bit noisy at present.

I could go either way. I think it's fine as-is.

components/script/script_thread.rs, line 1725 [r4]
https://reviewable.io:443/reviews/servo/servo/12441#-KMe69AXHbK77F_vL0cL:-KMe69AXHbK77F_vL0cM:-255094659
(raw file

servo/components/script/script_thread.rs

Line 1725 in 0488d5d

ReferrerPolicyHeader::OriginWhenCrossOrigin,

):
                ReferrerPolicy::Origin,
            ReferrerPolicyHeader::OriginWhenCrossOrigin =>
                ReferrerPolicyHeader::OriginWhenCrossOrigin,
This doesn't look like it compiles :)

Comments from Reviewable
https://reviewable.io:443/reviews/servo/servo/12441#-:-KMe6UuGxouGaD9f_bvr:1979839934

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#12441 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AGl8UxBddFQtKOEI9lKChQ2lVO5PLrgSks5qVlv5gaJpZM4JMFU1
.

bors-servo · 2016-07-14T20:09:45Z

☔ The latest upstream changes (presumably #12416) made this pull request unmergeable. Please resolve the merge conflicts.

aravind-pg · 2016-07-15T06:28:28Z

Rebased, renamed the variant and fixed the compile-error typo.

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

components/script/script_thread.rs, line 1713 [r2] (raw file):

Previously, jdm (Josh Matthews) wrote…

I could go either way. I think it's fine as-is.

I'll just leave it as-is then.

components/script/script_thread.rs, line 1725 [r4] (raw file):

Previously, jdm (Josh Matthews) wrote…

This doesn't look like it compiles :)

Fixed!

Comments from Reviewable


        Bump hyper to 0.9.10


        script: Obtain referrer policy from header


        test: Update wpt expectations


        msg: Rename `ReferrerPolicy::NoRefWhenDowngrade` `NoReferrerWhenDowng…

…rade` This is more consistent with the other variants.

jdm · 2016-07-15T17:16:09Z

@bors-servo: r+
Yay!

Reviewed 30 of 30 files at r5, 8 of 8 files at r6, 26 of 26 files at r7, 6 of 6 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

bors-servo · 2016-07-15T17:16:09Z

📌 Commit bfda32e has been approved by jdm

bors-servo · 2016-07-15T18:25:44Z

⌛ Testing commit bfda32e with merge b382cc2...


        Auto merge of #12441 - aravind-pg:referrer-pol-header, r=jdm

@jdm

Implement referrer policy delivery by header Adds a new `Option<ReferrerPolicy>` field to Document and sets it appropriately in `ScriptThread::load` if a Referrer-Policy header is present. r? @jdm  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11860 - [X] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12441)

bors-servo · 2016-07-15T19:22:24Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive assigned jdm Jul 14, 2016

highfive added the S-awaiting-review label Jul 14, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Jul 14, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 14, 2016

aravind-pg force-pushed the aravind-pg:referrer-pol-header branch from 2418ea8 to 0488d5d Jul 14, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Jul 14, 2016

highfive added the S-needs-rebase label Jul 14, 2016

aravind-pg force-pushed the aravind-pg:referrer-pol-header branch from 0488d5d to d188236 Jul 15, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 15, 2016

aravind-pg added 4 commits Jul 14, 2016

Bump hyper to 0.9.10

920a43d

script: Obtain referrer policy from header

1a242d8

test: Update wpt expectations

e7ae1e8

msg: Rename `ReferrerPolicy::NoRefWhenDowngrade` `NoReferrerWhenDowng…

Loading status checks…

bfda32e

…rade` This is more consistent with the other variants.

aravind-pg force-pushed the aravind-pg:referrer-pol-header branch from d188236 to bfda32e Jul 15, 2016

highfive removed S-awaiting-review S-needs-rebase labels Jul 15, 2016

highfive added the S-awaiting-merge label Jul 15, 2016

bors-servo mentioned this pull request Jul 15, 2016

Send log messages to the constellation #11841

Merged

4 of 4 tasks complete

bors-servo merged commit bfda32e into servo:master Jul 15, 2016
1 of 2 checks passed

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jul 15, 2016

bors-servo mentioned this pull request Jul 15, 2016

Integrate service worker manager thread #11727

Merged

4 of 4 tasks complete

aravind-pg deleted the aravind-pg:referrer-pol-header branch Jul 15, 2016

servo / servo

Implement referrer policy delivery by header #12441

Implement referrer policy delivery by header #12441

aravind-pg commented Jul 14, 2016 •

edited by larsbergstrom

highfive commented Jul 14, 2016

jdm commented Jul 14, 2016

aravind-pg commented Jul 14, 2016

aravind-pg commented Jul 14, 2016

jdm commented Jul 14, 2016

aravind-pg commented Jul 14, 2016

-S-awaiting-review +S-needs-code-changes

discussions.

This doesn't look like it compiles :)

bors-servo commented Jul 14, 2016

aravind-pg commented Jul 15, 2016

jdm commented Jul 15, 2016

bors-servo commented Jul 15, 2016

bors-servo commented Jul 15, 2016

bors-servo commented Jul 15, 2016

servo / servo

Join GitHub today

Implement referrer policy delivery by header #12441

Implement referrer policy delivery by header #12441

Conversation

aravind-pg commented Jul 14, 2016 • edited by larsbergstrom

highfive commented Jul 14, 2016

jdm commented Jul 14, 2016

aravind-pg commented Jul 14, 2016

aravind-pg commented Jul 14, 2016

jdm commented Jul 14, 2016

aravind-pg commented Jul 14, 2016

-S-awaiting-review +S-needs-code-changes

discussions.

This doesn't look like it compiles :)

bors-servo commented Jul 14, 2016

aravind-pg commented Jul 15, 2016

jdm commented Jul 15, 2016

bors-servo commented Jul 15, 2016

bors-servo commented Jul 15, 2016

bors-servo commented Jul 15, 2016

aravind-pg commented Jul 14, 2016 •

edited by larsbergstrom