Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(39)

Issue 2009453002: service worker: Don't control a subframe of an insecure context (Closed)

Created:
3 years, 1 month ago by falken
Modified:
3 years, 1 month ago
CC:
blink-reviews, blink-worker-reviews_chromium.org, chromium-reviews, Charlie Reis, creis+watch_chromium.org, darin-cc_chromium.org, estark, horo+watch_chromium.org, jam, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, kinuko+watch, kinuko+serviceworker, michaeln, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, nhiroki, raymes, serviceworker-reviews, site-isolation-reviews_chromium.org, timwillis, Tom Sepez, tzik
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

service worker: Don't control a subframe of an insecure context We must check isSecureContext when creating the network provider to adhere to https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-privileged. We already did this for getRegistration(), register(), unregister() but must also do this when deciding whether to control an in-scope document. BUG=607543 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/59a2e54eeb0e61971a0c27c44c54dd0c30b5d06d Cr-Commit-Position: refs/heads/master@{#398229}

Patch Set 1 #

Total comments: 4

Patch Set 2 : getSecurityOrigin #

Total comments: 16

Patch Set 3 : refactor the walk #

Patch Set 4 : add comments #

Patch Set 5 : fix site isolation #

Patch Set 6 : cover plznavigation case too #

Total comments: 8

Patch Set 7 : is_parent_frame_secure #

Patch Set 8 : rebase #

Patch Set 9 : rebase #

Patch Set 10 : add tests, refine #

Patch Set 11 : selfreview #

Total comments: 10

Patch Set 12 : rebase #

Patch Set 13 : rebase past build break #

Patch Set 14 : review comments #

Total comments: 10

Patch Set 15 : horo comments #

Total comments: 3

Patch Set 16 : rebase #

Patch Set 17 : consolidate tests #

Total comments: 5

Patch Set 18 : kinuko comments #

Total comments: 3

Patch Set 19 : rebase #

Patch Set 20 : expand comment for apitest #

Total comments: 2

Patch Set 21 : refactor errorMessage #

Unified diffs Side-by-side diffs Delta from patch set Stats (+491 lines, -141 lines) Patch
M chrome/browser/chrome_content_browser_client.h View 1 2 3 4 5 6 9 10 11 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/extensions/service_worker_apitest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +20 lines, -3 lines 0 comments Download
M content/browser/service_worker/service_worker_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +6 lines, -5 lines 0 comments Download
M content/browser/service_worker/service_worker_context_core.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +7 lines, -7 lines 0 comments Download
M content/browser/service_worker/service_worker_context_request_handler_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +6 lines, -5 lines 0 comments Download
M content/browser/service_worker/service_worker_context_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +12 lines, -4 lines 0 comments Download
M content/browser/service_worker/service_worker_controllee_request_handler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +12 lines, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 8 chunks +13 lines, -12 lines 0 comments Download
M content/browser/service_worker/service_worker_dispatcher_host.h View 1 2 3 4 5 6 1 chunk +2 lines, -1 line 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 3 chunks +12 lines, -5 lines 0 comments Download
M content/browser/service_worker/service_worker_dispatcher_host_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 5 chunks +13 lines, -7 lines 0 comments Download
M content/browser/service_worker/service_worker_handle_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +3 lines, -2 lines 0 comments Download
M content/browser/service_worker/service_worker_job_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +6 lines, -5 lines 0 comments Download
M content/browser/service_worker/service_worker_provider_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 5 chunks +25 lines, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_provider_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 6 chunks +22 lines, -1 line 0 comments Download
M content/browser/service_worker/service_worker_provider_host_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 5 chunks +64 lines, -9 lines 0 comments Download
M content/browser/service_worker/service_worker_request_handler_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +9 lines, -9 lines 0 comments Download
M content/browser/service_worker/service_worker_storage_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 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 6 7 8 9 10 11 12 13 14 13 chunks +17 lines, -20 lines 0 comments Download
M content/browser/service_worker/service_worker_version_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +5 lines, -3 lines 0 comments Download
M content/child/service_worker/service_worker_network_provider.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +11 lines, -4 lines 0 comments Download
M content/child/service_worker/service_worker_network_provider.cc View 1 2 3 4 5 6 7 8 9 10 5 chunks +25 lines, -11 lines 0 comments Download
M content/common/service_worker/service_worker_messages.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +13 lines, -2 lines 0 comments Download
M content/public/browser/content_browser_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +6 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 1 chunk +2 lines, -2 lines 0 comments Download
M content/renderer/service_worker/service_worker_context_client.cc View 1 2 3 4 5 6 1 chunk +2 lines, -1 line 0 comments Download
M content/renderer/shared_worker/embedded_shared_worker_stub.cc View 1 2 3 4 5 6 1 chunk +2 lines, -1 line 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/serviceworker/insecure-parent-frame.html View 1 2 3 4 5 6 7 8 9 10 1 chunk +35 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/insecure-inscope.html View 1 chunk +9 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/insecure-parent.html View 1 chunk +16 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Document.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/dom/Document.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 4 chunks +10 lines, -21 lines 0 comments Download
M third_party/WebKit/Source/core/frame/Frame.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +9 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/Frame.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +9 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebFrame.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +8 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/tests/WebFrameTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +58 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebFrame.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +9 lines, -0 lines 0 comments Download

