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

Issue 2678733002: [ServiceWorker] Mojofy ActivateEvent of Service Worker (Closed)

Created:
3 years, 10 months ago by xiaofengzhang
Modified:
3 years, 10 months ago
CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, viettrungluu+watch_chromium.org, qsr+mojo_chromium.org, mlamouri+watch-content_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, jam, nhiroki, abarth-chromium, Aaron Boodman, kinuko+serviceworker, yzshen+watch_chromium.org, horo+watch_chromium.org, darin-cc_chromium.org, kinuko+watch, tzik, darin (slow to review), blink-worker-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[ServiceWorker] Mojofy ActivateEvent of Service Worker This patch mojofies the following two messages: ServiceWorkerMsg_ActivateEvent ServiceWorkerHostMsg_ActivateEventFinished BUG=629701 TEST=content_unittests Review-Url: https://codereview.chromium.org/2678733002 Cr-Commit-Position: refs/heads/master@{#451513} Committed: https://chromium.googlesource.com/chromium/src/+/64975bc58e6b041ade1cfe2edb431896548845f3

Patch Set 1 #

Total comments: 12

Patch Set 2 : Rebase and address shimazu, falken and peter's comments #

Total comments: 26

Patch Set 3 : Address comments #25, #26, #33 #

Total comments: 3

Patch Set 4 : Address falken's comments #38 #

Patch Set 5 : Just rebase #

Messages

