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

Issue 25008006: Flush out initial [un]registerServiceWorker roundtrip (Closed)

Created:
7 years, 2 months ago by alecflett
Modified:
7 years, 1 month ago
Reviewers:
michaeln, kinuko, jam, Tom Sepez
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam
Visibility:
Public.

Description

Flush out initial [un]registerServiceWorker roundtrip This is a first cut at getting registerServiceWorker() to make a full round trip from and back into blink. At the moment the browser process just sends a dummy value back to the renderer over IPC. BUG=285976 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231688

Patch Set 1 #

Total comments: 24

Patch Set 2 : Fix thread access #

Patch Set 3 : Clean up view/frame access #

Patch Set 4 : Fix workerid #

Total comments: 21

Patch Set 5 : Update to reflect new Blink API #

Patch Set 6 : Update to ToT #

Total comments: 4

Patch Set 7 : Now with security checks #

Patch Set 8 : Additional comments for security #

Patch Set 9 : Untangle message_filters #

Total comments: 4

Patch Set 10 : Fix request_id leak #

Total comments: 17

Patch Set 11 : Fix naming, other small nits #

Total comments: 14

Patch Set 12 : Fix nits #

Patch Set 13 : Update to ToT #

Patch Set 14 : update to ToT #

Total comments: 12

Patch Set 15 : Use int64 for workers, remove policy check #

Patch Set 16 : Use int64 for workers, remove policy check #

Patch Set 17 : Use int64 for workers, remove policy check #

Total comments: 12

Patch Set 18 : Address review comments #

Total comments: 26

Patch Set 19 : Address review comments #

Total comments: 12

Patch Set 20 : Address jam's nits #

Patch Set 21 : Address jam's nits attempt 2 #

Patch Set 22 : allow_unused #

Patch Set 23 : Fix ALLOW_UNUSED again #

Patch Set 24 : Fix ALLOW_UNUSED again #

Patch Set 25 : Remove render_process_id_ for now #

Unified diffs Side-by-side diffs Delta from patch set Stats (+534 lines, -30 lines) Patch
M content/browser/renderer_host/render_process_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -1 line 0 comments Download
M content/browser/service_worker/service_worker_dispatcher_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 2 chunks +9 lines, -7 lines 0 comments Download
M content/browser/service_worker/service_worker_dispatcher_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 2 chunks +32 lines, -11 lines 0 comments Download
M content/child/child_thread.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 5 chunks +16 lines, -0 lines 0 comments Download
M content/child/child_thread.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 5 chunks +11 lines, -0 lines 0 comments Download
A + content/child/service_worker/OWNERS View 0 chunks +-1 lines, --1 lines 0 comments Download
A content/child/service_worker/service_worker_dispatcher.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +76 lines, -0 lines 0 comments Download
A content/child/service_worker/service_worker_dispatcher.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +131 lines, -0 lines 0 comments Download
A content/child/service_worker/service_worker_message_filter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +40 lines, -0 lines 0 comments Download
A content/child/service_worker/service_worker_message_filter.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +46 lines, -0 lines 0 comments Download
A content/child/service_worker/web_service_worker_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +29 lines, -0 lines 0 comments Download
A + content/child/service_worker/web_service_worker_impl.cc View 1 2 3 4 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -2 lines 0 comments Download
A content/child/service_worker/web_service_worker_provider_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +55 lines, -0 lines 0 comments Download
A content/child/service_worker/web_service_worker_provider_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +46 lines, -0 lines 0 comments Download
M content/common/service_worker_messages.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +15 lines, -10 lines 0 comments Download
M content/content_child.gypi View 1 2 3 4 1 chunk +8 lines, -0 lines 0 comments Download
M content/renderer/render_frame_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +3 lines, -0 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 4 chunks +15 lines, -0 lines 0 comments Download

Messages

