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

Issue 26442004: Service worker registration error support (Closed)

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

Description

Service worker registration error support This patch introduces use of WebServiceWorkerError as a means to propagate an error from the browser to the renderer, and further reflect this error via WebCallbacks into Blink BUG=285976 R=jam@chromium.org, michaeln@chromium.org, tsepez@chromium.org TBR=abarth Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233438

Patch Set 1 #

Patch Set 2 : switch ownership to callee #

Patch Set 3 : Update to reflect registration patch #

Patch Set 4 : Update to reflect new blink side error patch #

Patch Set 5 : Update to reflect new blink side error patch #

Patch Set 6 : Update to reflect new blink side error patch take 2 #

Patch Set 7 : Update to reflect changes from dependent bug #

Total comments: 14

Patch Set 8 : Review nits #

Total comments: 20

Patch Set 9 : Address michaeln's nits #

Total comments: 2

Patch Set 10 : Additional issues from michaeln #

Patch Set 11 : Additional issues from michaeln #

Total comments: 1

Patch Set 12 : Update to ToT, address jam's comments #

Patch Set 13 : Fix windows #

Patch Set 14 : re-fix tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+200 lines, -20 lines) Patch
M content/browser/DEPS View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_context.h View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +6 lines, -1 line 0 comments Download
M content/browser/service_worker/service_worker_context.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +7 lines, -0 lines 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 1 chunk +1 line, -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 4 chunks +37 lines, -6 lines 0 comments Download
A content/browser/service_worker/service_worker_dispatcher_host_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +96 lines, -0 lines 0 comments Download
M content/child/service_worker/service_worker_dispatcher.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +8 lines, -4 lines 0 comments Download
M content/child/service_worker/service_worker_dispatcher.cc View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +26 lines, -6 lines 0 comments Download
M content/common/DEPS View 1 chunk +1 line, -0 lines 0 comments Download
M content/common/service_worker_messages.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +14 lines, -0 lines 0 comments Download
M content/content_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +3 lines, -2 lines 0 comments Download

Messages

