|
|
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)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
xiaofeng.zhang@intel.com changed reviewers: + shimazu@chromium.org
shimazu@ Could you please review this? Thanks.
Thanks for continuing to work on this:) Overall looks good. https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... content/browser/service_worker/embedded_worker_test_helper.h:67: enum class Event { OnActivateEvent, LastEvent }; How about "enum class EventType { Activate, Last };"? https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... content/browser/service_worker/embedded_worker_test_helper.h:71: const std::vector<Event>& events() const { return events_; } dispatched_events() might be clearer. https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_browsertest.cc (right): https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_browsertest.cc:792: &ServiceWorkerVersion::OnSimpleEventFinished, version_, request_id)); base::Unretained(version_.get()) https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_registration.cc (right): https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_registration.cc:432: activating_version, request_id)); Could you use base::Unretained(activating_version.get()) instead of passing scoped_refptr<SWVersion> to avoid reference loop (version -> dispatcher -> callback -> version)
s/ActiviteEvent/ActivateEvent/ in the subject of this CL.
falken@chromium.org changed reviewers: + falken@chromium.org
https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... content/browser/service_worker/embedded_worker_test_helper.h:67: enum class Event { OnActivateEvent, LastEvent }; On 2017/02/07 04:43:47, shimazu wrote: > How about "enum class EventType { Activate, Last };"? nit: "LastEvent" doesn't seem used in this patch, so maybe you can just remove it. Otherwise "NumTypes" is clearer than "Last". A lot of places in the codebase use Last to refer to the last type, not the number of types, e.g.: enum { A, B, C, LAST = C };
Description was changed from ========== [ServiceWorker] Mojofy ActiviteEvent of Service Worker This patch mojofies the following two messages: ServiceWorkerMsg_ActivateEvent ServiceWorkerHostMsg_ActivateEventFinished BUG=629701 TEST=content_unittests ========== to ========== [ServiceWorker] Mojofy ActivateEvent of Service Worker This patch mojofies the following two messages: ServiceWorkerMsg_ActivateEvent ServiceWorkerHostMsg_ActivateEventFinished BUG=629701 TEST=content_unittests ==========
Description was changed from ========== [ServiceWorker] Mojofy ActivateEvent of Service Worker This patch mojofies the following two messages: ServiceWorkerMsg_ActivateEvent ServiceWorkerHostMsg_ActivateEventFinished BUG=629701 TEST=content_unittests ========== to ========== [ServiceWorker] Mojofy ActivateEvent of Service Worker This patch mojofies the following two messages: ServiceWorkerMsg_ActivateEvent ServiceWorkerHostMsg_ActivateEventFinished BUG=629701 TEST=content_unittests ==========
On 2017/02/07 04:44:31, shimazu wrote: > s/ActiviteEvent/ActivateEvent/ in the subject of this CL. Thanks, done :)
https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... content/browser/service_worker/embedded_worker_test_helper.h:67: enum class Event { OnActivateEvent, LastEvent }; On 2017/02/07 05:19:35, falken wrote: > On 2017/02/07 04:43:47, shimazu wrote: > > How about "enum class EventType { Activate, Last };"? > > nit: "LastEvent" doesn't seem used in this patch, so maybe you can just remove > it. Otherwise "NumTypes" is clearer than "Last". A lot of places in the > codebase use Last to refer to the last type, not the number of types, e.g.: > enum { A, B, C, LAST = C }; Thanks, I will remove it in this cl, InstallEvent will be added in another one. https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... content/browser/service_worker/embedded_worker_test_helper.h:71: const std::vector<Event>& events() const { return events_; } On 2017/02/07 04:43:48, shimazu wrote: > dispatched_events() might be clearer. Acknowledged. https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_registration.cc (right): https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_registration.cc:432: activating_version, request_id)); On 2017/02/07 04:43:48, shimazu wrote: > Could you use base::Unretained(activating_version.get()) instead of passing > scoped_refptr<SWVersion> to avoid reference loop (version -> dispatcher -> > callback -> version) Sure. But I am a little confused that other places in this file always use activating_version directly, such as line 426 and 342.., and in my previous patch(https://codereview.chromium.org/2569993002/), Peter comments that I should pass it directly for it is the scoped_refptr.
shimazu@chromium.org changed reviewers: + peter@chromium.org
+peter for asking a question about his comment on the previous CL. https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_registration.cc (right): https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_registration.cc:432: activating_version, request_id)); On 2017/02/07 06:15:36, xiaofengzhang wrote: > On 2017/02/07 04:43:48, shimazu wrote: > > Could you use base::Unretained(activating_version.get()) instead of passing > > scoped_refptr<SWVersion> to avoid reference loop (version -> dispatcher -> > > callback -> version) > > Sure. But I am a little confused that other places in this file always use > activating_version directly, such as line 426 and 342.., and in my previous > patch(https://codereview.chromium.org/2569993002/), Peter comments that I should > pass it directly for it is the scoped_refptr. Oops, I didn't realize that. I think base::Unretained(activating_version.get()) is better as I mentioned. (that might be confusing a little, though.) peter@: WDYT?
https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_registration.cc (right): https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_registration.cc:432: activating_version, request_id)); On 2017/02/07 07:19:31, shimazu wrote: > On 2017/02/07 06:15:36, xiaofengzhang wrote: > > On 2017/02/07 04:43:48, shimazu wrote: > > > Could you use base::Unretained(activating_version.get()) instead of passing > > > scoped_refptr<SWVersion> to avoid reference loop (version -> dispatcher -> > > > callback -> version) > > > > Sure. But I am a little confused that other places in this file always use > > activating_version directly, such as line 426 and 342.., and in my previous > > patch(https://codereview.chromium.org/2569993002/), Peter comments that I > should > > pass it directly for it is the scoped_refptr. > > Oops, I didn't realize that. I think base::Unretained(activating_version.get()) > is better as I mentioned. (that might be confusing a little, though.) > peter@: WDYT? That's great context - thanks shimazu! It would be very helpful if the Dispatch*Event methods could be annotated with a comment that says it's safe to use callbacks pointing to an (active) version w/ base::Unretained(), and why. Generally it's a bit scary seeing base::Unretained() in the code.
https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_registration.cc (right): https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_registration.cc:432: activating_version, request_id)); On 2017/02/08 15:13:16, Peter Beverloo wrote: > On 2017/02/07 07:19:31, shimazu wrote: > > On 2017/02/07 06:15:36, xiaofengzhang wrote: > > > On 2017/02/07 04:43:48, shimazu wrote: > > > > Could you use base::Unretained(activating_version.get()) instead of > passing > > > > scoped_refptr<SWVersion> to avoid reference loop (version -> dispatcher -> > > > > callback -> version) > > > > > > Sure. But I am a little confused that other places in this file always use > > > activating_version directly, such as line 426 and 342.., and in my previous > > > patch(https://codereview.chromium.org/2569993002/), Peter comments that I > > should > > > pass it directly for it is the scoped_refptr. > > > > Oops, I didn't realize that. I think > base::Unretained(activating_version.get()) > > is better as I mentioned. (that might be confusing a little, though.) > > peter@: WDYT? > > That's great context - thanks shimazu! > > It would be very helpful if the Dispatch*Event methods could be annotated with a > comment that says it's safe to use callbacks pointing to an (active) version w/ > base::Unretained(), and why. > > Generally it's a bit scary seeing base::Unretained() in the code. Yeah, I also feel it's scary. How about having a helper method to create a callback to call OnSimpleEventFinished with base::Unretained instead of using base::Unretained explicitly? == // Creates a callback passed to event_dispatcher(). StatusCallback ServiceWorkerVersion::CreateSimpleEventCallback(int request_id) { // base::Unretained(this) should be used here otherwise // version->event_dispatcher()->DispatchSomeEvent(callback) would have a // reference loop. return base::Bind(&ServiceWorkerVersion::OnSimpleEventFinished, base::Unretained(this), request_id); } ==
On 2017/02/10 05:42:27, shimazu wrote: > https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... > File content/browser/service_worker/service_worker_registration.cc (right): > > https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... > content/browser/service_worker/service_worker_registration.cc:432: > activating_version, request_id)); > On 2017/02/08 15:13:16, Peter Beverloo wrote: > > On 2017/02/07 07:19:31, shimazu wrote: > > > On 2017/02/07 06:15:36, xiaofengzhang wrote: > > > > On 2017/02/07 04:43:48, shimazu wrote: > > > > > Could you use base::Unretained(activating_version.get()) instead of > > passing > > > > > scoped_refptr<SWVersion> to avoid reference loop (version -> dispatcher > -> > > > > > callback -> version) > > > > > > > > Sure. But I am a little confused that other places in this file always use > > > > activating_version directly, such as line 426 and 342.., and in my > previous > > > > patch(https://codereview.chromium.org/2569993002/), Peter comments that I > > > should > > > > pass it directly for it is the scoped_refptr. > > > > > > Oops, I didn't realize that. I think > > base::Unretained(activating_version.get()) > > > is better as I mentioned. (that might be confusing a little, though.) > > > peter@: WDYT? > > > > That's great context - thanks shimazu! > > > > It would be very helpful if the Dispatch*Event methods could be annotated with > a > > comment that says it's safe to use callbacks pointing to an (active) version > w/ > > base::Unretained(), and why. > > > > Generally it's a bit scary seeing base::Unretained() in the code. > > Yeah, I also feel it's scary. > How about having a helper method to create a callback to call > OnSimpleEventFinished with base::Unretained instead of using base::Unretained > explicitly? > > == > // Creates a callback passed to event_dispatcher(). > StatusCallback > ServiceWorkerVersion::CreateSimpleEventCallback(int request_id) { > // base::Unretained(this) should be used here otherwise > // version->event_dispatcher()->DispatchSomeEvent(callback) would have a > // reference loop. > return base::Bind(&ServiceWorkerVersion::OnSimpleEventFinished, > base::Unretained(this), request_id); > } > == Agreed! Sent you a CL here: https://codereview.chromium.org/2689693003
On 2017/02/10 17:40:53, Peter Beverloo wrote: > On 2017/02/10 05:42:27, shimazu wrote: > > > https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... > > File content/browser/service_worker/service_worker_registration.cc (right): > > > > > https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... > > content/browser/service_worker/service_worker_registration.cc:432: > > activating_version, request_id)); > > On 2017/02/08 15:13:16, Peter Beverloo wrote: > > > On 2017/02/07 07:19:31, shimazu wrote: > > > > On 2017/02/07 06:15:36, xiaofengzhang wrote: > > > > > On 2017/02/07 04:43:48, shimazu wrote: > > > > > > Could you use base::Unretained(activating_version.get()) instead of > > > passing > > > > > > scoped_refptr<SWVersion> to avoid reference loop (version -> > dispatcher > > -> > > > > > > callback -> version) > > > > > > > > > > Sure. But I am a little confused that other places in this file always > use > > > > > activating_version directly, such as line 426 and 342.., and in my > > previous > > > > > patch(https://codereview.chromium.org/2569993002/), Peter comments that > I > > > > should > > > > > pass it directly for it is the scoped_refptr. > > > > > > > > Oops, I didn't realize that. I think > > > base::Unretained(activating_version.get()) > > > > is better as I mentioned. (that might be confusing a little, though.) > > > > peter@: WDYT? > > > > > > That's great context - thanks shimazu! > > > > > > It would be very helpful if the Dispatch*Event methods could be annotated > with > > a > > > comment that says it's safe to use callbacks pointing to an (active) version > > w/ > > > base::Unretained(), and why. > > > > > > Generally it's a bit scary seeing base::Unretained() in the code. > > > > Yeah, I also feel it's scary. > > How about having a helper method to create a callback to call > > OnSimpleEventFinished with base::Unretained instead of using base::Unretained > > explicitly? > > > > == > > // Creates a callback passed to event_dispatcher(). > > StatusCallback > > ServiceWorkerVersion::CreateSimpleEventCallback(int request_id) { > > // base::Unretained(this) should be used here otherwise > > // version->event_dispatcher()->DispatchSomeEvent(callback) would have a > > // reference loop. > > return base::Bind(&ServiceWorkerVersion::OnSimpleEventFinished, > > base::Unretained(this), request_id); > > } > > == > > Agreed! Sent you a CL here: > https://codereview.chromium.org/2689693003 Thanks! shimazu@, falken@, peter@, could you help to review again?
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Drive-by comments https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/embedded_worker_test_helper.h:72: const std::vector<Event>& dispatched_events() const { return events_; } nit: this should be next to the other getters around line 132 https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/embedded_worker_test_helper.h:234: std::vector<Event> events_; This has to be with the other members, probably after line 316. https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_context_unittest.cc:226: EXPECT_EQ(EmbeddedWorkerTestHelper::Event::Activate, Add: ASSERT_EQ(1UL, helper_->dispatched_events().size()); To make sure you're not accessing an invalid index on line 227. https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_context_unittest.cc:331: EXPECT_EQ(EmbeddedWorkerTestHelper::Event::Activate, dito re: the ASSERT_EQ https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_job_unittest.cc (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_job_unittest.cc:1470: : SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED, shimazu@ - it may make sense to generalize EventResultToStatus() somewhere. There's lots of code doing exactly the same conversions, often without checking for invalid values.
Thanks for updating! Added some comments. https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/embedded_worker_test_helper.h:234: std::vector<Event> events_; On 2017/02/14 15:48:13, Peter Beverloo wrote: > This has to be with the other members, probably after line 316. +1 for moving; this should be private. https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_context_unittest.cc:275: EXPECT_EQ(1UL, helper_->ipc_sink()->message_count()); Could you add a check if ActivateEvent didn't dispatched here like the following? -- EXPECT_EQ(0UL, helper_->dispatched_events.size()); https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_job_unittest.cc (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_job_unittest.cc:1470: : SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED, On 2017/02/14 15:48:13, Peter Beverloo wrote: > shimazu@ - it may make sense to generalize EventResultToStatus() somewhere. > There's lots of code doing exactly the same conversions, often without checking > for invalid values. I agree that we have too many duplicated codes for the conversion.. I feel great if we can remove the usage of SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED and SERVICE_WORKER_OK and use blink::WebServiceWorkerEventResult instead. Now we can use the public blink type in content/browser/. It should be in the separated patch anyway. I think this patch is good as is. Thanks for pointing it out! :)
https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/embedded_worker_test_helper.h:72: const std::vector<Event>& dispatched_events() const { return events_; } On 2017/02/14 15:48:13, Peter Beverloo wrote: > nit: this should be next to the other getters around line 132 Acknowledged. https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/embedded_worker_test_helper.h:234: std::vector<Event> events_; On 2017/02/15 01:29:02, shimazu wrote: > On 2017/02/14 15:48:13, Peter Beverloo wrote: > > This has to be with the other members, probably after line 316. > > +1 for moving; this should be private. Acknowledged. https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_context_unittest.cc:226: EXPECT_EQ(EmbeddedWorkerTestHelper::Event::Activate, On 2017/02/14 15:48:13, Peter Beverloo wrote: > Add: > ASSERT_EQ(1UL, helper_->dispatched_events().size()); > > To make sure you're not accessing an invalid index on line 227. Acknowledged. https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_context_unittest.cc:275: EXPECT_EQ(1UL, helper_->ipc_sink()->message_count()); On 2017/02/15 01:29:02, shimazu wrote: > Could you add a check if ActivateEvent didn't dispatched here like the > following? > -- > EXPECT_EQ(0UL, helper_->dispatched_events.size()); Acknowledged. https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_job_unittest.cc (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... 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 2017/02/14 15:48:13, Peter Beverloo wrote: > > shimazu@ - it may make sense to generalize EventResultToStatus() somewhere. > > There's lots of code doing exactly the same conversions, often without > checking > > for invalid values. > > I agree that we have too many duplicated codes for the conversion.. I feel great > if we can remove the usage of SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED and > SERVICE_WORKER_OK and use blink::WebServiceWorkerEventResult instead. Now we can > use the public blink type in content/browser/. > It should be in the separated patch anyway. I think this patch is good as is. > > Thanks for pointing it out! :) thanks a lot for both comments. :-)
On 2017/02/15 01:59:46, xiaofengzhang wrote: > https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... > File content/browser/service_worker/embedded_worker_test_helper.h (right): > > https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... > content/browser/service_worker/embedded_worker_test_helper.h:72: const > std::vector<Event>& dispatched_events() const { return events_; } > On 2017/02/14 15:48:13, Peter Beverloo wrote: > > nit: this should be next to the other getters around line 132 > > Acknowledged. > > https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... > content/browser/service_worker/embedded_worker_test_helper.h:234: > std::vector<Event> events_; > On 2017/02/15 01:29:02, shimazu wrote: > > On 2017/02/14 15:48:13, Peter Beverloo wrote: > > > This has to be with the other members, probably after line 316. > > > > +1 for moving; this should be private. > > Acknowledged. > > https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... > File content/browser/service_worker/service_worker_context_unittest.cc (right): > > https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... > content/browser/service_worker/service_worker_context_unittest.cc:226: > EXPECT_EQ(EmbeddedWorkerTestHelper::Event::Activate, > On 2017/02/14 15:48:13, Peter Beverloo wrote: > > Add: > > ASSERT_EQ(1UL, helper_->dispatched_events().size()); > > > > To make sure you're not accessing an invalid index on line 227. > > Acknowledged. > > https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... > content/browser/service_worker/service_worker_context_unittest.cc:275: > EXPECT_EQ(1UL, helper_->ipc_sink()->message_count()); > On 2017/02/15 01:29:02, shimazu wrote: > > Could you add a check if ActivateEvent didn't dispatched here like the > > following? > > -- > > EXPECT_EQ(0UL, helper_->dispatched_events.size()); > > Acknowledged. > > https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... > File content/browser/service_worker/service_worker_job_unittest.cc (right): > > https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... > 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 2017/02/14 15:48:13, Peter Beverloo wrote: > > > shimazu@ - it may make sense to generalize EventResultToStatus() somewhere. > > > There's lots of code doing exactly the same conversions, often without > > checking > > > for invalid values. > > > > I agree that we have too many duplicated codes for the conversion.. I feel > great > > if we can remove the usage of SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED > and > > SERVICE_WORKER_OK and use blink::WebServiceWorkerEventResult instead. Now we > can > > use the public blink type in content/browser/. > > It should be in the separated patch anyway. I think this patch is good as is. > > > > Thanks for pointing it out! :) > > thanks a lot for both comments. :-) @shimazu, @peter, please help to review again, thanks :-)
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/embedded_worker_test_helper.h:234: std::vector<Event> events_; On 2017/02/15 01:59:45, xiaofengzhang wrote: > On 2017/02/15 01:29:02, shimazu wrote: > > On 2017/02/14 15:48:13, Peter Beverloo wrote: > > > This has to be with the other members, probably after line 316. > > > > +1 for moving; this should be private. > > Acknowledged. sorry, it should be not private, because it will be used in line 103 of service_worker_context_unittest.cc
https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/embedded_worker_test_helper.h:234: std::vector<Event> events_; On 2017/02/15 02:15:30, xiaofengzhang wrote: > On 2017/02/15 01:59:45, xiaofengzhang wrote: > > On 2017/02/15 01:29:02, shimazu wrote: > > > On 2017/02/14 15:48:13, Peter Beverloo wrote: > > > > This has to be with the other members, probably after line 316. > > > > > > +1 for moving; this should be private. > > > > Acknowledged. > > sorry, it should be not private, because it will be used in line 103 of > service_worker_context_unittest.cc Ah, I see. How about protected for |events_| instead?
https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/embedded_worker_test_helper.h:234: std::vector<Event> events_; On 2017/02/15 02:30:11, shimazu wrote: > On 2017/02/15 02:15:30, xiaofengzhang wrote: > > On 2017/02/15 01:59:45, xiaofengzhang wrote: > > > On 2017/02/15 01:29:02, shimazu wrote: > > > > On 2017/02/14 15:48:13, Peter Beverloo wrote: > > > > > This has to be with the other members, probably after line 316. > > > > > > > > +1 for moving; this should be private. > > > > > > Acknowledged. > > > > sorry, it should be not private, because it will be used in line 103 of > > service_worker_context_unittest.cc > > Ah, I see. How about protected for |events_| instead? yeah, here is already protected, do you think ok? :-)
https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/embedded_worker_test_helper.h:234: std::vector<Event> events_; On 2017/02/15 02:30:11, shimazu wrote: > On 2017/02/15 02:15:30, xiaofengzhang wrote: > > On 2017/02/15 01:59:45, xiaofengzhang wrote: > > > On 2017/02/15 01:29:02, shimazu wrote: > > > > On 2017/02/14 15:48:13, Peter Beverloo wrote: > > > > > This has to be with the other members, probably after line 316. > > > > > > > > +1 for moving; this should be private. > > > > > > Acknowledged. > > > > sorry, it should be not private, because it will be used in line 103 of > > service_worker_context_unittest.cc > > Ah, I see. How about protected for |events_| instead? Technically by the style guide this must be private: https://google.github.io/styleguide/cppguide.html#Access_Control You can add a public or protected method for access to events, e.g., AddEvent. Or if we really need access just return a pointer to events.
lgtm with a nit, thanks! https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/embedded_worker_test_helper.h:234: std::vector<Event> events_; On 2017/02/15 02:39:30, xiaofengzhang wrote: > On 2017/02/15 02:30:11, shimazu wrote: > > On 2017/02/15 02:15:30, xiaofengzhang wrote: > > > On 2017/02/15 01:59:45, xiaofengzhang wrote: > > > > On 2017/02/15 01:29:02, shimazu wrote: > > > > > On 2017/02/14 15:48:13, Peter Beverloo wrote: > > > > > > This has to be with the other members, probably after line 316. > > > > > > > > > > +1 for moving; this should be private. > > > > > > > > Acknowledged. > > > > > > sorry, it should be not private, because it will be used in line 103 of > > > service_worker_context_unittest.cc > > > > Ah, I see. How about protected for |events_| instead? > > yeah, here is already protected, do you think ok? :-) Oops, sorry. Yes, it's good:) nit: could you add a break line before this member variable?
Hi xiaofengzhang, typically the patch author will upload a new patch incorporating comment feedback and reply "Done" to the comments. Then reviewers know when it's ready to look at again. "Acknowledged" just means the author has read the review feedback, but it does not make a statement about whether the feedback will be incorporated.
On 2017/02/15 05:11:47, falken wrote: > Hi xiaofengzhang, typically the patch author will upload a new patch > incorporating comment feedback and reply "Done" to the comments. > > Then reviewers know when it's ready to look at again. > > "Acknowledged" just means the author has read the review feedback, but it does > not make a statement about whether the feedback will be incorporated. Hi falken, Thanks a lot for your useful reminding! :-) I will upload a new patch later.
@falken, @peter, please help review again, thanks https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/1/content/browser/service_wor... content/browser/service_worker/embedded_worker_test_helper.h:71: const std::vector<Event>& events() const { return events_; } On 2017/02/07 04:43:48, shimazu wrote: > dispatched_events() might be clearer. Done. https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/embedded_worker_test_helper.h:72: const std::vector<Event>& dispatched_events() const { return events_; } On 2017/02/14 15:48:13, Peter Beverloo wrote: > nit: this should be next to the other getters around line 132 Done. https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/embedded_worker_test_helper.h:234: std::vector<Event> events_; On 2017/02/15 03:13:24, falken wrote: > On 2017/02/15 02:30:11, shimazu wrote: > > On 2017/02/15 02:15:30, xiaofengzhang wrote: > > > On 2017/02/15 01:59:45, xiaofengzhang wrote: > > > > On 2017/02/15 01:29:02, shimazu wrote: > > > > > On 2017/02/14 15:48:13, Peter Beverloo wrote: > > > > > > This has to be with the other members, probably after line 316. > > > > > > > > > > +1 for moving; this should be private. > > > > > > > > Acknowledged. > > > > > > sorry, it should be not private, because it will be used in line 103 of > > > service_worker_context_unittest.cc > > > > Ah, I see. How about protected for |events_| instead? > > Technically by the style guide this must be private: > https://google.github.io/styleguide/cppguide.html#Access_Control > > You can add a public or protected method for access to events, e.g., AddEvent. > Or if we really need access just return a pointer to events. Done. https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_context_unittest.cc:226: EXPECT_EQ(EmbeddedWorkerTestHelper::Event::Activate, On 2017/02/14 15:48:13, Peter Beverloo wrote: > Add: > ASSERT_EQ(1UL, helper_->dispatched_events().size()); > > To make sure you're not accessing an invalid index on line 227. Done. https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_context_unittest.cc:275: EXPECT_EQ(1UL, helper_->ipc_sink()->message_count()); On 2017/02/15 01:29:02, shimazu wrote: > Could you add a check if ActivateEvent didn't dispatched here like the > following? > -- > EXPECT_EQ(0UL, helper_->dispatched_events.size()); Done. https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_context_unittest.cc:331: EXPECT_EQ(EmbeddedWorkerTestHelper::Event::Activate, On 2017/02/14 15:48:13, Peter Beverloo wrote: > dito re: the ASSERT_EQ Done.
https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_job_unittest.cc (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... 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 2017/02/14 15:48:13, Peter Beverloo wrote: > > shimazu@ - it may make sense to generalize EventResultToStatus() somewhere. > > There's lots of code doing exactly the same conversions, often without > checking > > for invalid values. > > I agree that we have too many duplicated codes for the conversion.. I feel great > if we can remove the usage of SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED and > SERVICE_WORKER_OK and use blink::WebServiceWorkerEventResult instead. Now we can > use the public blink type in content/browser/. > It should be in the separated patch anyway. I think this patch is good as is. > > Thanks for pointing it out! :) Are you sure this is possible? Don't callbacks to install and activate expect to get more status codes like ERROR_NETWORK or ERROR_TIMEOUT? I think we should write cleanly now with what we have and not expect future code cleanup to necessarily happen. https://codereview.chromium.org/2678733002/diff/60001/content/browser/service... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/60001/content/browser/service... content/browser/service_worker/embedded_worker_test_helper.h:131: std::vector<Event>* events_ptr() { return &events_; } Having both a const& and ptr is a bit unusual and the naming is inconsistent. I suggest we just return std::vector<Event>* and call it dispatched_events. https://codereview.chromium.org/2678733002/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_job_unittest.cc (right): https://codereview.chromium.org/2678733002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_job_unittest.cc:1471: base::Time::Now()); I think this should reuse or duplicate EventResultToStatus. I think duplication is fine.
https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_job_unittest.cc (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... 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 2017/02/15 01:29:02, shimazu wrote: > > On 2017/02/14 15:48:13, Peter Beverloo wrote: > > > shimazu@ - it may make sense to generalize EventResultToStatus() somewhere. > > > There's lots of code doing exactly the same conversions, often without > > checking > > > for invalid values. > > > > I agree that we have too many duplicated codes for the conversion.. I feel > great > > if we can remove the usage of SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED > and > > SERVICE_WORKER_OK and use blink::WebServiceWorkerEventResult instead. Now we > can > > use the public blink type in content/browser/. > > It should be in the separated patch anyway. I think this patch is good as is. > > > > Thanks for pointing it out! :) > > Are you sure this is possible? Don't callbacks to install and activate expect to > get more status codes like ERROR_NETWORK or ERROR_TIMEOUT? > > I think we should write cleanly now with what we have and not expect future code > cleanup to necessarily happen. I see. I simply assumed that it's possible because all of "SimpleEvent" take only OK/REJECTED, but that seems a bit complex (and I've recalled now that I changed it). EventResultToStatus is basically used only in SWContextCore except for this unit test and callbacks for the legacy IPC, so I think it's okay to have EventResultToStatus in this file too.
Hi falken please help to review again, thanks :) https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_job_unittest.cc (right): https://codereview.chromium.org/2678733002/diff/20001/content/browser/service... 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 2017/02/15 01:29:02, shimazu wrote: > > On 2017/02/14 15:48:13, Peter Beverloo wrote: > > > shimazu@ - it may make sense to generalize EventResultToStatus() somewhere. > > > There's lots of code doing exactly the same conversions, often without > > checking > > > for invalid values. > > > > I agree that we have too many duplicated codes for the conversion.. I feel > great > > if we can remove the usage of SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED > and > > SERVICE_WORKER_OK and use blink::WebServiceWorkerEventResult instead. Now we > can > > use the public blink type in content/browser/. > > It should be in the separated patch anyway. I think this patch is good as is. > > > > Thanks for pointing it out! :) > > Are you sure this is possible? Don't callbacks to install and activate expect to > get more status codes like ERROR_NETWORK or ERROR_TIMEOUT? > > I think we should write cleanly now with what we have and not expect future code > cleanup to necessarily happen. Done. https://codereview.chromium.org/2678733002/diff/60001/content/browser/service... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2678733002/diff/60001/content/browser/service... content/browser/service_worker/embedded_worker_test_helper.h:131: std::vector<Event>* events_ptr() { return &events_; } On 2017/02/15 07:32:03, falken wrote: > Having both a const& and ptr is a bit unusual and the naming is inconsistent. I > suggest we just return std::vector<Event>* and call it dispatched_events. Done.
lgtm. thank you
The CQ bit was checked by xiaofeng.zhang@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from shimazu@chromium.org Link to the patchset: https://codereview.chromium.org/2678733002/#ps80001 (title: "Address falken's comments #38")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi shimazu, I received a email that patchset 4 needs a l-g-t-m from you, thanks.
The CQ bit was unchecked by commit-bot@chromium.org
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_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xiaofeng.zhang@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, shimazu@chromium.org Link to the patchset: https://codereview.chromium.org/2678733002/#ps100001 (title: "Just rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/17 01:23:36, xiaofengzhang wrote: > Hi shimazu, > > I received a email that patchset 4 needs a l-g-t-m from you, thanks. Thanks for being careful. If you're talking about the automated emails like "The patchset sent to the CQ was uploaded after l-g-t-m", that's just a warning to the reviewer. An l-g-t-m on a codereview remains an l-g-t-m unless explicitly revoked by the reviewer (via "not l-g-t-m"). However, it's good practice to ask a reviewer to re-review if substantial changes have indeed been made since their review. For this CL, I don't think substantial changes have been made, so it's fine to commit as is.
On 2017/02/17 02:47:29, falken wrote: > On 2017/02/17 01:23:36, xiaofengzhang wrote: > > Hi shimazu, > > > > I received a email that patchset 4 needs a l-g-t-m from you, thanks. > > Thanks for being careful. If you're talking about the automated emails like "The > patchset sent to the CQ was uploaded after l-g-t-m", that's just a warning to > the reviewer. An l-g-t-m on a codereview remains an l-g-t-m unless explicitly > revoked by the reviewer (via "not l-g-t-m"). However, it's good practice to ask > a reviewer to re-review if substantial changes have indeed been made since their > review. For this CL, I don't think substantial changes have been made, so it's > fine to commit as is. Ok, understood, thanks a lot for your nice and detailed explanation :-)
The CQ bit was unchecked by commit-bot@chromium.org
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 master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xiaofeng.zhang@intel.com changed reviewers: + tsepez@chromium.org
Hi, Tom, would you PTAL for OWNER review of mojom/typemap files? Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mojom LGTM
The CQ bit was checked by xiaofeng.zhang@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487473516637070, "parent_rev": "5418d62d2fd3dc113874faa6f8b1417ba5257f30", "commit_rev": "64975bc58e6b041ade1cfe2edb431896548845f3"}
Message was sent while issue was closed.
Description was changed from ========== [ServiceWorker] Mojofy ActivateEvent of Service Worker This patch mojofies the following two messages: ServiceWorkerMsg_ActivateEvent ServiceWorkerHostMsg_ActivateEventFinished BUG=629701 TEST=content_unittests ========== to ========== [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/+/64975bc58e6b041ade1cfe2edb43... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/64975bc58e6b041ade1cfe2edb43...
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.. |