Total messages: 55 (0 generated)
alecflett
This CL isn't 100% ready to land, I need feedback on a few things. 1) ...
7 years, 2 months ago (2013-09-28 00:04:12 UTC) #1
kinuko
Thanks for sending this, I'm still on my way catching up but let me start ...
7 years, 2 months ago (2013-09-30 12:41:16 UTC) #2
michaeln
https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/service_worker_dispatcher.cc File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/service_worker_dispatcher.cc#newcode122 content/child/service_worker/service_worker_dispatcher.cc:122: callbacks->onSuccess(worker); Why is a Worker passed as a result ...
7 years, 2 months ago (2013-09-30 23:41:27 UTC) #3
alecflett
Thanks for the feedback - I'll cleanup the leak and go over the threading stuff ...
7 years, 2 months ago (2013-10-01 00:17:03 UTC) #4
kinuko
https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/service_worker_message_filter.h File content/child/service_worker/service_worker_message_filter.h (right): https://codereview.chromium.org/25008006/diff/1/content/child/service_worker/service_worker_message_filter.h#newcode33 content/child/service_worker/service_worker_message_filter.h:33: RequestIdToThreadId request_id_map_; On 2013/09/30 23:41:27, michaeln wrote: > There's ...
7 years, 2 months ago (2013-10-01 05:28:09 UTC) #5
kinuko
https://codereview.chromium.org/25008006/diff/1/content/child/child_thread.cc File content/child/child_thread.cc (right): https://codereview.chromium.org/25008006/diff/1/content/child/child_thread.cc#newcode7 content/child/child_thread.cc:7: #include <string> On 2013/10/01 00:17:04, alecflett wrote: > On ...
7 years, 2 months ago (2013-10-01 07:48:11 UTC) #6
alecflett
So I looked into some of the issues, and they're more closely tied than I ...
7 years, 2 months ago (2013-10-02 18:59:55 UTC) #7
kinuko
On 2013/10/02 18:59:55, alecflett wrote: > So I looked into some of the issues, and ...
7 years, 2 months ago (2013-10-03 01:42:59 UTC) #8
alecflett
On 2013/10/03 01:42:59, kinuko wrote: > On 2013/10/02 18:59:55, alecflett wrote: > > So I ...
7 years, 2 months ago (2013-10-03 21:57:28 UTC) #9
alecflett
New patch updated... mind taking a look?
7 years, 2 months ago (2013-10-03 23:24:47 UTC) #10
michaeln
https://codereview.chromium.org/25008006/diff/23001/content/browser/service_worker/service_worker_dispatcher_host.cc File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/25008006/diff/23001/content/browser/service_worker/service_worker_dispatcher_host.cc#newcode40 content/browser/service_worker/service_worker_dispatcher_host.cc:40: static int32 NextWorkerId() { i think we usually prefer ...
7 years, 2 months ago (2013-10-04 01:26:07 UTC) #11
michaeln
https://codereview.chromium.org/25008006/diff/23001/content/child/service_worker/service_worker_dispatcher.cc File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/25008006/diff/23001/content/child/service_worker/service_worker_dispatcher.cc#newcode73 content/child/service_worker/service_worker_dispatcher.cc:73: message_filter_->RegisterRequestID(request_id, CurrentWorkerId()); On 2013/10/04 01:26:07, michaeln wrote: > Looks ...
7 years, 2 months ago (2013-10-04 01:51:52 UTC) #12
kinuko
https://codereview.chromium.org/25008006/diff/23001/content/child/service_worker/service_worker_dispatcher.cc File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/25008006/diff/23001/content/child/service_worker/service_worker_dispatcher.cc#newcode73 content/child/service_worker/service_worker_dispatcher.cc:73: message_filter_->RegisterRequestID(request_id, CurrentWorkerId()); On 2013/10/04 01:51:52, michaeln wrote: > On ...
7 years, 2 months ago (2013-10-04 14:55:47 UTC) #13
michaeln
https://codereview.chromium.org/25008006/diff/23001/content/content_child.gypi File content/content_child.gypi (right): https://codereview.chromium.org/25008006/diff/23001/content/content_child.gypi#newcode74 content/content_child.gypi:74: 'child/service_worker/web_service_worker_registry_proxy.h', On 2013/10/04 14:55:48, kinuko wrote: > I think ...
7 years, 2 months ago (2013-10-04 20:36:45 UTC) #14
alecflett
ok, I've updated this patch to reflect the review comments, and also to reflect the ...
7 years, 2 months ago (2013-10-07 18:33:58 UTC) #15
michaeln
we need to wait on the blink cl for this one.... https://codereview.chromium.org/25008006/diff/41001/content/child/service_worker/service_worker_dispatcher.h File content/child/service_worker/service_worker_dispatcher.h (right): ...
7 years, 2 months ago (2013-10-07 23:17:33 UTC) #16
alecflett
Ok, I've updated the blink side to include a WebURL origin with each request, and ...
7 years, 2 months ago (2013-10-07 23:36:51 UTC) #17
kinuko
(Some lighthearted comments while waiting for the other patch) https://codereview.chromium.org/25008006/diff/52001/content/child/service_worker/service_worker_dispatcher.cc File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/25008006/diff/52001/content/child/service_worker/service_worker_dispatcher.cc#newcode109 content/child/service_worker/service_worker_dispatcher.cc:109: ...
7 years, 2 months ago (2013-10-08 02:30:19 UTC) #18
alecflett
https://codereview.chromium.org/25008006/diff/52001/content/child/service_worker/service_worker_dispatcher.cc File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/25008006/diff/52001/content/child/service_worker/service_worker_dispatcher.cc#newcode109 content/child/service_worker/service_worker_dispatcher.cc:109: new WebServiceWorkerImpl(service_worker_id, true); On 2013/10/08 02:30:19, kinuko wrote: > ...
7 years, 2 months ago (2013-10-08 22:17:58 UTC) #19
michaeln
https://codereview.chromium.org/25008006/diff/57001/content/child/service_worker/service_worker_dispatcher.h File content/child/service_worker/service_worker_dispatcher.h (right): https://codereview.chromium.org/25008006/diff/57001/content/child/service_worker/service_worker_dispatcher.h#newcode35 content/child/service_worker/service_worker_dispatcher.h:35: virtual void OnWorkerRunLoopStopped() OVERRIDE; nit: could this method be ...
7 years, 2 months ago (2013-10-08 23:08:38 UTC) #20
alecflett
ok, addressed these as follows: https://codereview.chromium.org/25008006/diff/57001/content/child/service_worker/service_worker_dispatcher.h File content/child/service_worker/service_worker_dispatcher.h (right): https://codereview.chromium.org/25008006/diff/57001/content/child/service_worker/service_worker_dispatcher.h#newcode35 content/child/service_worker/service_worker_dispatcher.h:35: virtual void OnWorkerRunLoopStopped() OVERRIDE; ...
7 years, 2 months ago (2013-10-09 01:23:47 UTC) #21
kinuko
https://codereview.chromium.org/25008006/diff/57001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/25008006/diff/57001/content/renderer/render_frame_impl.cc#newcode233 content/renderer/render_frame_impl.cc:233: GURL(frame->document().securityOrigin().toString()), On 2013/10/09 01:23:47, alecflett wrote: > On 2013/10/08 ...
7 years, 2 months ago (2013-10-09 02:23:12 UTC) #22
kinuko
(nits only) https://codereview.chromium.org/25008006/diff/63001/content/browser/service_worker/service_worker_dispatcher_host.h File content/browser/service_worker/service_worker_dispatcher_host.h (right): https://codereview.chromium.org/25008006/diff/63001/content/browser/service_worker/service_worker_dispatcher_host.h#newcode32 content/browser/service_worker/service_worker_dispatcher_host.h:32: int32 registry_id, nit: request_id https://codereview.chromium.org/25008006/diff/63001/content/browser/service_worker/service_worker_dispatcher_host.h#newcode37 content/browser/service_worker/service_worker_dispatcher_host.h:37: int32 ...
7 years, 2 months ago (2013-10-09 12:13:25 UTC) #23
alecflett
all outstanding comments addressed. https://codereview.chromium.org/25008006/diff/63001/content/browser/service_worker/service_worker_dispatcher_host.h File content/browser/service_worker/service_worker_dispatcher_host.h (right): https://codereview.chromium.org/25008006/diff/63001/content/browser/service_worker/service_worker_dispatcher_host.h#newcode32 content/browser/service_worker/service_worker_dispatcher_host.h:32: int32 registry_id, On 2013/10/09 12:13:25, ...
7 years, 2 months ago (2013-10-09 18:54:29 UTC) #24
alecflett
Here's what I was thinking for the error messages: https://codereview.chromium.org/26631006/
7 years, 2 months ago (2013-10-09 23:39:02 UTC) #25
michaeln
https://codereview.chromium.org/25008006/diff/79001/content/browser/service_worker/service_worker_dispatcher_host.cc File content/browser/service_worker/service_worker_dispatcher_host.cc (left): https://codereview.chromium.org/25008006/diff/79001/content/browser/service_worker/service_worker_dispatcher_host.cc#oldcode42 content/browser/service_worker/service_worker_dispatcher_host.cc:42: // TODO(alecflett): Enforce that script_url must have the same ...
7 years, 2 months ago (2013-10-16 02:03:42 UTC) #26
alecflett
ok, issues addressed. PTAL? https://codereview.chromium.org/25008006/diff/79001/content/browser/service_worker/service_worker_dispatcher_host.cc File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/25008006/diff/79001/content/browser/service_worker/service_worker_dispatcher_host.cc#newcode42 content/browser/service_worker/service_worker_dispatcher_host.cc:42: static int32 NextWorkerId() { On ...
7 years, 2 months ago (2013-10-23 20:43:17 UTC) #27
kinuko
It looks the uploaded patches are screwed up. Can you try uploading them again?
7 years, 2 months ago (2013-10-24 03:35:36 UTC) #28
alecflett
On 2013/10/24 03:35:36, kinuko wrote: > It looks the uploaded patches are screwed up. Can ...
7 years, 2 months ago (2013-10-24 18:24:50 UTC) #29
alecflett
On 2013/10/24 18:24:50, alecflett wrote: > On 2013/10/24 03:35:36, kinuko wrote: > > It looks ...
7 years, 2 months ago (2013-10-24 18:40:19 UTC) #30
michaeln
https://codereview.chromium.org/25008006/diff/248001/content/browser/service_worker/service_worker_dispatcher_host.cc File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/25008006/diff/248001/content/browser/service_worker/service_worker_dispatcher_host.cc#newcode52 content/browser/service_worker/service_worker_dispatcher_host.cc:52: // registration permission. stray comment is n/a anymore, at ...
7 years, 2 months ago (2013-10-24 21:57:37 UTC) #31
alecflett
ok, comments addressed https://codereview.chromium.org/25008006/diff/248001/content/browser/service_worker/service_worker_dispatcher_host.cc File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/25008006/diff/248001/content/browser/service_worker/service_worker_dispatcher_host.cc#newcode52 content/browser/service_worker/service_worker_dispatcher_host.cc:52: // registration permission. On 2013/10/24 21:57:39, ...
7 years, 2 months ago (2013-10-24 22:41:21 UTC) #32
michaeln
lgtm pending greenbots and the small ctor change https://codereview.chromium.org/25008006/diff/298002/content/child/service_worker/web_service_worker_impl.h File content/child/service_worker/web_service_worker_impl.h (right): https://codereview.chromium.org/25008006/diff/298002/content/child/service_worker/web_service_worker_impl.h#newcode17 content/child/service_worker/web_service_worker_impl.h:17: WebServiceWorkerImpl(int64 ...
7 years, 2 months ago (2013-10-24 23:03:58 UTC) #33
alecflett
darin@ - this patch spreads across a large number of directories but most of what ...
7 years, 2 months ago (2013-10-24 23:11:07 UTC) #34
alecflett
tsepez@ - mind reviewing our messages stuff? You can also take a look at the ...
7 years, 2 months ago (2013-10-24 23:12:27 UTC) #35
Tom Sepez
LGTM after addressing the comment. Please don't remove your flag until the CPSP work is ...
7 years, 2 months ago (2013-10-24 23:25:51 UTC) #36
kinuko
lgtm + nits https://codereview.chromium.org/25008006/diff/298002/content/browser/service_worker/service_worker_dispatcher_host.h File content/browser/service_worker/service_worker_dispatcher_host.h (right): https://codereview.chromium.org/25008006/diff/298002/content/browser/service_worker/service_worker_dispatcher_host.h#newcode30 content/browser/service_worker/service_worker_dispatcher_host.h:30: nit: can we remove this line ...
7 years, 1 month ago (2013-10-25 15:28:00 UTC) #37
alecflett
On 2013/10/24 23:25:51, Tom Sepez wrote: > LGTM after addressing the comment. Please don't remove ...
7 years, 1 month ago (2013-10-25 18:01:00 UTC) #38
alecflett
https://codereview.chromium.org/25008006/diff/298002/content/browser/service_worker/service_worker_dispatcher_host.cc File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/25008006/diff/298002/content/browser/service_worker/service_worker_dispatcher_host.cc#newcode51 content/browser/service_worker/service_worker_dispatcher_host.cc:51: // TODO(alecflett): add a ServiceWorker-specific policy query in On ...
7 years, 1 month ago (2013-10-25 18:22:49 UTC) #39
alecflett
Over to jam. Mind taking a look, specifically for content/ hookups?
7 years, 1 month ago (2013-10-25 23:44:40 UTC) #40
jam
https://codereview.chromium.org/25008006/diff/588001/content/child/service_worker/service_worker_dispatcher.h File content/child/service_worker/service_worker_dispatcher.h (right): https://codereview.chromium.org/25008006/diff/588001/content/child/service_worker/service_worker_dispatcher.h#newcode1 content/child/service_worker/service_worker_dispatcher.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. ...
7 years, 1 month ago (2013-10-28 17:05:50 UTC) #41
alecflett
On 2013/10/28 17:05:50, jam wrote: > https://codereview.chromium.org/25008006/diff/588001/content/child/service_worker/service_worker_dispatcher.h > File content/child/service_worker/service_worker_dispatcher.h (right): > > https://codereview.chromium.org/25008006/diff/588001/content/child/service_worker/service_worker_dispatcher.h#newcode1 > ...
7 years, 1 month ago (2013-10-28 17:16:49 UTC) #42
jam
lgtm On 2013/10/28 17:16:49, alecflett wrote: > On 2013/10/28 17:05:50, jam wrote: > > > ...
7 years, 1 month ago (2013-10-28 17:26:54 UTC) #43
alecflett
https://codereview.chromium.org/25008006/diff/588001/content/browser/service_worker/service_worker_dispatcher_host.h File content/browser/service_worker/service_worker_dispatcher_host.h (right): https://codereview.chromium.org/25008006/diff/588001/content/browser/service_worker/service_worker_dispatcher_host.h#newcode37 content/browser/service_worker/service_worker_dispatcher_host.h:37: int render_process_id_ ALLOW_UNUSED; On 2013/10/28 17:26:55, jam wrote: > ...
7 years, 1 month ago (2013-10-28 21:11:42 UTC) #44
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alecflett@chromium.org/25008006/788001
7 years, 1 month ago (2013-10-28 21:21:08 UTC) #45
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 1 month ago (2013-10-28 22:10:46 UTC) #46
alecflett
allow_unused
7 years, 1 month ago (2013-10-28 22:17:32 UTC) #47
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alecflett@chromium.org/25008006/758002
7 years, 1 month ago (2013-10-28 22:19:22 UTC) #48
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 1 month ago (2013-10-28 23:24:59 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alecflett@chromium.org/25008006/1198001
7 years, 1 month ago (2013-10-28 23:41:06 UTC) #50
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 1 month ago (2013-10-29 00:47:19 UTC) #51
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alecflett@chromium.org/25008006/1348001
7 years, 1 month ago (2013-10-29 19:09:39 UTC) #52
commit-bot: I haz the power
Step "update" is always a major failure. Look at the try server FAQ for more ...
7 years, 1 month ago (2013-10-29 19:52:29 UTC) #53
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alecflett@chromium.org/25008006/1348001
7 years, 1 month ago (2013-10-29 21:15:22 UTC) #54
commit-bot: I haz the power
7 years, 1 month ago (2013-10-30 00:47:48 UTC) #55
Message was sent while issue was closed.
Change committed as 231688

Powered by Google App Engine
This is Rietveld 408576698