Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2108573002:
ServiceWorker: Add an API to fallback to renderer for CORS preflight (Closed)
|
Created:
4 years, 5 months ago by shimazu Modified:
4 years, 5 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, jam, kinuko+serviceworker, nhiroki, darin-cc_chromium.org, blink-reviews, horo+watch_chromium.org, kinuko+watch, blink-worker-reviews_chromium.org, falken Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionServiceWorker: Add an API to fallback to renderer for CORS preflight
Currently ServiceWorkerURLRequestJob::ForwardToNetwork invokes restart
of the request and the request goes to the network directly. In
subresource case, this causes an bug when the request needs CORS
preflight.
This patch adds an API to fallback with CORS check for subresources; this
is used at a subsequent patch: aka no-fetch optimization
http://crrev.com/2103063002.
BUG=621788, 605844
Committed: https://crrev.com/e14963a7be1f6445401efb43619301bf9fe3157d
Cr-Commit-Position: refs/heads/master@{#404114}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Add FallbackToNetworkOrRenderer and move a test to another CL #
Total comments: 10
Patch Set 3 : Add comments and rename functions #
Total comments: 4
Patch Set 4 : Update comment and move the check of foreign fetch #
Total comments: 4
Patch Set 5 : Use FallbackToNetwork and update the comment #
Total comments: 6
Patch Set 6 : Fix grammatical problems on the comments #Patch Set 7 : Fix grammatical problems on the comments #Patch Set 8 : Remove dependency #
Messages
Total messages: 41 (17 generated)
shimazu@chromium.org changed reviewers: + horo@chromium.org
PTAL:) Note: http://crrev.com/2103063002 is a subsequent CL.
https://codereview.chromium.org/2108573002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_url_request_job.cc (right): https://codereview.chromium.org/2108573002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_url_request_job.cc:251: void ServiceWorkerURLRequestJob::FallbackToNetwork() { This function name should be FallbackToNetworkOrRenderer(). And it should be called from the error handling codes in: - ServiceWorkerControlleeRequestHandler::OnVersionStatusChanged - ServiceWorkerControlleeRequestHandler::DidUpdateRegistration - ServiceWorkerControlleeRequestHandler::OnUpdatedVersionStatusChanged FallbackToNetwork() should have DCHECK(IsNeedToFallbackToRenderer()). And ForeignFetchRequestHandler and DidLookupRegistrationForMainResource() should directly call FallbackToNetwork(). https://codereview.chromium.org/2108573002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_url_request_job.h (right): https://codereview.chromium.org/2108573002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_url_request_job.h:214: bool IsNeedToFallbackToRenderer(); nit: const https://codereview.chromium.org/2108573002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/fetch/serviceworker/thorough/cors-preflight-nofetch.html (right): https://codereview.chromium.org/2108573002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/fetch/serviceworker/thorough/cors-preflight-nofetch.html:1: <!doctype html> If my understanding is correct, the test files in LayoutTests/http/tests/fetch/serviceworker/ are generated by generate.py. https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test... So, if you want to add "no-fetch" ServiceWorker fetch tests, you should add "serviceworker-nofetch" folder. @hiroshige What do you think about it?
falken@chromium.org changed reviewers: + falken@chromium.org, hiroshige@chromium.org
+hiroshige ^^^
https://codereview.chromium.org/2108573002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/fetch/resources/thorough-util.js (right): https://codereview.chromium.org/2108573002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/fetch/resources/thorough-util.js:25: var EMPTY_WORKER_URL = '/fetch/resources/empty-worker.js' Do we need |BASE_ORIGIN +| before '/fetch/resources/empty-worker.js'? We need a semicolon at the end of the statement. https://google.github.io/styleguide/javascriptguide.xml?showone=Semicolons#Se... https://codereview.chromium.org/2108573002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/fetch/serviceworker/thorough/cors-preflight-nofetch.html (right): https://codereview.chromium.org/2108573002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/fetch/serviceworker/thorough/cors-preflight-nofetch.html:1: <!doctype html> On 2016/06/29 05:31:36, horo wrote: > If my understanding is correct, the test files in > LayoutTests/http/tests/fetch/serviceworker/ are generated by generate.py. > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test... > So, if you want to add "no-fetch" ServiceWorker fetch tests, you should add > "serviceworker-nofetch" folder. > > @hiroshige > What do you think about it? Is this test only needed/meaningful for "no-fetch" cases? Are other tests under thorough/ needed/meaningful for "no-fetch" cases? If yes, adding script-generated serviceworker-nofetch/ folder will make sense. Otherwise, putting a handwritten test somewhere under http/tests/fetch/ (just under http/tests/fetch/?) or http/tests/serviceworker/ might be good. (For http/tests/fetch/, currently handwritten tests are placed under http/tests/fetch/chromium/ and http/tests/fetch/referrer/, chromium-specific and referrer tests respectively)
Patchset #2 (id:20001) has been deleted
Updated. PTAL again:) https://codereview.chromium.org/2108573002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_url_request_job.cc (right): https://codereview.chromium.org/2108573002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_url_request_job.cc:251: void ServiceWorkerURLRequestJob::FallbackToNetwork() { On 2016/06/29 05:31:35, horo wrote: > This function name should be FallbackToNetworkOrRenderer(). > And it should be called from the error handling codes in: > - ServiceWorkerControlleeRequestHandler::OnVersionStatusChanged > - ServiceWorkerControlleeRequestHandler::DidUpdateRegistration > - ServiceWorkerControlleeRequestHandler::OnUpdatedVersionStatusChanged > > FallbackToNetwork() should have DCHECK(IsNeedToFallbackToRenderer()). > And ForeignFetchRequestHandler and DidLookupRegistrationForMainResource() should > directly call FallbackToNetwork(). Done. https://codereview.chromium.org/2108573002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_url_request_job.h (right): https://codereview.chromium.org/2108573002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_url_request_job.h:214: bool IsNeedToFallbackToRenderer(); On 2016/06/29 05:31:35, horo wrote: > nit: const Done. https://codereview.chromium.org/2108573002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/fetch/resources/thorough-util.js (right): https://codereview.chromium.org/2108573002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/fetch/resources/thorough-util.js:25: var EMPTY_WORKER_URL = '/fetch/resources/empty-worker.js' On 2016/06/29 16:12:46, hiroshige wrote: > Do we need |BASE_ORIGIN +| before '/fetch/resources/empty-worker.js'? > > We need a semicolon at the end of the statement. > https://google.github.io/styleguide/javascriptguide.xml?showone=Semicolons#Se... Done. https://codereview.chromium.org/2108573002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/fetch/serviceworker/thorough/cors-preflight-nofetch.html (right): https://codereview.chromium.org/2108573002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/fetch/serviceworker/thorough/cors-preflight-nofetch.html:1: <!doctype html> On 2016/06/29 16:12:46, hiroshige wrote: > On 2016/06/29 05:31:36, horo wrote: > > If my understanding is correct, the test files in > > LayoutTests/http/tests/fetch/serviceworker/ are generated by generate.py. > > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test... > > So, if you want to add "no-fetch" ServiceWorker fetch tests, you should add > > "serviceworker-nofetch" folder. > > > > @hiroshige > > What do you think about it? > > Is this test only needed/meaningful for "no-fetch" cases? > Are other tests under thorough/ needed/meaningful for "no-fetch" cases? > > If yes, adding script-generated serviceworker-nofetch/ folder will make sense. > Otherwise, putting a handwritten test somewhere under http/tests/fetch/ (just > under http/tests/fetch/?) or http/tests/serviceworker/ might be good. > (For http/tests/fetch/, currently handwritten tests are placed under > http/tests/fetch/chromium/ and http/tests/fetch/referrer/, chromium-specific and > referrer tests respectively) Thanks, I didn't know about generate.py. I found other bugs when trying to add no-fetch tests by using generate.py, so I created another CL(http://crrev.com/2116373002/) to add these tests.
driveby nits on the header file https://codereview.chromium.org/2108573002/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_url_request_job.h (right): https://codereview.chromium.org/2108573002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_url_request_job.h:107: void FallbackToNetworkOrRenderer(); Please document why the caller should use ToNetwork vs ToNetworkOrRenderer. https://codereview.chromium.org/2108573002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_url_request_job.h:206: void FinalizeToFallbackToNetwork(); "FinalizeTo" is a bit strange grammar. "FinalizeTo -> "Finalize". Better yet, maybe "CommitFallback*" similar to the "CommitResponseHeader" function. https://codereview.chromium.org/2108573002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_url_request_job.h:213: // true if need to send back a response with fall_back_required set as true to super nit: First letter should be capitalized (see git gs "// true" vs "// True"). https://codereview.chromium.org/2108573002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_url_request_job.h:215: bool IsNeedToFallbackToRenderer() const; "IsNeedTo" -> "Is*Needed" (see git gs "IsNeedTo" vs git gs "Is.*Needed")
lgtm Please incorporate with falken's comment.
https://codereview.chromium.org/2108573002/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_url_request_job.h (right): https://codereview.chromium.org/2108573002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_url_request_job.h:107: void FallbackToNetworkOrRenderer(); On 2016/07/04 08:23:02, falken wrote: > Please document why the caller should use ToNetwork vs ToNetworkOrRenderer. Done. https://codereview.chromium.org/2108573002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_url_request_job.h:206: void FinalizeToFallbackToNetwork(); On 2016/07/04 08:23:02, falken wrote: > "FinalizeTo" is a bit strange grammar. "FinalizeTo -> "Finalize". Better yet, > maybe "CommitFallback*" similar to the "CommitResponseHeader" function. Sorry, what do you mean for the latter sentence? (If my understanding is correct, I think "Finalize" is better than "Commit" because it's clearer to show here is the end of this job.) https://codereview.chromium.org/2108573002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_url_request_job.h:213: // true if need to send back a response with fall_back_required set as true to On 2016/07/04 08:23:02, falken wrote: > super nit: First letter should be capitalized (see git gs "// true" vs "// > True"). Done. https://codereview.chromium.org/2108573002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_url_request_job.h:215: bool IsNeedToFallbackToRenderer() const; On 2016/07/04 08:23:02, falken wrote: > "IsNeedTo" -> "Is*Needed" (see git gs "IsNeedTo" vs git gs "Is.*Needed") Done.
Sorry to come to this review late. https://codereview.chromium.org/2108573002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_url_request_job.cc (right): https://codereview.chromium.org/2108573002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_url_request_job.cc:251: void ServiceWorkerURLRequestJob::FallbackToNetwork() { On 2016/06/29 05:31:35, horo wrote: > This function name should be FallbackToNetworkOrRenderer(). > And it should be called from the error handling codes in: > - ServiceWorkerControlleeRequestHandler::OnVersionStatusChanged > - ServiceWorkerControlleeRequestHandler::DidUpdateRegistration > - ServiceWorkerControlleeRequestHandler::OnUpdatedVersionStatusChanged > > FallbackToNetwork() should have DCHECK(IsNeedToFallbackToRenderer()). > And ForeignFetchRequestHandler and DidLookupRegistrationForMainResource() should > directly call FallbackToNetwork(). What's the reason for both FallbackToNetwork() and FallbackToNetworkOrRenderer()? Having just one function that always calls IsFallbackToRendererNeeded() would simplify the API. IsFallbackToRendererNeeded() is also light enough that I think there wouldn't be a performance impact. https://codereview.chromium.org/2108573002/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_url_request_job.h (right): https://codereview.chromium.org/2108573002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_url_request_job.h:206: void FinalizeToFallbackToNetwork(); On 2016/07/05 03:33:57, shimazu wrote: > On 2016/07/04 08:23:02, falken wrote: > > "FinalizeTo" is a bit strange grammar. "FinalizeTo -> "Finalize". Better yet, > > maybe "CommitFallback*" similar to the "CommitResponseHeader" function. > > Sorry, what do you mean for the latter sentence? > > (If my understanding is correct, I think "Finalize" is better than "Commit" > because it's clearer to show here is the end of this job.) Hm, OK. Either one is OK. Finalize means the same thing as Commit to me... and since there is already a Commit* function, it'd be consistent to also use Commit here. Finalize didn't really imply end of job to me. https://codereview.chromium.org/2108573002/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_url_request_job.cc (right): https://codereview.chromium.org/2108573002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_url_request_job.cc:727: FinalizeFallbackToRenderer(); Does this patch change behavior for foreign fetch? It looks like before this patch, a foreign fetch event could reach lines 732-734. Or is request_mode_ always FETCH_REQUEST_MODE_CORS_* when event type is foreign fetch? https://codereview.chromium.org/2108573002/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_url_request_job.h (right): https://codereview.chromium.org/2108573002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_url_request_job.h:110: // the request should go to the network directly. Reading this makes me feel there should just be one Fallback*() function. Is it really worth splitting into two functions? I see patch set 1 originally just had one function.
falken@, PTAL again? https://codereview.chromium.org/2108573002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_url_request_job.cc (right): https://codereview.chromium.org/2108573002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_url_request_job.cc:251: void ServiceWorkerURLRequestJob::FallbackToNetwork() { On 2016/07/05 06:49:28, falken wrote: > On 2016/06/29 05:31:35, horo wrote: > > This function name should be FallbackToNetworkOrRenderer(). > > And it should be called from the error handling codes in: > > - ServiceWorkerControlleeRequestHandler::OnVersionStatusChanged > > - ServiceWorkerControlleeRequestHandler::DidUpdateRegistration > > - ServiceWorkerControlleeRequestHandler::OnUpdatedVersionStatusChanged > > > > FallbackToNetwork() should have DCHECK(IsNeedToFallbackToRenderer()). > > And ForeignFetchRequestHandler and DidLookupRegistrationForMainResource() > should > > directly call FallbackToNetwork(). > > What's the reason for both FallbackToNetwork() and > FallbackToNetworkOrRenderer()? Having just one function that always calls > IsFallbackToRendererNeeded() would simplify the API. > IsFallbackToRendererNeeded() is also light enough that I think there wouldn't be > a performance impact. It's just for error cases. I think we should handle such an error case as an error instead of falling back to the network directly, but currently I kept the same behavior as the current. https://codereview.chromium.org/2108573002/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_url_request_job.h (right): https://codereview.chromium.org/2108573002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_url_request_job.h:206: void FinalizeToFallbackToNetwork(); On 2016/07/05 06:49:28, falken wrote: > On 2016/07/05 03:33:57, shimazu wrote: > > On 2016/07/04 08:23:02, falken wrote: > > > "FinalizeTo" is a bit strange grammar. "FinalizeTo -> "Finalize". Better > yet, > > > maybe "CommitFallback*" similar to the "CommitResponseHeader" function. > > > > Sorry, what do you mean for the latter sentence? > > > > (If my understanding is correct, I think "Finalize" is better than "Commit" > > because it's clearer to show here is the end of this job.) > > Hm, OK. Either one is OK. Finalize means the same thing as Commit to me... and > since there is already a Commit* function, it'd be consistent to also use Commit > here. Finalize didn't really imply end of job to me. Done. https://codereview.chromium.org/2108573002/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_url_request_job.cc (right): https://codereview.chromium.org/2108573002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_url_request_job.cc:727: FinalizeFallbackToRenderer(); On 2016/07/05 06:49:28, falken wrote: > Does this patch change behavior for foreign fetch? It looks like before this > patch, a foreign fetch event could reach lines 732-734. Or is request_mode_ > always FETCH_REQUEST_MODE_CORS_* when event type is foreign fetch? Oh, I misunderstood the condition of fetch_type_. It's not what I indended. Fixed. https://codereview.chromium.org/2108573002/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_url_request_job.h (right): https://codereview.chromium.org/2108573002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_url_request_job.h:110: // the request should go to the network directly. On 2016/07/05 06:49:28, falken wrote: > Reading this makes me feel there should just be one Fallback*() function. Is it > really worth splitting into two functions? I see patch set 1 originally just had > one function. Replied to the patch set 1
https://codereview.chromium.org/2108573002/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_controllee_request_handler.cc (right): https://codereview.chromium.org/2108573002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler.cc:288: job_->FallbackToNetworkOrRenderer(); Sorry I no longer understand this. We only get here from DidLookupRegistrationForMainResource only, which means job_ is for a main resource request. So FallbackToNetworkOrRenderer would always FallbackToNetwork, no? Same for the call sites below. https://codereview.chromium.org/2108573002/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_url_request_job.h (right): https://codereview.chromium.org/2108573002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_url_request_job.h:110: // it's apparent that the request should go to the network directly. I don't understand the error case thing anymore too. The callsites of FallbackToNetwork after this patch are: (A) ForeignFetchRequestHandler (2) (B) SWControlleeRequestHandler::DidLookupRegistrationForMainResource (4) (A) makes sense because IsFallbackToRendererNeeded returns false when the event type is FOREIGN_FETCH. (B) makes sense because IsFallbackToRendererNeeded returns false when the resource type is main resource. I'm not sure error cases have anything to do with it?
https://codereview.chromium.org/2108573002/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_controllee_request_handler.cc (right): https://codereview.chromium.org/2108573002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler.cc:288: job_->FallbackToNetworkOrRenderer(); On 2016/07/07 01:11:12, falken wrote: > Sorry I no longer understand this. We only get here from > DidLookupRegistrationForMainResource only, which means job_ is for a main > resource request. So FallbackToNetworkOrRenderer would always FallbackToNetwork, > no? > > Same for the call sites below. Ah, yes, it seems correct. Actually I'd like to use FallbackToNetworkOrRenderer at the next patch: http://crrev.com/2103063002. Fixed. https://codereview.chromium.org/2108573002/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_url_request_job.h (right): https://codereview.chromium.org/2108573002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_url_request_job.h:110: // it's apparent that the request should go to the network directly. On 2016/07/07 01:11:12, falken wrote: > I don't understand the error case thing anymore too. The callsites of > FallbackToNetwork after this patch are: > > (A) ForeignFetchRequestHandler (2) > (B) SWControlleeRequestHandler::DidLookupRegistrationForMainResource (4) > > (A) makes sense because IsFallbackToRendererNeeded returns false when the event > type is FOREIGN_FETCH. > (B) makes sense because IsFallbackToRendererNeeded returns false when the > resource type is main resource. > > I'm not sure error cases have anything to do with it? > I see, I misunderstood what it's doing. Updated the comment, thanks:)
I think this lg to me now. If I understand correctly, there is no behavior change added by this CL. Can you revise the CL description to explain the new patch? Also, horo@ should have another look because the patch changed substantially since his review. https://codereview.chromium.org/2108573002/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_url_request_job.cc (right): https://codereview.chromium.org/2108573002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_url_request_job.cc:267: DCHECK(fetch_type_ != ServiceWorkerFetchType::FOREIGN_FETCH); DCHECK_NE https://codereview.chromium.org/2108573002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_url_request_job.cc:898: // we returns a fall_back_required response to the renderer. return https://codereview.chromium.org/2108573002/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_url_request_job.h (right): https://codereview.chromium.org/2108573002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_url_request_job.h:106: // When an in-flight request possibly need CORS check, use needs
Description was changed from ========== ServiceWorker: Send back the response when fallbacking CORS request Currently ServiceWorkerURLRequestJob::ForwardToNetwork invokes restart of the request and the request goes to the network directly though DocumentThreadableLoader intended to ask to the service worker. This causes an bug when it's CORS request and failing SW launching by some reason because DocumentThreadableLoader doesn't know the request directly goes to the network in the case when CORS preflight is needed. BUG=621788,605844 TEST=./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Debug http/tests/fetch/serviceworker/thorough/cors-preflight-nofetch.html ========== to ========== ServiceWorker: Send back the response when fallbacking CORS request Currently ServiceWorkerURLRequestJob::ForwardToNetwork invokes restart of the request and the request goes to the network directly. In subresource case, this causes an bug when the request needs CORS preflight. This patch adds an API to fallback with CORS check for subresources; this is used at a subsequent patch: aka no-fetch optimization http://crrev.com/2103063002. BUG=621788,605844 ==========
On 2016/07/07 05:15:35, falken wrote: > I think this lg to me now. If I understand correctly, there is no behavior > change added by this CL. Can you revise the CL description to explain the new > patch? Also, horo@ should have another look because the patch changed > substantially since his review. > > https://codereview.chromium.org/2108573002/diff/100001/content/browser/servic... > File content/browser/service_worker/service_worker_url_request_job.cc (right): > > https://codereview.chromium.org/2108573002/diff/100001/content/browser/servic... > content/browser/service_worker/service_worker_url_request_job.cc:267: > DCHECK(fetch_type_ != ServiceWorkerFetchType::FOREIGN_FETCH); > DCHECK_NE > > https://codereview.chromium.org/2108573002/diff/100001/content/browser/servic... > content/browser/service_worker/service_worker_url_request_job.cc:898: // we > returns a fall_back_required response to the renderer. > return > > https://codereview.chromium.org/2108573002/diff/100001/content/browser/servic... > File content/browser/service_worker/service_worker_url_request_job.h (right): > > https://codereview.chromium.org/2108573002/diff/100001/content/browser/servic... > content/browser/service_worker/service_worker_url_request_job.h:106: // When an > in-flight request possibly need CORS check, use > needs Thanks, Updated the description. horo@-san, PTAL again:)
still lgtm thanks
The CQ bit was checked by shimazu@google.com
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2116373002 Patch 1). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
lgtm but first line of CL description seems wrong. This patch is basically adding an API and refactoring.
Description was changed from ========== ServiceWorker: Send back the response when fallbacking CORS request Currently ServiceWorkerURLRequestJob::ForwardToNetwork invokes restart of the request and the request goes to the network directly. In subresource case, this causes an bug when the request needs CORS preflight. This patch adds an API to fallback with CORS check for subresources; this is used at a subsequent patch: aka no-fetch optimization http://crrev.com/2103063002. BUG=621788,605844 ========== to ========== ServiceWorker: Add an API to fallback to renderer for CORS preflight Currently ServiceWorkerURLRequestJob::ForwardToNetwork invokes restart of the request and the request goes to the network directly. In subresource case, this causes an bug when the request needs CORS preflight. This patch adds an API to fallback with CORS check for subresources; this is used at a subsequent patch: aka no-fetch optimization http://crrev.com/2103063002. BUG=621788,605844 ==========
shimazu@chromium.org changed reviewers: - shimazu@google.com
Thanks, updated the first line. https://codereview.chromium.org/2108573002/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_url_request_job.cc (right): https://codereview.chromium.org/2108573002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_url_request_job.cc:267: DCHECK(fetch_type_ != ServiceWorkerFetchType::FOREIGN_FETCH); On 2016/07/07 05:15:34, falken wrote: > DCHECK_NE Done. https://codereview.chromium.org/2108573002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_url_request_job.cc:898: // we returns a fall_back_required response to the renderer. On 2016/07/07 05:15:34, falken wrote: > return Done. https://codereview.chromium.org/2108573002/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_url_request_job.h (right): https://codereview.chromium.org/2108573002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_url_request_job.h:106: // When an in-flight request possibly need CORS check, use On 2016/07/07 05:15:34, falken wrote: > needs Done.
Description was changed from ========== ServiceWorker: Add an API to fallback to renderer for CORS preflight Currently ServiceWorkerURLRequestJob::ForwardToNetwork invokes restart of the request and the request goes to the network directly. In subresource case, this causes an bug when the request needs CORS preflight. This patch adds an API to fallback with CORS check for subresources; this is used at a subsequent patch: aka no-fetch optimization http://crrev.com/2103063002. BUG=621788,605844 ========== to ========== ServiceWorker: Add an API to fallback to renderer for CORS preflight Currently ServiceWorkerURLRequestJob::ForwardToNetwork invokes restart of the request and the request goes to the network directly. In subresource case, this causes an bug when the request needs CORS preflight. This patch adds an API to fallback with CORS check for subresources; this is used at a subsequent patch: aka no-fetch optimization http://crrev.com/2103063002. BUG=621788,605844 NO_DEPENDENCY_CHECKS=true ==========
The CQ bit was checked by shimazu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, horo@chromium.org Link to the patchset: https://codereview.chromium.org/2108573002/#ps140001 (title: "Fix grammatical problems on the comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== ServiceWorker: Add an API to fallback to renderer for CORS preflight Currently ServiceWorkerURLRequestJob::ForwardToNetwork invokes restart of the request and the request goes to the network directly. In subresource case, this causes an bug when the request needs CORS preflight. This patch adds an API to fallback with CORS check for subresources; this is used at a subsequent patch: aka no-fetch optimization http://crrev.com/2103063002. BUG=621788,605844 NO_DEPENDENCY_CHECKS=true ========== to ========== ServiceWorker: Add an API to fallback to renderer for CORS preflight Currently ServiceWorkerURLRequestJob::ForwardToNetwork invokes restart of the request and the request goes to the network directly. In subresource case, this causes an bug when the request needs CORS preflight. This patch adds an API to fallback with CORS check for subresources; this is used at a subsequent patch: aka no-fetch optimization http://crrev.com/2103063002. BUG=621788,605844 ==========
The CQ bit was checked by shimazu@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, horo@chromium.org Link to the patchset: https://codereview.chromium.org/2108573002/#ps160001 (title: "Remove dependency")
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.
Description was changed from ========== ServiceWorker: Add an API to fallback to renderer for CORS preflight Currently ServiceWorkerURLRequestJob::ForwardToNetwork invokes restart of the request and the request goes to the network directly. In subresource case, this causes an bug when the request needs CORS preflight. This patch adds an API to fallback with CORS check for subresources; this is used at a subsequent patch: aka no-fetch optimization http://crrev.com/2103063002. BUG=621788,605844 ========== to ========== ServiceWorker: Add an API to fallback to renderer for CORS preflight Currently ServiceWorkerURLRequestJob::ForwardToNetwork invokes restart of the request and the request goes to the network directly. In subresource case, this causes an bug when the request needs CORS preflight. This patch adds an API to fallback with CORS check for subresources; this is used at a subsequent patch: aka no-fetch optimization http://crrev.com/2103063002. BUG=621788,605844 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== ServiceWorker: Add an API to fallback to renderer for CORS preflight Currently ServiceWorkerURLRequestJob::ForwardToNetwork invokes restart of the request and the request goes to the network directly. In subresource case, this causes an bug when the request needs CORS preflight. This patch adds an API to fallback with CORS check for subresources; this is used at a subsequent patch: aka no-fetch optimization http://crrev.com/2103063002. BUG=621788,605844 ========== to ========== ServiceWorker: Add an API to fallback to renderer for CORS preflight Currently ServiceWorkerURLRequestJob::ForwardToNetwork invokes restart of the request and the request goes to the network directly. In subresource case, this causes an bug when the request needs CORS preflight. This patch adds an API to fallback with CORS check for subresources; this is used at a subsequent patch: aka no-fetch optimization http://crrev.com/2103063002. BUG=621788,605844 Committed: https://crrev.com/e14963a7be1f6445401efb43619301bf9fe3157d Cr-Commit-Position: refs/heads/master@{#404114} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/e14963a7be1f6445401efb43619301bf9fe3157d Cr-Commit-Position: refs/heads/master@{#404114} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