Total messages: 67 (28 generated)
xiaofengzhang
shimazu@ Could you please review this? Thanks.
3 years, 10 months ago (2017-02-07 01:23:25 UTC) #6
shimazu
Thanks for continuing to work on this:) Overall looks good. https://codereview.chromium.org/2678733002/diff/1/content/browser/service_worker/embedded_worker_test_helper.h File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/1/content/browser/service_worker/embedded_worker_test_helper.h#newcode67 ...
3 years, 10 months ago (2017-02-07 04:43:48 UTC) #7
shimazu
s/ActiviteEvent/ActivateEvent/ in the subject of this CL.
3 years, 10 months ago (2017-02-07 04:44:31 UTC) #8
falken
https://codereview.chromium.org/2678733002/diff/1/content/browser/service_worker/embedded_worker_test_helper.h File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/1/content/browser/service_worker/embedded_worker_test_helper.h#newcode67 content/browser/service_worker/embedded_worker_test_helper.h:67: enum class Event { OnActivateEvent, LastEvent }; On 2017/02/07 ...
3 years, 10 months ago (2017-02-07 05:19:35 UTC) #10
xiaofengzhang
On 2017/02/07 04:44:31, shimazu wrote: > s/ActiviteEvent/ActivateEvent/ in the subject of this CL. Thanks, done ...
3 years, 10 months ago (2017-02-07 05:57:44 UTC) #13
xiaofengzhang
https://codereview.chromium.org/2678733002/diff/1/content/browser/service_worker/embedded_worker_test_helper.h File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/1/content/browser/service_worker/embedded_worker_test_helper.h#newcode67 content/browser/service_worker/embedded_worker_test_helper.h:67: enum class Event { OnActivateEvent, LastEvent }; On 2017/02/07 ...
3 years, 10 months ago (2017-02-07 06:15:36 UTC) #14
shimazu
+peter for asking a question about his comment on the previous CL. https://codereview.chromium.org/2678733002/diff/1/content/browser/service_worker/service_worker_registration.cc File content/browser/service_worker/service_worker_registration.cc ...
3 years, 10 months ago (2017-02-07 07:19:31 UTC) #16
Peter Beverloo
https://codereview.chromium.org/2678733002/diff/1/content/browser/service_worker/service_worker_registration.cc File content/browser/service_worker/service_worker_registration.cc (right): https://codereview.chromium.org/2678733002/diff/1/content/browser/service_worker/service_worker_registration.cc#newcode432 content/browser/service_worker/service_worker_registration.cc:432: activating_version, request_id)); On 2017/02/07 07:19:31, shimazu wrote: > On ...
3 years, 10 months ago (2017-02-08 15:13:16 UTC) #17
shimazu
https://codereview.chromium.org/2678733002/diff/1/content/browser/service_worker/service_worker_registration.cc File content/browser/service_worker/service_worker_registration.cc (right): https://codereview.chromium.org/2678733002/diff/1/content/browser/service_worker/service_worker_registration.cc#newcode432 content/browser/service_worker/service_worker_registration.cc:432: activating_version, request_id)); On 2017/02/08 15:13:16, Peter Beverloo wrote: > ...
3 years, 10 months ago (2017-02-10 05:42:27 UTC) #18
Peter Beverloo
On 2017/02/10 05:42:27, shimazu wrote: > https://codereview.chromium.org/2678733002/diff/1/content/browser/service_worker/service_worker_registration.cc > File content/browser/service_worker/service_worker_registration.cc (right): > > https://codereview.chromium.org/2678733002/diff/1/content/browser/service_worker/service_worker_registration.cc#newcode432 > ...
3 years, 10 months ago (2017-02-10 17:40:53 UTC) #19
xiaofengzhang
On 2017/02/10 17:40:53, Peter Beverloo wrote: > On 2017/02/10 05:42:27, shimazu wrote: > > > ...
3 years, 10 months ago (2017-02-14 06:34:57 UTC) #20
Peter Beverloo
Drive-by comments https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/embedded_worker_test_helper.h File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/embedded_worker_test_helper.h#newcode72 content/browser/service_worker/embedded_worker_test_helper.h:72: const std::vector<Event>& dispatched_events() const { return events_; ...
3 years, 10 months ago (2017-02-14 15:48:13 UTC) #25
shimazu
Thanks for updating! Added some comments. https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/embedded_worker_test_helper.h File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/embedded_worker_test_helper.h#newcode234 content/browser/service_worker/embedded_worker_test_helper.h:234: std::vector<Event> events_; On ...
3 years, 10 months ago (2017-02-15 01:29:02 UTC) #26
xiaofengzhang
https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/embedded_worker_test_helper.h File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/embedded_worker_test_helper.h#newcode72 content/browser/service_worker/embedded_worker_test_helper.h:72: const std::vector<Event>& dispatched_events() const { return events_; } On ...
3 years, 10 months ago (2017-02-15 01:59:46 UTC) #27
xiaofengzhang
On 2017/02/15 01:59:46, xiaofengzhang wrote: > https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/embedded_worker_test_helper.h > File content/browser/service_worker/embedded_worker_test_helper.h (right): > > https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/embedded_worker_test_helper.h#newcode72 > ...
3 years, 10 months ago (2017-02-15 02:03:21 UTC) #28
xiaofengzhang
https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/embedded_worker_test_helper.h File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/embedded_worker_test_helper.h#newcode234 content/browser/service_worker/embedded_worker_test_helper.h:234: std::vector<Event> events_; On 2017/02/15 01:59:45, xiaofengzhang wrote: > On ...
3 years, 10 months ago (2017-02-15 02:15:30 UTC) #30
shimazu
https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/embedded_worker_test_helper.h File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/embedded_worker_test_helper.h#newcode234 content/browser/service_worker/embedded_worker_test_helper.h:234: std::vector<Event> events_; On 2017/02/15 02:15:30, xiaofengzhang wrote: > On ...
3 years, 10 months ago (2017-02-15 02:30:12 UTC) #31
xiaofengzhang
https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/embedded_worker_test_helper.h File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/embedded_worker_test_helper.h#newcode234 content/browser/service_worker/embedded_worker_test_helper.h:234: std::vector<Event> events_; On 2017/02/15 02:30:11, shimazu wrote: > On ...
3 years, 10 months ago (2017-02-15 02:39:30 UTC) #32
falken
https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/embedded_worker_test_helper.h File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/embedded_worker_test_helper.h#newcode234 content/browser/service_worker/embedded_worker_test_helper.h:234: std::vector<Event> events_; On 2017/02/15 02:30:11, shimazu wrote: > On ...
3 years, 10 months ago (2017-02-15 03:13:24 UTC) #33
shimazu
lgtm with a nit, thanks! https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/embedded_worker_test_helper.h File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/embedded_worker_test_helper.h#newcode234 content/browser/service_worker/embedded_worker_test_helper.h:234: std::vector<Event> events_; On 2017/02/15 ...
3 years, 10 months ago (2017-02-15 03:14:54 UTC) #34
falken
Hi xiaofengzhang, typically the patch author will upload a new patch incorporating comment feedback and ...
3 years, 10 months ago (2017-02-15 05:11:47 UTC) #35
xiaofengzhang
On 2017/02/15 05:11:47, falken wrote: > Hi xiaofengzhang, typically the patch author will upload a ...
3 years, 10 months ago (2017-02-15 05:18:08 UTC) #36
xiaofengzhang
@falken, @peter, please help review again, thanks https://codereview.chromium.org/2678733002/diff/1/content/browser/service_worker/embedded_worker_test_helper.h File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/1/content/browser/service_worker/embedded_worker_test_helper.h#newcode71 content/browser/service_worker/embedded_worker_test_helper.h:71: const std::vector<Event>& ...
3 years, 10 months ago (2017-02-15 06:14:45 UTC) #37
falken
https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/service_worker_job_unittest.cc File content/browser/service_worker/service_worker_job_unittest.cc (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/service_worker_job_unittest.cc#newcode1470 content/browser/service_worker/service_worker_job_unittest.cc:1470: : SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED, On 2017/02/15 01:29:02, shimazu wrote: > On ...
3 years, 10 months ago (2017-02-15 07:32:03 UTC) #38
shimazu
https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/service_worker_job_unittest.cc File content/browser/service_worker/service_worker_job_unittest.cc (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/service_worker_job_unittest.cc#newcode1470 content/browser/service_worker/service_worker_job_unittest.cc:1470: : SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED, On 2017/02/15 07:32:03, falken wrote: > On ...
3 years, 10 months ago (2017-02-15 08:25:35 UTC) #39
xiaofengzhang
Hi falken please help to review again, thanks :) https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/service_worker_job_unittest.cc File content/browser/service_worker/service_worker_job_unittest.cc (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service_worker/service_worker_job_unittest.cc#newcode1470 content/browser/service_worker/service_worker_job_unittest.cc:1470: ...
3 years, 10 months ago (2017-02-16 06:17:50 UTC) #40
falken
lgtm. thank you
3 years, 10 months ago (2017-02-16 08:55:00 UTC) #41
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/2678733002/80001
3 years, 10 months ago (2017-02-17 01:20:18 UTC) #44
xiaofengzhang
Hi shimazu, I received a email that patchset 4 needs a l-g-t-m from you, thanks.
3 years, 10 months ago (2017-02-17 01:23:36 UTC) #45
commit-bot: I haz the power
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clang_dbg_recipe/builds/213695) android_cronet on master.tryserver.chromium.android (JOB_FAILED, ...
3 years, 10 months ago (2017-02-17 01:27:13 UTC) #47
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/2678733002/100001
3 years, 10 months ago (2017-02-17 02:33:05 UTC) #50
falken
On 2017/02/17 01:23:36, xiaofengzhang wrote: > Hi shimazu, > > I received a email that ...
3 years, 10 months ago (2017-02-17 02:47:29 UTC) #51
xiaofengzhang
On 2017/02/17 02:47:29, falken wrote: > On 2017/02/17 01:23:36, xiaofengzhang wrote: > > Hi shimazu, ...
3 years, 10 months ago (2017-02-17 02:50:02 UTC) #52
commit-bot: I haz the power
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on ...
3 years, 10 months ago (2017-02-17 04:36:26 UTC) #54
xiaofengzhang
Hi, Tom, would you PTAL for OWNER review of mojom/typemap files? Thanks.
3 years, 10 months ago (2017-02-17 06:59:12 UTC) #58
Tom Sepez
mojom LGTM
3 years, 10 months ago (2017-02-17 19:35:43 UTC) #61
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/2678733002/100001
3 years, 10 months ago (2017-02-19 03:05:25 UTC) #63
commit-bot: I haz the power
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/64975bc58e6b041ade1cfe2edb431896548845f3
3 years, 10 months ago (2017-02-19 04:59:29 UTC) #66
shrike
3 years, 9 months ago (2017-03-02 19:46:00 UTC) #67
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:100001) has been created in
https://codereview.chromium.org/2724243003/ by shrike@chromium.org.

The reason for reverting is: Checking for power use regressions..

Powered by Google App Engine
This is Rietveld 408576698