Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow using CORS filtered image responses as WebGL textures #24340

Merged
merged 6 commits into from Oct 8, 2019

Conversation

@jdm
Copy link
Member

jdm commented Oct 1, 2019

More specifically, this makes the "is this image same origin?" check consider the CORS status of the original response, rather than relying on an overly-strict "is this image's response's URL same-origin with a particular global?" check. To do this, we make the image cache double keyed based on the requested URL as well as the requesting origin, and store the CORS status of the eventual response with the final image that eventually gets sent to the HTMLImageElement consumer.


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

This change is Reviewable

@highfive
Copy link

highfive commented Oct 1, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/htmlvideoelement.rs, components/script/dom/canvasrenderingcontext2d.rs, components/script/dom/htmlimageelement.rs, components/script/dom/htmlcanvaselement.rs
  • @KiChjang: components/script/dom/htmlvideoelement.rs, components/script/dom/canvasrenderingcontext2d.rs, components/net/image_cache.rs, components/net_traits/image/base.rs, components/net_traits/image_cache.rs and 2 more
  • @emilio: components/layout/context.rs
@highfive
Copy link

highfive commented Oct 1, 2019

warning Warning warning

  • These commits modify net, layout, and script code, but no tests are modified. Please consider adding a test!
@jdm
Copy link
Member Author

jdm commented Oct 1, 2019

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Oct 1, 2019

Trying commit 77a1c6a with merge daef8bd...

bors-servo added a commit that referenced this pull request Oct 1, 2019
Allow using CORS filtered image responses as WebGL textures

More specifically, this makes the "is this image same origin?" check consider the CORS status of the original response, rather than relying on an overly-strict "is this image's response's URL same-origin with a particular global?" check. To do this, we make the image cache double keyed based on the requested URL as well as the requesting origin, and store the CORS status of the eventual response with the final image that eventually gets sent to the HTMLImageElement consumer.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #24330
- [x] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24340)
<!-- Reviewable:end -->
@jdm
Copy link
Member Author

jdm commented Oct 1, 2019

This change makes the floor appear in http://paulrouget.com/test/bbjs/.

@bors-servo
Copy link
Contributor

bors-servo commented Oct 1, 2019

💔 Test failed - linux-rel-css

@jdm jdm force-pushed the jdm:image-cache-cors branch from 77a1c6a to 359dfd9 Oct 1, 2019
@highfive highfive removed the S-tests-failed label Oct 1, 2019
@jdm
Copy link
Member Author

jdm commented Oct 1, 2019

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Oct 1, 2019

Trying commit 359dfd9 with merge 8657576...

bors-servo added a commit that referenced this pull request Oct 1, 2019
Allow using CORS filtered image responses as WebGL textures

More specifically, this makes the "is this image same origin?" check consider the CORS status of the original response, rather than relying on an overly-strict "is this image's response's URL same-origin with a particular global?" check. To do this, we make the image cache double keyed based on the requested URL as well as the requesting origin, and store the CORS status of the eventual response with the final image that eventually gets sent to the HTMLImageElement consumer.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #24330
- [x] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24340)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 1, 2019

💔 Test failed - linux-rel-css

@jdm jdm force-pushed the jdm:image-cache-cors branch from ed30fe7 to b6700ac Oct 1, 2019
@jdm
Copy link
Member Author

jdm commented Oct 1, 2019

@asajeffrey Your thoughts on the second commit here would be interesting.

@jdm
Copy link
Member Author

jdm commented Oct 2, 2019

@highfive highfive assigned Manishearth and unassigned SimonSapin Oct 2, 2019
@paulrouget
Copy link
Contributor

paulrouget commented Oct 3, 2019

This change makes the floor appear in http://paulrouget.com/test/bbjs/

I changed the demo to use same-domain images.

I tested the PR and it doesn't fix it.

@paulrouget
Copy link
Contributor

paulrouget commented Oct 3, 2019

@jdm
Copy link
Member Author

jdm commented Oct 3, 2019

I tested the change and the floor appeared for me :<

@paulrouget
Copy link
Contributor

paulrouget commented Oct 3, 2019

I tested the change and the floor appeared for me :<

It's possible that I changed the demo as you were writing the PR. Or maybe I haven't properly tested your patch.

components/net/image_cache.rs Show resolved Hide resolved
components/net_traits/image_cache.rs Show resolved Hide resolved
@jdm
Copy link
Member Author

jdm commented Oct 3, 2019

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Oct 3, 2019

Trying commit e6f37b6 with merge d1b95d9...

@jdm
Copy link
Member Author

jdm commented Oct 7, 2019

@bors-servo r=Manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2019

📌 Commit 27c0e0e has been approved by Manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2019

Testing commit 27c0e0e with merge b90c28c...

bors-servo added a commit that referenced this pull request Oct 7, 2019
Allow using CORS filtered image responses as WebGL textures

More specifically, this makes the "is this image same origin?" check consider the CORS status of the original response, rather than relying on an overly-strict "is this image's response's URL same-origin with a particular global?" check. To do this, we make the image cache double keyed based on the requested URL as well as the requesting origin, and store the CORS status of the eventual response with the final image that eventually gets sent to the HTMLImageElement consumer.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #24330 and fix #24368
- [x] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24340)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2019

💔 Test failed - linux-rel-wpt

@jdm jdm force-pushed the jdm:image-cache-cors branch from 27c0e0e to 6dd4096 Oct 7, 2019
@jdm
Copy link
Member Author

jdm commented Oct 7, 2019

@bors-servo r=Manishearth

@highfive highfive removed the S-tests-failed label Oct 7, 2019
@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2019

📌 Commit 6dd4096 has been approved by Manishearth

@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Oct 7, 2019

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#19529.

bors-servo added a commit that referenced this pull request Oct 7, 2019
Allow using CORS filtered image responses as WebGL textures

More specifically, this makes the "is this image same origin?" check consider the CORS status of the original response, rather than relying on an overly-strict "is this image's response's URL same-origin with a particular global?" check. To do this, we make the image cache double keyed based on the requested URL as well as the requesting origin, and store the CORS status of the eventual response with the final image that eventually gets sent to the HTMLImageElement consumer.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #24330 and fix #24368
- [x] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24340)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2019

Testing commit 6dd4096 with merge 75548f4...

@bors-servo
Copy link
Contributor

bors-servo commented Oct 8, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: Manishearth
Pushing 75548f4 to master...

@bors-servo bors-servo merged commit 6dd4096 into servo:master Oct 8, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
Taskcluster (pull_request) TaskGroup: success
Details
homu Test successful
Details
@jdm jdm moved this from TODO:2D to fixed in babylonjs Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.