Total messages: 30 (0 generated)
alecflett
Here's the chrome side of the error handler, with a working unittest. (The unittest itself ...
7 years, 2 months ago (2013-10-16 00:24:27 UTC) #1
alecflett
Also, I'm not particularly psyched to be touching DEPS files here, but it seemed better ...
7 years, 2 months ago (2013-10-16 00:26:03 UTC) #2
alecflett
Ok, new patch is up to reflect ownership change from https://codereview.chromium.org/27278002/
7 years, 2 months ago (2013-10-16 01:09:09 UTC) #3
alecflett
tsepez@chromium.org: this is a followon to http://crrev.com/25008006 to propagate the errors back to the caller
7 years, 1 month ago (2013-10-25 18:33:49 UTC) #4
Tom Sepez
LGTM. Please forgive the nits. https://codereview.chromium.org/26442004/diff/159001/content/browser/DEPS File content/browser/DEPS (right): https://codereview.chromium.org/26442004/diff/159001/content/browser/DEPS#newcode42 content/browser/DEPS:42: "+third_party/WebKit/public/platform/WebScreenInfo.h", nit: alphabetize. https://codereview.chromium.org/26442004/diff/159001/content/browser/service_worker/service_worker_dispatcher_host.cc ...
7 years, 1 month ago (2013-10-25 18:46:58 UTC) #5
alecflett
thanks! https://codereview.chromium.org/26442004/diff/159001/content/browser/DEPS File content/browser/DEPS (right): https://codereview.chromium.org/26442004/diff/159001/content/browser/DEPS#newcode42 content/browser/DEPS:42: "+third_party/WebKit/public/platform/WebScreenInfo.h", On 2013/10/25 18:46:59, Tom Sepez wrote: > ...
7 years, 1 month ago (2013-10-25 20:05:02 UTC) #6
michaeln
https://codereview.chromium.org/26442004/diff/159001/content/common/service_worker_messages.h File content/common/service_worker_messages.h (right): https://codereview.chromium.org/26442004/diff/159001/content/common/service_worker_messages.h#newcode55 content/common/service_worker_messages.h:55: string16 /* message */) On 2013/10/25 20:05:02, alecflett wrote: ...
7 years, 1 month ago (2013-10-25 21:41:24 UTC) #7
michaeln
lgtm (modulo nits)
7 years, 1 month ago (2013-10-25 22:02:18 UTC) #8
alecflett
https://codereview.chromium.org/26442004/diff/319001/content/browser/service_worker/service_worker_dispatcher_host_unittest.cc File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/26442004/diff/319001/content/browser/service_worker/service_worker_dispatcher_host_unittest.cc#newcode22 content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:22: ASSERT_TRUE(dir_.CreateUniqueTempDir()); On 2013/10/25 21:41:25, michaeln wrote: > we can ...
7 years, 1 month ago (2013-10-25 23:41:55 UTC) #9
alecflett
jam@ this one builds on http://crrev.com/25008006
7 years, 1 month ago (2013-10-25 23:45:35 UTC) #10
michaeln
https://codereview.chromium.org/26442004/diff/319001/content/child/service_worker/service_worker_dispatcher.cc File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/26442004/diff/319001/content/child/service_worker/service_worker_dispatcher.cc#newcode140 content/child/service_worker/service_worker_dispatcher.cc:140: if (!callbacks) On 2013/10/25 23:41:56, alecflett wrote: > On ...
7 years, 1 month ago (2013-10-26 00:41:00 UTC) #11
michaeln
https://codereview.chromium.org/26442004/diff/319001/content/browser/service_worker/service_worker_dispatcher_host.cc File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/26442004/diff/319001/content/browser/service_worker/service_worker_dispatcher_host.cc#newcode60 content/browser/service_worker/service_worker_dispatcher_host.cc:60: if (scope.GetOrigin() != script_url.GetOrigin()) On 2013/10/25 21:41:25, michaeln wrote: ...
7 years, 1 month ago (2013-10-26 00:57:39 UTC) #12
alecflett
ok, think I got all of 'em this time. I keep getting 500's uploading but ...
7 years, 1 month ago (2013-10-28 17:03:13 UTC) #13
michaeln
https://codereview.chromium.org/26442004/diff/319001/content/child/service_worker/service_worker_dispatcher.cc File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/26442004/diff/319001/content/child/service_worker/service_worker_dispatcher.cc#newcode85 content/child/service_worker/service_worker_dispatcher.cc:85: g_dispatcher_tls.Pointer()->Set(NULL); I think i see the potential reason for ...
7 years, 1 month ago (2013-10-28 20:46:35 UTC) #14
alecflett
jam@ can I get a quick review for content stuff? (I think the only thing ...
7 years, 1 month ago (2013-10-30 02:00:10 UTC) #15
jam
lgtm https://codereview.chromium.org/26442004/diff/479001/content/browser/service_worker/service_worker_context.h File content/browser/service_worker/service_worker_context.h (right): https://codereview.chromium.org/26442004/diff/479001/content/browser/service_worker/service_worker_context.h#newcode35 content/browser/service_worker/service_worker_context.h:35: bool enabled(); nit: per style guide, this has ...
7 years, 1 month ago (2013-10-31 20:39:29 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alecflett@chromium.org/26442004/619001
7 years, 1 month ago (2013-11-04 22:10:05 UTC) #17
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=34338
7 years, 1 month ago (2013-11-05 00:42:13 UTC) #18
alecflett
abarth@ - can I get a quick lgtm because I'm adding third_party/WebKit/platform/public/platform/WebServiceWorkerError.h to content/browser ? ...
7 years, 1 month ago (2013-11-05 01:05:15 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alecflett@chromium.org/26442004/619001
7 years, 1 month ago (2013-11-05 04:27:20 UTC) #20
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-11-05 05:07:03 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alecflett@chromium.org/26442004/619001
7 years, 1 month ago (2013-11-05 15:37:58 UTC) #22
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-11-05 15:57:55 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alecflett@chromium.org/26442004/619001
7 years, 1 month ago (2013-11-06 01:23:55 UTC) #24
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-11-06 02:26:14 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alecflett@chromium.org/26442004/1229001
7 years, 1 month ago (2013-11-06 16:35:52 UTC) #26
commit-bot: I haz the power
Retried try job too often on linux_rel for step(s) content_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&number=186012
7 years, 1 month ago (2013-11-06 17:22:24 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alecflett@chromium.org/26442004/1469001
7 years, 1 month ago (2013-11-06 18:32:09 UTC) #28
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=220857
7 years, 1 month ago (2013-11-06 23:43:12 UTC) #29
alecflett
7 years, 1 month ago (2013-11-07 00:39:47 UTC) #30
Message was sent while issue was closed.
Committed patchset #14 manually as r233438 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698