Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2476943002:
Ensure video poster preloads use the crossorigin attribute (Closed)
|
Created:
4 years, 1 month ago by Charlie Harrison Modified:
4 years, 1 month ago Reviewers:
Yoav Weiss CC:
chromium-reviews, Yoav Weiss, posciak+watch_chromium.org, blink-reviews-html_chromium.org, loading-reviews+parser_chromium.org, dglazkov+blink, blink-reviews, kinuko+watch Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnsure video poster preloads use the crossorigin attribute
Previously, the preload request is sent out no-cors even if crossorigin
attribute is present.
BUG=658575
Committed: https://crrev.com/b5132796d548ab65d4b537f03f72969b93217833
Cr-Commit-Position: refs/heads/master@{#429988}
Patch Set 1 #Patch Set 2 : Remove empty script block (trybots previous) #
Total comments: 4
Patch Set 3 : add tests #
Messages
Total messages: 18 (8 generated)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
csharrison@chromium.org changed reviewers: + yoav@yoav.ws
Yoav, PTAL. Looks like the other preloads got this check except for posters.
Code change looks good (probably an omission when we added poster support). I don't really understand what the test is testing. Would also be great to test more attribute values and make sure they're respected. https://codereview.chromium.org/2476943002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/preload/preload-video-cors.html (right): https://codereview.chromium.org/2476943002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/preload/preload-video-cors.html:8: assert_equals(entries.length, 0); Not sure I get what this test is checking.
https://codereview.chromium.org/2476943002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/preload/preload-video-cors.html (right): https://codereview.chromium.org/2476943002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/preload/preload-video-cors.html:8: assert_equals(entries.length, 0); On 2016/11/04 16:07:37, Yoav Weiss wrote: > Not sure I get what this test is checking. I was trying to check that we never successfully download the resource in a web visible way. I'm open to better ways of doing this though.
https://codereview.chromium.org/2476943002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/preload/preload-video-cors.html (right): https://codereview.chromium.org/2476943002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/preload/preload-video-cors.html:8: assert_equals(entries.length, 0); On 2016/11/04 16:11:32, Charlie Harrison wrote: > On 2016/11/04 16:07:37, Yoav Weiss wrote: > > Not sure I get what this test is checking. > > I was trying to check that we never successfully download the resource in a web > visible way. I'm open to better ways of doing this though. OK. Can you also add tests for: * the same crossorigin value where the server allows CORS and see that only 1 entry is added (so no double download). * a "no-cors" value where the server disallows CORS and see again only a single entry. <rant> The problem here is that we have no way to test for a double download when the request is expected to fail, but I don't have a good solution for that, other than changing RT to also include entries for CORS failures(not sure this is a valid use case), or creating an internal API that does that. A smarter, stateful server is probably a better way to do that. </rant>
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2476943002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/preload/preload-video-cors.html (right): https://codereview.chromium.org/2476943002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/preload/preload-video-cors.html:8: assert_equals(entries.length, 0); On 2016/11/04 16:45:30, Yoav Weiss wrote: > On 2016/11/04 16:11:32, Charlie Harrison wrote: > > On 2016/11/04 16:07:37, Yoav Weiss wrote: > > > Not sure I get what this test is checking. > > > > I was trying to check that we never successfully download the resource in a > web > > visible way. I'm open to better ways of doing this though. > > OK. Can you also add tests for: > * the same crossorigin value where the server allows CORS and see that only 1 > entry is added (so no double download). > * a "no-cors" value where the server disallows CORS and see again only a single > entry. > > <rant> > The problem here is that we have no way to test for a double download when the > request is expected to fail, but I don't have a good solution for that, other > than changing RT to also include entries for CORS failures(not sure this is a > valid use case), or creating an internal API that does that. A smarter, stateful > server is probably a better way to do that. </rant> I've added the tests in this file. Your rant is noted :) I'm also not quite sure the best solution here.
LGTM!
Thanks :)
The CQ bit was unchecked by csharrison@chromium.org
The CQ bit was checked by csharrison@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Ensure video poster preloads use the crossorigin attribute Previously, the preload request is sent out no-cors even if crossorigin attribute is present. BUG=658575 ========== to ========== Ensure video poster preloads use the crossorigin attribute Previously, the preload request is sent out no-cors even if crossorigin attribute is present. BUG=658575 Committed: https://crrev.com/b5132796d548ab65d4b537f03f72969b93217833 Cr-Commit-Position: refs/heads/master@{#429988} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b5132796d548ab65d4b537f03f72969b93217833 Cr-Commit-Position: refs/heads/master@{#429988} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
