Make url for "client" referrer mandatory #26926

MDeiml · 2020-06-15T16:55:53Z

I added a url attribute to Referrer::Client so that the referrer header can be set accordingly when fetching.

Referrer::Client has to be kept separate from Referrer::ReferrerUrl as they differ in this method

Lines 566 to 576 in 6b0d9af

    
           fn Referrer(&self) -> USVString { 
        
               let r = self.request.borrow(); 
        
               USVString(match r.referrer { 
        
                   NetTraitsRequestReferrer::NoReferrer => String::from(""), 
        
                   NetTraitsRequestReferrer::Client => String::from("about:client"), 
        
                   NetTraitsRequestReferrer::ReferrerUrl(ref u) => { 
        
                       let u_c = u.clone(); 
        
                       u_c.into_string() 
        
                   }, 
        
               }) 
        
           }

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #26570 (GitHub issue number if applicable)

There are tests for these changes OR
These changes do not require tests because ___

highfive · 2020-06-15T16:56:03Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @ferjm (or someone else) soon.

highfive · 2020-06-15T16:56:04Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/request.rs, components/script/dom/xmlhttprequest.rs, components/constellation/network_listener.rs, components/script/dom/location.rs, components/script/dom/htmlvideoelement.rs and 23 more
@cbrewster: components/constellation/network_listener.rs, components/constellation/constellation.rs
@jgraham: components/webdriver_server/lib.rs, components/webdriver_server/Cargo.toml
@KiChjang: components/script/dom/request.rs, components/script/dom/xmlhttprequest.rs, components/script_traits/lib.rs, components/script/dom/location.rs, components/script/dom/htmlvideoelement.rs and 27 more

MDeiml · 2020-06-15T16:58:36Z

I know this is quite a lot of changes but most of it is just replacing Referrer::Client with global_scope.get_referrer(). Maybe it would still be best to split this up into smaller changes?

Also I didn't add tests yet because I don't know what the appropriate place to do so would be.

jdm · 2020-06-15T17:15:19Z

@bors-servo try=wpt


        Auto merge of #26926 - MDeiml:referrer-26570, r=<try>

Make url for "client" referrer mandatory  I added a url attribute to `Referrer::Client` so that the referrer header can be set accordingly when fetching. `Referrer::Client` has to be kept separate from `Referrer::ReferrerUrl` as they differ in this method https://github.com/servo/servo/blob/6b0d9afd6fdc28356ad44af0104ddd25a7b6438d/components/script/dom/request.rs#L566-L576 ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #26570 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-06-15T17:15:25Z

⌛ Trying commit 6850fac with merge d05eadd...

bors-servo · 2020-06-15T17:39:59Z

💔 Test failed - status-taskcluster

jdm · 2020-06-15T18:11:00Z

Alright, now we're talking! You're going to want to run ./mach test-wpt tests/wpt/web-platform-tests/referrer-policy /html/semantics/scripting-1/the-script-element/module/ tests/wpt/web-platform-tests/fetch/api /html/semantics/embedded-content/the-img-element/image-loading-lazy-referrerpolicy-change.sub.html tests/wpt/web-platform-tests/xhr --log-raw wpt.log and then ./mach update-wpt wpt.log when that's complete. Then you will want to use git add tests/wpt/metadata so the new expected test results are included in the PR.

MDeiml · 2020-06-15T21:31:56Z

Some tests that previously passed no failed, do I need to check those all individually?

jdm · 2020-06-16T01:56:57Z

@bors-servo try=wpt


        Auto merge of #26926 - MDeiml:referrer-26570, r=<try>

Make url for "client" referrer mandatory  I added a url attribute to `Referrer::Client` so that the referrer header can be set accordingly when fetching. `Referrer::Client` has to be kept separate from `Referrer::ReferrerUrl` as they differ in this method https://github.com/servo/servo/blob/6b0d9afd6fdc28356ad44af0104ddd25a7b6438d/components/script/dom/request.rs#L566-L576 ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #26570 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-06-16T01:57:03Z

⌛ Trying commit 70af0bf with merge 5a6d408...

bors-servo · 2020-06-16T02:41:02Z

☀️ Test successful - status-taskcluster
State: approved= try=True

jdm

These changes look like a great improvement!

components/script/dom/xmlhttprequest.rs

components/script/script_module.rs

jdm · 2020-06-17T16:03:52Z

This PR will need to be rebased against a recent upstream revision as well.


        Make url for "client" referrer mandatory


        Update expected wpt results


        Fix invalid use of ReferrerUrl

jdm · 2020-06-18T14:25:09Z

The failure appears to be consistent. I recommend updating the appropriate ini file manually.

jdm · 2020-06-19T13:56:39Z

@bors-servo r+

bors-servo · 2020-06-19T13:56:40Z

📌 Commit 4039ba3 has been approved by jdm

bors-servo · 2020-06-19T14:22:13Z

⌛ Testing commit 4039ba3 with merge c88b3af...


        Auto merge of #26926 - MDeiml:referrer-26570, r=jdm

Make url for "client" referrer mandatory  I added a url attribute to `Referrer::Client` so that the referrer header can be set accordingly when fetching. `Referrer::Client` has to be kept separate from `Referrer::ReferrerUrl` as they differ in this method https://github.com/servo/servo/blob/6b0d9afd6fdc28356ad44af0104ddd25a7b6438d/components/script/dom/request.rs#L566-L576 ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #26570 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-06-19T15:21:16Z

💔 Test failed - status-taskcluster

jdm · 2020-06-19T15:37:21Z

@bors-servo retry

#24622


        Update expected wpt results

MDeiml · 2020-06-19T15:54:31Z

Oops, you were faster

jdm · 2020-06-19T15:55:33Z

@bors-servo r+

bors-servo · 2020-06-19T15:55:34Z

📌 Commit a0345ee has been approved by jdm

bors-servo · 2020-06-19T15:57:08Z

⌛ Testing commit a0345ee with merge 854cc79...

bors-servo · 2020-06-19T16:23:02Z

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 854cc79 to master...

bors-servo · 2020-06-19T16:28:04Z

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 854cc79 to master...

MDeiml · 2020-06-19T17:06:28Z

This probably also solves #26569

highfive assigned ferjm Jun 15, 2020

highfive added the S-awaiting-review label Jun 15, 2020

highfive added the S-tests-failed label Jun 15, 2020

highfive removed the S-tests-failed label Jun 15, 2020

jdm requested changes Jun 17, 2020

View changes

components/script/dom/xmlhttprequest.rs Outdated Show resolved Hide resolved

components/script/script_module.rs Outdated Show resolved Hide resolved

jdm added S-needs-code-changes and removed S-awaiting-review labels Jun 17, 2020

jdm assigned jdm and unassigned ferjm Jun 17, 2020

highfive added S-awaiting-review and removed S-needs-code-changes labels Jun 17, 2020

jdm added S-needs-rebase and removed S-awaiting-review labels Jun 17, 2020

MDeiml added 3 commits Jun 15, 2020

Make url for "client" referrer mandatory

fa18cf6

Update expected wpt results

23a1b0d

Fix invalid use of ReferrerUrl

4735576

highfive added S-awaiting-review and removed S-tests-failed labels Jun 19, 2020

highfive added S-awaiting-merge and removed S-awaiting-review labels Jun 19, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Jun 19, 2020

Update expected wpt results

Loading status checks…

a0345ee

MDeiml force-pushed the MDeiml:referrer-26570 branch from 4039ba3 to a0345ee Jun 19, 2020

highfive added S-awaiting-review and removed S-tests-failed labels Jun 19, 2020

highfive added S-awaiting-merge and removed S-awaiting-review labels Jun 19, 2020

bors-servo merged commit 854cc79 into servo:master Jun 19, 2020
1 of 2 checks passed

1 of 2 checks passed

Community-TC (pull_request) TaskGroup: Pending (for pull_request.synchronize)
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jun 19, 2020

atouchet mentioned this pull request Jun 19, 2020

Anchor elements and image elements do not include a correct referrer URL #26569

Closed

servo / servo

Make url for "client" referrer mandatory #26926

Make url for "client" referrer mandatory #26926

MDeiml commented Jun 15, 2020 •

edited

highfive commented Jun 15, 2020

highfive commented Jun 15, 2020

MDeiml commented Jun 15, 2020

jdm commented Jun 15, 2020

bors-servo commented Jun 15, 2020

bors-servo commented Jun 15, 2020

jdm commented Jun 15, 2020 •

edited

MDeiml commented Jun 15, 2020

jdm commented Jun 16, 2020

bors-servo commented Jun 16, 2020

bors-servo commented Jun 16, 2020

jdm left a comment

jdm commented Jun 17, 2020

jdm commented Jun 18, 2020

jdm commented Jun 19, 2020

bors-servo commented Jun 19, 2020

bors-servo commented Jun 19, 2020

bors-servo commented Jun 19, 2020

jdm commented Jun 19, 2020

MDeiml commented Jun 19, 2020

jdm commented Jun 19, 2020

bors-servo commented Jun 19, 2020

bors-servo commented Jun 19, 2020

bors-servo commented Jun 19, 2020

bors-servo commented Jun 19, 2020

MDeiml commented Jun 19, 2020

servo / servo

Join GitHub today

Make url for "client" referrer mandatory #26926

Make url for "client" referrer mandatory #26926

Conversation

MDeiml commented Jun 15, 2020 • edited

highfive commented Jun 15, 2020

highfive commented Jun 15, 2020

MDeiml commented Jun 15, 2020

jdm commented Jun 15, 2020

bors-servo commented Jun 15, 2020

bors-servo commented Jun 15, 2020

jdm commented Jun 15, 2020 • edited

MDeiml commented Jun 15, 2020

jdm commented Jun 16, 2020

bors-servo commented Jun 16, 2020

bors-servo commented Jun 16, 2020

jdm left a comment

jdm commented Jun 17, 2020

jdm commented Jun 18, 2020

jdm commented Jun 19, 2020

bors-servo commented Jun 19, 2020

bors-servo commented Jun 19, 2020

bors-servo commented Jun 19, 2020

jdm commented Jun 19, 2020

MDeiml commented Jun 19, 2020

jdm commented Jun 19, 2020

bors-servo commented Jun 19, 2020

bors-servo commented Jun 19, 2020

bors-servo commented Jun 19, 2020

bors-servo commented Jun 19, 2020

MDeiml commented Jun 19, 2020

MDeiml commented Jun 15, 2020 •

edited

jdm commented Jun 15, 2020 •

edited