Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2787883003:
[ServiceWorker] Add EmbeddedWorkerInstanceHost Interface. (Closed)
|
Created:
3 years, 8 months ago by leonhsl(Using Gerrit) Modified:
3 years, 8 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/heads/master Project:
chromium Visibility:
Public. |
Description[ServiceWorker] Add EmbeddedWorkerInstanceHost Interface.
This CL converts these IPCs
EmbeddedWorkerHostMsg_WorkerReadyForInspection
EmbeddedWorkerHostMsg_WorkerScriptLoaded
EmbeddedWorkerHostMsg_WorkerScriptLoadFailed
EmbeddedWorkerHostMsg_WorkerScriptEvaluated
EmbeddedWorkerHostMsg_WorkerStarted
EmbeddedWorkerHostMsg_WorkerStopped
EmbeddedWorkerHostMsg_ReportException
EmbeddedWorkerHostMsg_ReportConsoleMessage
EmbeddedWorkerHostMsg_WorkerThreadStarted
into mojo interface EmbeddedWorkerInstanceHost, and associate it with
existing interface EmbeddedWorkerInstanceClient.
BUG=629701
TEST=content_unittests
Review-Url: https://codereview.chromium.org/2787883003
Cr-Commit-Position: refs/heads/master@{#464878}
Committed: https://chromium.googlesource.com/chromium/src/+/7a57143828b914abad790775f41ea97255ae9c11
Patch Set 1 #
Total comments: 5
Patch Set 2 #
Total comments: 6
Patch Set 3 : Fix test: ServiceWorkerContextTest.UnregisterMultiple #
Total comments: 22
Patch Set 4 : Address comments from shimazu@ #
Total comments: 10
Patch Set 5 : Rebase only #Patch Set 6 : Remove EmbeddedWorkerDispatcher #
Total comments: 4
Patch Set 7 : Address comments from shimazu@ #
Total comments: 12
Patch Set 8 : Address comments from falken@ #
Total comments: 14
Patch Set 9 : Refine code comments #
Total comments: 31
Patch Set 10 : Refine code comments #Messages
Total messages: 84 (50 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 checked by leon.han@intel.com to run a CQ dry run
Description was changed from ========== [ServiceWorker] Add EmbeddedWorkerInstanceHost Interface. This CL converts these IPCs EmbeddedWorkerHostMsg_WorkerReadyForInspection EmbeddedWorkerHostMsg_WorkerScriptLoaded EmbeddedWorkerHostMsg_WorkerScriptLoadFailed EmbeddedWorkerHostMsg_WorkerScriptEvaluated EmbeddedWorkerHostMsg_WorkerStarted EmbeddedWorkerHostMsg_WorkerStopped EmbeddedWorkerHostMsg_ReportException EmbeddedWorkerHostMsg_ReportConsoleMessage EmbeddedWorkerHostMsg_WorkerThreadStarted into mojo interface EmbeddedWorkerInstanceHost, and associate it with existing interface EmbeddedWorkerInstanceClient. BUG=629701 TEST=content_unittests [ServiceWorker] Remove useless ServiceWorkerVersion::DispatchSimpleEvent(). As all simple events have been converted into mojo interface ServiceWorkerEventDispatcher, now there are no codes using ServiceWorkerVersion::DispatchSimpleEvent(), so this CL removes it completely. No other code behavior change. BUG=629701 ========== to ========== [ServiceWorker] Add EmbeddedWorkerInstanceHost Interface. This CL converts these IPCs EmbeddedWorkerHostMsg_WorkerReadyForInspection EmbeddedWorkerHostMsg_WorkerScriptLoaded EmbeddedWorkerHostMsg_WorkerScriptLoadFailed EmbeddedWorkerHostMsg_WorkerScriptEvaluated EmbeddedWorkerHostMsg_WorkerStarted EmbeddedWorkerHostMsg_WorkerStopped EmbeddedWorkerHostMsg_ReportException EmbeddedWorkerHostMsg_ReportConsoleMessage EmbeddedWorkerHostMsg_WorkerThreadStarted into mojo interface EmbeddedWorkerInstanceHost, and associate it with existing interface EmbeddedWorkerInstanceClient. BUG=629701 TEST=content_unittests ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
leon.han@intel.com changed reviewers: + falken@chromium.org, shimazu@chromium.org
Ps#1 adds the EmbeddedWorkerInstanceHost interface and converts only one IPC OnReadyForInspection, I'm planning to convert all the 9 IPCs together in this CL, but I want to send ps#1 out to you for an initial review then I'll proceed to complete if there has no any big misses, Thanks! https://codereview.chromium.org/2787883003/diff/20001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2787883003/diff/20001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:574: instance_host_->OnReadyForInspection(); Some IPCs like OnScriptEvaluated are triggered on worker thread, as |instance_host_| is bound on main thread, I'll post a task to main thread to call 'instance_host_->OnScriptEvaluated().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for working on that! Overall looks good. I put a couple of comments. https://codereview.chromium.org/2787883003/diff/20001/content/common/service_... File content/common/service_worker/embedded_worker.mojom (right): https://codereview.chromium.org/2787883003/diff/20001/content/common/service_... content/common/service_worker/embedded_worker.mojom:26: // Interface to control a browser-side embedded worker instance. Please add a comment about the ordering. https://codereview.chromium.org/2787883003/diff/20001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2787883003/diff/20001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:574: instance_host_->OnReadyForInspection(); On 2017/03/31 06:28:02, leonhsl wrote: > Some IPCs like OnScriptEvaluated are triggered on worker thread, as > |instance_host_| is bound on main thread, I'll post a task to main thread to > call 'instance_host_->OnScriptEvaluated(). Yeah and you can use ThreadSafeAssociatedInterfacePtr instead. https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/thread_safe_int...
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 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...
Patchset #2 (id:40001) has been deleted
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...
Patchset #2 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Uploaded ps#2 which has completed implementations and modified related tests, PTAL, Thanks! https://codereview.chromium.org/2787883003/diff/20001/content/common/service_... File content/common/service_worker/embedded_worker.mojom (right): https://codereview.chromium.org/2787883003/diff/20001/content/common/service_... content/common/service_worker/embedded_worker.mojom:26: // Interface to control a browser-side embedded worker instance. On 2017/03/31 09:56:22, shimazu wrote: > Please add a comment about the ordering. Done. https://codereview.chromium.org/2787883003/diff/20001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2787883003/diff/20001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:574: instance_host_->OnReadyForInspection(); On 2017/03/31 09:56:22, shimazu wrote: > On 2017/03/31 06:28:02, leonhsl wrote: > > Some IPCs like OnScriptEvaluated are triggered on worker thread, as > > |instance_host_| is bound on main thread, I'll post a task to main thread to > > call 'instance_host_->OnScriptEvaluated(). > > Yeah and you can use ThreadSafeAssociatedInterfacePtr instead. > https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/thread_safe_int... Done. Thanks a lot for pointing out this class! https://codereview.chromium.org/2787883003/diff/80001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2787883003/diff/80001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:765: if (!registry_->OnWorkerStarted(process_id(), embedded_worker_id_)) Need to do some necessary work in registry. https://codereview.chromium.org/2787883003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/2787883003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_context_unittest.cc:481: // in inverse order with the 4 RegisterServiceWorker calls. I'm still investigating,, it would be very helpful if you have some insights about this ;-) https://codereview.chromium.org/2787883003/diff/80001/content/renderer/servic... File content/renderer/service_worker/embedded_worker_dispatcher.cc (left): https://codereview.chromium.org/2787883003/diff/80001/content/renderer/servic... content/renderer/service_worker/embedded_worker_dispatcher.cc:57: new EmbeddedWorkerHostMsg_WorkerStopped(embedded_worker_id)); All callsites of EmbeddedWorkerDispatcher::WorkerContextDestroyed() are in ServiceWorkerContextClient, send EmbeddedWorkerHostMsg_WorkerStopped there instead.
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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.
Uploaded ps#3 which fixed the mentioned test failure in ps#2, PTAnL, Thanks. https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.cc (right): https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.cc:545: base::RunLoop().RunUntilIdle(); Seems in such cases it's not necessary to let EWInstanceHost function calls run synchronously, here we can just send out the function calls and let them be looped to execute later. If this RunLoop is nested, RunUntilIdle() does not take effect but just return immediately, the outer RunLoop will loop all tasks.
https://codereview.chromium.org/2787883003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/2787883003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_context_unittest.cc:481: // in inverse order with the 4 RegisterServiceWorker calls. On 2017/04/04 07:14:04, leonhsl wrote: > I'm still investigating,, it would be very helpful if you have some insights > about this ;-) What was the cause of this? https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:494: ServiceWorkerStatusCode status = Could you remove this non-mojo path? https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:600: mojom::EmbeddedWorkerInstanceHostAssociatedPtrInfo host_ptr_info; DCHECK(!instance_host_binding_.is_bound()); https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:628: TRACE_EVENT0("ServiceWorker", "EmbeddedWorkerInstance::OnReadyForInspection"); Is it possible to integrate these TRACE_EVENTs into TRACE_EVENT_ASYNC... starting from StartTask? For example, can we have async traces in EWInstance instead of EWInstance::StartTask for tracking these messages? If you can work on that, please use TRACE_EVENT_NESTABLE_ASYNC_{BEGIN,END,INSTANT}[0-2]. TRACE_EVENT_ASYNC* has already been deprecated. https://cs.chromium.org/chromium/src/base/trace_event/common/trace_event_comm... https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:875: process_handle_.reset(); instance_host_binding_.Close(); https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.h (right): https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:224: // mojom::EmbeddedWorkerInstanceHost // Implements mojom::EmbeddedWorkerInstanceHost. https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.cc (right): https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.cc:282: next_provider_id_(1000), Do you have any reason to use 1000 as the default? https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.cc:545: base::RunLoop().RunUntilIdle(); On 2017/04/06 02:49:52, leonhsl wrote: > Seems in such cases it's not necessary to let EWInstanceHost function calls run > synchronously, here we can just send out the function calls and let them be > looped to execute later. > If this RunLoop is nested, RunUntilIdle() does not take effect but just return > immediately, the outer RunLoop will loop all tasks. It makes sense. https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.cc:584: // Prepare a provider host to be used by bellowing OnThreadStarted(). following? https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_context_unittest.cc:492: Could you remote this extra line? https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:57: } I still don't understand why we need nested loop here. Isn't base::RunLoop().RunUntilIdle() enough? and also doesn't this function create a new nested loop, right? // Sorry if I miss something! https://codereview.chromium.org/2787883003/diff/100001/content/common/service... File content/common/service_worker/embedded_worker.mojom (right): https://codereview.chromium.org/2787883003/diff/100001/content/common/service... content/common/service_worker/embedded_worker.mojom:21: StopWorker() => (); I think we don't need to have the callback because we already have OnStopped. Could you remove that and add a comment about that?
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...
Uploaded ps#4, it gets better now! Thanks a lot for review! PTAnL. https://codereview.chromium.org/2787883003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/2787883003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_context_unittest.cc:481: // in inverse order with the 4 RegisterServiceWorker calls. On 2017/04/06 05:01:36, shimazu wrote: > On 2017/04/04 07:14:04, leonhsl wrote: > > I'm still investigating,, it would be very helpful if you have some insights > > about this ;-) > > What was the cause of this? The root cause is that I was using base::MessageLoop::ScopedNestableTaskAllower in EmbeddedWorkerTestHelper::SimulateWorker*() functions to try to execute all EWInstanceHost interface functions synchronously. Then, for this test case, the following scenario comes out: EmbeddedWorkerTestHelper::OnStartWorker(0) --> SimulateWorkerReadyForInspection(0) here will try to execute all tasks in current message loop, because EmbeddedWorkerTestHelper::OnStartWorker({1, 2, 3}) for other 3 registrations have been already queued there, they will get executed, inside this SimulateWorkerReadyForInspection(0). --> EmbeddedWorkerTestHelper::OnStartWorker(1) --> EmbeddedWorkerTestHelper::OnStartWorker(2) --> EmbeddedWorkerTestHelper::OnStartWorker(3) --> OnStarted(3) --> OnStarted(2) --> OnStarted(1) --> OnStarted(0) https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:494: ServiceWorkerStatusCode status = On 2017/04/06 05:01:36, shimazu wrote: > Could you remove this non-mojo path? Done. Removed IPC EmbeddedWorkerMsg_StopWorker and related code path in renderer side. https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:600: mojom::EmbeddedWorkerInstanceHostAssociatedPtrInfo host_ptr_info; On 2017/04/06 05:01:36, shimazu wrote: > DCHECK(!instance_host_binding_.is_bound()); Done. https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:628: TRACE_EVENT0("ServiceWorker", "EmbeddedWorkerInstance::OnReadyForInspection"); On 2017/04/06 05:01:36, shimazu wrote: > Is it possible to integrate these TRACE_EVENTs into TRACE_EVENT_ASYNC... > starting from StartTask? > For example, can we have async traces in EWInstance instead of > EWInstance::StartTask for tracking these messages? > If you can work on that, please use > TRACE_EVENT_NESTABLE_ASYNC_{BEGIN,END,INSTANT}[0-2]. TRACE_EVENT_ASYNC* has > already been deprecated. > > https://cs.chromium.org/chromium/src/base/trace_event/common/trace_event_comm... Oh, This is totally new to me, I just moved here these TRACE_EVENTs from SWDispatcherHost but did not think more about it ;-) Can I keep it as-is for now and integrate all these EVENTs with a follow-up CL? Thanks! https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:875: process_handle_.reset(); On 2017/04/06 05:01:36, shimazu wrote: > instance_host_binding_.Close(); Done and Thanks a lot! https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.h (right): https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:224: // mojom::EmbeddedWorkerInstanceHost On 2017/04/06 05:01:36, shimazu wrote: > // Implements mojom::EmbeddedWorkerInstanceHost. Done. https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.cc (right): https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.cc:282: next_provider_id_(1000), On 2017/04/06 05:01:36, shimazu wrote: > Do you have any reason to use 1000 as the default? It's possible that other test files create/add provider host by themselves, for example, link_header_support_unittest.cc etc. They are using some mock provider id such as 0, 1 or 100, to avoid conflict I use 1000 as start value here. https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.cc:584: // Prepare a provider host to be used by bellowing OnThreadStarted(). On 2017/04/06 05:01:36, shimazu wrote: > following? Done. https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_context_unittest.cc:492: On 2017/04/06 05:01:36, shimazu wrote: > Could you remote this extra line? Done. https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2787883003/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:57: } On 2017/04/06 05:01:36, shimazu wrote: > I still don't understand why we need nested loop here. Isn't > base::RunLoop().RunUntilIdle() enough? > and also doesn't this function create a new nested loop, right? > > // Sorry if I miss something! Yeah actually we do not need nested loop here, I should have changed this file but missed it.. base::RunLoop().RunUntilIdle() is enough. Changed. https://codereview.chromium.org/2787883003/diff/100001/content/common/service... File content/common/service_worker/embedded_worker.mojom (right): https://codereview.chromium.org/2787883003/diff/100001/content/common/service... content/common/service_worker/embedded_worker.mojom:21: StopWorker() => (); On 2017/04/06 05:01:36, shimazu wrote: > I think we don't need to have the callback because we already have OnStopped. > Could you remove that and add a comment about that? Done. Yeah I was confused about this.. Thanks for the clarification!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for updating! Added several comments. https://codereview.chromium.org/2787883003/diff/120001/content/browser/servic... File content/browser/service_worker/embedded_worker_registry.cc (right): https://codereview.chromium.org/2787883003/diff/120001/content/browser/servic... content/browser/service_worker/embedded_worker_registry.cc:87: void EmbeddedWorkerRegistry::OnWorkerStoppedd(int process_id, nit: Stopped https://codereview.chromium.org/2787883003/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_version_unittest.cc (right): https://codereview.chromium.org/2787883003/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_version_unittest.cc:307: std::move(instance_host)); What happen if the |instance_host| isn't kept? It's a test of disallowing StartWorker, so I assume |instance_host| might be unbound state and OnStopWorker wouldn't be called. https://codereview.chromium.org/2787883003/diff/120001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_dispatcher.h (right): https://codereview.chromium.org/2787883003/diff/120001/content/renderer/servi... content/renderer/service_worker/embedded_worker_dispatcher.h:30: class EmbeddedWorkerDispatcher { Sorry for additional comment. EWDispatcher seems no longer needed. Could you remove this also and move WorkerWrapper into EWInstanceClient?
https://codereview.chromium.org/2787883003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/2787883003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_context_unittest.cc:481: // in inverse order with the 4 RegisterServiceWorker calls. On 2017/04/06 09:58:55, leonhsl wrote: > On 2017/04/06 05:01:36, shimazu wrote: > > On 2017/04/04 07:14:04, leonhsl wrote: > > > I'm still investigating,, it would be very helpful if you have some insights > > > about this ;-) > > > > What was the cause of this? > > The root cause is that I was using base::MessageLoop::ScopedNestableTaskAllower > in EmbeddedWorkerTestHelper::SimulateWorker*() functions to try to execute all > EWInstanceHost interface functions synchronously. > > Then, for this test case, the following scenario comes out: > EmbeddedWorkerTestHelper::OnStartWorker(0) > --> SimulateWorkerReadyForInspection(0) here will try to execute all tasks in > current message loop, because EmbeddedWorkerTestHelper::OnStartWorker({1, 2, 3}) > for other 3 registrations have been already queued there, they will get > executed, inside this SimulateWorkerReadyForInspection(0). > --> EmbeddedWorkerTestHelper::OnStartWorker(1) > --> EmbeddedWorkerTestHelper::OnStartWorker(2) > --> EmbeddedWorkerTestHelper::OnStartWorker(3) > --> OnStarted(3) > --> OnStarted(2) > --> OnStarted(1) > --> OnStarted(0) Ah, thanks, I got it.
https://codereview.chromium.org/2787883003/diff/120001/content/browser/servic... File content/browser/service_worker/embedded_worker_registry.cc (right): https://codereview.chromium.org/2787883003/diff/120001/content/browser/servic... content/browser/service_worker/embedded_worker_registry.cc:87: void EmbeddedWorkerRegistry::OnWorkerStoppedd(int process_id, On 2017/04/10 05:09:22, shimazu wrote: > nit: Stopped Acknowledged. And thanks! https://codereview.chromium.org/2787883003/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_version_unittest.cc (right): https://codereview.chromium.org/2787883003/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_version_unittest.cc:307: std::move(instance_host)); On 2017/04/10 05:09:22, shimazu wrote: > What happen if the |instance_host| isn't kept? > It's a test of disallowing StartWorker, so I assume |instance_host| might be > unbound state and OnStopWorker wouldn't be called. In the following 2 tests OnStopWorker() will be called: ServiceWorkerFailToStartTest.RestartStalledWorker It firstly sets STALL mode to make the first OnStartWorker() to fail, and then sets SUCCEED mode and stops the worker, this will make OnStopWorker() here to be called, and the test wants to verify that the worker can be restarted again successfully. ServiceWorkerFailToStartTest.Timeout It sets STALL mode to make the OnStartWorker() to fail, and then simulate triggering the timeout timer function, which will stop the worker, thus OnStopWorker() here will be called. So we need to bind |instance_host| here for preparation. https://codereview.chromium.org/2787883003/diff/120001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_dispatcher.h (right): https://codereview.chromium.org/2787883003/diff/120001/content/renderer/servi... content/renderer/service_worker/embedded_worker_dispatcher.h:30: class EmbeddedWorkerDispatcher { On 2017/04/10 05:09:22, shimazu wrote: > Sorry for additional comment. > EWDispatcher seems no longer needed. Could you remove this also and move > WorkerWrapper into EWInstanceClient? Yeah, thank you very much for the confirmation! I had somehow similar feeling but was not sure so I did not speak out about this ;-) Could we do this work with a follow-up CL? Because I want to keep this CL to be not so big and its' goal to be clear. Or do you think we'd better do this together within this CL because it should be part of this CL's work? I'm happy to follow your opinion. Thanks!
https://codereview.chromium.org/2787883003/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_version_unittest.cc (right): https://codereview.chromium.org/2787883003/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_version_unittest.cc:307: std::move(instance_host)); On 2017/04/10 06:10:36, leonhsl wrote: > On 2017/04/10 05:09:22, shimazu wrote: > > What happen if the |instance_host| isn't kept? > > It's a test of disallowing StartWorker, so I assume |instance_host| might be > > unbound state and OnStopWorker wouldn't be called. > > In the following 2 tests OnStopWorker() will be called: > ServiceWorkerFailToStartTest.RestartStalledWorker > It firstly sets STALL mode to make the first OnStartWorker() to fail, and then > sets SUCCEED mode and stops the worker, this will make OnStopWorker() here to be > called, and the test wants to verify that the worker can be restarted again > successfully. > > ServiceWorkerFailToStartTest.Timeout > It sets STALL mode to make the OnStartWorker() to fail, and then simulate > triggering the timeout timer function, which will stop the worker, thus > OnStopWorker() here will be called. > > So we need to bind |instance_host| here for preparation. Ah, got it. Thanks! I feel code is a bit duplicated because MessageReceiverDisallowStart has its own map though EWTestHelper has it. It's also weird that ServiceWorkerFailToStartTest.RestartStalledWorker actually doesn't want to fail StartWorker but want to control the order of replies which are conveyed via EWHost interface. I don't have good idea for now but I think we might be able to make the tests simpler. I'd like to investigate more tomorrow. https://codereview.chromium.org/2787883003/diff/120001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_dispatcher.h (right): https://codereview.chromium.org/2787883003/diff/120001/content/renderer/servi... content/renderer/service_worker/embedded_worker_dispatcher.h:30: class EmbeddedWorkerDispatcher { On 2017/04/10 06:10:36, leonhsl wrote: > On 2017/04/10 05:09:22, shimazu wrote: > > Sorry for additional comment. > > EWDispatcher seems no longer needed. Could you remove this also and move > > WorkerWrapper into EWInstanceClient? > > Yeah, thank you very much for the confirmation! I had somehow similar feeling > but was not sure so I did not speak out about this ;-) Could we do this work > with a follow-up CL? Because I want to keep this CL to be not so big and its' > goal to be clear. Or do you think we'd better do this together within this CL > because it should be part of this CL's work? I'm happy to follow your opinion. > Thanks! I'm happy if you would remove this class in this CL because I assume the removal of this class won't be so big, but I can also take it over if you want. How will you do?
https://codereview.chromium.org/2787883003/diff/120001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_dispatcher.h (right): https://codereview.chromium.org/2787883003/diff/120001/content/renderer/servi... content/renderer/service_worker/embedded_worker_dispatcher.h:30: class EmbeddedWorkerDispatcher { On 2017/04/10 09:33:40, shimazu wrote: > On 2017/04/10 06:10:36, leonhsl wrote: > > On 2017/04/10 05:09:22, shimazu wrote: > > > Sorry for additional comment. > > > EWDispatcher seems no longer needed. Could you remove this also and move > > > WorkerWrapper into EWInstanceClient? > > > > Yeah, thank you very much for the confirmation! I had somehow similar feeling > > but was not sure so I did not speak out about this ;-) Could we do this work > > with a follow-up CL? Because I want to keep this CL to be not so big and its' > > goal to be clear. Or do you think we'd better do this together within this CL > > because it should be part of this CL's work? I'm happy to follow your opinion. > > Thanks! > > I'm happy if you would remove this class in this CL because I assume the removal > of this class won't be so big, but I can also take it over if you want. > How will you do? Understood, I'd like to do this in this CL :)
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 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.
Uploaded ps#6 to remove EmbeddedWorkerDispatcher completely, PTAnL, Thanks.
Thanks a lot!:) Let me add a couple of minor commets; https://codereview.chromium.org/2787883003/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_version_unittest.cc (right): https://codereview.chromium.org/2787883003/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_version_unittest.cc:307: std::move(instance_host)); On 2017/04/10 09:33:40, shimazu wrote: > On 2017/04/10 06:10:36, leonhsl wrote: > > On 2017/04/10 05:09:22, shimazu wrote: > > > What happen if the |instance_host| isn't kept? > > > It's a test of disallowing StartWorker, so I assume |instance_host| might be > > > unbound state and OnStopWorker wouldn't be called. > > > > In the following 2 tests OnStopWorker() will be called: > > ServiceWorkerFailToStartTest.RestartStalledWorker > > It firstly sets STALL mode to make the first OnStartWorker() to fail, and > then > > sets SUCCEED mode and stops the worker, this will make OnStopWorker() here to > be > > called, and the test wants to verify that the worker can be restarted again > > successfully. > > > > ServiceWorkerFailToStartTest.Timeout > > It sets STALL mode to make the OnStartWorker() to fail, and then simulate > > triggering the timeout timer function, which will stop the worker, thus > > OnStopWorker() here will be called. > > > > So we need to bind |instance_host| here for preparation. > > Ah, got it. Thanks! > I feel code is a bit duplicated because MessageReceiverDisallowStart has its own > map though EWTestHelper has it. It's also weird that > ServiceWorkerFailToStartTest.RestartStalledWorker actually doesn't want to fail > StartWorker but want to control the order of replies which are conveyed via > EWHost interface. > > I don't have good idea for now but I think we might be able to make the tests > simpler. I'd like to investigate more tomorrow. Sorry for late! The reason why I feel a bit weird might be that EWTestHelper originally simulated IPCs by calling Simulate~~~ with stepping through On~~~ implemented in EWTestHelper, but it could be decoupled from EWTestHelper if these things are moved to MockEWInstanceClient. It'll be big change, so I think this test is good as is. https://codereview.chromium.org/2787883003/diff/160001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.cc (right): https://codereview.chromium.org/2787883003/diff/160001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.cc:587: auto host = CreateProviderHostForServiceWorkerContext( Could you use std::unique_ptr<ServiceWorkerProviderHost> instead of auto? https://codereview.chromium.org/2787883003/diff/160001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_instance_client_impl.cc (right): https://codereview.chromium.org/2787883003/diff/160001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:50: wrapper_ = nullptr; I prefer wrapper_.reset() for unique_ptr.
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...
Uploaded ps#7 to address comments, PTAnL, Thanks. https://codereview.chromium.org/2787883003/diff/160001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.cc (right): https://codereview.chromium.org/2787883003/diff/160001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.cc:587: auto host = CreateProviderHostForServiceWorkerContext( On 2017/04/11 04:32:40, shimazu wrote: > Could you use std::unique_ptr<ServiceWorkerProviderHost> instead of auto? Done. https://codereview.chromium.org/2787883003/diff/160001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_instance_client_impl.cc (right): https://codereview.chromium.org/2787883003/diff/160001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:50: wrapper_ = nullptr; On 2017/04/11 04:32:40, shimazu wrote: > I prefer wrapper_.reset() for unique_ptr. Done.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/11 05:56:48, shimazu wrote: > lgtm Thanks shimazu@ a lot for the great help on this CL! Then I'll add other OWNERs(for //content/ and mojom/message files). Hi, falken@, happy to see your comments if you'd like to take a look, too. Thank you :)
leon.han@intel.com changed reviewers: + kinuko@chromium.org, tsepez@chromium.org
+kinuko@: OWNER review for //content/ except service_worker bits. +tsepez@: OWNER review for embedded_worker.mojom and embedded_worker_messages.h. Thanks all.
I just quickly looked at the mojom file. https://codereview.chromium.org/2787883003/diff/180001/content/common/service... File content/common/service_worker/embedded_worker.mojom (right): https://codereview.chromium.org/2787883003/diff/180001/content/common/service... content/common/service_worker/embedded_worker.mojom:16: // Interface to control a renderer-side worker's environment. Interface to control a renderer-side embedded worker. The browser uses this interface to start, stop, and issue commands to the worker. https://codereview.chromium.org/2787883003/diff/180001/content/common/service... content/common/service_worker/embedded_worker.mojom:29: // with interface EmbeddedWorkerInstanceClient. Interface to control a browser-side embedded worker instance. The renderer uses this interface to report embedded worker state back to the browser. This interface is associated with the EmbeddedWorkerInstanceClient interface. not sure I understand the "be associated with interface EmbeddedWorkerInstanceClient" part. https://codereview.chromium.org/2787883003/diff/180001/content/common/service... content/common/service_worker/embedded_worker.mojom:32: // Indicate that the worker is ready for inspection. Indicates* here and below. https://codereview.chromium.org/2787883003/diff/180001/content/common/service... content/common/service_worker/embedded_worker.mojom:43: OnThreadStarted(int32 thread_id, int32 provider_id); Can you comment what |thread_id| is and |provider_id| is? Are they both unique in the renderer process? https://codereview.chromium.org/2787883003/diff/180001/content/common/service... content/common/service_worker/embedded_worker.mojom:44: // Indicate that the worker has evaluated the script. Can you comment what |success| is. IIRC it means evaluating the script completed and no uncaught exception occurred. https://codereview.chromium.org/2787883003/diff/180001/content/common/service... content/common/service_worker/embedded_worker.mojom:48: // Report an exception. Reports*
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...
Thanks falken@ for review! Uploaded ps#8 to address comments, PTAnL. https://codereview.chromium.org/2787883003/diff/180001/content/common/service... File content/common/service_worker/embedded_worker.mojom (right): https://codereview.chromium.org/2787883003/diff/180001/content/common/service... content/common/service_worker/embedded_worker.mojom:16: // Interface to control a renderer-side worker's environment. On 2017/04/11 08:29:27, falken wrote: > Interface to control a renderer-side embedded worker. The browser uses this > interface to start, stop, and issue commands to the worker. Done. https://codereview.chromium.org/2787883003/diff/180001/content/common/service... content/common/service_worker/embedded_worker.mojom:29: // with interface EmbeddedWorkerInstanceClient. On 2017/04/11 08:29:27, falken wrote: > Interface to control a browser-side embedded worker instance. The renderer uses > this interface to report embedded worker state back to the browser. This > interface is associated with the EmbeddedWorkerInstanceClient interface. > > not sure I understand the "be associated with interface > EmbeddedWorkerInstanceClient" part. Done and Thanks! All mojo interfaces associated together use the same one underlying message pipe, so that they can achieve FIFO ordering for their mojo calls. https://codereview.chromium.org/2787883003/diff/180001/content/common/service... content/common/service_worker/embedded_worker.mojom:32: // Indicate that the worker is ready for inspection. On 2017/04/11 08:29:27, falken wrote: > Indicates* here and below. Done. https://codereview.chromium.org/2787883003/diff/180001/content/common/service... content/common/service_worker/embedded_worker.mojom:43: OnThreadStarted(int32 thread_id, int32 provider_id); On 2017/04/11 08:29:27, falken wrote: > Can you comment what |thread_id| is and |provider_id| is? Are they both unique > in the renderer process? Done. But I'm not quite sure whether platform thread id can always keep unique in a process, OS(Linux/Win etc.) maybe reuse thread id?.. https://codereview.chromium.org/2787883003/diff/180001/content/common/service... content/common/service_worker/embedded_worker.mojom:44: // Indicate that the worker has evaluated the script. On 2017/04/11 08:29:27, falken wrote: > Can you comment what |success| is. IIRC it means evaluating the script completed > and no uncaught exception occurred. Done and Thanks! https://codereview.chromium.org/2787883003/diff/180001/content/common/service... content/common/service_worker/embedded_worker.mojom:48: // Report an exception. On 2017/04/11 08:29:27, falken wrote: > Reports* Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nits https://codereview.chromium.org/2787883003/diff/200001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.h (right): https://codereview.chromium.org/2787883003/diff/200001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:225: // Be called when the worker instance has ack'ed that nit: 'Called' seems to be more common than 'Be called' in SW code base? Also: we prefer descriptive comments rather than imperative ones for function declarations, so if we change this 'Gets called' is probably preferable https://google.github.io/styleguide/cppguide.html#Comments
https://codereview.chromium.org/2787883003/diff/200001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.h (right): https://codereview.chromium.org/2787883003/diff/200001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:225: // Be called when the worker instance has ack'ed that On 2017/04/11 14:03:17, kinuko wrote: > nit: 'Called' seems to be more common than 'Be called' in SW code base? > > Also: we prefer descriptive comments rather than imperative ones for function > declarations, so if we change this 'Gets called' is probably preferable > > https://google.github.io/styleguide/cppguide.html#Comments I'm thinking we should move all these comments to the mojom file. Generally, the mojo interface should be a well-documented interface that the C++ class implements. Then almost all that's needed here is "mojom::EmbeddedWorkerInstanceHost implementation". We also would have less duplicated comments. What would belong here are implementation details only like: "This will change the internal status from STARTING to RUNNING."
This is a nice change! Some more nitty comments, I'm viewing this mojofication stuff as a chance to have better documented interfaces. https://codereview.chromium.org/2787883003/diff/200001/content/common/service... File content/common/service_worker/embedded_worker.mojom (right): https://codereview.chromium.org/2787883003/diff/200001/content/common/service... content/common/service_worker/embedded_worker.mojom:33: // Sent on main thread. I'm curious why it's important to document sent on main thread vs worker thread. Is it just describing the current code, or a warning that you shouldn't call the function from the wrong thread? Where is it defined which thread on the receiver the function is called on? Is it always the IO thread for these mojo functions? Is it valuable to document that, or is it obvious? https://codereview.chromium.org/2787883003/diff/200001/content/common/service... content/common/service_worker/embedded_worker.mojom:40: // Indicates that the worker is stopped. nit: just "that the worker stopped" https://codereview.chromium.org/2787883003/diff/200001/content/common/service... content/common/service_worker/embedded_worker.mojom:53: // Indicates that the worker is started. nit: just "that the worker started"
lgtm
https://codereview.chromium.org/2787883003/diff/200001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.h (right): https://codereview.chromium.org/2787883003/diff/200001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:225: // Be called when the worker instance has ack'ed that On 2017/04/11 14:10:05, falken wrote: > On 2017/04/11 14:03:17, kinuko wrote: > > nit: 'Called' seems to be more common than 'Be called' in SW code base? > > > > Also: we prefer descriptive comments rather than imperative ones for function > > declarations, so if we change this 'Gets called' is probably preferable > > > > https://google.github.io/styleguide/cppguide.html#Comments > > I'm thinking we should move all these comments to the mojom file. Generally, the > mojo interface should be a well-documented interface that the C++ class > implements. Then almost all that's needed here is > "mojom::EmbeddedWorkerInstanceHost implementation". We also would have less > duplicated comments. > > What would belong here are implementation details only like: > "This will change the internal status from STARTING to RUNNING." Yeah I agree, SGTM
I'll be OOO this afternoon and will update ASAP later, Thanks all for the review :) https://codereview.chromium.org/2787883003/diff/200001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.h (right): https://codereview.chromium.org/2787883003/diff/200001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:225: // Be called when the worker instance has ack'ed that On 2017/04/12 00:49:44, kinuko wrote: > On 2017/04/11 14:10:05, falken wrote: > > On 2017/04/11 14:03:17, kinuko wrote: > > > nit: 'Called' seems to be more common than 'Be called' in SW code base? > > > > > > Also: we prefer descriptive comments rather than imperative ones for > function > > > declarations, so if we change this 'Gets called' is probably preferable > > > > > > https://google.github.io/styleguide/cppguide.html#Comments > > > > I'm thinking we should move all these comments to the mojom file. Generally, > the > > mojo interface should be a well-documented interface that the C++ class > > implements. Then almost all that's needed here is > > "mojom::EmbeddedWorkerInstanceHost implementation". We also would have less > > duplicated comments. > > > > What would belong here are implementation details only like: > > "This will change the internal status from STARTING to RUNNING." > > Yeah I agree, SGTM I see, and agree that we should document mojom file well and add comments about impl details in C++ header file. Will update. https://codereview.chromium.org/2787883003/diff/200001/content/common/service... File content/common/service_worker/embedded_worker.mojom (right): https://codereview.chromium.org/2787883003/diff/200001/content/common/service... content/common/service_worker/embedded_worker.mojom:33: // Sent on main thread. On 2017/04/11 14:27:36, falken wrote: > I'm curious why it's important to document sent on main thread vs worker thread. > Is it just describing the current code, or a warning that you shouldn't call the > function from the wrong thread? > > Where is it defined which thread on the receiver the function is called on? Is > it always the IO thread for these mojo functions? Is it valuable to document > that, or is it obvious? > Acknowledged. https://codereview.chromium.org/2787883003/diff/200001/content/common/service... content/common/service_worker/embedded_worker.mojom:40: // Indicates that the worker is stopped. On 2017/04/11 14:27:36, falken wrote: > nit: just "that the worker stopped" Acknowledged. https://codereview.chromium.org/2787883003/diff/200001/content/common/service... content/common/service_worker/embedded_worker.mojom:53: // Indicates that the worker is started. On 2017/04/11 14:27:36, falken wrote: > nit: just "that the worker started" Acknowledged.
Uploaded ps#9 to refine code comments, PTAnL, Thanks. https://codereview.chromium.org/2787883003/diff/200001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.h (right): https://codereview.chromium.org/2787883003/diff/200001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:225: // Be called when the worker instance has ack'ed that On 2017/04/12 03:34:03, leonhsl wrote: > On 2017/04/12 00:49:44, kinuko wrote: > > On 2017/04/11 14:10:05, falken wrote: > > > On 2017/04/11 14:03:17, kinuko wrote: > > > > nit: 'Called' seems to be more common than 'Be called' in SW code base? > > > > > > > > Also: we prefer descriptive comments rather than imperative ones for > > function > > > > declarations, so if we change this 'Gets called' is probably preferable > > > > > > > > https://google.github.io/styleguide/cppguide.html#Comments > > > > > > I'm thinking we should move all these comments to the mojom file. Generally, > > the > > > mojo interface should be a well-documented interface that the C++ class > > > implements. Then almost all that's needed here is > > > "mojom::EmbeddedWorkerInstanceHost implementation". We also would have less > > > duplicated comments. > > > > > > What would belong here are implementation details only like: > > > "This will change the internal status from STARTING to RUNNING." > > > > Yeah I agree, SGTM > > I see, and agree that we should document mojom file well and add comments about > impl details in C++ header file. Will update. Done. https://codereview.chromium.org/2787883003/diff/200001/content/common/service... File content/common/service_worker/embedded_worker.mojom (right): https://codereview.chromium.org/2787883003/diff/200001/content/common/service... content/common/service_worker/embedded_worker.mojom:33: // Sent on main thread. On 2017/04/12 03:34:04, leonhsl wrote: > On 2017/04/11 14:27:36, falken wrote: > > I'm curious why it's important to document sent on main thread vs worker > thread. > > Is it just describing the current code, or a warning that you shouldn't call > the > > function from the wrong thread? > > > > Where is it defined which thread on the receiver the function is called on? Is > > it always the IO thread for these mojo functions? Is it valuable to document > > that, or is it obvious? > > > > Acknowledged. Yes 'sent on xxx thread' is just describing the current code, so I removed them from this mojom file. And also added comments into embedded_worker_instance.h saying that all implementation functions run on browser IO thread. https://codereview.chromium.org/2787883003/diff/200001/content/common/service... content/common/service_worker/embedded_worker.mojom:40: // Indicates that the worker is stopped. On 2017/04/12 03:34:04, leonhsl wrote: > On 2017/04/11 14:27:36, falken wrote: > > nit: just "that the worker stopped" > > Acknowledged. Done. https://codereview.chromium.org/2787883003/diff/200001/content/common/service... content/common/service_worker/embedded_worker.mojom:53: // Indicates that the worker is started. On 2017/04/12 03:34:04, leonhsl wrote: > On 2017/04/11 14:27:36, falken wrote: > > nit: just "that the worker started" > > Acknowledged. Done.
This is looking good. Thanks for bearing with my comments. Due to the size of this CL and the tricky ordering issues we've run into before with Mojo, I think it'd make sense to hold off on committing until after the 59 branch cut at the end of this week. WDYT? https://codereview.chromium.org/2787883003/diff/220001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.h (right): https://codereview.chromium.org/2787883003/diff/220001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:225: // All these implementation functions run on browser IO thread. These functions all run on the IO thread. https://codereview.chromium.org/2787883003/diff/220001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:226: // This will only notify DevToolsManager about this. OK to remove this. I'm less enthusiastic about these implementation detail comments. I hope the mojom interface comments are largely sufficient and we only need to add comments here when they really elucidate something. https://codereview.chromium.org/2787883003/diff/220001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:228: // This will set the internal starting phase status to SCRIPT_LOADED. I think we can remove this. The starting phase status is a pretty deep implementation detail. https://codereview.chromium.org/2787883003/diff/220001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:233: // status to THREAD_STARTED. To tighten it up, "Notifies the corresponding provider host that the thread has started and is ready to receive messages." https://codereview.chromium.org/2787883003/diff/220001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:237: // fire the callback received by Start(). +1 to mentioning this fires the callback. I'd remove the starting phase status. "Fires the callback passed to Start()." https://codereview.chromium.org/2787883003/diff/220001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:243: // etc. "Resets the embedded worker instance to the initial state. This will change the internal status from STARTING or RUNNING to STOPPED." https://codereview.chromium.org/2787883003/diff/220001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2787883003/diff/220001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.h:177: // This calls SimulateWorkerStopped(embedded_worker_id) by default. nit: can just say SimulateWorkerStopped() https://codereview.chromium.org/2787883003/diff/220001/content/common/service... File content/common/service_worker/embedded_worker.mojom (right): https://codereview.chromium.org/2787883003/diff/220001/content/common/service... content/common/service_worker/embedded_worker.mojom:41: // of each ServiceWorkerNetworkProvider instance, which is unique in one nit: s/each/the https://codereview.chromium.org/2787883003/diff/220001/content/common/service... content/common/service_worker/embedded_worker.mojom:43: // counterpart ServiceWorkerProviderHost instance. This is called after OnScriptLoaded. https://codereview.chromium.org/2787883003/diff/220001/content/common/service... content/common/service_worker/embedded_worker.mojom:47: // This is always before OnStarted. This is called after OnThreadStarted. https://codereview.chromium.org/2787883003/diff/220001/content/common/service... content/common/service_worker/embedded_worker.mojom:50: // This is always after OnScriptEvaluated. This is called after OnScriptEvaluated. https://codereview.chromium.org/2787883003/diff/220001/content/common/service... content/common/service_worker/embedded_worker.mojom:52: // Reports an exception. Reports that an uncaught exception occurred in the worker. https://codereview.chromium.org/2787883003/diff/220001/content/common/service... content/common/service_worker/embedded_worker.mojom:55: // Reports console message. Reports that a console message was emitted to the worker's console. https://codereview.chromium.org/2787883003/diff/220001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_instance_client_impl.h (right): https://codereview.chromium.org/2787883003/diff/220001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:93: std::unique_ptr<WorkerWrapper> wrapper_ = nullptr; nit: don't need to say = nullptr https://codereview.chromium.org/2787883003/diff/220001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2787883003/diff/220001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:708: // (2) ProviderDestroyed (via following what does "following" mean here? Does it mean subsequent, as in StopWorkerCompleted occurs due to OnStopped()? But then why isn't the ordering guaranteed?
https://codereview.chromium.org/2787883003/diff/220001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2787883003/diff/220001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:708: // (2) ProviderDestroyed (via following On 2017/04/12 14:30:18, falken wrote: > what does "following" mean here? Does it mean subsequent, as in > StopWorkerCompleted occurs due to OnStopped()? But then why isn't the ordering > guaranteed? Ah, I missed that when I reviewed, sorry. EmbeddedWorkerInstanceClientImpl::StopWorkerCompleted should be EmbeddedWorkerInstanceClientImpl::WorkerContextDestroyed here. WorkerContextDestroyed removes its WorkerWrapper, and WebEmbeddedWorkerImpl is destructed. WebEmbeddedWorkerImpl has the dummy frame (the shadow page), so WebServiceWorkerNetworkProvider also gets destructed. Currently EWInstanceHost is associated with EWInstanceClient, and OnDestroyed is sent via SWDispatcherHost which is associated with the IPC channel. That will be solved once SWProvider{Host,Client} is mojoified and they are also associated with EWInstanceClient in other CLs (https://crrev.com/2653493009 and https://crrev.com/2779763004).
Thanks a lot for the refinement! uploaded ps#10, PTAnL. https://codereview.chromium.org/2787883003/diff/220001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.h (right): https://codereview.chromium.org/2787883003/diff/220001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:225: // All these implementation functions run on browser IO thread. On 2017/04/12 14:30:18, falken wrote: > These functions all run on the IO thread. Done. https://codereview.chromium.org/2787883003/diff/220001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:226: // This will only notify DevToolsManager about this. On 2017/04/12 14:30:18, falken wrote: > OK to remove this. > > I'm less enthusiastic about these implementation detail comments. I hope the > mojom interface comments are largely sufficient and we only need to add comments > here when they really elucidate something. Done. https://codereview.chromium.org/2787883003/diff/220001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:228: // This will set the internal starting phase status to SCRIPT_LOADED. On 2017/04/12 14:30:18, falken wrote: > I think we can remove this. The starting phase status is a pretty deep > implementation detail. Done. https://codereview.chromium.org/2787883003/diff/220001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:233: // status to THREAD_STARTED. On 2017/04/12 14:30:18, falken wrote: > To tighten it up, "Notifies the corresponding provider host that the thread has > started and is ready to receive messages." Done. https://codereview.chromium.org/2787883003/diff/220001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:237: // fire the callback received by Start(). On 2017/04/12 14:30:18, falken wrote: > +1 to mentioning this fires the callback. I'd remove the starting phase status. > > "Fires the callback passed to Start()." Done. https://codereview.chromium.org/2787883003/diff/220001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:243: // etc. On 2017/04/12 14:30:18, falken wrote: > "Resets the embedded worker instance to the initial state. This will change the > internal status from STARTING or RUNNING to STOPPED." Done. https://codereview.chromium.org/2787883003/diff/220001/content/browser/servic... File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/2787883003/diff/220001/content/browser/servic... content/browser/service_worker/embedded_worker_test_helper.h:177: // This calls SimulateWorkerStopped(embedded_worker_id) by default. On 2017/04/12 14:30:18, falken wrote: > nit: can just say SimulateWorkerStopped() Done. https://codereview.chromium.org/2787883003/diff/220001/content/common/service... File content/common/service_worker/embedded_worker.mojom (right): https://codereview.chromium.org/2787883003/diff/220001/content/common/service... content/common/service_worker/embedded_worker.mojom:41: // of each ServiceWorkerNetworkProvider instance, which is unique in one On 2017/04/12 14:30:18, falken wrote: > nit: s/each/the Done. https://codereview.chromium.org/2787883003/diff/220001/content/common/service... content/common/service_worker/embedded_worker.mojom:43: // counterpart ServiceWorkerProviderHost instance. On 2017/04/12 14:30:18, falken wrote: > This is called after OnScriptLoaded. Done. https://codereview.chromium.org/2787883003/diff/220001/content/common/service... content/common/service_worker/embedded_worker.mojom:47: // This is always before OnStarted. On 2017/04/12 14:30:18, falken wrote: > This is called after OnThreadStarted. Done. https://codereview.chromium.org/2787883003/diff/220001/content/common/service... content/common/service_worker/embedded_worker.mojom:50: // This is always after OnScriptEvaluated. On 2017/04/12 14:30:18, falken wrote: > This is called after OnScriptEvaluated. Done. https://codereview.chromium.org/2787883003/diff/220001/content/common/service... content/common/service_worker/embedded_worker.mojom:52: // Reports an exception. On 2017/04/12 14:30:18, falken wrote: > Reports that an uncaught exception occurred in the worker. Done. https://codereview.chromium.org/2787883003/diff/220001/content/common/service... content/common/service_worker/embedded_worker.mojom:55: // Reports console message. On 2017/04/12 14:30:18, falken wrote: > Reports that a console message was emitted to the worker's console. Done. https://codereview.chromium.org/2787883003/diff/220001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_instance_client_impl.h (right): https://codereview.chromium.org/2787883003/diff/220001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:93: std::unique_ptr<WorkerWrapper> wrapper_ = nullptr; On 2017/04/12 14:30:18, falken wrote: > nit: don't need to say = nullptr Done. https://codereview.chromium.org/2787883003/diff/220001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2787883003/diff/220001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:708: // (2) ProviderDestroyed (via following On 2017/04/13 00:58:44, shimazu wrote: > On 2017/04/12 14:30:18, falken wrote: > > what does "following" mean here? Does it mean subsequent, as in > > StopWorkerCompleted occurs due to OnStopped()? But then why isn't the > ordering > > guaranteed? > > Ah, I missed that when I reviewed, sorry. > EmbeddedWorkerInstanceClientImpl::StopWorkerCompleted should be > EmbeddedWorkerInstanceClientImpl::WorkerContextDestroyed here. > WorkerContextDestroyed removes its WorkerWrapper, and WebEmbeddedWorkerImpl is > destructed. WebEmbeddedWorkerImpl has the dummy frame (the shadow page), so > WebServiceWorkerNetworkProvider also gets destructed. > > Currently EWInstanceHost is associated with EWInstanceClient, and OnDestroyed is > sent via SWDispatcherHost which is associated with the IPC channel. That will be > solved once SWProvider{Host,Client} is mojoified and they are also associated > with EWInstanceClient in other CLs (https://crrev.com/2653493009 and > https://crrev.com/2779763004). I have to say sorry that I just copied/pasted this comment here without further digging into it.. Thanks Shimazu-san a lot for the detailed clarification, changed the comment and also changed the TODO owner to you :)
On 2017/04/12 14:30:19, falken wrote: > This is looking good. Thanks for bearing with my comments. > > Due to the size of this CL and the tricky ordering issues we've run into before > with Mojo, I think it'd make sense to hold off on committing until after the 59 > branch cut at the end of this week. WDYT? > Sure, I agree we play for safety, the ordering issues are really tricky... I'd like to land this after the completion of M59 branch cut.
lgtm. this looks great, thanks let's land this after the branch point, which should be in the afternoon on Friday. To be safe, let's wait for the branch announcement on chromium-dev/blink-dev.
M59 branch has been announced out at https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/JgMDrp__ucg, so let's land this now. Thank you all for help!
The CQ bit was checked by leon.han@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from shimazu@chromium.org, kinuko@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/2787883003/#ps240001 (title: "Refine code comments")
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": 240001, "attempt_start_ts": 1492305888185840,
"parent_rev": "b1b326893990633387f90546607ce8271d151a3a", "commit_rev":
"7a57143828b914abad790775f41ea97255ae9c11"}
Message was sent while issue was closed.
Description was changed from ========== [ServiceWorker] Add EmbeddedWorkerInstanceHost Interface. This CL converts these IPCs EmbeddedWorkerHostMsg_WorkerReadyForInspection EmbeddedWorkerHostMsg_WorkerScriptLoaded EmbeddedWorkerHostMsg_WorkerScriptLoadFailed EmbeddedWorkerHostMsg_WorkerScriptEvaluated EmbeddedWorkerHostMsg_WorkerStarted EmbeddedWorkerHostMsg_WorkerStopped EmbeddedWorkerHostMsg_ReportException EmbeddedWorkerHostMsg_ReportConsoleMessage EmbeddedWorkerHostMsg_WorkerThreadStarted into mojo interface EmbeddedWorkerInstanceHost, and associate it with existing interface EmbeddedWorkerInstanceClient. BUG=629701 TEST=content_unittests ========== to ========== [ServiceWorker] Add EmbeddedWorkerInstanceHost Interface. This CL converts these IPCs EmbeddedWorkerHostMsg_WorkerReadyForInspection EmbeddedWorkerHostMsg_WorkerScriptLoaded EmbeddedWorkerHostMsg_WorkerScriptLoadFailed EmbeddedWorkerHostMsg_WorkerScriptEvaluated EmbeddedWorkerHostMsg_WorkerStarted EmbeddedWorkerHostMsg_WorkerStopped EmbeddedWorkerHostMsg_ReportException EmbeddedWorkerHostMsg_ReportConsoleMessage EmbeddedWorkerHostMsg_WorkerThreadStarted into mojo interface EmbeddedWorkerInstanceHost, and associate it with existing interface EmbeddedWorkerInstanceClient. BUG=629701 TEST=content_unittests Review-Url: https://codereview.chromium.org/2787883003 Cr-Commit-Position: refs/heads/master@{#464878} Committed: https://chromium.googlesource.com/chromium/src/+/7a57143828b914abad790775f41e... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:240001) as https://chromium.googlesource.com/chromium/src/+/7a57143828b914abad790775f41e... |
