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

Prerender and preload remote video cache. #35272

Merged

Conversation

gmajoulet
Copy link
Contributor

@gmajoulet gmajoulet commented Jul 16, 2021

Prerender and preload remote video cache.

Additional information:
The XHR request marked as prerenderSafe goes to domain-com.cdn.ampproject.org/**.
The video sources that preload are loaded from the exact same system as the existing video cache that preloads today.

@amp-owners-bot
Copy link

amp-owners-bot bot commented Jul 22, 2021

Hey @ampproject/wg-caching! These files were changed:

validator/testdata/transformed_feature_tests/amp-video-cached-source.html
validator/testdata/transformed_feature_tests/amp-video-cached-source.out

Copy link
Contributor Author

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added transformed AMP validation tests to ensure i-amphtml-video-cached-source is not a valid attr.

PTAL

extensions/amp-video/0.1/amp-video.js Show resolved Hide resolved
extensions/amp-video/0.1/amp-video.js Show resolved Hide resolved
@gmajoulet gmajoulet added this to In progress in wg-stories Sprint via automation Jul 22, 2021
@honeybadgerdontcare
Copy link
Contributor

Added transformed AMP validation tests to ensure i-amphtml-video-cached-source is not a valid attr.

PTAL

Note this isn't a transformed AMP test, but a regular AMP test. Should it be valid for transformed AMP?

Those test cases are under validator/testdata/transformed_feature_tests

Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for regular AMP test, but probably should add a transformed AMP test as well.

@gmajoulet
Copy link
Contributor Author

It should be invalid for both regular and transformed AMP. Should I simply move the files to this folder? I transformed the HTML using the AMP optimizer.

@honeybadgerdontcare
Copy link
Contributor

It should be invalid for both regular and transformed AMP.

This isn't something that the optimizer or AMP caches should do then.

Should I simply move the files to this folder? I transformed the HTML using the AMP optimizer.

Yes and no. Yes they should be in that folder but only copy over the parts specific to this change...

...I'd prefer not to use the optimizer output for these tests. I'd rather they be very targeted to their testing purpose. Normally I'd say use the minimum valid one but that includes the full CSS. An easy one to start off of would be the nonce one (removing the nonce of course) and making it explicit to the test criteria here.

@gmajoulet
Copy link
Contributor Author

The goal of this test is to ensure that i-amphtml-video-cached-source is invalid for both normal and transformed, as making it valid would allow an attacker to break privacy preserving prerendering. So removing the CSS etc sounds good, I'll update the test

@honeybadgerdontcare
Copy link
Contributor

The goal of this test is to ensure that i-amphtml-video-cached-source is invalid for both normal and transformed, as making it valid would allow an attacker to break privacy preserving prerendering. So removing the CSS etc sounds good, I'll update the test

Great, thank you. Please also move it to the feature tests directory for transformed AMP (testdata/transformed_feature_tests/)

@gmajoulet gmajoulet enabled auto-merge (squash) July 22, 2021 19:29
Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ty

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants