Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(143)

Issue 2941883003: [ServiceWorker] Fetch event should return integrity value (Closed)

Created:
3 years, 6 months ago by xiaofengzhang
Modified:
3 years, 5 months ago
CC:
chromium-reviews, kenjibaheux+watch_chromium.org, qsr+mojo_chromium.org, tzik, kouhei+script_chromium.org, eae+blinkwatch, yzshen+watch_chromium.org, kinuko+watch, rwlbuis, jsbell+serviceworker_chromium.org, blink-reviews-html_chromium.org, jam, abarth-chromium, blink-reviews-dom_chromium.org, dglazkov+blink, darin-cc_chromium.org, gavinp+loader_chromium.org, blink-reviews, falken+watch_chromium.org, loading-reviews_chromium.org, blink-reviews-api_chromium.org, blink-worker-reviews_chromium.org, kochi+script_chromium.org, mlamouri+watch-content_chromium.org, blink-reviews-w3ctests_chromium.org, blink-reviews-style_chromium.org, sof, viettrungluu+watch_chromium.org, nhiroki, hiroshige+script_chromium.org, rdsmith+watch_chromium.org, haraken, loading-reviews+fetch_chromium.org, Nate Chapin, michaeln, shimazu+serviceworker_chromium.org, tyoshino+watch_chromium.org, serviceworker-reviews, Aaron Boodman, kinuko+serviceworker, horo+watch_chromium.org, darin (slow to review), mmenke
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[ServiceWorker] FetchEvent should returns integrity attribute This patch is to pass the web-platform-test:fetch-request-resources.https.html. Current failed cases(script_integrity_test and css_integrity_test) are caused by integrity attribute data will not be returned in FetchEvent. BUG=718935, 678905 Review-Url: https://codereview.chromium.org/2941883003 Cr-Commit-Position: refs/heads/master@{#487710} Committed: https://chromium.googlesource.com/chromium/src/+/3f0fe92c5ed031281aa70e8598e03b538b73585a

Patch Set 1 #

Patch Set 2 : Rebase #

Total comments: 23

Patch Set 3 : Rebase and address shimazu and yhirano's comments #

Total comments: 9

Patch Set 4 : Address yhirano and shimazu's comments #

Patch Set 5 : Rebase #

Total comments: 24

Patch Set 6 : Address falken's comments #65 #

Total comments: 4

Patch Set 7 : Address yhirano's comment #78 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+136 lines, -283 lines) Patch
M content/browser/loader/resource_dispatcher_host_impl.cc View 1 2 3 4 5 1 chunk +6 lines, -6 lines 0 comments Download
M content/browser/service_worker/foreign_fetch_request_handler.h View 1 2 3 4 3 chunks +3 lines, -0 lines 0 comments Download
M content/browser/service_worker/foreign_fetch_request_handler.cc View 1 2 3 4 5 chunks +6 lines, -3 lines 0 comments Download
M content/browser/service_worker/foreign_fetch_request_handler_unittest.cc View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M content/browser/service_worker/link_header_support_unittest.cc View 1 2 3 4 5 1 chunk +3 lines, -3 lines 0 comments Download
M content/browser/service_worker/service_worker_context_request_handler_unittest.cc View 1 2 3 4 5 2 chunks +6 lines, -4 lines 0 comments Download
M content/browser/service_worker/service_worker_controllee_request_handler.h View 1 2 3 4 2 chunks +2 lines, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_controllee_request_handler.cc View 1 2 3 4 5 3 chunks +3 lines, -1 line 0 comments Download
M content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_provider_host.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_provider_host.cc View 1 2 3 4 5 2 chunks +3 lines, -2 lines 0 comments Download
M content/browser/service_worker/service_worker_request_handler.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_request_handler.cc View 1 2 3 4 5 4 chunks +10 lines, -9 lines 0 comments Download
M content/browser/service_worker/service_worker_request_handler_unittest.cc View 1 2 3 4 5 1 chunk +3 lines, -3 lines 0 comments Download
M content/browser/service_worker/service_worker_url_request_job.h View 1 2 3 4 2 chunks +2 lines, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_url_request_job.cc View 1 2 3 4 3 chunks +3 lines, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_url_request_job_unittest.cc View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc View 1 2 3 4 5 1 chunk +3 lines, -2 lines 0 comments Download
M content/child/web_url_loader_impl.cc View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M content/child/web_url_request_util.h View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M content/child/web_url_request_util.cc View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M content/common/resource_messages.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M content/common/service_worker/service_worker_fetch_request_struct_traits.h View 1 chunk +5 lines, -0 lines 0 comments Download
M content/common/service_worker/service_worker_fetch_request_struct_traits.cc View 1 chunk +1 line, -0 lines 0 comments Download
M content/common/service_worker/service_worker_messages.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M content/common/service_worker/service_worker_types.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/resource_request.h View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M content/renderer/service_worker/service_worker_context_client.cc View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
D third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-resources.https-expected.txt View 1 chunk +0 lines, -4 lines 0 comments Download
D third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.fetch-request-resources.html View 1 chunk +0 lines, -241 lines 0 comments Download
M third_party/WebKit/Source/core/dom/ScriptLoader.cpp View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/LinkStyle.cpp View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/fetch/FetchRequestData.cpp View 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/fetch/Request.cpp View 1 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/exported/WebServiceWorkerRequest.cpp View 3 chunks +9 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/exported/WebURLRequest.cpp View 1 chunk +8 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.h View 1 2 3 4 3 chunks +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.cpp View 1 2 3 4 5 6 2 chunks +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/mojo/FetchAPIRequestStructTraits.h View 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/mojo/FetchAPIRequestStructTraits.cpp View 4 chunks +11 lines, -1 line 0 comments Download
M third_party/WebKit/public/platform/WebURLRequest.h View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/public/platform/modules/fetch/fetch_api_request.mojom View 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerRequest.h View 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 107 (68 generated)
xiaofengzhang
Hi shimazu, Could you help to review? Even many files are changed, the code is ...
3 years, 6 months ago (2017-06-19 03:17:05 UTC) #22
shimazu
Thanks for working on this! Is this patch only for delivering the integrity param to ...
3 years, 6 months ago (2017-06-19 10:10:28 UTC) #23
xiaofengzhang
Yes, this patch is only for delivering the integrity param. The integrity feature was already ...
3 years, 6 months ago (2017-06-20 02:09:25 UTC) #24
shimazu
Thanks for your explanation! I could probably understand. Your patch modifies two paths: 1. Page ...
3 years, 6 months ago (2017-06-20 05:58:24 UTC) #25
xiaofengzhang
Yeah, agree, I also think we should know the spec accurately before next step. Actually ...
3 years, 6 months ago (2017-06-20 06:07:27 UTC) #26
xiaofengzhang
Hi shimazu, WDYT the info in github by @wanderview? "The FetchEvent just wraps the internal ...
3 years, 6 months ago (2017-06-23 05:51:59 UTC) #27
shimazu
Thanks for working on this! My comment is based on a design like - adding ...
3 years, 6 months ago (2017-06-26 06:13:33 UTC) #28
shimazu
+yhirano for loading side
3 years, 5 months ago (2017-06-27 11:08:06 UTC) #30
xiaofengzhang
Thanks shimazu, I will improve the patch based on your comments.
3 years, 5 months ago (2017-06-28 01:54:03 UTC) #31
yhirano
https://codereview.chromium.org/2941883003/diff/80001/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h File third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h (right): https://codereview.chromium.org/2941883003/diff/80001/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h#newcode93 third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h:93: const String& IntegrityValue() const { return integrity_; } Please ...
3 years, 5 months ago (2017-06-28 04:55:09 UTC) #32
xiaofengzhang
Hi Shimazu PTAL. For you unittests or browsertests: Sorry, I didn't find a good way ...
3 years, 5 months ago (2017-06-29 02:26:06 UTC) #34
xiaofengzhang
The test may be added into ServiceWorkerVersionBrowserTest of service_worker_browsertest.cc. but I am thinking if it's ...
3 years, 5 months ago (2017-06-29 04:43:58 UTC) #37
yhirano
https://codereview.chromium.org/2941883003/diff/100001/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h File third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h (right): https://codereview.chromium.org/2941883003/diff/100001/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h#newcode158 third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h:158: void SetIntegrityValue(const String& integrity) { Please remove this and ...
3 years, 5 months ago (2017-06-30 09:11:58 UTC) #40
xiaofengzhang
On 2017/06/30 09:11:58, yhirano wrote: > https://codereview.chromium.org/2941883003/diff/100001/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h > File third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h (right): > > https://codereview.chromium.org/2941883003/diff/100001/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h#newcode158 > ...
3 years, 5 months ago (2017-07-05 02:10:13 UTC) #41
shimazu
Sorry for delay! I think it overall looks good after you address yhirano@'s comments. https://codereview.chromium.org/2941883003/diff/100001/content/browser/service_worker/service_worker_request_handler.cc ...
3 years, 5 months ago (2017-07-06 07:47:06 UTC) #42
xiaofengzhang
Thanks, shimazu. https://codereview.chromium.org/2941883003/diff/100001/content/browser/service_worker/service_worker_request_handler.cc File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/2941883003/diff/100001/content/browser/service_worker/service_worker_request_handler.cc#newcode103 content/browser/service_worker/service_worker_request_handler.cc:103: FetchRedirectMode::MANUAL_MODE, "" /*integrity*/, resource_type, On 2017/07/06 07:47:06, ...
3 years, 5 months ago (2017-07-06 08:00:25 UTC) #43
shimazu
https://codereview.chromium.org/2941883003/diff/100001/content/browser/service_worker/service_worker_request_handler.cc File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/2941883003/diff/100001/content/browser/service_worker/service_worker_request_handler.cc#newcode103 content/browser/service_worker/service_worker_request_handler.cc:103: FetchRedirectMode::MANUAL_MODE, "" /*integrity*/, resource_type, On 2017/07/06 08:00:25, xiaofengzhang wrote: ...
3 years, 5 months ago (2017-07-06 08:14:23 UTC) #44
xiaofengzhang
Hi shimazu, yhirano PTAL, thanks a lot! :-) https://codereview.chromium.org/2941883003/diff/100001/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h File third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h (right): https://codereview.chromium.org/2941883003/diff/100001/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h#newcode158 third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h:158: void ...
3 years, 5 months ago (2017-07-07 00:10:14 UTC) #63
falken
DBC. yhirano: can you check my question at the end? https://codereview.chromium.org/2941883003/diff/180001/content/browser/service_worker/foreign_fetch_request_handler_unittest.cc File content/browser/service_worker/foreign_fetch_request_handler_unittest.cc (right): https://codereview.chromium.org/2941883003/diff/180001/content/browser/service_worker/foreign_fetch_request_handler_unittest.cc#newcode152 ...
3 years, 5 months ago (2017-07-07 00:56:11 UTC) #65
xiaofengzhang
On 2017/07/07 00:56:11, falken wrote: > DBC. yhirano: can you check my question at the ...
3 years, 5 months ago (2017-07-12 06:05:53 UTC) #66
yhirano
https://codereview.chromium.org/2941883003/diff/180001/content/child/web_url_request_util.cc File content/child/web_url_request_util.cc (right): https://codereview.chromium.org/2941883003/diff/180001/content/child/web_url_request_util.cc#newcode386 content/child/web_url_request_util.cc:386: return request.GetFetchIntegrity().Utf8(); On 2017/07/07 00:56:11, falken wrote: > yhirano: ...
3 years, 5 months ago (2017-07-12 06:14:15 UTC) #67
xiaofengzhang
Hi shimazu, yhirano, falken PTAL, thanks a lot! https://codereview.chromium.org/2941883003/diff/180001/content/browser/service_worker/foreign_fetch_request_handler_unittest.cc File content/browser/service_worker/foreign_fetch_request_handler_unittest.cc (right): https://codereview.chromium.org/2941883003/diff/180001/content/browser/service_worker/foreign_fetch_request_handler_unittest.cc#newcode152 content/browser/service_worker/foreign_fetch_request_handler_unittest.cc:152: "" ...
3 years, 5 months ago (2017-07-12 09:13:53 UTC) #77
yhirano
https://codereview.chromium.org/2941883003/diff/220001/third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.cpp File third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.cpp (right): https://codereview.chromium.org/2941883003/diff/220001/third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.cpp#newcode117 third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.cpp:117: SetFetchIntegrity(data->fetch_integrity_); .IsolatedCopy() https://codereview.chromium.org/2941883003/diff/220001/third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.cpp#newcode168 third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.cpp:168: data->fetch_integrity_ = fetch_integrity_; .IsolatedCopy()
3 years, 5 months ago (2017-07-12 10:18:37 UTC) #78
xiaofengzhang
Hi yhirano, shimazu PTAL, thanks a lot. https://codereview.chromium.org/2941883003/diff/220001/third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.cpp File third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.cpp (right): https://codereview.chromium.org/2941883003/diff/220001/third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.cpp#newcode117 third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.cpp:117: SetFetchIntegrity(data->fetch_integrity_); On ...
3 years, 5 months ago (2017-07-14 01:17:35 UTC) #83
shimazu
lgtm
3 years, 5 months ago (2017-07-14 04:48:57 UTC) #84
xiaofengzhang
@yhirano Do you have more comments? thanks :-)
3 years, 5 months ago (2017-07-18 01:51:58 UTC) #85
yhirano
lgtm
3 years, 5 months ago (2017-07-18 01:59:03 UTC) #86
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2941883003/240001
3 years, 5 months ago (2017-07-18 01:59:53 UTC) #88
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/492052)
3 years, 5 months ago (2017-07-18 02:13:30 UTC) #90
xiaofengzhang
Hi Tom, kinuko, could you help to review as the owner of some files?
3 years, 5 months ago (2017-07-18 02:42:59 UTC) #92
falken
On 2017/07/18 02:42:59, xiaofengzhang wrote: > Hi Tom, kinuko, > could you help to review ...
3 years, 5 months ago (2017-07-18 04:32:38 UTC) #93
xiaofengzhang
On 2017/07/18 04:32:38, falken wrote: > On 2017/07/18 02:42:59, xiaofengzhang wrote: > > Hi Tom, ...
3 years, 5 months ago (2017-07-18 04:34:27 UTC) #94
kinuko
lgtm lgtm You'll still need security review by Tom for mojom/traits files.
3 years, 5 months ago (2017-07-18 04:36:16 UTC) #96
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2941883003/240001
3 years, 5 months ago (2017-07-18 04:36:27 UTC) #97
kinuko
On 2017/07/18 04:36:27, commit-bot: I haz the power wrote: > CQ is trying da patch. ...
3 years, 5 months ago (2017-07-18 04:37:48 UTC) #99
haraken
WebKit LGTM
3 years, 5 months ago (2017-07-18 05:04:55 UTC) #101
Tom Sepez
lgtm
3 years, 5 months ago (2017-07-18 22:23:06 UTC) #102
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2941883003/240001
3 years, 5 months ago (2017-07-19 01:17:13 UTC) #104
commit-bot: I haz the power
3 years, 5 months ago (2017-07-19 01:27:22 UTC) #107
Message was sent while issue was closed.
Committed patchset #7 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/3f0fe92c5ed031281aa70e8598e0...

Powered by Google App Engine
This is Rietveld 408576698