-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add YouTube test page #68
Conversation
I don't have permission to assign you, but want to take a look @kdzwinel ? |
aea1f9b
to
e725057
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I left some comments + can we add self-evaluation like Charlie did for facebook? So that page tries to figure out, as much as possible, and report back on status of those iframes? We may be able to figure out what has loaded using resource timing api and e.g. check if calls were upgraded to youtube-nocookie.com.
index.html
Outdated
@@ -56,7 +56,8 @@ <h2>Privacy Protections Tests</h2> | |||
<li><a href='./privacy-protections/referrer-trimming/'>Referrer trimming</a></li> | |||
<li><a href='./privacy-protections/https-upgrades/'>HTTPS upgrades</a></li> | |||
<li><a href='./privacy-protections/https-loop-protection/'>HTTPS upgrade loop protection</a></li> | |||
<li><a href='./privacy-protections/click-to-load/'>Facebook click to load</a></li> | |||
<li><a href='./privacy-protections/facebook-click-to-load/'>Facebook click to load</a></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I'm afraid you can't change that URL because some integration tests (like in our extension) already depend on it. So our options are:
- make a redirect in server.js so that /click-to-load/ loads /facebook-click-to-load/
- keep facebook where it is now
- change url but update all clients that depend on it (this might be only our extension? but I'd also check with macOS folks)
- move youtube to a subfolder in /click-to-load/ ? Not sure about this one, but maybe it makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I didn't realise that sorry. I've moved it back for now, we can always rethink if it becomes problematic.
e725057
to
4505c68
Compare
Good point about automating the tests. I'm not exactly sure how that would work with videos, but I will give it some thought. Would you mind if I did that as a follow-up though? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's OK, but please remember to add it before you wrap up your project 👍
There are lots of different types of embedded YouTube videos and ways of controlling them using the YouTube Iframe API[1]. Adding a test page so that we can easily test they all work correctly with our extension and browsers. 1 - https://developers.google.com/youtube/iframe_api_reference
4505c68
to
32417a5
Compare
@kdzwinel OK cool, I have adjusted the page to trigger that CORS bug on Firefox < 89 and also improved the copy. Any chance you could take another pass? |
LGTM, thanks! |
There are lots of different types of embedded YouTube videos and ways
of controlling them using the YouTube Iframe API[1]. Adding a test
page so that we can easily test they all work correctly with our
extension and browsers.
1 - https://developers.google.com/youtube/iframe_api_reference