Implement "create a potential-CORS request" and remove deprecatedSetAsPotentiallyCrossOrigin.
Created attachment 385860 [details] Patch
Created attachment 385863 [details] Patch
Created attachment 385871 [details] Patch
Created attachment 385874 [details] Patch
Created attachment 385879 [details] Patch
Created attachment 385886 [details] Patch
Comment on attachment 385886 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385886&action=review > LayoutTests/ChangeLog:8 > + Adjusts test to same-origin fallback behavior by using CORS. These changes seem to suggest high likelihood of breaking behaviors websites could rely on. What evidence exists to demonstrate them being fine? > LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/track/track-element/cloneNode-expected.txt:1 > +CONSOLE MESSAGE: Unsafe attempt to load URL javascript:"network error" from origin http://localhost:8800. Domains, protocols and ports must match. Can you elaborate on why this is right, in particular?
(In reply to Alexey Proskuryakov from comment #7) > Comment on attachment 385886 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=385886&action=review > > > LayoutTests/ChangeLog:8 > > + Adjusts test to same-origin fallback behavior by using CORS. > > These changes seem to suggest high likelihood of breaking behaviors websites > could rely on. What evidence exists to demonstrate them being fine? It is specified that way for track loading: https://html.spec.whatwg.org/multipage/media.html#sourcing-out-of-band-text-tracks:create-a-potential-cors-request Chrome and Firefox implement it (I should have added that to the Changelog, will fix). > > LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/track/track-element/cloneNode-expected.txt:1 > > +CONSOLE MESSAGE: Unsafe attempt to load URL javascript:"network error" from origin http://localhost:8800. Domains, protocols and ports must match. > > Can you elaborate on why this is right, in particular? That does look a bit odd, I will debug what is triggering those console messages.
Created attachment 385947 [details] Patch
Comment on attachment 385947 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385947&action=review > Source/WebCore/ChangeLog:11 > + The new behavior matches that of Chrome and Firefox. Can you detail what behavioural changes are one there?
Created attachment 386201 [details] Patch
Comment on attachment 385947 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385947&action=review >> Source/WebCore/ChangeLog:11 >> + The new behavior matches that of Chrome and Firefox. > > Can you detail what behavioural changes are one there? I tried to add more details in all ChangeLog files. Note that if the same-origin fallback is controversial I can remove it from the patch.
Comment on attachment 386201 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386201&action=review > Source/WebCore/ChangeLog:13 > + not specified. Could we split the patch in two parts: one which is just a refactoring and then the one about text track loading? > Source/WebCore/loader/CrossOriginAccessControl.cpp:122 > +CachedResourceRequest createPotentialAccessControlRequest(ResourceRequest&& request, Document& document, const String& crossOriginAttribute, ResourceLoaderOptions&& options, bool sameOriginFlag) Can we move the options as the second parameter, that will allow to group the moved parameters together. > Source/WebCore/loader/CrossOriginAccessControl.cpp:131 > + auto cachedRequest = CachedResourceRequest { WTFMove(request), WTFMove(options) }; CachedResourceRequest cachedRequest { WTFMove(request), WTFMove(options) }; > Source/WebCore/loader/CrossOriginAccessControl.cpp:142 > auto cachedRequest = CachedResourceRequest { WTFMove(request), WTFMove(options) }; CachedResourceRequest cachedRequest { WTFMove(request), WTFMove(options) }; > Source/WebCore/loader/TextTrackLoader.cpp:163 > + auto cueRequest = createPotentialAccessControlRequest(WTFMove(resourceRequest), document, element.mediaElementCrossOriginAttribute(), WTFMove(options), true); An enum would be slightly clearer.
@EricCarlson, any concern about changing text track loading to same-origin by default, as done by Chrome and Firefox nowadays apparently?
Created attachment 386673 [details] Patch
Comment on attachment 386201 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386201&action=review I think I addressed your comments. >> Source/WebCore/ChangeLog:13 >> + not specified. > > Could we split the patch in two parts: one which is just a refactoring and then the one about text track loading? Doing that now, the new patch does not contain the text track loading changes. >> Source/WebCore/loader/CrossOriginAccessControl.cpp:122 >> +CachedResourceRequest createPotentialAccessControlRequest(ResourceRequest&& request, Document& document, const String& crossOriginAttribute, ResourceLoaderOptions&& options, bool sameOriginFlag) > > Can we move the options as the second parameter, that will allow to group the moved parameters together. Done and adjusted the call sites. >> Source/WebCore/loader/CrossOriginAccessControl.cpp:131 >> + auto cachedRequest = CachedResourceRequest { WTFMove(request), WTFMove(options) }; > > CachedResourceRequest cachedRequest { WTFMove(request), WTFMove(options) }; Done. >> Source/WebCore/loader/CrossOriginAccessControl.cpp:142 >> auto cachedRequest = CachedResourceRequest { WTFMove(request), WTFMove(options) }; > > CachedResourceRequest cachedRequest { WTFMove(request), WTFMove(options) }; Done.
The commit-queue encountered the following flaky tests while processing attachment 386673 [details]: imported/w3c/web-platform-tests/IndexedDB/large-requests-abort.html bug 205724 (author: youennf@gmail.com) imported/w3c/web-platform-tests/IndexedDB/fire-error-event-exception.html bug 201481 (authors: shvaikalesh@gmail.com and youennf@gmail.com) The commit-queue is continuing to process your patch.
The commit-queue encountered the following flaky tests while processing attachment 386673 [details]: imported/w3c/web-platform-tests/IndexedDB/interleaved-cursors-large.html bug 201849 The commit-queue is continuing to process your patch.
Comment on attachment 386673 [details] Patch Clearing flags on attachment: 386673 Committed r254000: <https://trac.webkit.org/changeset/254000>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58298224>
Reopening as a reminder that Eric still needs to react (comment 14) :)
(In reply to youenn fablet from comment #14) > @EricCarlson, any concern about changing text track loading to same-origin > by default, as done by Chrome and Firefox nowadays apparently? No, I think it is the right thing.
(In reply to Rob Buis from comment #22) > Reopening as a reminder that Eric still needs to react (comment 14) :) Closing bug. Thanks for checking in Rob!
Reopening to attach new patch.
Created attachment 388140 [details] Patch
Created attachment 388141 [details] Patch
Created attachment 388214 [details] Patch
Created attachment 388215 [details] Patch
Comment on attachment 388215 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388215&action=review > Source/WebCore/loader/CrossOriginAccessControl.cpp:140 > + options.storedCredentialsPolicy = credentials == FetchOptions::Credentials::Include ? StoredCredentialsPolicy::Use : document.securityOrigin().canRequest(request.url()) ? StoredCredentialsPolicy::Use : StoredCredentialsPolicy::DoNotUse; Can we write it with something like: switch (credentials) { case FetchOptions::Credentials::Omit: ... case FetchOptions::Credentials::Omit: ... case FetchOptions::Credentials::Include: ... case FetchOptions::Credentials::SameOrigin: ... } This should be easier to use and we would also ensure consistency between the two values (for instance omit => DoNotUse).
Created attachment 388225 [details] Patch
The commit-queue encountered the following flaky tests while processing attachment 388225 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch.
Comment on attachment 388225 [details] Patch Clearing flags on attachment: 388225 Committed r254821: <https://trac.webkit.org/changeset/254821>
(In reply to youenn fablet from comment #30) > Comment on attachment 388215 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388215&action=review > > > Source/WebCore/loader/CrossOriginAccessControl.cpp:140 > > + options.storedCredentialsPolicy = credentials == FetchOptions::Credentials::Include ? StoredCredentialsPolicy::Use : document.securityOrigin().canRequest(request.url()) ? StoredCredentialsPolicy::Use : StoredCredentialsPolicy::DoNotUse; > > Can we write it with something like: > switch (credentials) { > case FetchOptions::Credentials::Omit: > ... > case FetchOptions::Credentials::Omit: > ... > case FetchOptions::Credentials::Include: > ... > case FetchOptions::Credentials::SameOrigin: > ... > } > This should be easier to use and we would also ensure consistency between > the two values (for instance omit => DoNotUse). Yes, this did make the code easier to read and reason about.