Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1313733006:
Allowing registering arbitrary schemes as supporting the fetch API. (Closed)
|
Created:
5 years, 4 months ago by not at google - send to devlin Modified:
5 years, 3 months ago CC:
blink-reviews, falken Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAllowing registering arbitrary schemes as supporting the fetch API.
Currently it's only exposed to the HTTP family, but now that Chrome
extensions support service workers they need to support the fetch API.
This patch adds registerURLSchemeAsSupportingFetchAPI to the Blink API,
queried by the fetch API code as shouldTreatURLSchemeAsSupportingFetchAPI.
BUG=466876
R=yhirano@chromium.org, dcheng@chromium.org, pdr@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201492
Patch Set 1 #
Total comments: 2
Patch Set 2 : SupportingFetchAPI #
Total comments: 14
Patch Set 3 : comment #Patch Set 4 : comments #Patch Set 5 : rebase #
Total comments: 1
Patch Set 6 : negation fix #
Total comments: 3
Patch Set 7 : http-like #2 #
Messages
Total messages: 37 (8 generated)
Does this look right? +falken for the SW context.
falken@chromium.org changed reviewers: + falken@chromium.org
Fetch and SW are now totally separate APIs (although historically Fetch was
implemented for SW at first). So unlike SW, Fetch is exposed to HTTP sites.
But this check looks like it's only checking the URL being fetched, not the
execution context of the fetch call. Is the goal to allow calls like
fetch('chrome-extension://xxxxx')? If so, it might be cleanest to do a new
function like: SchemeRegistry::shouldTreatURLSchemeAsFetchable
yhirano would have definitive advice but seems he's away today.
falken@chromium.org changed reviewers: + hiroshige@chromium.org
+hiroshige
tyoshino@chromium.org changed reviewers: + tyoshino@chromium.org
https://codereview.chromium.org/1313733006/diff/1/Source/modules/fetch/FetchM... File Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1313733006/diff/1/Source/modules/fetch/FetchM... Source/modules/fetch/FetchManager.cpp:460: ASSERT(SchemeRegistry::ShouldTreatURLShemeAsAllowingServiceWorkers(m_request->url().procol())); Maybe just WIP. But Sheme -> Scheme Should -> should procol -> protocol
On 2015/08/25 03:43:14, falken wrote:
> Fetch and SW are now totally separate APIs (although historically Fetch was
> implemented for SW at first). So unlike SW, Fetch is exposed to HTTP sites.
>
> But this check looks like it's only checking the URL being fetched, not the
> execution context of the fetch call. Is the goal to allow calls like
> fetch('chrome-extension://xxxxx')? If so, it might be cleanest to do a new
> function like: SchemeRegistry::shouldTreatURLSchemeAsFetchable
>
> yhirano would have definitive advice but seems he's away today.
Agreed. They should be separate condition and implemented as a separate
predicates in the scheme registry.
Patchset #2 (id:20001) has been deleted
Thanks for the feedback. PTAL. I've uploaded the Chromium side here: https://codereview.chromium.org/1310953003 for some context. I'm currently writing tests for it but have checked by hand that everything works. https://codereview.chromium.org/1313733006/diff/1/Source/modules/fetch/FetchM... File Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1313733006/diff/1/Source/modules/fetch/FetchM... Source/modules/fetch/FetchManager.cpp:460: ASSERT(SchemeRegistry::ShouldTreatURLShemeAsAllowingServiceWorkers(m_request->url().procol())); On 2015/08/25 05:16:48, tyoshino wrote: > Maybe just WIP. But > > Sheme -> Scheme > Should -> should > procol -> protocol Yeah epic typos here :-)
kalman@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng for Source/web and public/web. Assuming this looks good to the Fetch API folks, does this LGTY?
Doing some more reviewers shuffling. - tyoshino@ you seem to be looking at the code, so could you be primary reviewer? - mwkst@ could you look at Source/platform/weborigin assuming this looks good to tyoshino?
https://codereview.chromium.org/1313733006/diff/40001/Source/platform/weborig... File Source/platform/weborigin/SchemeRegistry.cpp (right): https://codereview.chromium.org/1313733006/diff/40001/Source/platform/weborig... Source/platform/weborigin/SchemeRegistry.cpp:377: if (scheme.isEmpty()) Out of curiosity, any particular reason to special case this?
kalman@chromium.org changed reviewers: + pdr@chromium.org
mkwst -> pdr.
https://codereview.chromium.org/1313733006/diff/40001/Source/platform/weborig... File Source/platform/weborigin/SchemeRegistry.cpp (right): https://codereview.chromium.org/1313733006/diff/40001/Source/platform/weborig... Source/platform/weborigin/SchemeRegistry.cpp:377: if (scheme.isEmpty()) On 2015/08/25 20:07:32, dcheng wrote: > Out of curiosity, any particular reason to special case this? No idea, I just copied service worker code, but I agree in both cases it's weird to be checking for an empty scheme here. It's (hopefully) not like URLSchemesSet will crash with an empty scheme.
I don't know this code very well so I'll apologize in advance for the noob review. The actual plumbing looks fine. https://codereview.chromium.org/1313733006/diff/40001/Source/modules/fetch/Fe... File Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1313733006/diff/40001/Source/modules/fetch/Fe... Source/modules/fetch/FetchManager.cpp:444: if (SchemeRegistry::shouldTreatURLSchemeAsSupportingFetchAPI(m_request->url().protocol())) { This code is in the critical path but has to wait for a mutex. Is that acceptable perf-wise? https://codereview.chromium.org/1313733006/diff/40001/Source/platform/weborig... File Source/platform/weborigin/SchemeRegistry.cpp (right): https://codereview.chromium.org/1313733006/diff/40001/Source/platform/weborig... Source/platform/weborigin/SchemeRegistry.cpp:377: if (scheme.isEmpty()) On 2015/08/25 at 20:09:03, kalman wrote: > On 2015/08/25 20:07:32, dcheng wrote: > > Out of curiosity, any particular reason to special case this? > > No idea, I just copied service worker code, but I agree in both cases it's weird to be checking for an empty scheme here. It's (hopefully) not like URLSchemesSet will crash with an empty scheme. Lets just assert !scheme.isEmpty(). https://codereview.chromium.org/1313733006/diff/40001/Source/platform/weborig... File Source/platform/weborigin/SchemeRegistry.h (right): https://codereview.chromium.org/1313733006/diff/40001/Source/platform/weborig... Source/platform/weborigin/SchemeRegistry.h:96: static bool shouldTreatURLSchemeAsSupportingFetchAPI(const String& scheme); Why is there no unregister?
https://codereview.chromium.org/1313733006/diff/40001/Source/modules/fetch/Fe... File Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1313733006/diff/40001/Source/modules/fetch/Fe... Source/modules/fetch/FetchManager.cpp:444: if (SchemeRegistry::shouldTreatURLSchemeAsSupportingFetchAPI(m_request->url().protocol())) { On 2015/08/25 21:25:09, pdr wrote: > This code is in the critical path but has to wait for a mutex. Is that > acceptable perf-wise? (I will defer to fetch owner to answer this) https://codereview.chromium.org/1313733006/diff/40001/Source/platform/weborig... File Source/platform/weborigin/SchemeRegistry.cpp (right): https://codereview.chromium.org/1313733006/diff/40001/Source/platform/weborig... Source/platform/weborigin/SchemeRegistry.cpp:377: if (scheme.isEmpty()) On 2015/08/25 21:25:10, pdr wrote: > On 2015/08/25 at 20:09:03, kalman wrote: > > On 2015/08/25 20:07:32, dcheng wrote: > > > Out of curiosity, any particular reason to special case this? > > > > No idea, I just copied service worker code, but I agree in both cases it's > weird to be checking for an empty scheme here. It's (hopefully) not like > URLSchemesSet will crash with an empty scheme. > > Lets just assert !scheme.isEmpty(). SGTM, if you're confident, but it would be odd for this function to be different from every other which does check for it. I'd be more comfortable in just removing every check. https://codereview.chromium.org/1313733006/diff/40001/Source/platform/weborig... File Source/platform/weborigin/SchemeRegistry.h (right): https://codereview.chromium.org/1313733006/diff/40001/Source/platform/weborig... Source/platform/weborigin/SchemeRegistry.h:96: static bool shouldTreatURLSchemeAsSupportingFetchAPI(const String& scheme); On 2015/08/25 21:25:10, pdr wrote: > Why is there no unregister? No need for it in our use case, which is registering chrome-extension - that isn't going to change. Note that the other scheme registration functions in this file can't be unregistered either.
LGTM % perf issue. I'm okay with either direction on the assert (my suggestion, or Kalman's current patch).
lgtm https://codereview.chromium.org/1313733006/diff/40001/Source/modules/fetch/Fe... File Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1313733006/diff/40001/Source/modules/fetch/Fe... Source/modules/fetch/FetchManager.cpp:444: if (SchemeRegistry::shouldTreatURLSchemeAsSupportingFetchAPI(m_request->url().protocol())) { On 2015/08/25 22:04:45, kalman wrote: > On 2015/08/25 21:25:09, pdr wrote: > > This code is in the critical path but has to wait for a mutex. Is that > > acceptable perf-wise? > > (I will defer to fetch owner to answer this) We already have one in DocumentThreadableLoader. As we visit here only once for each fetch(), it should be acceptable. Ideally, this kind of registry should be initialized and frozen like ones in src/url/url_util.cc (has just DCHECK on standard_schemes_locked). I'm not sure we can do it on Blink. https://codereview.chromium.org/1313733006/diff/40001/Source/platform/weborig... File Source/platform/weborigin/SchemeRegistry.cpp (right): https://codereview.chromium.org/1313733006/diff/40001/Source/platform/weborig... Source/platform/weborigin/SchemeRegistry.cpp:377: if (scheme.isEmpty()) On 2015/08/25 22:04:45, kalman wrote: > On 2015/08/25 21:25:10, pdr wrote: > > On 2015/08/25 at 20:09:03, kalman wrote: > > > On 2015/08/25 20:07:32, dcheng wrote: > > > > Out of curiosity, any particular reason to special case this? > > > > > > No idea, I just copied service worker code, but I agree in both cases it's > > weird to be checking for an empty scheme here. It's (hopefully) not like > > URLSchemesSet will crash with an empty scheme. > > > > Lets just assert !scheme.isEmpty(). > > SGTM, if you're confident, but it would be odd for this function to be different > from every other which does check for it. I'd be more comfortable in just > removing every check. +1. Change to other methods could be done as a separate CL. https://codereview.chromium.org/1313733006/diff/40001/Source/platform/weborig... File Source/platform/weborigin/SchemeRegistry.h (right): https://codereview.chromium.org/1313733006/diff/40001/Source/platform/weborig... Source/platform/weborigin/SchemeRegistry.h:94: // Schemes that can register as supporting the Fetch API. how about "Schemes that are treated as supporting the Fetch API." if this comment is about schemes registered by this function. If this comment is only about this function, how about "Register schemes that should be treated as supporting the Fetch API."
Let's make sure to add tests later to cover https://code.google.com/p/chromium/issues/detail?id=466876#c20.
recusing myself as it's the domain of fetch api now
On 2015/08/26 07:33:23, tyoshino wrote: > Let's make sure to add tests later to cover > https://code.google.com/p/chromium/issues/detail?id=466876#c20. acknowledged.
dcheng: ping https://codereview.chromium.org/1313733006/diff/40001/Source/platform/weborig... File Source/platform/weborigin/SchemeRegistry.cpp (right): https://codereview.chromium.org/1313733006/diff/40001/Source/platform/weborig... Source/platform/weborigin/SchemeRegistry.cpp:377: if (scheme.isEmpty()) On 2015/08/26 07:24:48, tyoshino wrote: > On 2015/08/25 22:04:45, kalman wrote: > > On 2015/08/25 21:25:10, pdr wrote: > > > On 2015/08/25 at 20:09:03, kalman wrote: > > > > On 2015/08/25 20:07:32, dcheng wrote: > > > > > Out of curiosity, any particular reason to special case this? > > > > > > > > No idea, I just copied service worker code, but I agree in both cases it's > > > weird to be checking for an empty scheme here. It's (hopefully) not like > > > URLSchemesSet will crash with an empty scheme. > > > > > > Lets just assert !scheme.isEmpty(). > > > > SGTM, if you're confident, but it would be odd for this function to be > different > > from every other which does check for it. I'd be more comfortable in just > > removing every check. > > +1. Change to other methods could be done as a separate CL. Acknowledged. https://codereview.chromium.org/1313733006/diff/40001/Source/platform/weborig... File Source/platform/weborigin/SchemeRegistry.h (right): https://codereview.chromium.org/1313733006/diff/40001/Source/platform/weborig... Source/platform/weborigin/SchemeRegistry.h:94: // Schemes that can register as supporting the Fetch API. On 2015/08/26 07:24:48, tyoshino wrote: > how about > > "Schemes that are treated as supporting the Fetch API." > > if this comment is about schemes registered by this function. > > If this comment is only about this function, how about > > "Register schemes that should be treated as supporting the Fetch API." Done. I'll go with the former, looks consistent with the other comments in the file.
Thanks for implementing this feature! 1. Name of shouldTreatURLSchemeAsSupportingFetchAPI(): > schemes as supporting the fetch API. > shouldTreatURLSchemeAsSupportingFetchAPI() How about "HTTP-like scheme supporting the Fetch API" rather than "scheme supporting the Fetch API"? From the code in this CL, the schemes for which shouldTreatURLSchemeAsSupportingFetchAPI() returns true are treated as if HTTP family in Fetch API. There are other schemes, namely "blob", "about", and "data", that can be supported by Fetch API but not handled as if HTTP family; Handling of these schemes are defined in the spec, but different paths from "http"/"https" are taken, so I think shouldTreatURLSchemeAsSupportingFetchAPI() should return false for "blob", "about", and "data". (FYI: my CL for supporting "blob" and "data": https://codereview.chromium.org/1283163003/) 2. Should we have an entry in https://www.chromestatus.com/ or intent-to-ship? 3. > Let's make sure to add tests later to cover > https://code.google.com/p/chromium/issues/detail?id=466876#c20. +1. Have you (manually) tested it? https://codereview.chromium.org/1313733006/diff/40001/Source/modules/fetch/Fe... File Source/modules/fetch/FetchManager.cpp (right): https://codereview.chromium.org/1313733006/diff/40001/Source/modules/fetch/Fe... Source/modules/fetch/FetchManager.cpp:400: if (!m_request->url().protocolIsInHTTPFamily()) { shouldTreatURLSchemeAsSupportingFetchAPI() should be used also here if we want to allow CORS requests to the chrome-extension:// scheme.
On 2015/08/27 09:39:51, hiroshige (slow) wrote: > Thanks for implementing this feature! > > 1. Name of shouldTreatURLSchemeAsSupportingFetchAPI(): > > > schemes as supporting the fetch API. > > shouldTreatURLSchemeAsSupportingFetchAPI() > > How about > "HTTP-like scheme supporting the Fetch API" > rather than > "scheme supporting the Fetch API"? Makes sense given comment below, done. > > From the code in this CL, the schemes for which > shouldTreatURLSchemeAsSupportingFetchAPI() returns true are treated as if HTTP > family in Fetch API. > There are other schemes, namely "blob", "about", and "data", that can be > supported by Fetch API but not handled as if HTTP family; Handling of these > schemes are defined in the spec, but different paths from "http"/"https" are > taken, so I think shouldTreatURLSchemeAsSupportingFetchAPI() should return false > for "blob", "about", and "data". So you want me to specifically filter out those protocols? In SchemeRegistry or FetchManager? I can do that, but this change is getting more complicated than I want. I'm on the chrome extensions team. The only risk here seems to be that registerURLSchemeAsSupportingFetchAPI might get passed some protocol that isn't supported at the blink layer, but I presume that'd be caught at runtime as much as adding any other check would? > (FYI: my CL for supporting "blob" and "data": > https://codereview.chromium.org/1283163003/) > > 2. > Should we have an entry in https://www.chromestatus.com/ or intent-to-ship? No, this is a Chrome extensions feature, we have our own launch process (besides which it's really a bug that this doesn't work already - it's basically "XHR works but fetch does'" for us). > > 3. > > Let's make sure to add tests later to cover > > https://code.google.com/p/chromium/issues/detail?id=466876#c20. > +1. > Have you (manually) tested it? Yes. > > https://codereview.chromium.org/1313733006/diff/40001/Source/modules/fetch/Fe... > File Source/modules/fetch/FetchManager.cpp (right): > > https://codereview.chromium.org/1313733006/diff/40001/Source/modules/fetch/Fe... > Source/modules/fetch/FetchManager.cpp:400: if > (!m_request->url().protocolIsInHTTPFamily()) { > shouldTreatURLSchemeAsSupportingFetchAPI() should be used also here if we want > to allow CORS requests to the chrome-extension:// scheme. Thanks, done.
PTAL, this is blocking me and review latency is quite high. Suggest another reviewer if necessary. https://chromiumcodereview.appspot.com/1313733006/diff/100001/Source/modules/... File Source/modules/fetch/FetchManager.cpp (right): https://chromiumcodereview.appspot.com/1313733006/diff/100001/Source/modules/... Source/modules/fetch/FetchManager.cpp:400: if (SchemeRegistry::shouldTreatURLSchemeAsSupportingFetchAPI(m_request->url().protocol())) { oops, this should have been "!SchemeRegistry(...)" not "SchemeRegistry(...)". https://chromiumcodereview.appspot.com/1313733006/diff/120001/Source/modules/... File Source/modules/fetch/FetchManager.cpp (right): https://chromiumcodereview.appspot.com/1313733006/diff/120001/Source/modules/... Source/modules/fetch/FetchManager.cpp:400: // This may include other HTTP-like schemes if the embedder has added them I added this comment here.
lgtm
lgtm. > So you want me to specifically filter out those protocols? In SchemeRegistry or > FetchManager? I can do that, but this change is getting more complicated than I > want. I'm on the chrome extensions team. No, clarifying that registerURLSchemeAsSupportingFetchAPI shouldn't be called for non-HTTP-like schemes is sufficient (and the current patch set looks good).
https://codereview.chromium.org/1313733006/diff/120001/public/web/WebSecurity... File public/web/WebSecurityPolicy.h (right): https://codereview.chromium.org/1313733006/diff/120001/public/web/WebSecurity... public/web/WebSecurityPolicy.h:79: // Registers a URL scheme that supports the Fetch API. Er, adding "HTTP-like" also here would be better.
lgtm
https://codereview.chromium.org/1313733006/diff/120001/public/web/WebSecurity... File public/web/WebSecurityPolicy.h (right): https://codereview.chromium.org/1313733006/diff/120001/public/web/WebSecurity... public/web/WebSecurityPolicy.h:79: // Registers a URL scheme that supports the Fetch API. On 2015/08/31 08:37:07, hiroshige (slow) wrote: > Er, adding "HTTP-like" also here would be better. Done.
The CQ bit was checked by kalman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tyoshino@chromium.org, pdr@chromium.org, hiroshige@chromium.org, dcheng@chromium.org, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/1313733006/#ps140001 (title: "http-like #2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1313733006/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1313733006/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201492 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
