Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2313653002:
ServiceWorker: Exchange InterfaceProviders when starting worker thread (Closed)
|
Created:
4 years, 3 months ago by shimazu Modified:
4 years, 2 months ago CC:
Aaron Boodman, abarth-chromium, blink-worker-reviews_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, horo+watch_chromium.org, jam, jsbell+serviceworker_chromium.org, kinuko+watch, kinuko+serviceworker, michaeln, mlamouri+watch-content_chromium.org, nhiroki, qsr+mojo_chromium.org, serviceworker-reviews, tzik, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionServiceWorker: Exchange InterfaceProviders when starting worker thread
Browser process sends the InterfaceProvider bound to InterfaceRegistry on
EmbeddedWorkerInstance in this patch. This enables SWContextClient to send IPCs
via mojo when its context is starting.
BUG=629701
Committed: https://crrev.com/34d1b0c8709f5533ef22ba843f49fa94bf5fa6dd
Cr-Commit-Position: refs/heads/master@{#425978}
Patch Set 1 #Patch Set 2 : Use AsWeakPtr for base::Bind #Patch Set 3 : Indent #
Total comments: 12
Patch Set 4 : Rebase #
Total comments: 2
Patch Set 5 : Rebase and updated comments #Patch Set 6 : Added a comment #
Total comments: 22
Patch Set 7 : Update comments, added DCHECK and rebase #
Total comments: 14
Patch Set 8 : Rebase #Patch Set 9 : Passed InterfaceProviderRequest with StartWorker #
Total comments: 18
Patch Set 10 : Updated comments and fixed a typo #Messages
Total messages: 30 (11 generated)
shimazu@chromium.org changed reviewers: + horo@chromium.org
ptal :~)
nhiroki@chromium.org changed reviewers: + nhiroki@chromium.org
Drive-by comments https://codereview.chromium.org/2313653002/diff/40001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2313653002/diff/40001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:729: if (!ServiceWorkerUtils::IsMojoForServiceWorkerEnabled()) { This condition may make a code reader surprised because we set up mojo interfaces even if IsMojoForServiceWorkerEnabled() returns *false*. Can you add a comment why this is valid? https://codereview.chromium.org/2313653002/diff/40001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.h (right): https://codereview.chromium.org/2313653002/diff/40001/content/browser/service... content/browser/service_worker/embedded_worker_instance.h:145: // Methods overrides mojom::EmbeddedWorkerInstance // Overrides mojom::EmbeddedWorkerInstance https://codereview.chromium.org/2313653002/diff/40001/content/browser/service... content/browser/service_worker/embedded_worker_instance.h:207: // |binding_| "// Binds a mojo endpoint to |binding_|." ? ('Factory' sounds like a factory method, but this generates nothing) https://codereview.chromium.org/2313653002/diff/40001/content/common/service_... File content/common/service_worker/embedded_worker.mojom (right): https://codereview.chromium.org/2313653002/diff/40001/content/common/service_... content/common/service_worker/embedded_worker.mojom:35: AttachClientInterfaceProvider( AttachClientInterface*To*Provider ? https://codereview.chromium.org/2313653002/diff/40001/content/renderer/servic... File content/renderer/service_worker/embedded_worker_instance_client_impl.h (right): https://codereview.chromium.org/2313653002/diff/40001/content/renderer/servic... content/renderer/service_worker/embedded_worker_instance_client_impl.h:82: void EmbeddedWorkerInstanceClientImpl::GetInterfaceOnMainThread( Do we really need to make this thread-safe? At least for now, this is called only from a non-main thread. Generally, it's preferable that a caller explicitly posts a task to a target thread: See https://codereview.chromium.org/796393005/ https://codereview.chromium.org/2313653002/diff/40001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2313653002/diff/40001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:1078: void ServiceWorkerContextClient::OnDidGetEmbeddedWorkerInstance() { DCHECK(worker_task_runner_->RunsTasksOnCurrentThread());
https://codereview.chromium.org/2313653002/diff/60001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2313653002/diff/60001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:815: remote_interfaces_->Bind(std::move(client_interfaces)); What happens if EmbeddedWorkerInstance::GetRemoteInterfaces() is called before AttachClientInterfaceProvider()? Doesn't this happen?
https://codereview.chromium.org/2313653002/diff/40001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2313653002/diff/40001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:729: if (!ServiceWorkerUtils::IsMojoForServiceWorkerEnabled()) { On 2016/09/06 01:38:05, nhiroki wrote: > This condition may make a code reader surprised because we set up mojo > interfaces even if IsMojoForServiceWorkerEnabled() returns *false*. Can you add > a comment why this is valid? Done. https://codereview.chromium.org/2313653002/diff/40001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.h (right): https://codereview.chromium.org/2313653002/diff/40001/content/browser/service... content/browser/service_worker/embedded_worker_instance.h:145: // Methods overrides mojom::EmbeddedWorkerInstance On 2016/09/06 01:38:05, nhiroki wrote: > // Overrides mojom::EmbeddedWorkerInstance Done. https://codereview.chromium.org/2313653002/diff/40001/content/browser/service... content/browser/service_worker/embedded_worker_instance.h:207: // |binding_| On 2016/09/06 01:38:05, nhiroki wrote: > "// Binds a mojo endpoint to |binding_|." ? > > ('Factory' sounds like a factory method, but this generates nothing) "Factory" is a mojo word defined here: https://cs.chromium.org/chromium/src/services/shell/public/cpp/interface_regi... but I think it's confising, too. Simplified the comment. https://codereview.chromium.org/2313653002/diff/40001/content/common/service_... File content/common/service_worker/embedded_worker.mojom (right): https://codereview.chromium.org/2313653002/diff/40001/content/common/service_... content/common/service_worker/embedded_worker.mojom:35: AttachClientInterfaceProvider( On 2016/09/06 01:38:05, nhiroki wrote: > AttachClientInterface*To*Provider ? Sorry, this might be confuising. This means 'Attach "Client's InterfaceProvider" (to the host)'. I've simplified as "AttachInterfaceProvider". How about this? https://codereview.chromium.org/2313653002/diff/40001/content/renderer/servic... File content/renderer/service_worker/embedded_worker_instance_client_impl.h (right): https://codereview.chromium.org/2313653002/diff/40001/content/renderer/servic... content/renderer/service_worker/embedded_worker_instance_client_impl.h:82: void EmbeddedWorkerInstanceClientImpl::GetInterfaceOnMainThread( On 2016/09/06 01:38:05, nhiroki wrote: > Do we really need to make this thread-safe? At least for now, this is called > only from a non-main thread. Generally, it's preferable that a caller explicitly > posts a task to a target thread: > See https://codereview.chromium.org/796393005/ Done. https://codereview.chromium.org/2313653002/diff/40001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2313653002/diff/40001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:1078: void ServiceWorkerContextClient::OnDidGetEmbeddedWorkerInstance() { On 2016/09/06 01:38:05, nhiroki wrote: > DCHECK(worker_task_runner_->RunsTasksOnCurrentThread()); Done. https://codereview.chromium.org/2313653002/diff/60001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2313653002/diff/60001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:815: remote_interfaces_->Bind(std::move(client_interfaces)); On 2016/09/21 05:12:07, horo wrote: > What happens if EmbeddedWorkerInstance::GetRemoteInterfaces() is called before > AttachClientInterfaceProvider()? > > Doesn't this happen? Such situation is possible, but this is not changed from the current implementation. In that case, InterfaceProvider::GetInterface(&ptr) will be succeeded when AttachClientInterfaceProvider() is called, and the IPC calls to ptr passed to GetInterface() will be happen when it's connected.
https://codereview.chromium.org/2313653002/diff/100001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2313653002/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:704: // worker is enabled, so this code isn't necessary when the flag is enabled. Thank you for adding this comment. https://codereview.chromium.org/2313653002/diff/100001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.h (right): https://codereview.chromium.org/2313653002/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:327: // Bound when the worker thread is ready and prepares the mojo pointer. "prepares" sounds a bit strange to me because this is not a function but a field. This could be "Bound when the worker thread is ready and used to prepare a mojo pointer."? https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_instance_client_impl.cc (right): https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:59: const StopWorkerCallback& callback) { Can we have the thread check here based on a header comment? DCHECK(main_thread_task_runner_->BelongsToCurrentThread()); https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_instance_client_impl.h (right): https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:17: This class is multithreaded. Generally, it'd be preferable to have a class comment about thread affinity for such a class. For example, which thread should we create/destroy this class on? See [1] and [2] for concrete examples. [1] https://cs.chromium.org/chromium/src/content/renderer/service_worker/service_... [2] https://cs.chromium.org/chromium/src/content/browser/service_worker/service_w... https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:27: mojo::InterfaceRequest<mojom::EmbeddedWorkerInstanceClient> request); Do we need to expose both the ctor and Create()? (You don't have to address this in this CL) https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:61: // For cross thread This is obvious and may not make sense. https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:64: // Stores callbacks ditto. https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:405: // service worker is enabled. "if mojo for the service worker is enabled." Is |embedded_worker_client_| set only when the flag is enabled? If so, this could be more intuitive: if (ServiceWorkerUtils::IsMojoForServiceWorkerEnabled()) { DCHECK(embedded_worker_client_); GetInterfaceOnMainThread(...); } https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:409: mojo::GetProxy(&context_->embedded_worker_host)); How about inlining GetInterfaceOnMainThread() here because it's used only from here? Do you plan to reuse it on other places? https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.h:248: void OnDidGetEmbeddedWorkerInstance(); not used?
https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:126: void GetInterfaceOnMainThread(base::SingleThreadTaskRunner* runner, Sorry, I don't understand mojo well yet. Is it impossible to pass |remote_interfaces_| from the main thread to the worker thread?
Updated. PTAL again. https://codereview.chromium.org/2313653002/diff/100001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2313653002/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:704: // worker is enabled, so this code isn't necessary when the flag is enabled. On 2016/09/21 07:57:05, nhiroki (ooo until Sep. 30) wrote: > Thank you for adding this comment. :) https://codereview.chromium.org/2313653002/diff/100001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.h (right): https://codereview.chromium.org/2313653002/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.h:327: // Bound when the worker thread is ready and prepares the mojo pointer. On 2016/09/21 07:57:05, nhiroki (ooo until Sep. 30) wrote: > "prepares" sounds a bit strange to me because this is not a function but a > field. This could be "Bound when the worker thread is ready and used to prepare > a mojo pointer."? Done. https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_instance_client_impl.cc (right): https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:59: const StopWorkerCallback& callback) { On 2016/09/21 07:57:05, nhiroki (ooo until Sep. 30) wrote: > Can we have the thread check here based on a header comment? > > DCHECK(main_thread_task_runner_->BelongsToCurrentThread()); Done. https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_instance_client_impl.h (right): https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:17: On 2016/09/21 07:57:05, nhiroki (slow) wrote: > This class is multithreaded. Generally, it'd be preferable to have a class > comment about thread affinity for such a class. For example, which thread should > we create/destroy this class on? See [1] and [2] for concrete examples. > > [1] > https://cs.chromium.org/chromium/src/content/renderer/service_worker/service_... > [2] > https://cs.chromium.org/chromium/src/content/browser/service_worker/service_w... Done. https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:27: mojo::InterfaceRequest<mojom::EmbeddedWorkerInstanceClient> request); On 2016/09/21 07:57:05, nhiroki (slow) wrote: > Do we need to expose both the ctor and Create()? > (You don't have to address this in this CL) It used to be used for MakeStrongBinding, but in this patch private is okay for ctor. Moved. https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:61: // For cross thread On 2016/09/21 07:57:05, nhiroki (slow) wrote: > This is obvious and may not make sense. Done. https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:64: // Stores callbacks On 2016/09/21 07:57:05, nhiroki (slow) wrote: > ditto. Done. https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:126: void GetInterfaceOnMainThread(base::SingleThreadTaskRunner* runner, On 2016/09/21 10:18:59, horo wrote: > Sorry, I don't understand mojo well yet. > Is it impossible to pass |remote_interfaces_| from the main thread to the worker > thread? I think it's impossible once InterfaceProvider is connected to InterfaceRegistry. I'm planning to use |remote_interfaces_| for IPCs before a worker thread is created (workerContextFailedToStart, workerScriptLoaded() and hasAssociatedRegistration()). https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:405: // service worker is enabled. On 2016/09/21 07:57:05, nhiroki (ooo until Sep. 30) wrote: > "if mojo for the service worker is enabled." > > Is |embedded_worker_client_| set only when the flag is enabled? If so, this > could be more intuitive: > > if (ServiceWorkerUtils::IsMojoForServiceWorkerEnabled()) { > DCHECK(embedded_worker_client_); > GetInterfaceOnMainThread(...); > } Done. https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:409: mojo::GetProxy(&context_->embedded_worker_host)); On 2016/09/21 07:57:05, nhiroki (ooo until Sep. 30) wrote: > How about inlining GetInterfaceOnMainThread() here because it's used only from > here? Do you plan to reuse it on other places? Yes, I'm planning to use GetInterfaceOnMainThread in other patches, but let's inline it because I haven't created the patch yet. https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2313653002/diff/100001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.h:248: void OnDidGetEmbeddedWorkerInstance(); On 2016/09/21 07:57:05, nhiroki (ooo until Sep. 30) wrote: > not used? Done.
https://codereview.chromium.org/2313653002/diff/120001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance_unittest.cc (right): https://codereview.chromium.org/2313653002/diff/120001/content/browser/servic... content/browser/service_worker/embedded_worker_instance_unittest.cc:35: void NoOpStatusCallback(ServiceWorkerStatusCode code) {} unused? https://codereview.chromium.org/2313653002/diff/120001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_instance_client_impl.h (right): https://codereview.chromium.org/2313653002/diff/120001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:29: // This should be called on the main thread. This looks redundant in favor of the class comment. https://codereview.chromium.org/2313653002/diff/120001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:53: // These objects are bound to the main thread. ditto. https://codereview.chromium.org/2313653002/diff/120001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:64: // Used for checking the functions are called on main thread. In the latest patchset, this class doesn't do cross-thread operations, right? If we use the task runner only for checking, we could use "DCHECK(ChildThreadImpl::current());" instead and remove the task runner. https://codereview.chromium.org/2313653002/diff/120001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2313653002/diff/120001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:206: // Pending callbacks for Background Sync Events Can you add a period at the end of the comment? https://codereview.chromium.org/2313653002/diff/120001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:211: // Reference to a browser-side object corresponding to EmbeddedWorkerInstance ditto.
https://codereview.chromium.org/2313653002/diff/120001/content/common/service... File content/common/service_worker/embedded_worker.mojom (right): https://codereview.chromium.org/2313653002/diff/120001/content/common/service... content/common/service_worker/embedded_worker.mojom:16: shell.mojom.InterfaceProvider browser_interface); As we discussed offline, please consider sending InterfaceProviderRequest in StartWorker(). StartWorker(EmbeddedWorkerStartParams params, shell.mojom.InterfaceProvider& client_interfaces, shell.mojom.InterfaceProvider browser_interface) And remove AttachInterfaceProvider() from EmbeddedWorkerInstance.
Patchset #9 (id:160001) has been deleted
updated. ptal again:) https://codereview.chromium.org/2313653002/diff/120001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance_unittest.cc (right): https://codereview.chromium.org/2313653002/diff/120001/content/browser/servic... content/browser/service_worker/embedded_worker_instance_unittest.cc:35: void NoOpStatusCallback(ServiceWorkerStatusCode code) {} On 2016/10/06 05:53:21, nhiroki (slow) wrote: > unused? Done. https://codereview.chromium.org/2313653002/diff/120001/content/common/service... File content/common/service_worker/embedded_worker.mojom (right): https://codereview.chromium.org/2313653002/diff/120001/content/common/service... content/common/service_worker/embedded_worker.mojom:16: shell.mojom.InterfaceProvider browser_interface); On 2016/10/06 08:46:37, horo wrote: > As we discussed offline, please consider sending InterfaceProviderRequest in > StartWorker(). > > StartWorker(EmbeddedWorkerStartParams params, > shell.mojom.InterfaceProvider& client_interfaces, > shell.mojom.InterfaceProvider browser_interface) > > And remove AttachInterfaceProvider() from EmbeddedWorkerInstance. Done. https://codereview.chromium.org/2313653002/diff/120001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_instance_client_impl.h (right): https://codereview.chromium.org/2313653002/diff/120001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:29: // This should be called on the main thread. On 2016/10/06 05:53:21, nhiroki (slow) wrote: > This looks redundant in favor of the class comment. Done. https://codereview.chromium.org/2313653002/diff/120001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:53: // These objects are bound to the main thread. On 2016/10/06 05:53:21, nhiroki (slow) wrote: > ditto. Done. https://codereview.chromium.org/2313653002/diff/120001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:64: // Used for checking the functions are called on main thread. On 2016/10/06 05:53:21, nhiroki (slow) wrote: > In the latest patchset, this class doesn't do cross-thread operations, right? If > we use the task runner only for checking, we could use > "DCHECK(ChildThreadImpl::current());" instead and remove the task runner. Done. https://codereview.chromium.org/2313653002/diff/120001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2313653002/diff/120001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:206: // Pending callbacks for Background Sync Events On 2016/10/06 05:53:21, nhiroki (slow) wrote: > Can you add a period at the end of the comment? Done. https://codereview.chromium.org/2313653002/diff/120001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:211: // Reference to a browser-side object corresponding to EmbeddedWorkerInstance On 2016/10/06 05:53:21, nhiroki (slow) wrote: > ditto. Done.
LGTM. I'm not familiar with mojo code, so please wait for horo-san's solid reviews :) https://codereview.chromium.org/2313653002/diff/180001/content/common/service... File content/common/service_worker/embedded_worker.mojom (right): https://codereview.chromium.org/2313653002/diff/180001/content/common/service... content/common/service_worker/embedded_worker.mojom:17: service_manager.mojom.InterfaceProvider& renderer_request); Is this "&" necessary? (on other places, |renderer_request| doesn't have it) https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_instance_client_impl.cc (right): https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:49: service_manager::mojom::InterfaceProviderRequest renderer_request) { "DCHECK(ChildThreadImpl::current())" to correspond with a check on StopWorker(). https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_instance_client_impl.h (right): https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:20: // All methods should be called on the main thread. // Unless otherwise noted, all methods should be called on the main thread. ... would be more appropriate because ExposeInterfacesToBrowser() can be called from any threads. https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:31: void ExopseInterfacesToBrowser( s/Exopse/Expose https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:37: // Called from ServiceWorkerContextClient. Must call on the main thread. "Must call on the main thread" looks redundant in favor of the class comment.
looks good. https://codereview.chromium.org/2313653002/diff/180001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2313653002/diff/180001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:709: // This code is for BackgroundSync, which has been already mojofied. s/BackgroundSync/BackgroundSync and FetchEvent/ https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_instance_client_impl.h (right): https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:35: void GetInterface(mojo::InterfaceRequest<Interface> request); Who call this method? https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:79: DCHECK(ChildThreadImpl::current()); What is the purpose of this DCHECK? https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:219: service_manager::InterfaceProvider remote_interfaces; Please add comments that this |remote_interfaces| in not used when MojoForServiceWorker is enabled.
Thanks, updated. https://codereview.chromium.org/2313653002/diff/180001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2313653002/diff/180001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:709: // This code is for BackgroundSync, which has been already mojofied. On 2016/10/18 01:28:56, horo wrote: > s/BackgroundSync/BackgroundSync and FetchEvent/ Done. https://codereview.chromium.org/2313653002/diff/180001/content/common/service... File content/common/service_worker/embedded_worker.mojom (right): https://codereview.chromium.org/2313653002/diff/180001/content/common/service... content/common/service_worker/embedded_worker.mojom:17: service_manager.mojom.InterfaceProvider& renderer_request); On 2016/10/18 00:26:57, nhiroki (slow) wrote: > Is this "&" necessary? (on other places, |renderer_request| doesn't have it) Yes. In mojom file, SomeType& means InterfaceRequest<SomeType> instead of InterfacePtr<SomeType> (which is equivalent to SomeType). ex) https://cs.chromium.org/chromium/src/content/common/frame.mojom?q=file:mojom$... https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_instance_client_impl.cc (right): https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:49: service_manager::mojom::InterfaceProviderRequest renderer_request) { On 2016/10/18 00:26:57, nhiroki (slow) wrote: > "DCHECK(ChildThreadImpl::current())" to correspond with a check on StopWorker(). Done. https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_instance_client_impl.h (right): https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:20: // All methods should be called on the main thread. On 2016/10/18 00:26:57, nhiroki (slow) wrote: > // Unless otherwise noted, all methods should be called on the main thread. > > ... would be more appropriate because ExposeInterfacesToBrowser() can be called > from any threads. Done. https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:31: void ExopseInterfacesToBrowser( On 2016/10/18 00:26:57, nhiroki (slow) wrote: > s/Exopse/Expose Done. https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:35: void GetInterface(mojo::InterfaceRequest<Interface> request); On 2016/10/18 01:28:56, horo wrote: > Who call this method? Done. https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:37: // Called from ServiceWorkerContextClient. Must call on the main thread. On 2016/10/18 00:26:57, nhiroki (slow) wrote: > "Must call on the main thread" looks redundant in favor of the class comment. Done. https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.h:79: DCHECK(ChildThreadImpl::current()); On 2016/10/18 01:28:56, horo wrote: > What is the purpose of this DCHECK? For checking if this method is running on the main thread. # but this method is not used in this patch, so removed it. https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2313653002/diff/180001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:219: service_manager::InterfaceProvider remote_interfaces; On 2016/10/18 01:28:56, horo wrote: > Please add comments that this |remote_interfaces| in not used when > MojoForServiceWorker is enabled. Done.
The CQ bit was checked by shimazu@chromium.org 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.
lgtm
shimazu@chromium.org changed reviewers: + mkwst@chromium.org
mkwst@: could you take a look at mojom?
mojo LGTM.
The CQ bit was checked by shimazu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org Link to the patchset: https://codereview.chromium.org/2313653002/#ps200001 (title: "Updated comments and fixed a typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== ServiceWorker: Exchange InterfaceProviders when starting worker thread Browser process sends the InterfaceProvider bound to InterfaceRegistry on EmbeddedWorkerInstance in this patch. This enables SWContextClient to send IPCs via mojo when its context is starting. BUG=629701 ========== to ========== ServiceWorker: Exchange InterfaceProviders when starting worker thread Browser process sends the InterfaceProvider bound to InterfaceRegistry on EmbeddedWorkerInstance in this patch. This enables SWContextClient to send IPCs via mojo when its context is starting. BUG=629701 Committed: https://crrev.com/34d1b0c8709f5533ef22ba843f49fa94bf5fa6dd Cr-Commit-Position: refs/heads/master@{#425978} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/34d1b0c8709f5533ef22ba843f49fa94bf5fa6dd Cr-Commit-Position: refs/heads/master@{#425978} |