Messages

Total messages: 72 (16 generated)
falken
PTAL horo: everything kinuko: content/renderer clamy: frame_host jww: security review
3 years, 1 month ago (2016-05-24 09:31:03 UTC) #3
falken
+creis for OOPIF question https://codereview.chromium.org/2009453002/diff/1/content/child/service_worker/service_worker_network_provider.cc File content/child/service_worker/service_worker_network_provider.cc (right): https://codereview.chromium.org/2009453002/diff/1/content/child/service_worker/service_worker_network_provider.cc#newcode89 content/child/service_worker/service_worker_network_provider.cc:89: if (parent && !parent->document().isSecureContext(errorMessage)) Some ...
3 years, 1 month ago (2016-05-24 11:29:58 UTC) #5
clamy
content/browser/frame_host lgtm.
3 years, 1 month ago (2016-05-24 11:54:14 UTC) #6
falken
https://codereview.chromium.org/2009453002/diff/1/content/child/service_worker/service_worker_network_provider.cc File content/child/service_worker/service_worker_network_provider.cc (right): https://codereview.chromium.org/2009453002/diff/1/content/child/service_worker/service_worker_network_provider.cc#newcode89 content/child/service_worker/service_worker_network_provider.cc:89: if (parent && !parent->document().isSecureContext(errorMessage)) On 2016/05/24 11:29:58, falken wrote: ...
3 years, 1 month ago (2016-05-24 13:16:38 UTC) #7
Marijn Kruisselbrink
drive-by comment https://codereview.chromium.org/2009453002/diff/20001/content/child/service_worker/service_worker_network_provider.cc File content/child/service_worker/service_worker_network_provider.cc (right): https://codereview.chromium.org/2009453002/diff/20001/content/child/service_worker/service_worker_network_provider.cc#newcode86 content/child/service_worker/service_worker_network_provider.cc:86: while (parent && should_create_provider_for_window) { On 2016/05/24 ...
3 years, 1 month ago (2016-05-24 17:03:35 UTC) #8
Charlie Reis
https://codereview.chromium.org/2009453002/diff/1/content/child/service_worker/service_worker_network_provider.cc File content/child/service_worker/service_worker_network_provider.cc (right): https://codereview.chromium.org/2009453002/diff/1/content/child/service_worker/service_worker_network_provider.cc#newcode89 content/child/service_worker/service_worker_network_provider.cc:89: if (parent && !parent->document().isSecureContext(errorMessage)) On 2016/05/24 13:16:37, falken wrote: ...
3 years, 1 month ago (2016-05-24 17:22:00 UTC) #10
jww
https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_host/navigation_request.cc File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_host/navigation_request.cc#newcode304 content/browser/frame_host/navigation_request.cc:304: // TODO(falken): This should do an isSecureContext check too. ...
3 years, 1 month ago (2016-05-24 18:06:04 UTC) #12
alexmos
https://codereview.chromium.org/2009453002/diff/1/content/child/service_worker/service_worker_network_provider.cc File content/child/service_worker/service_worker_network_provider.cc (right): https://codereview.chromium.org/2009453002/diff/1/content/child/service_worker/service_worker_network_provider.cc#newcode89 content/child/service_worker/service_worker_network_provider.cc:89: if (parent && !parent->document().isSecureContext(errorMessage)) On 2016/05/24 17:22:00, Charlie Reis ...
3 years, 1 month ago (2016-05-24 18:51:55 UTC) #13
falken
https://codereview.chromium.org/2009453002/diff/20001/content/child/service_worker/service_worker_network_provider.cc File content/child/service_worker/service_worker_network_provider.cc (right): https://codereview.chromium.org/2009453002/diff/20001/content/child/service_worker/service_worker_network_provider.cc#newcode86 content/child/service_worker/service_worker_network_provider.cc:86: while (parent && should_create_provider_for_window) { On 2016/05/24 18:06:03, jww ...
3 years, 1 month ago (2016-05-25 01:33:34 UTC) #14
falken
https://codereview.chromium.org/2009453002/diff/20001/content/child/service_worker/service_worker_network_provider.cc File content/child/service_worker/service_worker_network_provider.cc (right): https://codereview.chromium.org/2009453002/diff/20001/content/child/service_worker/service_worker_network_provider.cc#newcode86 content/child/service_worker/service_worker_network_provider.cc:86: while (parent && should_create_provider_for_window) { On 2016/05/25 01:33:34, falken ...
3 years, 1 month ago (2016-05-25 04:09:45 UTC) #15
falken
I've refactored out the parent walk. Please take another look. No check is added for ...
3 years, 1 month ago (2016-05-25 09:19:58 UTC) #17
clamy
https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_host/navigation_request.cc File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_host/navigation_request.cc#newcode304 content/browser/frame_host/navigation_request.cc:304: // TODO(falken): This should do an isSecureContext check too. ...
3 years, 1 month ago (2016-05-25 11:21:46 UTC) #18
falken
I fixed the site isolation error. alexmos, jww: Can you review this again? https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_host/navigation_request.cc File ...
3 years, 1 month ago (2016-05-26 02:09:49 UTC) #19
clamy
https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_host/navigation_request.cc File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_host/navigation_request.cc#newcode304 content/browser/frame_host/navigation_request.cc:304: // TODO(falken): This should do an isSecureContext check too. ...
3 years, 1 month ago (2016-05-26 15:45:31 UTC) #20
Marijn Kruisselbrink
drive-by nit https://codereview.chromium.org/2009453002/diff/120001/content/browser/service_worker/service_worker_request_handler.cc File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/2009453002/diff/120001/content/browser/service_worker/service_worker_request_handler.cc#newcode219 content/browser/service_worker/service_worker_request_handler.cc:219: if (new_provider_id == kInvalidServiceWorkerVersionId) Shouldn't this be ...
3 years, 1 month ago (2016-05-26 20:37:11 UTC) #21
alexmos
https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_host/navigation_request.cc File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_host/navigation_request.cc#newcode304 content/browser/frame_host/navigation_request.cc:304: // TODO(falken): This should do an isSecureContext check too. ...
3 years, 1 month ago (2016-05-26 22:13:35 UTC) #22
falken
Thanks! comment-only for now. https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_host/navigation_request.cc File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_host/navigation_request.cc#newcode304 content/browser/frame_host/navigation_request.cc:304: // TODO(falken): This should do ...
3 years, 1 month ago (2016-05-27 05:47:04 UTC) #24
alexmos
https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/public/web/WebFrame.h File third_party/WebKit/public/web/WebFrame.h (right): https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/public/web/WebFrame.h#newcode555 third_party/WebKit/public/web/WebFrame.h:555: BLINK_EXPORT bool canHaveSecureChild(WebString& errorMessage) const; On 2016/05/27 05:47:04, falken ...
3 years, 1 month ago (2016-05-27 22:44:30 UTC) #25
jww
On 2016/05/27 22:44:30, alexmos wrote: > https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/public/web/WebFrame.h > File third_party/WebKit/public/web/WebFrame.h (right): > > https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/public/web/WebFrame.h#newcode555 > ...
3 years, 1 month ago (2016-05-28 01:35:14 UTC) #26
jww
https://codereview.chromium.org/2009453002/diff/20001/content/child/service_worker/service_worker_network_provider.cc File content/child/service_worker/service_worker_network_provider.cc (right): https://codereview.chromium.org/2009453002/diff/20001/content/child/service_worker/service_worker_network_provider.cc#newcode86 content/child/service_worker/service_worker_network_provider.cc:86: while (parent && should_create_provider_for_window) { On 2016/05/25 04:09:45, falken ...
3 years, 1 month ago (2016-05-28 01:35:25 UTC) #27
raymes
+cc timwillis https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/public/web/WebFrame.h File third_party/WebKit/public/web/WebFrame.h (right): https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/public/web/WebFrame.h#newcode555 third_party/WebKit/public/web/WebFrame.h:555: BLINK_EXPORT bool canHaveSecureChild(WebString& errorMessage) const; The plugin ...
3 years, 1 month ago (2016-05-30 01:08:08 UTC) #28
falken
I think jww is right: it's incorrect to ignore scheme exceptions here, since extension service ...
3 years, 1 month ago (2016-05-30 10:20:53 UTC) #29
clamy
https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_host/navigation_request.cc File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_host/navigation_request.cc#newcode304 content/browser/frame_host/navigation_request.cc:304: // TODO(falken): This should do an isSecureContext check too. ...
3 years, 1 month ago (2016-05-30 16:22:42 UTC) #30
jww
On 2016/05/30 16:22:42, clamy wrote: > https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_host/navigation_request.cc > File content/browser/frame_host/navigation_request.cc (right): > > https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_host/navigation_request.cc#newcode304 > ...
3 years, 1 month ago (2016-05-31 19:26:41 UTC) #31
alexmos
> falken: That sounds good to me, although I think alexmos@ really needs to give ...
3 years, 1 month ago (2016-06-01 01:30:10 UTC) #32
alexmos
> I’m also wondering about frame reparenting. Is it possible to go from a secure ...
3 years, 1 month ago (2016-06-01 01:34:21 UTC) #34
Marijn Kruisselbrink
On 2016/06/01 at 01:34:21, alexmos wrote: > > I’m also wondering about frame reparenting. Is ...
3 years, 1 month ago (2016-06-01 01:47:55 UTC) #35
falken
On 2016/06/01 01:30:10, alexmos wrote: > > falken: That sounds good to me, although I ...
3 years, 1 month ago (2016-06-01 03:20:11 UTC) #36
falken
alexmos, jww: please review again I decided not to change the plugin callsite since I ...
3 years, 1 month ago (2016-06-01 07:14:44 UTC) #37
falken
alexmos, jww: ping
3 years, 1 month ago (2016-06-02 01:13:06 UTC) #38
Marijn Kruisselbrink
drive-by suggestion, as I'm trying to build on top of this change https://codereview.chromium.org/2009453002/diff/220001/content/browser/service_worker/service_worker_controllee_request_handler.cc File content/browser/service_worker/service_worker_controllee_request_handler.cc ...
3 years, 1 month ago (2016-06-02 22:44:44 UTC) #39
jww
On 2016/06/02 22:44:44, Marijn Kruisselbrink wrote: > drive-by suggestion, as I'm trying to build on ...
3 years, 1 month ago (2016-06-02 22:56:45 UTC) #40
jww
On 2016/06/02 22:56:45, jww wrote: > On 2016/06/02 22:44:44, Marijn Kruisselbrink wrote: > > drive-by ...
3 years, 1 month ago (2016-06-02 22:57:07 UTC) #41
alexmos
> Yep, all of ServiceWorkerControlleeRequestHandler is on the IO thread. Are > FrameTreeNodes used on ...
3 years, 1 month ago (2016-06-02 23:54:47 UTC) #42
falken
Thank you all. Pulling in more owners: horo: Can you review everything? devlin: chrome/browser/extensions/service_worker_apitest.cc tsepez: ...
3 years, 1 month ago (2016-06-03 08:22:05 UTC) #44
horo
https://codereview.chromium.org/2009453002/diff/280001/content/browser/service_worker/service_worker_context_core.cc File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/2009453002/diff/280001/content/browser/service_worker/service_worker_context_core.cc#newcode650 content/browser/service_worker/service_worker_context_core.cc:650: return false; Do we need the returned value? https://codereview.chromium.org/2009453002/diff/280001/content/browser/service_worker/service_worker_provider_host.h ...
3 years, 1 month ago (2016-06-03 09:54:45 UTC) #45
falken
Thank you. https://codereview.chromium.org/2009453002/diff/280001/content/browser/service_worker/service_worker_context_core.cc File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/2009453002/diff/280001/content/browser/service_worker/service_worker_context_core.cc#newcode650 content/browser/service_worker/service_worker_context_core.cc:650: return false; On 2016/06/03 09:54:44, horo wrote: ...
3 years, 1 month ago (2016-06-03 12:10:45 UTC) #46
Devlin
service_worker_apitest.cc lgtm, but +lazyboy@ as FYI and in case he sees something I missed.
3 years, 1 month ago (2016-06-03 15:06:45 UTC) #48
Tom Sepez
https://codereview.chromium.org/2009453002/diff/300001/third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right): https://codereview.chromium.org/2009453002/diff/300001/third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp#newcode362 third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:362: bool SecurityOrigin::isPotentiallyTrustworthy(String* errorMessage) const Why do we need to ...
3 years, 1 month ago (2016-06-03 17:29:58 UTC) #49
Tom Sepez
3 years, 1 month ago (2016-06-03 17:32:51 UTC) #50
horo
lgtm
3 years, 1 month ago (2016-06-06 01:54:33 UTC) #51
falken
Thanks, updated. Please take another look. devlin, lazyboy: I made some changes in /service_worker_apitest.cc. I ...
3 years, 1 month ago (2016-06-06 06:38:13 UTC) #53
kinuko
nits only https://codereview.chromium.org/2009453002/diff/360001/content/browser/service_worker/service_worker_provider_host.h File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/2009453002/diff/360001/content/browser/service_worker/service_worker_provider_host.h#newcode96 content/browser/service_worker/service_worker_provider_host.h:96: } nit: why do we always translate ...
3 years, 1 month ago (2016-06-06 06:43:16 UTC) #54
falken
Updated. https://codereview.chromium.org/2009453002/diff/360001/content/browser/service_worker/service_worker_provider_host.h File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/2009453002/diff/360001/content/browser/service_worker/service_worker_provider_host.h#newcode96 content/browser/service_worker/service_worker_provider_host.h:96: } On 2016/06/06 06:43:16, kinuko wrote: > nit: ...
3 years, 1 month ago (2016-06-06 06:56:55 UTC) #55
kinuko
(rs-ish) lgtm https://codereview.chromium.org/2009453002/diff/360001/content/browser/service_worker/service_worker_provider_host.h File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/2009453002/diff/360001/content/browser/service_worker/service_worker_provider_host.h#newcode96 content/browser/service_worker/service_worker_provider_host.h:96: } On 2016/06/06 06:56:55, falken wrote: > ...
3 years, 1 month ago (2016-06-06 09:28:04 UTC) #56
falken
Thanks. Refreshing reviewer roster: +dcheng: web/ and messages +devlin/lazyboy: changes to service_worker_apitest.cc since last review ...
3 years, 1 month ago (2016-06-06 13:32:37 UTC) #58
Devlin
https://codereview.chromium.org/2009453002/diff/380001/chrome/browser/extensions/service_worker_apitest.cc File chrome/browser/extensions/service_worker_apitest.cc (right): https://codereview.chromium.org/2009453002/diff/380001/chrome/browser/extensions/service_worker_apitest.cc#newcode638 chrome/browser/extensions/service_worker_apitest.cc:638: // Extension service workers must be able to control ...
3 years, 1 month ago (2016-06-06 15:20:57 UTC) #59
Marijn Kruisselbrink
https://codereview.chromium.org/2009453002/diff/380001/chrome/browser/extensions/service_worker_apitest.cc File chrome/browser/extensions/service_worker_apitest.cc (right): https://codereview.chromium.org/2009453002/diff/380001/chrome/browser/extensions/service_worker_apitest.cc#newcode638 chrome/browser/extensions/service_worker_apitest.cc:638: // Extension service workers must be able to control ...
3 years, 1 month ago (2016-06-06 17:31:20 UTC) #60
falken
https://codereview.chromium.org/2009453002/diff/380001/chrome/browser/extensions/service_worker_apitest.cc File chrome/browser/extensions/service_worker_apitest.cc (right): https://codereview.chromium.org/2009453002/diff/380001/chrome/browser/extensions/service_worker_apitest.cc#newcode638 chrome/browser/extensions/service_worker_apitest.cc:638: // Extension service workers must be able to control ...
3 years, 1 month ago (2016-06-07 01:15:30 UTC) #61
Devlin
On 2016/06/07 01:15:30, falken wrote: > Thanks, I revised the comment. What do you think? ...
3 years, 1 month ago (2016-06-07 01:17:19 UTC) #62
dcheng
blink/ipc lgtm but two concerns that I'd like to see addressed in followups 1) I ...
3 years, 1 month ago (2016-06-07 02:21:20 UTC) #63
falken
Thanks. I made one minor change to Document to simplify the errorMessage population. Will put ...
3 years, 1 month ago (2016-06-07 03:14:29 UTC) #64
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2009453002/440001
3 years, 1 month ago (2016-06-07 03:15:47 UTC) #67
commit-bot: I haz the power
Committed patchset #21 (id:440001)
3 years, 1 month ago (2016-06-07 05:06:29 UTC) #69
commit-bot: I haz the power
Patchset 21 (id:??) landed as https://crrev.com/59a2e54eeb0e61971a0c27c44c54dd0c30b5d06d Cr-Commit-Position: refs/heads/master@{#398229}
3 years, 1 month ago (2016-06-07 05:08:43 UTC) #71
dcheng
3 years, 1 month ago (2016-06-08 18:03:53 UTC) #72
Message was sent while issue was closed.
A revert of this CL (patchset #21 id:440001) has been created in
https://codereview.chromium.org/2055433002/ by dcheng@chromium.org.

The reason for reverting is:
ServiceWorkerProviderHost::SetControllerVersionAttribute CHECK is firing in
release builds: https://crbug.com/618365.

Powered by Google App Engine
This is Rietveld 408576698