|
|
Created:
3 years, 8 months ago by horo Modified:
3 years, 7 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, nasko+codewatch_chromium.org, yzshen+watch_chromium.org, kinuko+watch, jsbell+serviceworker_chromium.org, Yoav Weiss, tzik, jam, abarth-chromium, dglazkov+blink, darin-cc_chromium.org, gavinp+loader_chromium.org, blink-reviews, blink-reviews-api_chromium.org, blink-worker-reviews_chromium.org, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nhiroki, loading-reviews_chromium.org, loading-reviews+fetch_chromium.org, Nate Chapin, michaeln, shimazu+serviceworker_chromium.org, tyoshino+watch_chromium.org, serviceworker-reviews, Aaron Boodman, kinuko+serviceworker, horo+watch_chromium.org, darin (slow to review), yhirano Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement the infrastructure of creating WorkerFetchContext in worker global scope.
This CL implements the infrastructure of creating WorkerFetchContext in worker
global scope of Dedicated Worker, Shared Worker and Service Worker.
1. WorkerFetchContext for Dedicated Worker
1.1. DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy()
calls RenderFrameImpl::CreateWorkerFetchContext() on the main thread.
1.2. RenderFrameImpl::CreateWorkerFetchContext() gets the interface of
WorkerURLLoaderFactoryProvider which is implemented in the browser process
as WorkerURLLoaderFactoryProviderImpl, and creates a
WorkerFetchContextImpl with the interface. WorkerFetchContextImpl keeps
the interface as |provider_info_| until InitializeOnWorkerThread() is
called on the worker thread.
1.3. RenderFrameImpl::CreateWorkerFetchContext() copies the service worker
state (ServiceWorkerProviderID and IsControlledByServiceWorker) of the
frame to the WorkerFetchContextImpl.
1.4. ProvideWorkerFetchContextToWorker() creates a WorkerFetchContextHolder
which holds the WorkerFetchContextImpl and provides it to the
WorkerClients using the Supplement mechanism.
1.5. After the worker thread is created, the WorkerClients is reattached to
the worker thread in the constructor of WorkerGlobalScope.
1.6. WorkerGlobalScope::FetchContext() will be called to get the
WorkerFetchContext in ThreadableLoader::Create() When fetch() is called in
the worker thread. (This is not in this CL yet.)
1.7. When WorkerGlobalScope::FetchContext() is called for the first time,
WorkerFetchContext::Create() is called to get the WorkerFetchContextHolder
from the WorkerClients and take the WorkerFetchContextImpl from it and
create the WorkerFetchContext.
1.8. The constructor of WorkerFetchContext calls WorkerFetchContextImpl::
InitializeOnWorkerThread() which creates a ResourceDispatcher and calls
WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactoryAndRegisterClient()
in the browser process via mojo IPC with a request of URLLoaderFactory and
the pointer (ServiceWorkerWorkerClient) of itself.
1.9. WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactoryAndRegisterClient()
binds the request of URLLoaderFactory to ResourceMessageFilter and calls
ServiceWorkerContextWrapper::BindWorkerFetchContext() to bind the
ServiceWorkerWorkerClient (the pointer to WorkerFetchContextImpl in the
worker thread) with the parent frame's ServiceWorkerProviderHost. When the
controller service worker of the ServiceWorkerProviderHost changed,
WorkerFetchContextImpl::SetControllerServiceWorker() will be called via
mojo. So WorkerFetchContextImpl::IsControlledByServiceWorker() can return
the correct status.
1.10. WorkerFetchContext::CreateURLLoader() calls WorkerFetchContextImpl::
CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer
of URLLoaderFactory.
2. WorkerFetchContext for Shared Worker is created almost same as the Dedicated
Worker. The only difference is that CreateWorkerFetchContext() is called in
WebSharedWorkerImpl::OnScriptLoaderFinished().
3. WorkerFetchContext for Service Worker
3.1. WebEmbeddedWorkerImpl::StartWorkerThread() calls
ServiceWorkerContextClient::CreateServiceWorkerFetchContext() on the main
thread.
3.2. ServiceWorkerContextClient::CreateServiceWorkerFetchContext() gets the
interface of WorkerURLLoaderFactoryProvider which is implemented in the
browser process as WorkerURLLoaderFactoryProviderImpl, and creates a
ServiceWorkerFetchContextImpl with the interface.
ServiceWorkerFetchContextImpl keeps the interface as |provider_info_|
until InitializeOnWorkerThread() is called on the worker thread.
3.3. In WebEmbeddedWorkerImpl::StartWorkerThread(), the fetch context related
information (ex: DataSaverEnabled) is set to the
ServiceWorkerFetchContextImpl. (This is not in this CL yet.)
3.4. Same as 1.4. ProvideWorkerFetchContextToWorker() creates a
WorkerFetchContextHolder which holds the ServiceWorkerFetchContextImpl and
provides it to the WorkerClientsusing the Supplement mechanism.
3.5. Same as 1.5. the WorkerClients is reattached to the worker thread.
3.6. Same as 1.6.
3.7. Same as 1.7. When WorkerGlobalScope::FetchContext() is called for the
first time, WorkerFetchContext::Create() is called to get the
WorkerFetchContextHolder from the WorkerClients and take the
ServiceWorkerFetchContextImpl from it and create the WorkerFetchContext.
3.8. The constructor of WorkerFetchContext calls ServiceWorkerFetchContextImpl
::InitializeOnWorkerThread() which creates a ResourceDispatcher and calls
WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactory() in the browser
process via mojo IPC with a request of URLLoaderFactory.
3.9. WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactory() binds the
request of URLLoaderFactory to ResourceMessageFilter.
3.10. WorkerFetchContext::CreateURLLoader() calls
ServiceWorkerFetchContextImpl::CreateURLLoader() which returns a new
WebURLLoaderImpl with the pointer of URLLoaderFactory.
BUG=443374
Review-Url: https://codereview.chromium.org/2804843005
Cr-Commit-Position: refs/heads/master@{#467200}
Committed: https://chromium.googlesource.com/chromium/src/+/e612058238bb3f549a917d55f94badf32377f85c
Patch Set 1 #
Total comments: 16
Patch Set 2 : incorporated nhiroki and kinuko's comment #
Total comments: 17
Patch Set 3 : incorporated kinuko's comment #Patch Set 4 : clean up #Patch Set 5 : rebase #
Total comments: 42
Patch Set 6 : incorporated kinuko and nhiroki's comment #Patch Set 7 : rebase #Patch Set 8 : move CreateWorkerFetchContext from Platform to WebFrameClient #
Total comments: 8
Patch Set 9 : implement EmbeddedSharedWorkerStub::CreateWorkerFetchContext #Patch Set 10 : incorporated kinuko's comment #
Total comments: 30
Patch Set 11 : incorporated yhirno, nhiroki and kinuko's comment #Patch Set 12 : fix android build error #Patch Set 13 : rebase #
Total comments: 4
Patch Set 14 : incorporated yhirano's comment #
Total comments: 3
Patch Set 15 : rebase #Patch Set 16 : s/WebScheduler.h/web_scheduler.h/ #Messages
Total messages: 139 (101 generated)
The CQ bit was checked by horo@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.
The CQ bit was checked by horo@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 checked by horo@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 checked by horo@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 checked by horo@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.
The CQ bit was checked by horo@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 checked by horo@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.
The CQ bit was checked by horo@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...
Patchset #7 (id:120001) has been deleted
Patchset #6 (id:100001) has been deleted
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== test BUG= ========== to ========== Implements the infrastructure of creating WorkerFetchContext in worker global scope. This CL implements the infrastructure of creating WorkerFetchContext in worker global scope of Dedicated Worker, Shared Worker and Service Worker. 1. WorkerFetchContext for Dedicated Worker 1.1. DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() calls RendererBlinkPlatformImpl::CreateWorkerFetchContextInfo() in the main thread. 1.2. RendererBlinkPlatformImpl::CreateWorkerFetchContextInfo() gets the interface of WorkerFetchContextFactory which is implemented in the browser process as WorkerFetchContextFactoryImpl (render_process_host_impl.cc), and creates a WorkerFetchContextInfoImpl with the interface. WorkerFetchContextInfoImpl keeps the interface until CreateContext() is called in the worker thread. 1.3. In DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() the fetch context related information (ex: service worker provider id) is set to the WorkerFetchContextInfoImpl. (This is not in this CL yet.) 1.4. ProvideWorkerFetchContextInfoToWorker() provides a WorkerFetchContextInfo which holds the WorkerFetchContextInfoImpl to the WorkerClients using the Supplement mechanism. 1.5. After the worker thread is created, the WorkerClients is reattached to the worker thread in the constructor of WorkerGlobalScope. 1.6. When fetch() is called in the worker thread, WorkerFetchContext::From() is called to get the WorkerFetchContext in ThreadableLoader::Create(). (This is not in this CL yet.) 1.7. WorkerFetchContext::From() calls WorkerContextSupplement::From() to get the WorkerFetchContextInfo from the WorkerClients and creates the WorkerFetchContext by calling WorkerFetchContextInfo::CreateContext() which calls WorkerFetchContextInfoImpl::CreateContext(). 1.8. WorkerFetchContextInfoImpl::CreateContext() creates a WorkerFetchContextImpl. The constructor of it calls WorkerFetchContextFactoryImpl::CreateWorkerFetchContext() in the browser process via mojo IPC with a request of URLLoaderFactory and the pointer (mojo::ServiceWorkerClient) of itself. 1.9. WorkerFetchContextFactoryImpl::CreateWorkerFetchContext() binds the request of URLLoaderFactory to ResourceMessageFilter and calls ServiceWorkerContextWrapper::BindWorkerFetchContext() to bind the ServiceWorkerClient (the pointer to WorkerFetchContextImpl in the worker thread) with the parent frame's ServiceWorkerProviderHost. When the controller service worker of the ServiceWorkerProviderHost changed, WorkerFetchContextImpl::SetControllerServiceWorker() will be called via mojo. So WorkerFetchContextImpl::IsControlledByServiceWorker() can return the correct status. 1.10. WorkerFetchContext::CreateURLLoader() calls WorkerFetchContextImpl:: CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. 2. WorkerFetchContext for Shared Worker is created almost same as the Dedicated Worker. The only difference is that CreateWorkerFetchContextInfo() is called in WebSharedWorkerImpl::OnScriptLoaderFinished(). 3. WorkerFetchContext for Service Worker 3.1. WebEmbeddedWorkerImpl::StartWorkerThread() calls ServiceWorkerContextClient::CreateServiceWorkerFetchContextInfo() in the main thread. 3.2. ServiceWorkerContextClient::CreateServiceWorkerFetchContextInfo() gets the interface of WorkerFetchContextFactory which is implemented in the browser process as WorkerFetchContextFactoryImpl (render_process_host_impl.cc), and creates a ServiceWorkerFetchContextInfoImpl with the interface. ServiceWorkerFetchContextInfoImpl keeps the interface until CreateContext() is called in the worker thread. 3.3. In ServiceWorkerContextClient::CreateServiceWorkerFetchContextInfo(), the fetch context related information (ex: DataSaverEnabled) is set to the ServiceWorkerFetchContextInfoImpl. (This is not in this CL yet.) 3.4. Same as 1.4. ProvideWorkerFetchContextInfoToWorker() provides the ServiceWorkerFetchContextInfoImpl to the WorkerClients. 3.5. Same as 1.5. the WorkerClients is reattached to the worker thread. 3.6. Same as 1.6. WorkerFetchContext::From() is called. 3.7. Same as 1.7. WorkerFetchContext::From() calls ServiceWorkerFetchContextInfoImpl::CreateContext(). 3.8. ServiceWorkerFetchContextInfoImpl::CreateContext() creates a ServiceWorkerFetchContextImpl. The constructor of it calls WorkerFetchContextFactoryImpl::GetURLLoaderFactory() in the browser process via mojo IPC with a request of URLLoaderFactory. 3.9. WorkerFetchContextFactoryImpl::GetURLLoaderFactory() binds the request of URLLoaderFactory to ResourceMessageFilter. 3.10. WorkerFetchContext::CreateURLLoader() calls WorkerFetchContextFactoryImpl::CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. BUG=443374 ==========
Patchset #1 (id:140001) has been deleted
The CQ bit was checked by horo@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...
Patchset #1 (id:160001) has been deleted
The CQ bit was checked by horo@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...
horo@chromium.org changed reviewers: + kinuko@chromium.org
kinuko@ Could you please review this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Looking very promising! Sorry for slow comments, I have a few high-level questions (I'm pretty sure I'm missing lots of details in those questions though) before I do deeper dive. https://codereview.chromium.org/2804843005/diff/180001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2804843005/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:515: mojo::AssociatedBinding<mojom::URLLoaderFactory> url_loader_factory_binding_; Question: if ResourceMessageFilter implements its own Bind and accepts multiple requests from process could we make the browser-side impl simpler? https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebWorkerFetchContext.h (right): https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContext.h:15: class WebWorkerFetchContext { I wonder if this could be implemented using WebServiceWorkerNetworkProvider interface? Platform::CreateURLLoader could return thread-local instance of ResourceFetcher. (Putting aside if that's better design, my main interest is if it's possible to slim down the necessary changes in that way) https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContext.h:24: // handle the rquest correctly in the loading stack later. (Example: service rquest -> request https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebWorkerFetchContextInfo.h (right): https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContextInfo.h:16: // WebWorkerFetchContextInfo is created in a main thread and passed to worker nit: in a main thread -> on the main thread passed to worker -> passed to a worker https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContextInfo.h:17: // (dedicated, shared, service worker). It contins a information about the a information -> information
nhiroki@chromium.org changed reviewers: + nhiroki@chromium.org
DBC from the core/worker POV. https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp (right): https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp:43: ->ToSingleThreadTaskRunner()); FYI: CurrentThread()->Scheduler()->LoadingTaskRunner() is not encouraged after the per-global-scope scheduler is landed[1]. This should be replaced with... TaskRunnerHelper::Get(TaskType::kUnspecedLoading, global_scope)->ToSingleThreadTaskRunner(); [1] https://codereview.chromium.org/2806623004/ https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp:60: static WorkerContextSupplement* From(ExecutionContext& execution_context) { Since ExecutionContext must not be accessed from the non-context thread, can you add DCHECK(execution_context->IsContextThread()); here? https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerFetchContext.h (right): https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerFetchContext.h:12: #include "wtf/Forward.h" wtf/Forward.h is deprecated. Can you replace this with platform/wtf/Forward.h?
Description was changed from ========== Implements the infrastructure of creating WorkerFetchContext in worker global scope. This CL implements the infrastructure of creating WorkerFetchContext in worker global scope of Dedicated Worker, Shared Worker and Service Worker. 1. WorkerFetchContext for Dedicated Worker 1.1. DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() calls RendererBlinkPlatformImpl::CreateWorkerFetchContextInfo() in the main thread. 1.2. RendererBlinkPlatformImpl::CreateWorkerFetchContextInfo() gets the interface of WorkerFetchContextFactory which is implemented in the browser process as WorkerFetchContextFactoryImpl (render_process_host_impl.cc), and creates a WorkerFetchContextInfoImpl with the interface. WorkerFetchContextInfoImpl keeps the interface until CreateContext() is called in the worker thread. 1.3. In DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() the fetch context related information (ex: service worker provider id) is set to the WorkerFetchContextInfoImpl. (This is not in this CL yet.) 1.4. ProvideWorkerFetchContextInfoToWorker() provides a WorkerFetchContextInfo which holds the WorkerFetchContextInfoImpl to the WorkerClients using the Supplement mechanism. 1.5. After the worker thread is created, the WorkerClients is reattached to the worker thread in the constructor of WorkerGlobalScope. 1.6. When fetch() is called in the worker thread, WorkerFetchContext::From() is called to get the WorkerFetchContext in ThreadableLoader::Create(). (This is not in this CL yet.) 1.7. WorkerFetchContext::From() calls WorkerContextSupplement::From() to get the WorkerFetchContextInfo from the WorkerClients and creates the WorkerFetchContext by calling WorkerFetchContextInfo::CreateContext() which calls WorkerFetchContextInfoImpl::CreateContext(). 1.8. WorkerFetchContextInfoImpl::CreateContext() creates a WorkerFetchContextImpl. The constructor of it calls WorkerFetchContextFactoryImpl::CreateWorkerFetchContext() in the browser process via mojo IPC with a request of URLLoaderFactory and the pointer (mojo::ServiceWorkerClient) of itself. 1.9. WorkerFetchContextFactoryImpl::CreateWorkerFetchContext() binds the request of URLLoaderFactory to ResourceMessageFilter and calls ServiceWorkerContextWrapper::BindWorkerFetchContext() to bind the ServiceWorkerClient (the pointer to WorkerFetchContextImpl in the worker thread) with the parent frame's ServiceWorkerProviderHost. When the controller service worker of the ServiceWorkerProviderHost changed, WorkerFetchContextImpl::SetControllerServiceWorker() will be called via mojo. So WorkerFetchContextImpl::IsControlledByServiceWorker() can return the correct status. 1.10. WorkerFetchContext::CreateURLLoader() calls WorkerFetchContextImpl:: CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. 2. WorkerFetchContext for Shared Worker is created almost same as the Dedicated Worker. The only difference is that CreateWorkerFetchContextInfo() is called in WebSharedWorkerImpl::OnScriptLoaderFinished(). 3. WorkerFetchContext for Service Worker 3.1. WebEmbeddedWorkerImpl::StartWorkerThread() calls ServiceWorkerContextClient::CreateServiceWorkerFetchContextInfo() in the main thread. 3.2. ServiceWorkerContextClient::CreateServiceWorkerFetchContextInfo() gets the interface of WorkerFetchContextFactory which is implemented in the browser process as WorkerFetchContextFactoryImpl (render_process_host_impl.cc), and creates a ServiceWorkerFetchContextInfoImpl with the interface. ServiceWorkerFetchContextInfoImpl keeps the interface until CreateContext() is called in the worker thread. 3.3. In ServiceWorkerContextClient::CreateServiceWorkerFetchContextInfo(), the fetch context related information (ex: DataSaverEnabled) is set to the ServiceWorkerFetchContextInfoImpl. (This is not in this CL yet.) 3.4. Same as 1.4. ProvideWorkerFetchContextInfoToWorker() provides the ServiceWorkerFetchContextInfoImpl to the WorkerClients. 3.5. Same as 1.5. the WorkerClients is reattached to the worker thread. 3.6. Same as 1.6. WorkerFetchContext::From() is called. 3.7. Same as 1.7. WorkerFetchContext::From() calls ServiceWorkerFetchContextInfoImpl::CreateContext(). 3.8. ServiceWorkerFetchContextInfoImpl::CreateContext() creates a ServiceWorkerFetchContextImpl. The constructor of it calls WorkerFetchContextFactoryImpl::GetURLLoaderFactory() in the browser process via mojo IPC with a request of URLLoaderFactory. 3.9. WorkerFetchContextFactoryImpl::GetURLLoaderFactory() binds the request of URLLoaderFactory to ResourceMessageFilter. 3.10. WorkerFetchContext::CreateURLLoader() calls WorkerFetchContextFactoryImpl::CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. BUG=443374 ========== to ========== Implement the infrastructure of creating WorkerFetchContext in worker global scope. This CL implements the infrastructure of creating WorkerFetchContext in worker global scope of Dedicated Worker, Shared Worker and Service Worker. 1. WorkerFetchContext for Dedicated Worker 1.1. DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() calls RendererBlinkPlatformImpl::CreateWorkerFetchContextInfo() in the main thread. 1.2. RendererBlinkPlatformImpl::CreateWorkerFetchContextInfo() gets the interface of WorkerFetchContextFactory which is implemented in the browser process as WorkerFetchContextFactoryImpl (render_process_host_impl.cc), and creates a WorkerFetchContextInfoImpl with the interface. WorkerFetchContextInfoImpl keeps the interface until CreateContext() is called in the worker thread. 1.3. In DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() the fetch context related information (ex: service worker provider id) is set to the WorkerFetchContextInfoImpl. (This is not in this CL yet.) 1.4. ProvideWorkerFetchContextInfoToWorker() provides a WorkerFetchContextInfo which holds the WorkerFetchContextInfoImpl to the WorkerClients using the Supplement mechanism. 1.5. After the worker thread is created, the WorkerClients is reattached to the worker thread in the constructor of WorkerGlobalScope. 1.6. When fetch() is called in the worker thread, WorkerFetchContext::From() is called to get the WorkerFetchContext in ThreadableLoader::Create(). (This is not in this CL yet.) 1.7. WorkerFetchContext::From() calls WorkerContextSupplement::From() to get the WorkerFetchContextInfo from the WorkerClients and creates the WorkerFetchContext by calling WorkerFetchContextInfo::CreateContext() which calls WorkerFetchContextInfoImpl::CreateContext(). 1.8. WorkerFetchContextInfoImpl::CreateContext() creates a WorkerFetchContextImpl. The constructor of it calls WorkerFetchContextFactoryImpl::CreateWorkerFetchContext() in the browser process via mojo IPC with a request of URLLoaderFactory and the pointer (mojo::ServiceWorkerClient) of itself. 1.9. WorkerFetchContextFactoryImpl::CreateWorkerFetchContext() binds the request of URLLoaderFactory to ResourceMessageFilter and calls ServiceWorkerContextWrapper::BindWorkerFetchContext() to bind the ServiceWorkerClient (the pointer to WorkerFetchContextImpl in the worker thread) with the parent frame's ServiceWorkerProviderHost. When the controller service worker of the ServiceWorkerProviderHost changed, WorkerFetchContextImpl::SetControllerServiceWorker() will be called via mojo. So WorkerFetchContextImpl::IsControlledByServiceWorker() can return the correct status. 1.10. WorkerFetchContext::CreateURLLoader() calls WorkerFetchContextImpl:: CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. 2. WorkerFetchContext for Shared Worker is created almost same as the Dedicated Worker. The only difference is that CreateWorkerFetchContextInfo() is called in WebSharedWorkerImpl::OnScriptLoaderFinished(). 3. WorkerFetchContext for Service Worker 3.1. WebEmbeddedWorkerImpl::StartWorkerThread() calls ServiceWorkerContextClient::CreateServiceWorkerFetchContextInfo() in the main thread. 3.2. ServiceWorkerContextClient::CreateServiceWorkerFetchContextInfo() gets the interface of WorkerFetchContextFactory which is implemented in the browser process as WorkerFetchContextFactoryImpl (render_process_host_impl.cc), and creates a ServiceWorkerFetchContextInfoImpl with the interface. ServiceWorkerFetchContextInfoImpl keeps the interface until CreateContext() is called in the worker thread. 3.3. In ServiceWorkerContextClient::CreateServiceWorkerFetchContextInfo(), the fetch context related information (ex: DataSaverEnabled) is set to the ServiceWorkerFetchContextInfoImpl. (This is not in this CL yet.) 3.4. Same as 1.4. ProvideWorkerFetchContextInfoToWorker() provides the ServiceWorkerFetchContextInfoImpl to the WorkerClients. 3.5. Same as 1.5. the WorkerClients is reattached to the worker thread. 3.6. Same as 1.6. WorkerFetchContext::From() is called. 3.7. Same as 1.7. WorkerFetchContext::From() calls ServiceWorkerFetchContextInfoImpl::CreateContext(). 3.8. ServiceWorkerFetchContextInfoImpl::CreateContext() creates a ServiceWorkerFetchContextImpl. The constructor of it calls WorkerFetchContextFactoryImpl::GetURLLoaderFactory() in the browser process via mojo IPC with a request of URLLoaderFactory. 3.9. WorkerFetchContextFactoryImpl::GetURLLoaderFactory() binds the request of URLLoaderFactory to ResourceMessageFilter. 3.10. WorkerFetchContext::CreateURLLoader() calls WorkerFetchContextFactoryImpl::CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. BUG=443374 ==========
(Just noting what we discussed offline) Will take further look. https://codereview.chromium.org/2804843005/diff/180001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2804843005/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:515: mojo::AssociatedBinding<mojom::URLLoaderFactory> url_loader_factory_binding_; On 2017/04/14 02:27:02, kinuko wrote: > Question: if ResourceMessageFilter implements its own Bind and accepts multiple > requests from process could we make the browser-side impl simpler? Per offline discussion it looks it's not possible to use channel-associated interface from worker thread, so we need to establish different message pipe. (And for SW we also want communication channel for propagating controlling status change) https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebWorkerFetchContext.h (right): https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContext.h:15: class WebWorkerFetchContext { On 2017/04/14 02:27:02, kinuko wrote: > I wonder if this could be implemented using WebServiceWorkerNetworkProvider > interface? Platform::CreateURLLoader could return thread-local instance of > ResourceFetcher. (Putting aside if that's better design, my main interest is if > it's possible to slim down the necessary changes in that way) This part could be probably cleaned up a bit but we agreed that this's not only about SW-specific part so it might make sense to have this.
The CQ bit was checked by horo@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...
https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp (right): https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp:43: ->ToSingleThreadTaskRunner()); On 2017/04/14 04:09:24, nhiroki wrote: > FYI: CurrentThread()->Scheduler()->LoadingTaskRunner() is not encouraged after > the per-global-scope scheduler is landed[1]. This should be replaced with... > > TaskRunnerHelper::Get(TaskType::kUnspecedLoading, > global_scope)->ToSingleThreadTaskRunner(); > > [1] https://codereview.chromium.org/2806623004/ Acknowledged. Thank you for letting me know. https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp:60: static WorkerContextSupplement* From(ExecutionContext& execution_context) { On 2017/04/14 04:09:24, nhiroki wrote: > Since ExecutionContext must not be accessed from the non-context thread, can you > add DCHECK(execution_context->IsContextThread()); here? Done. https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerFetchContext.h (right): https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerFetchContext.h:12: #include "wtf/Forward.h" On 2017/04/14 04:09:24, nhiroki wrote: > wtf/Forward.h is deprecated. Can you replace this with platform/wtf/Forward.h? Done. https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebWorkerFetchContext.h (right): https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContext.h:24: // handle the rquest correctly in the loading stack later. (Example: service On 2017/04/14 02:27:02, kinuko wrote: > rquest -> request Done. https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebWorkerFetchContextInfo.h (right): https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContextInfo.h:16: // WebWorkerFetchContextInfo is created in a main thread and passed to worker On 2017/04/14 02:27:02, kinuko wrote: > nit: in a main thread -> on the main thread > passed to worker -> passed to a worker Done. https://codereview.chromium.org/2804843005/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContextInfo.h:17: // (dedicated, shared, service worker). It contins a information about the On 2017/04/14 02:27:02, kinuko wrote: > a information -> information Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Some more comments. Overall this is looking good. (I think I'll probably have more comments as I digest this, I hope you can bear with me) https://codereview.chromium.org/2804843005/diff/200001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2804843005/diff/200001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1352: service_worker_context)); nit: it feels a little strange why we pass RMF as unretained raw ptr (while it's ref-counted and also supports WeakPtr) while we pass ServiceWorkerContext as a ref-ptr https://codereview.chromium.org/2804843005/diff/200001/content/common/worker_... File content/common/worker_fetch_context_factory.mojom (right): https://codereview.chromium.org/2804843005/diff/200001/content/common/worker_... content/common/worker_fetch_context_factory.mojom:10: // Called when the ServiceWorkerClient (which is Dedicated Worker or Shared Too generic name if it's only for clients that live on worker threads? https://codereview.chromium.org/2804843005/diff/200001/content/common/worker_... content/common/worker_fetch_context_factory.mojom:15: interface WorkerFetchContextFactory { Can we also have a interface-level comment? I'm having slightly uneasy feeling about this interface name (and method names). While it's true that this is for FetchContext in Blink context, calling this WorkerURLLoaderFactoryProvider or something like that may make better sense in mojo / browser-side context? https://codereview.chromium.org/2804843005/diff/200001/content/common/worker_... content/common/worker_fetch_context_factory.mojom:25: int32 service_worker_provider_id); Similarly, this seems to be more about setting up URLLoaderFactory (as well as returning a control channel for SW). https://codereview.chromium.org/2804843005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp (right): https://codereview.chromium.org/2804843005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp:56: public Supplement<ExecutionContext> { Not fully sure why we need this class? Can't WorkerFetchContext inherit Supplement<WorkerClients> and just be provided to / retrieved from WorkerClients? Or it looks it's also possible that WorkerGlobalScope just has WorkerFetchContext as a member field (we're not in modules/ here). https://codereview.chromium.org/2804843005/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebWorkerFetchContextInfo.h (right): https://codereview.chromium.org/2804843005/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContextInfo.h:17: // worker (dedicated, shared, service worker). It contins information about the contins -> contains https://codereview.chromium.org/2804843005/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContextInfo.h:19: class WebWorkerFetchContextInfo { I wonder if it's possible to only expose WebWorkerFetchContext. I think passing mojo PtrInfo from main thread to worker thread is the main motivation of WebWorkerFetchContextInfo, is that right? Naively it feels it's possible that context directly implements WebWorkerFetchContext, and expose a method like InitializeOnWorkerThread() where we initialize resource dispatcher and binding_. (While-- if it didn't require additional public interfaces I would have been just happy to have this info class)
The CQ bit was checked by horo@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...
Patchset #3 (id:220001) has been deleted
The CQ bit was checked by horo@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...
Description was changed from ========== Implement the infrastructure of creating WorkerFetchContext in worker global scope. This CL implements the infrastructure of creating WorkerFetchContext in worker global scope of Dedicated Worker, Shared Worker and Service Worker. 1. WorkerFetchContext for Dedicated Worker 1.1. DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() calls RendererBlinkPlatformImpl::CreateWorkerFetchContextInfo() in the main thread. 1.2. RendererBlinkPlatformImpl::CreateWorkerFetchContextInfo() gets the interface of WorkerFetchContextFactory which is implemented in the browser process as WorkerFetchContextFactoryImpl (render_process_host_impl.cc), and creates a WorkerFetchContextInfoImpl with the interface. WorkerFetchContextInfoImpl keeps the interface until CreateContext() is called in the worker thread. 1.3. In DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() the fetch context related information (ex: service worker provider id) is set to the WorkerFetchContextInfoImpl. (This is not in this CL yet.) 1.4. ProvideWorkerFetchContextInfoToWorker() provides a WorkerFetchContextInfo which holds the WorkerFetchContextInfoImpl to the WorkerClients using the Supplement mechanism. 1.5. After the worker thread is created, the WorkerClients is reattached to the worker thread in the constructor of WorkerGlobalScope. 1.6. When fetch() is called in the worker thread, WorkerFetchContext::From() is called to get the WorkerFetchContext in ThreadableLoader::Create(). (This is not in this CL yet.) 1.7. WorkerFetchContext::From() calls WorkerContextSupplement::From() to get the WorkerFetchContextInfo from the WorkerClients and creates the WorkerFetchContext by calling WorkerFetchContextInfo::CreateContext() which calls WorkerFetchContextInfoImpl::CreateContext(). 1.8. WorkerFetchContextInfoImpl::CreateContext() creates a WorkerFetchContextImpl. The constructor of it calls WorkerFetchContextFactoryImpl::CreateWorkerFetchContext() in the browser process via mojo IPC with a request of URLLoaderFactory and the pointer (mojo::ServiceWorkerClient) of itself. 1.9. WorkerFetchContextFactoryImpl::CreateWorkerFetchContext() binds the request of URLLoaderFactory to ResourceMessageFilter and calls ServiceWorkerContextWrapper::BindWorkerFetchContext() to bind the ServiceWorkerClient (the pointer to WorkerFetchContextImpl in the worker thread) with the parent frame's ServiceWorkerProviderHost. When the controller service worker of the ServiceWorkerProviderHost changed, WorkerFetchContextImpl::SetControllerServiceWorker() will be called via mojo. So WorkerFetchContextImpl::IsControlledByServiceWorker() can return the correct status. 1.10. WorkerFetchContext::CreateURLLoader() calls WorkerFetchContextImpl:: CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. 2. WorkerFetchContext for Shared Worker is created almost same as the Dedicated Worker. The only difference is that CreateWorkerFetchContextInfo() is called in WebSharedWorkerImpl::OnScriptLoaderFinished(). 3. WorkerFetchContext for Service Worker 3.1. WebEmbeddedWorkerImpl::StartWorkerThread() calls ServiceWorkerContextClient::CreateServiceWorkerFetchContextInfo() in the main thread. 3.2. ServiceWorkerContextClient::CreateServiceWorkerFetchContextInfo() gets the interface of WorkerFetchContextFactory which is implemented in the browser process as WorkerFetchContextFactoryImpl (render_process_host_impl.cc), and creates a ServiceWorkerFetchContextInfoImpl with the interface. ServiceWorkerFetchContextInfoImpl keeps the interface until CreateContext() is called in the worker thread. 3.3. In ServiceWorkerContextClient::CreateServiceWorkerFetchContextInfo(), the fetch context related information (ex: DataSaverEnabled) is set to the ServiceWorkerFetchContextInfoImpl. (This is not in this CL yet.) 3.4. Same as 1.4. ProvideWorkerFetchContextInfoToWorker() provides the ServiceWorkerFetchContextInfoImpl to the WorkerClients. 3.5. Same as 1.5. the WorkerClients is reattached to the worker thread. 3.6. Same as 1.6. WorkerFetchContext::From() is called. 3.7. Same as 1.7. WorkerFetchContext::From() calls ServiceWorkerFetchContextInfoImpl::CreateContext(). 3.8. ServiceWorkerFetchContextInfoImpl::CreateContext() creates a ServiceWorkerFetchContextImpl. The constructor of it calls WorkerFetchContextFactoryImpl::GetURLLoaderFactory() in the browser process via mojo IPC with a request of URLLoaderFactory. 3.9. WorkerFetchContextFactoryImpl::GetURLLoaderFactory() binds the request of URLLoaderFactory to ResourceMessageFilter. 3.10. WorkerFetchContext::CreateURLLoader() calls WorkerFetchContextFactoryImpl::CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. BUG=443374 ========== to ========== Implement the infrastructure of creating WorkerFetchContext in worker global scope. This CL implements the infrastructure of creating WorkerFetchContext in worker global scope of Dedicated Worker, Shared Worker and Service Worker. 1. WorkerFetchContext for Dedicated Worker 1.1. DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() calls RendererBlinkPlatformImpl::CreateWorkerFetchContext() in the main thread. 1.2. RendererBlinkPlatformImpl::CreateWorkerFetchContext() gets the interface of WorkerURLLoaderFactoryProvider which is implemented in the browser process as WorkerURLLoaderFactoryProviderImpl, and creates a WorkerFetchContextImpl with the interface. WorkerFetchContextImpl keeps the interface as |provider_info_| until InitializeOnWorkerThread() is called in the worker thread. 1.3. In DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() the fetch context related information (ex: service worker provider id) is set to the WorkerFetchContextImpl. (This is not in this CL yet.) 1.4. ProvideWorkerFetchContextToWorker() creates a WorkerFetchContextInfo which holds the WorkerFetchContextImpl and provides it to the WorkerClients using the Supplement mechanism. 1.5. After the worker thread is created, the WorkerClients is reattached to the worker thread in the constructor of WorkerGlobalScope. 1.6. WorkerGlobalScope::FetchContext() will be called to get the WorkerFetchContext in ThreadableLoader::Create() When fetch() is called in the worker thread. (This is not in this CL yet.) 1.7. When WorkerGlobalScope::FetchContext() is called for the first time, WorkerFetchContext::Create() is called to get the WorkerFetchContextInfo from the WorkerClients and take the WorkerFetchContextImpl from it and create the WorkerFetchContext. 1.8. The constructor of WorkerFetchContext calls WorkerFetchContextImpl:: InitializeOnWorkerThread() which creates a ResourceDispatcher and calls WorkerURLLoaderFactoryProviderImpl::CreateWorkerFetchContext() in the browser process via mojo IPC with a request of URLLoaderFactory and the pointer (ServiceWorkerWorkerClient) of itself. 1.9. WorkerURLLoaderFactoryProviderImpl::CreateWorkerFetchContext() binds the request of URLLoaderFactory to ResourceMessageFilter and calls ServiceWorkerContextWrapper::BindWorkerFetchContext() to bind the ServiceWorkerWorkerClient (the pointer to WorkerFetchContextImpl in the worker thread) with the parent frame's ServiceWorkerProviderHost. When the controller service worker of the ServiceWorkerProviderHost changed, WorkerFetchContextImpl::SetControllerServiceWorker() will be called via mojo. So WorkerFetchContextImpl::IsControlledByServiceWorker() can return the correct status. 1.10. WorkerFetchContext::CreateURLLoader() calls WorkerFetchContextImpl:: CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. 2. WorkerFetchContext for Shared Worker is created almost same as the Dedicated Worker. The only difference is that CreateWorkerFetchContext() is called in WebSharedWorkerImpl::OnScriptLoaderFinished(). 3. WorkerFetchContext for Service Worker 3.1. WebEmbeddedWorkerImpl::StartWorkerThread() calls ServiceWorkerContextClient::CreateServiceWorkerFetchContext() in the main thread. 3.2. ServiceWorkerContextClient::CreateServiceWorkerFetchContext() gets the interface of WorkerURLLoaderFactoryProvider which is implemented in the browser process as WorkerURLLoaderFactoryProviderImpl, and creates a ServiceWorkerFetchContextImpl with the interface. ServiceWorkerFetchContextImpl keeps the interface as |provider_info_| until InitializeOnWorkerThread() is called in the worker thread. 3.3. In WebEmbeddedWorkerImpl::StartWorkerThread(), the fetch context related information (ex: DataSaverEnabled) is set to the ServiceWorkerFetchContextImpl. (This is not in this CL yet.) 3.4. Same as 1.4. ProvideWorkerFetchContextToWorker() creates a WorkerFetchContextInfo which holds the ServiceWorkerFetchContextImpl and provides it to the WorkerClientsusing the Supplement mechanism. 3.5. Same as 1.5. the WorkerClients is reattached to the worker thread. 3.6. Same as 1.6. 3.7. Same as 1.7. 3.8. The constructor of WorkerFetchContext calls ServiceWorkerFetchContextImpl ::InitializeOnWorkerThread() which creates a ResourceDispatcher and calls WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactory() in the browser process via mojo IPC with a request of URLLoaderFactory. 3.9. WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactory() binds the request of URLLoaderFactory to ResourceMessageFilter. 3.10. WorkerFetchContext::CreateURLLoader() calls ServiceWorkerFetchContextImpl::CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. BUG=443374 ==========
Description was changed from ========== Implement the infrastructure of creating WorkerFetchContext in worker global scope. This CL implements the infrastructure of creating WorkerFetchContext in worker global scope of Dedicated Worker, Shared Worker and Service Worker. 1. WorkerFetchContext for Dedicated Worker 1.1. DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() calls RendererBlinkPlatformImpl::CreateWorkerFetchContext() in the main thread. 1.2. RendererBlinkPlatformImpl::CreateWorkerFetchContext() gets the interface of WorkerURLLoaderFactoryProvider which is implemented in the browser process as WorkerURLLoaderFactoryProviderImpl, and creates a WorkerFetchContextImpl with the interface. WorkerFetchContextImpl keeps the interface as |provider_info_| until InitializeOnWorkerThread() is called in the worker thread. 1.3. In DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() the fetch context related information (ex: service worker provider id) is set to the WorkerFetchContextImpl. (This is not in this CL yet.) 1.4. ProvideWorkerFetchContextToWorker() creates a WorkerFetchContextInfo which holds the WorkerFetchContextImpl and provides it to the WorkerClients using the Supplement mechanism. 1.5. After the worker thread is created, the WorkerClients is reattached to the worker thread in the constructor of WorkerGlobalScope. 1.6. WorkerGlobalScope::FetchContext() will be called to get the WorkerFetchContext in ThreadableLoader::Create() When fetch() is called in the worker thread. (This is not in this CL yet.) 1.7. When WorkerGlobalScope::FetchContext() is called for the first time, WorkerFetchContext::Create() is called to get the WorkerFetchContextInfo from the WorkerClients and take the WorkerFetchContextImpl from it and create the WorkerFetchContext. 1.8. The constructor of WorkerFetchContext calls WorkerFetchContextImpl:: InitializeOnWorkerThread() which creates a ResourceDispatcher and calls WorkerURLLoaderFactoryProviderImpl::CreateWorkerFetchContext() in the browser process via mojo IPC with a request of URLLoaderFactory and the pointer (ServiceWorkerWorkerClient) of itself. 1.9. WorkerURLLoaderFactoryProviderImpl::CreateWorkerFetchContext() binds the request of URLLoaderFactory to ResourceMessageFilter and calls ServiceWorkerContextWrapper::BindWorkerFetchContext() to bind the ServiceWorkerWorkerClient (the pointer to WorkerFetchContextImpl in the worker thread) with the parent frame's ServiceWorkerProviderHost. When the controller service worker of the ServiceWorkerProviderHost changed, WorkerFetchContextImpl::SetControllerServiceWorker() will be called via mojo. So WorkerFetchContextImpl::IsControlledByServiceWorker() can return the correct status. 1.10. WorkerFetchContext::CreateURLLoader() calls WorkerFetchContextImpl:: CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. 2. WorkerFetchContext for Shared Worker is created almost same as the Dedicated Worker. The only difference is that CreateWorkerFetchContext() is called in WebSharedWorkerImpl::OnScriptLoaderFinished(). 3. WorkerFetchContext for Service Worker 3.1. WebEmbeddedWorkerImpl::StartWorkerThread() calls ServiceWorkerContextClient::CreateServiceWorkerFetchContext() in the main thread. 3.2. ServiceWorkerContextClient::CreateServiceWorkerFetchContext() gets the interface of WorkerURLLoaderFactoryProvider which is implemented in the browser process as WorkerURLLoaderFactoryProviderImpl, and creates a ServiceWorkerFetchContextImpl with the interface. ServiceWorkerFetchContextImpl keeps the interface as |provider_info_| until InitializeOnWorkerThread() is called in the worker thread. 3.3. In WebEmbeddedWorkerImpl::StartWorkerThread(), the fetch context related information (ex: DataSaverEnabled) is set to the ServiceWorkerFetchContextImpl. (This is not in this CL yet.) 3.4. Same as 1.4. ProvideWorkerFetchContextToWorker() creates a WorkerFetchContextInfo which holds the ServiceWorkerFetchContextImpl and provides it to the WorkerClientsusing the Supplement mechanism. 3.5. Same as 1.5. the WorkerClients is reattached to the worker thread. 3.6. Same as 1.6. 3.7. Same as 1.7. 3.8. The constructor of WorkerFetchContext calls ServiceWorkerFetchContextImpl ::InitializeOnWorkerThread() which creates a ResourceDispatcher and calls WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactory() in the browser process via mojo IPC with a request of URLLoaderFactory. 3.9. WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactory() binds the request of URLLoaderFactory to ResourceMessageFilter. 3.10. WorkerFetchContext::CreateURLLoader() calls ServiceWorkerFetchContextImpl::CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. BUG=443374 ========== to ========== Implement the infrastructure of creating WorkerFetchContext in worker global scope. This CL implements the infrastructure of creating WorkerFetchContext in worker global scope of Dedicated Worker, Shared Worker and Service Worker. 1. WorkerFetchContext for Dedicated Worker 1.1. DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() calls RendererBlinkPlatformImpl::CreateWorkerFetchContext() in the main thread. 1.2. RendererBlinkPlatformImpl::CreateWorkerFetchContext() gets the interface of WorkerURLLoaderFactoryProvider which is implemented in the browser process as WorkerURLLoaderFactoryProviderImpl, and creates a WorkerFetchContextImpl with the interface. WorkerFetchContextImpl keeps the interface as |provider_info_| until InitializeOnWorkerThread() is called in the worker thread. 1.3. In DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() the fetch context related information (ex: service worker provider id) is set to the WorkerFetchContextImpl. (This is not in this CL yet.) 1.4. ProvideWorkerFetchContextToWorker() creates a WorkerFetchContextInfo which holds the WorkerFetchContextImpl and provides it to the WorkerClients using the Supplement mechanism. 1.5. After the worker thread is created, the WorkerClients is reattached to the worker thread in the constructor of WorkerGlobalScope. 1.6. WorkerGlobalScope::FetchContext() will be called to get the WorkerFetchContext in ThreadableLoader::Create() When fetch() is called in the worker thread. (This is not in this CL yet.) 1.7. When WorkerGlobalScope::FetchContext() is called for the first time, WorkerFetchContext::Create() is called to get the WorkerFetchContextInfo from the WorkerClients and take the WorkerFetchContextImpl from it and create the WorkerFetchContext. 1.8. The constructor of WorkerFetchContext calls WorkerFetchContextImpl:: InitializeOnWorkerThread() which creates a ResourceDispatcher and calls WorkerURLLoaderFactoryProviderImpl::CreateWorkerFetchContext() in the browser process via mojo IPC with a request of URLLoaderFactory and the pointer (ServiceWorkerWorkerClient) of itself. 1.9. WorkerURLLoaderFactoryProviderImpl::CreateWorkerFetchContext() binds the request of URLLoaderFactory to ResourceMessageFilter and calls ServiceWorkerContextWrapper::BindWorkerFetchContext() to bind the ServiceWorkerWorkerClient (the pointer to WorkerFetchContextImpl in the worker thread) with the parent frame's ServiceWorkerProviderHost. When the controller service worker of the ServiceWorkerProviderHost changed, WorkerFetchContextImpl::SetControllerServiceWorker() will be called via mojo. So WorkerFetchContextImpl::IsControlledByServiceWorker() can return the correct status. 1.10. WorkerFetchContext::CreateURLLoader() calls WorkerFetchContextImpl:: CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. 2. WorkerFetchContext for Shared Worker is created almost same as the Dedicated Worker. The only difference is that CreateWorkerFetchContext() is called in WebSharedWorkerImpl::OnScriptLoaderFinished(). 3. WorkerFetchContext for Service Worker 3.1. WebEmbeddedWorkerImpl::StartWorkerThread() calls ServiceWorkerContextClient::CreateServiceWorkerFetchContext() in the main thread. 3.2. ServiceWorkerContextClient::CreateServiceWorkerFetchContext() gets the interface of WorkerURLLoaderFactoryProvider which is implemented in the browser process as WorkerURLLoaderFactoryProviderImpl, and creates a ServiceWorkerFetchContextImpl with the interface. ServiceWorkerFetchContextImpl keeps the interface as |provider_info_| until InitializeOnWorkerThread() is called in the worker thread. 3.3. In WebEmbeddedWorkerImpl::StartWorkerThread(), the fetch context related information (ex: DataSaverEnabled) is set to the ServiceWorkerFetchContextImpl. (This is not in this CL yet.) 3.4. Same as 1.4. ProvideWorkerFetchContextToWorker() creates a WorkerFetchContextInfo which holds the ServiceWorkerFetchContextImpl and provides it to the WorkerClientsusing the Supplement mechanism. 3.5. Same as 1.5. the WorkerClients is reattached to the worker thread. 3.6. Same as 1.6. 3.7. Same as 1.7. When WorkerGlobalScope::FetchContext() is called for the first time, WorkerFetchContext::Create() is called to get the WorkerFetchContextInfo from the WorkerClients and take the ServiceWorkerFetchContextImpl from it and create the WorkerFetchContext. 3.8. The constructor of WorkerFetchContext calls ServiceWorkerFetchContextImpl ::InitializeOnWorkerThread() which creates a ResourceDispatcher and calls WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactory() in the browser process via mojo IPC with a request of URLLoaderFactory. 3.9. WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactory() binds the request of URLLoaderFactory to ResourceMessageFilter. 3.10. WorkerFetchContext::CreateURLLoader() calls ServiceWorkerFetchContextImpl::CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. BUG=443374 ==========
https://codereview.chromium.org/2804843005/diff/200001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2804843005/diff/200001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1352: service_worker_context)); On 2017/04/14 14:00:26, kinuko wrote: > nit: it feels a little strange why we pass RMF as unretained raw ptr (while it's > ref-counted and also supports WeakPtr) while we pass ServiceWorkerContext as a > ref-ptr Done. https://codereview.chromium.org/2804843005/diff/200001/content/common/worker_... File content/common/worker_fetch_context_factory.mojom (right): https://codereview.chromium.org/2804843005/diff/200001/content/common/worker_... content/common/worker_fetch_context_factory.mojom:10: // Called when the ServiceWorkerClient (which is Dedicated Worker or Shared On 2017/04/14 14:00:26, kinuko wrote: > Too generic name if it's only for clients that live on worker threads? Done. https://codereview.chromium.org/2804843005/diff/200001/content/common/worker_... content/common/worker_fetch_context_factory.mojom:15: interface WorkerFetchContextFactory { On 2017/04/14 14:00:26, kinuko wrote: > Can we also have a interface-level comment? > > I'm having slightly uneasy feeling about this interface name (and method names). > While it's true that this is for FetchContext in Blink context, calling this > WorkerURLLoaderFactoryProvider or something like that may make better sense in > mojo / browser-side context? Done. https://codereview.chromium.org/2804843005/diff/200001/content/common/worker_... content/common/worker_fetch_context_factory.mojom:25: int32 service_worker_provider_id); On 2017/04/14 14:00:26, kinuko wrote: > Similarly, this seems to be more about setting up URLLoaderFactory (as well as > returning a control channel for SW). Does 'this' mean GetURLLoaderFactory? GetURLLoaderFactory is just returning the URLLoaderFactory. https://codereview.chromium.org/2804843005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp (right): https://codereview.chromium.org/2804843005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp:56: public Supplement<ExecutionContext> { On 2017/04/14 14:00:26, kinuko wrote: > Not fully sure why we need this class? Can't WorkerFetchContext inherit > Supplement<WorkerClients> and just be provided to / retrieved from > WorkerClients? Or it looks it's also possible that WorkerGlobalScope just has > WorkerFetchContext as a member field (we're not in modules/ here). > Done. https://codereview.chromium.org/2804843005/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebWorkerFetchContextInfo.h (right): https://codereview.chromium.org/2804843005/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContextInfo.h:17: // worker (dedicated, shared, service worker). It contins information about the On 2017/04/14 14:00:27, kinuko wrote: > contins -> contains Done. https://codereview.chromium.org/2804843005/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContextInfo.h:19: class WebWorkerFetchContextInfo { On 2017/04/14 14:00:26, kinuko wrote: > I wonder if it's possible to only expose WebWorkerFetchContext. I think passing > mojo PtrInfo from main thread to worker thread is the main motivation of > WebWorkerFetchContextInfo, is that right? Naively it feels it's possible that > context directly implements WebWorkerFetchContext, and expose a method like > InitializeOnWorkerThread() where we initialize resource dispatcher and binding_. > (While-- if it didn't require additional public interfaces I would have been > just happy to have this info class) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by horo@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.
https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp:21: class WorkerFetchContextInfo final Can you add a class-level comment? https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerFetchContext.h (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerFetchContext.h:24: class WorkerFetchContext final : public FetchContext { Can you add a class-level comment? https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerFetchContext.h:28: explicit WorkerFetchContext(std::unique_ptr<WebWorkerFetchContext>); Exposing both a creation function and the ctor looks strange. Can we make the ctor private? https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:304: WorkerFetchContext* WorkerGlobalScope::FetchContext() { DCHECK(base::FeatureList::IsEnabled(kOffMainThreadFetch))? https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerGlobalScope.h (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerGlobalScope.h:157: // Availavle only when off-main-thread-fetch is enabled. s/Availavle/Available/ https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/DedicatedWorkerMessagingProxyProviderImpl.cpp (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/DedicatedWorkerMessagingProxyProviderImpl.cpp:74: // |web_worker_fetch_context| is null if off-main-thread-fetch is disabled. If possible, I'd prefer to avoid checking the feature availability by a return value of the creation method. If it's determined only by the feature flag, how about checking it like this? if (base::FeatureList::IsEnabled(kOffMainThreadFetch)) { std::unique_ptr<WebWorkerFetchContext> web_worker_fetch_context = Platform::Current()->CreateWorkerFetchContext(); DCHECK(web_worker_fetch_context); ProvideWorkerFetchContextToWorker( worker_clients, std::move(web_worker_fetch_context)); } https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:437: std::unique_ptr<WebWorkerFetchContext> web_worker_fetch_context = ditto. https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:344: std::unique_ptr<WebWorkerFetchContext> web_worker_fetch_context = ditto.
Sending some comments before I look at everything https://codereview.chromium.org/2804843005/diff/200001/content/common/worker_... File content/common/worker_fetch_context_factory.mojom (right): https://codereview.chromium.org/2804843005/diff/200001/content/common/worker_... content/common/worker_fetch_context_factory.mojom:25: int32 service_worker_provider_id); On 2017/04/17 15:12:04, horo wrote: > On 2017/04/14 14:00:26, kinuko wrote: > > Similarly, this seems to be more about setting up URLLoaderFactory (as well as > > returning a control channel for SW). > > Does 'this' mean GetURLLoaderFactory? > GetURLLoaderFactory is just returning the URLLoaderFactory. Sorry, I meant 'CreateWorkerFetchContext' one. This IPC doesn't really create a worker fetch context, does it? https://codereview.chromium.org/2804843005/diff/280001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/2804843005/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.h:421: std::unordered_map<mojom::ServiceWorkerWorkerClient*, Can we have comments about this? https://codereview.chromium.org/2804843005/diff/280001/content/common/worker_... File content/common/worker_url_loader_factory_provider.mojom (right): https://codereview.chromium.org/2804843005/diff/280001/content/common/worker_... content/common/worker_url_loader_factory_provider.mojom:9: interface ServiceWorkerWorkerClient { can we also have an interface-level comment for this one too, a very simple one is fine (e.g. "A renderer-side interface that is returned by CreateWorkerFetchContext for the browser to notify the renderer process when there's a controller change") https://codereview.chromium.org/2804843005/diff/280001/content/common/worker_... content/common/worker_url_loader_factory_provider.mojom:15: // An interface which provides URLLoaderFactory for worker context. nit: can you make it clear about the IPC direction, e.g. "A browser-side interface which..." "worker contexts in the renderer process" https://codereview.chromium.org/2804843005/diff/280001/content/renderer/servi... File content/renderer/service_worker/worker_fetch_context_impl.cc (right): https://codereview.chromium.org/2804843005/diff/280001/content/renderer/servi... content/renderer/service_worker/worker_fetch_context_impl.cc:21: base::SingleThreadTaskRunner* loading_task_runner) { DCHECK(loading_task_runner->RunsTasksOnCurrentThread()) DCHECK(!binding_) https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebWorkerFetchContext.h (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContext.h:17: // WebWorkerFetchContext is created on the main thread and passed to a worker nit: and -> , https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContext.h:17: // WebWorkerFetchContext is created on the main thread and passed to a worker It'd be nicer to note that this is a per-worker object https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContext.h:41: // the main thread. Who calls these? https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContext.h:41: // the main thread. Ok I see, I looked at the dependent patch. Hmm... I have a mixed feeling now. It certainly feels that it might be cleaner to have a separate class at least for all the setting part. Maybe have an internal Settings class and have Initialize(Settings) method? What do you think? (Sorry for the back and forth) By the way: Do they really need to be called on the main thread, or do they just need to be called *before* InitializeOnWorkerThread is called? Also reg: RenderFrameImpl::PropagateAuxiliaryFetchContext() (not included in this CL)-- do we really need this? Note that we now have DocumentLoader::GetServiceWorkerNetworkProvider(), so I think we can fill the info entirely within Blink?
Some more comments. (I'll also look into the entire patch again) https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/DedicatedWorkerMessagingProxyProviderImpl.cpp (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/DedicatedWorkerMessagingProxyProviderImpl.cpp:79: std::move(web_worker_fetch_context)); I really don't think we need to use supplement here but can just pass it as WorkerThreadStartupData or something like that, though, it might end up requiring more changes (I quickly searched but current code setting is not very convenient for us to do that). It doesn't need to be addressed in this CL but you can probably chat with nhiroki@ about that. https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... File third_party/WebKit/public/BUILD.gn (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... third_party/WebKit/public/BUILD.gn:347: "platform/WebWorkerFetchContextInfo.h", Now Info one's gone? (Unless we revive it) https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebWorkerFetchContext.h (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContext.h:41: // the main thread. On 2017/04/18 08:00:31, kinuko wrote: > Ok I see, I looked at the dependent patch. Hmm... I have a mixed feeling now. > It certainly feels that it might be cleaner to have a separate class at least > for all the setting part. Maybe have an internal Settings class and have > Initialize(Settings) method? What do you think? (Sorry for the back and forth) Or we could also revisit this later too when we start to have more fields. (And now I understand more clearly how WorkerFetchContext's implemented) > By the way: > Do they really need to be called on the main thread, or do they just need to be > called *before* InitializeOnWorkerThread is called? > > Also reg: RenderFrameImpl::PropagateAuxiliaryFetchContext() (not included in > this CL)-- do we really need this? Note that we now have > DocumentLoader::GetServiceWorkerNetworkProvider(), so I think we can fill the > info entirely within Blink? https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... File third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h:236: virtual blink::WebWorkerFetchContext* CreateServiceWorkerFetchContext() { nit: please add a method comment here too
The CQ bit was checked by horo@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...
https://codereview.chromium.org/2804843005/diff/200001/content/common/worker_... File content/common/worker_fetch_context_factory.mojom (right): https://codereview.chromium.org/2804843005/diff/200001/content/common/worker_... content/common/worker_fetch_context_factory.mojom:25: int32 service_worker_provider_id); On 2017/04/18 08:00:31, kinuko wrote: > On 2017/04/17 15:12:04, horo wrote: > > On 2017/04/14 14:00:26, kinuko wrote: > > > Similarly, this seems to be more about setting up URLLoaderFactory (as well > as > > > returning a control channel for SW). > > > > Does 'this' mean GetURLLoaderFactory? > > GetURLLoaderFactory is just returning the URLLoaderFactory. > > Sorry, I meant 'CreateWorkerFetchContext' one. This IPC doesn't really create a > worker fetch context, does it? This IPC is used to get the URLLoaderFactory and register the worker fetch context to the service worker context in the browser process. Can't we say this is "creating" the worker fetch context? https://codereview.chromium.org/2804843005/diff/280001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/2804843005/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.h:421: std::unordered_map<mojom::ServiceWorkerWorkerClient*, On 2017/04/18 08:00:31, kinuko wrote: > Can we have comments about this? Done. https://codereview.chromium.org/2804843005/diff/280001/content/common/worker_... File content/common/worker_url_loader_factory_provider.mojom (right): https://codereview.chromium.org/2804843005/diff/280001/content/common/worker_... content/common/worker_url_loader_factory_provider.mojom:9: interface ServiceWorkerWorkerClient { On 2017/04/18 08:00:31, kinuko wrote: > can we also have an interface-level comment for this one too, a very simple one > is fine (e.g. "A renderer-side interface that is returned by > CreateWorkerFetchContext for the browser to notify the renderer process when > there's a controller change") Done. https://codereview.chromium.org/2804843005/diff/280001/content/common/worker_... content/common/worker_url_loader_factory_provider.mojom:15: // An interface which provides URLLoaderFactory for worker context. On 2017/04/18 08:00:31, kinuko wrote: > nit: can you make it clear about the IPC direction, e.g. "A browser-side > interface which..." "worker contexts in the renderer process" Done. https://codereview.chromium.org/2804843005/diff/280001/content/renderer/servi... File content/renderer/service_worker/worker_fetch_context_impl.cc (right): https://codereview.chromium.org/2804843005/diff/280001/content/renderer/servi... content/renderer/service_worker/worker_fetch_context_impl.cc:21: base::SingleThreadTaskRunner* loading_task_runner) { On 2017/04/18 08:00:31, kinuko wrote: > DCHECK(loading_task_runner->RunsTasksOnCurrentThread()) > DCHECK(!binding_) Done. https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp:21: class WorkerFetchContextInfo final On 2017/04/18 07:56:41, nhiroki wrote: > Can you add a class-level comment? Done. https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerFetchContext.h (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerFetchContext.h:24: class WorkerFetchContext final : public FetchContext { On 2017/04/18 07:56:41, nhiroki wrote: > Can you add a class-level comment? Done. https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerFetchContext.h:28: explicit WorkerFetchContext(std::unique_ptr<WebWorkerFetchContext>); On 2017/04/18 07:56:41, nhiroki wrote: > Exposing both a creation function and the ctor looks strange. Can we make the > ctor private? Done. https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp:304: WorkerFetchContext* WorkerGlobalScope::FetchContext() { On 2017/04/18 07:56:41, nhiroki wrote: > DCHECK(base::FeatureList::IsEnabled(kOffMainThreadFetch))? Done. https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerGlobalScope.h (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerGlobalScope.h:157: // Availavle only when off-main-thread-fetch is enabled. On 2017/04/18 07:56:41, nhiroki wrote: > s/Availavle/Available/ Done. https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/DedicatedWorkerMessagingProxyProviderImpl.cpp (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/DedicatedWorkerMessagingProxyProviderImpl.cpp:74: // |web_worker_fetch_context| is null if off-main-thread-fetch is disabled. On 2017/04/18 07:56:41, nhiroki wrote: > If possible, I'd prefer to avoid checking the feature availability by a return > value of the creation method. If it's determined only by the feature flag, how > about checking it like this? > > if (base::FeatureList::IsEnabled(kOffMainThreadFetch)) { > std::unique_ptr<WebWorkerFetchContext> web_worker_fetch_context = > Platform::Current()->CreateWorkerFetchContext(); > DCHECK(web_worker_fetch_context); > ProvideWorkerFetchContextToWorker( > worker_clients, std::move(web_worker_fetch_context)); > } Done. https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/DedicatedWorkerMessagingProxyProviderImpl.cpp:79: std::move(web_worker_fetch_context)); On 2017/04/18 08:35:37, kinuko wrote: > I really don't think we need to use supplement here but can just pass it as > WorkerThreadStartupData or something like that, though, it might end up > requiring more changes (I quickly searched but current code setting is not very > convenient for us to do that). It doesn't need to be addressed in this CL but > you can probably chat with nhiroki@ about that. Acknowledged. https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:437: std::unique_ptr<WebWorkerFetchContext> web_worker_fetch_context = On 2017/04/18 07:56:41, nhiroki wrote: > ditto. Done. https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:344: std::unique_ptr<WebWorkerFetchContext> web_worker_fetch_context = On 2017/04/18 07:56:41, nhiroki wrote: > ditto. Done. https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... File third_party/WebKit/public/BUILD.gn (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... third_party/WebKit/public/BUILD.gn:347: "platform/WebWorkerFetchContextInfo.h", On 2017/04/18 08:35:37, kinuko wrote: > Now Info one's gone? (Unless we revive it) Done. https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebWorkerFetchContext.h (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContext.h:17: // WebWorkerFetchContext is created on the main thread and passed to a worker On 2017/04/18 08:00:31, kinuko wrote: > It'd be nicer to note that this is a per-worker object Done. https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContext.h:17: // WebWorkerFetchContext is created on the main thread and passed to a worker On 2017/04/18 08:00:31, kinuko wrote: > nit: and -> , Done. https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContext.h:41: // the main thread. On 2017/04/18 08:00:32, kinuko wrote: > Who calls these? This will be called in DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() and WebSharedWorkerImpl::OnScriptLoaderFinished(). https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContext.h:41: // the main thread. On 2017/04/18 08:00:31, kinuko wrote: > Ok I see, I looked at the dependent patch. Hmm... I have a mixed feeling now. > It certainly feels that it might be cleaner to have a separate class at least > for all the setting part. Maybe have an internal Settings class and have > Initialize(Settings) method? What do you think? (Sorry for the back and forth) > > By the way: > Do they really need to be called on the main thread, or do they just need to be > called *before* InitializeOnWorkerThread is called? We don't have the information on the worker thread. So it must be set on the main thread. > Also reg: RenderFrameImpl::PropagateAuxiliaryFetchContext() (not included in > this CL)-- do we really need this? Note that we now have > DocumentLoader::GetServiceWorkerNetworkProvider(), so I think we can fill the > info entirely within Blink? We don't need to set the provider ID in RenderFrameImpl::PropagateAuxiliaryFetchContext(). But we have to set the parent frame ID to pass WorkerTest.WorkerTlsClientAuth test. The parent frame ID is not available within Blink. https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContext.h:41: // the main thread. On 2017/04/18 08:35:37, kinuko wrote: > On 2017/04/18 08:00:31, kinuko wrote: > > Ok I see, I looked at the dependent patch. Hmm... I have a mixed feeling now. > > > It certainly feels that it might be cleaner to have a separate class at least > > for all the setting part. Maybe have an internal Settings class and have > > Initialize(Settings) method? What do you think? (Sorry for the back and > forth) > > Or we could also revisit this later too when we start to have more fields. (And > now I understand more clearly how WorkerFetchContext's implemented) > > > By the way: > > Do they really need to be called on the main thread, or do they just need to > be > > called *before* InitializeOnWorkerThread is called? > > > > Also reg: RenderFrameImpl::PropagateAuxiliaryFetchContext() (not included in > > this CL)-- do we really need this? Note that we now have > > DocumentLoader::GetServiceWorkerNetworkProvider(), so I think we can fill the > > info entirely within Blink? > Acknowledged. https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... File third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h:236: virtual blink::WebWorkerFetchContext* CreateServiceWorkerFetchContext() { On 2017/04/18 08:35:37, kinuko wrote: > nit: please add a method comment here too Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
(Responses only, and will take another shot of serious review.) https://codereview.chromium.org/2804843005/diff/200001/content/common/worker_... File content/common/worker_fetch_context_factory.mojom (right): https://codereview.chromium.org/2804843005/diff/200001/content/common/worker_... content/common/worker_fetch_context_factory.mojom:25: int32 service_worker_provider_id); On 2017/04/18 12:53:33, horo wrote: > On 2017/04/18 08:00:31, kinuko wrote: > > On 2017/04/17 15:12:04, horo wrote: > > > On 2017/04/14 14:00:26, kinuko wrote: > > > > Similarly, this seems to be more about setting up URLLoaderFactory (as > well > > as > > > > returning a control channel for SW). > > > > > > Does 'this' mean GetURLLoaderFactory? > > > GetURLLoaderFactory is just returning the URLLoaderFactory. > > > > Sorry, I meant 'CreateWorkerFetchContext' one. This IPC doesn't really create > a > > worker fetch context, does it? > > This IPC is used to get the URLLoaderFactory and register the worker fetch > context to the service worker context in the browser process. Can't we say this > is "creating" the worker fetch context? This IPC is used to get the URLLoaderFactory and register the ServiceWorkerWorkerClient in the service worker's provider host, that's what I'm seeing. WorkerFetchContext seems to be the renderer-side consequence that doesn't need to be surfaced at this IPC layer... that's my view. (Apologies for biked-sheding, naming is hard) https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebWorkerFetchContext.h (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContext.h:41: // the main thread. On 2017/04/18 12:53:35, horo wrote: > On 2017/04/18 08:00:31, kinuko wrote: > > Ok I see, I looked at the dependent patch. Hmm... I have a mixed feeling now. > > > It certainly feels that it might be cleaner to have a separate class at least > > for all the setting part. Maybe have an internal Settings class and have > > Initialize(Settings) method? What do you think? (Sorry for the back and > forth) > > > > By the way: > > Do they really need to be called on the main thread, or do they just need to > be > > called *before* InitializeOnWorkerThread is called? > > We don't have the information on the worker thread. > So it must be set on the main thread. That's the caller side's constraint, not this class's constraint.
https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebWorkerFetchContext.h (right): https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebWorkerFetchContext.h:41: // the main thread. On 2017/04/18 12:53:35, horo wrote: > On 2017/04/18 08:00:31, kinuko wrote: > > Ok I see, I looked at the dependent patch. Hmm... I have a mixed feeling now. > > > It certainly feels that it might be cleaner to have a separate class at least > > for all the setting part. Maybe have an internal Settings class and have > > Initialize(Settings) method? What do you think? (Sorry for the back and > forth) > > > > By the way: > > Do they really need to be called on the main thread, or do they just need to > be > > called *before* InitializeOnWorkerThread is called? > > We don't have the information on the worker thread. > So it must be set on the main thread. > > > > Also reg: RenderFrameImpl::PropagateAuxiliaryFetchContext() (not included in > > this CL)-- do we really need this? Note that we now have > > DocumentLoader::GetServiceWorkerNetworkProvider(), so I think we can fill the > > info entirely within Blink? > > We don't need to set the provider ID in > RenderFrameImpl::PropagateAuxiliaryFetchContext(). > > But we have to set the parent frame ID to pass WorkerTest.WorkerTlsClientAuth > test. The parent frame ID is not available within Blink. I see. CreateWorkerFetchContext might rather be on WebFrameClient then?
On 2017/04/19 03:50:49, kinuko wrote: > https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... > File third_party/WebKit/public/platform/WebWorkerFetchContext.h (right): > > https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... > third_party/WebKit/public/platform/WebWorkerFetchContext.h:41: // the main > thread. > On 2017/04/18 12:53:35, horo wrote: > > On 2017/04/18 08:00:31, kinuko wrote: > > > Ok I see, I looked at the dependent patch. Hmm... I have a mixed feeling > now. > > > > > It certainly feels that it might be cleaner to have a separate class at > least > > > for all the setting part. Maybe have an internal Settings class and have > > > Initialize(Settings) method? What do you think? (Sorry for the back and > > forth) > > > > > > By the way: > > > Do they really need to be called on the main thread, or do they just need to > > be > > > called *before* InitializeOnWorkerThread is called? > > > > We don't have the information on the worker thread. > > So it must be set on the main thread. > > > > > > > Also reg: RenderFrameImpl::PropagateAuxiliaryFetchContext() (not included in > > > this CL)-- do we really need this? Note that we now have > > > DocumentLoader::GetServiceWorkerNetworkProvider(), so I think we can fill > the > > > info entirely within Blink? > > > > We don't need to set the provider ID in > > RenderFrameImpl::PropagateAuxiliaryFetchContext(). > > > > But we have to set the parent frame ID to pass WorkerTest.WorkerTlsClientAuth > > test. The parent frame ID is not available within Blink. > > I see. CreateWorkerFetchContext might rather be on WebFrameClient then? I thought that you are decoupling the fetch related code from frames. So it shouldn't be on WebFrameClient while you are refactoring it.
On 2017/04/19 04:30:22, horo wrote: > On 2017/04/19 03:50:49, kinuko wrote: > > > https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... > > File third_party/WebKit/public/platform/WebWorkerFetchContext.h (right): > > > > > https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... > > third_party/WebKit/public/platform/WebWorkerFetchContext.h:41: // the main > > thread. > > On 2017/04/18 12:53:35, horo wrote: > > > On 2017/04/18 08:00:31, kinuko wrote: > > > > Ok I see, I looked at the dependent patch. Hmm... I have a mixed feeling > > now. > > > > > > > It certainly feels that it might be cleaner to have a separate class at > > least > > > > for all the setting part. Maybe have an internal Settings class and have > > > > Initialize(Settings) method? What do you think? (Sorry for the back and > > > forth) > > > > > > > > By the way: > > > > Do they really need to be called on the main thread, or do they just need > to > > > be > > > > called *before* InitializeOnWorkerThread is called? > > > > > > We don't have the information on the worker thread. > > > So it must be set on the main thread. > > > > > > > > > > Also reg: RenderFrameImpl::PropagateAuxiliaryFetchContext() (not included > in > > > > this CL)-- do we really need this? Note that we now have > > > > DocumentLoader::GetServiceWorkerNetworkProvider(), so I think we can fill > > the > > > > info entirely within Blink? > > > > > > We don't need to set the provider ID in > > > RenderFrameImpl::PropagateAuxiliaryFetchContext(). > > > > > > But we have to set the parent frame ID to pass > WorkerTest.WorkerTlsClientAuth > > > test. The parent frame ID is not available within Blink. > > > > I see. CreateWorkerFetchContext might rather be on WebFrameClient then? > > I thought that you are decoupling the fetch related code from frames. > So it shouldn't be on WebFrameClient while you are refactoring it. I see what you intended, sorry if I made your choice harder! And I surely do like to avoid adding Frame dependency if we can, but looks like it's not the case in this patch, so it seems we at least need some point to associate both. Let me try to clarify my stance on that one: I think having some fetch/loading related context class created in a frame context makes sense, in that way we can hide all the frame-related dependency inside the context class, and it leaves the possibility that the context can be created in different path when frame's not available (e.g. in SW cases we take that path in your patch). What I do like to avoid is to plumb all the detailed signaling/callbacks directly go through the frame-related classes (like what current LocalFrameClient does), that way it's pretty hard to decouple the dependency.
The CQ bit was checked by horo@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...
Description was changed from ========== Implement the infrastructure of creating WorkerFetchContext in worker global scope. This CL implements the infrastructure of creating WorkerFetchContext in worker global scope of Dedicated Worker, Shared Worker and Service Worker. 1. WorkerFetchContext for Dedicated Worker 1.1. DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() calls RendererBlinkPlatformImpl::CreateWorkerFetchContext() in the main thread. 1.2. RendererBlinkPlatformImpl::CreateWorkerFetchContext() gets the interface of WorkerURLLoaderFactoryProvider which is implemented in the browser process as WorkerURLLoaderFactoryProviderImpl, and creates a WorkerFetchContextImpl with the interface. WorkerFetchContextImpl keeps the interface as |provider_info_| until InitializeOnWorkerThread() is called in the worker thread. 1.3. In DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() the fetch context related information (ex: service worker provider id) is set to the WorkerFetchContextImpl. (This is not in this CL yet.) 1.4. ProvideWorkerFetchContextToWorker() creates a WorkerFetchContextInfo which holds the WorkerFetchContextImpl and provides it to the WorkerClients using the Supplement mechanism. 1.5. After the worker thread is created, the WorkerClients is reattached to the worker thread in the constructor of WorkerGlobalScope. 1.6. WorkerGlobalScope::FetchContext() will be called to get the WorkerFetchContext in ThreadableLoader::Create() When fetch() is called in the worker thread. (This is not in this CL yet.) 1.7. When WorkerGlobalScope::FetchContext() is called for the first time, WorkerFetchContext::Create() is called to get the WorkerFetchContextInfo from the WorkerClients and take the WorkerFetchContextImpl from it and create the WorkerFetchContext. 1.8. The constructor of WorkerFetchContext calls WorkerFetchContextImpl:: InitializeOnWorkerThread() which creates a ResourceDispatcher and calls WorkerURLLoaderFactoryProviderImpl::CreateWorkerFetchContext() in the browser process via mojo IPC with a request of URLLoaderFactory and the pointer (ServiceWorkerWorkerClient) of itself. 1.9. WorkerURLLoaderFactoryProviderImpl::CreateWorkerFetchContext() binds the request of URLLoaderFactory to ResourceMessageFilter and calls ServiceWorkerContextWrapper::BindWorkerFetchContext() to bind the ServiceWorkerWorkerClient (the pointer to WorkerFetchContextImpl in the worker thread) with the parent frame's ServiceWorkerProviderHost. When the controller service worker of the ServiceWorkerProviderHost changed, WorkerFetchContextImpl::SetControllerServiceWorker() will be called via mojo. So WorkerFetchContextImpl::IsControlledByServiceWorker() can return the correct status. 1.10. WorkerFetchContext::CreateURLLoader() calls WorkerFetchContextImpl:: CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. 2. WorkerFetchContext for Shared Worker is created almost same as the Dedicated Worker. The only difference is that CreateWorkerFetchContext() is called in WebSharedWorkerImpl::OnScriptLoaderFinished(). 3. WorkerFetchContext for Service Worker 3.1. WebEmbeddedWorkerImpl::StartWorkerThread() calls ServiceWorkerContextClient::CreateServiceWorkerFetchContext() in the main thread. 3.2. ServiceWorkerContextClient::CreateServiceWorkerFetchContext() gets the interface of WorkerURLLoaderFactoryProvider which is implemented in the browser process as WorkerURLLoaderFactoryProviderImpl, and creates a ServiceWorkerFetchContextImpl with the interface. ServiceWorkerFetchContextImpl keeps the interface as |provider_info_| until InitializeOnWorkerThread() is called in the worker thread. 3.3. In WebEmbeddedWorkerImpl::StartWorkerThread(), the fetch context related information (ex: DataSaverEnabled) is set to the ServiceWorkerFetchContextImpl. (This is not in this CL yet.) 3.4. Same as 1.4. ProvideWorkerFetchContextToWorker() creates a WorkerFetchContextInfo which holds the ServiceWorkerFetchContextImpl and provides it to the WorkerClientsusing the Supplement mechanism. 3.5. Same as 1.5. the WorkerClients is reattached to the worker thread. 3.6. Same as 1.6. 3.7. Same as 1.7. When WorkerGlobalScope::FetchContext() is called for the first time, WorkerFetchContext::Create() is called to get the WorkerFetchContextInfo from the WorkerClients and take the ServiceWorkerFetchContextImpl from it and create the WorkerFetchContext. 3.8. The constructor of WorkerFetchContext calls ServiceWorkerFetchContextImpl ::InitializeOnWorkerThread() which creates a ResourceDispatcher and calls WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactory() in the browser process via mojo IPC with a request of URLLoaderFactory. 3.9. WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactory() binds the request of URLLoaderFactory to ResourceMessageFilter. 3.10. WorkerFetchContext::CreateURLLoader() calls ServiceWorkerFetchContextImpl::CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. BUG=443374 ========== to ========== Implement the infrastructure of creating WorkerFetchContext in worker global scope. This CL implements the infrastructure of creating WorkerFetchContext in worker global scope of Dedicated Worker, Shared Worker and Service Worker. 1. WorkerFetchContext for Dedicated Worker 1.1. DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() calls RenderFrameImpl::CreateWorkerFetchContext() in the main thread. 1.2. RenderFrameImpl::CreateWorkerFetchContext() gets the interface of WorkerURLLoaderFactoryProvider which is implemented in the browser process as WorkerURLLoaderFactoryProviderImpl, and creates a WorkerFetchContextImpl with the interface. WorkerFetchContextImpl keeps the interface as |provider_info_| until InitializeOnWorkerThread() is called in the worker thread. 1.3. RenderFrameImpl::CreateWorkerFetchContext() copies the service worker state (ServiceWorkerProviderID and IsControlledByServiceWorker) of the frame to the WorkerFetchContextImpl. 1.4. ProvideWorkerFetchContextToWorker() creates a WorkerFetchContextInfo which holds the WorkerFetchContextImpl and provides it to the WorkerClients using the Supplement mechanism. 1.5. After the worker thread is created, the WorkerClients is reattached to the worker thread in the constructor of WorkerGlobalScope. 1.6. WorkerGlobalScope::FetchContext() will be called to get the WorkerFetchContext in ThreadableLoader::Create() When fetch() is called in the worker thread. (This is not in this CL yet.) 1.7. When WorkerGlobalScope::FetchContext() is called for the first time, WorkerFetchContext::Create() is called to get the WorkerFetchContextInfo from the WorkerClients and take the WorkerFetchContextImpl from it and create the WorkerFetchContext. 1.8. The constructor of WorkerFetchContext calls WorkerFetchContextImpl:: InitializeOnWorkerThread() which creates a ResourceDispatcher and calls WorkerURLLoaderFactoryProviderImpl::CreateWorkerFetchContext() in the browser process via mojo IPC with a request of URLLoaderFactory and the pointer (ServiceWorkerWorkerClient) of itself. 1.9. WorkerURLLoaderFactoryProviderImpl::CreateWorkerFetchContext() binds the request of URLLoaderFactory to ResourceMessageFilter and calls ServiceWorkerContextWrapper::BindWorkerFetchContext() to bind the ServiceWorkerWorkerClient (the pointer to WorkerFetchContextImpl in the worker thread) with the parent frame's ServiceWorkerProviderHost. When the controller service worker of the ServiceWorkerProviderHost changed, WorkerFetchContextImpl::SetControllerServiceWorker() will be called via mojo. So WorkerFetchContextImpl::IsControlledByServiceWorker() can return the correct status. 1.10. WorkerFetchContext::CreateURLLoader() calls WorkerFetchContextImpl:: CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. 2. WorkerFetchContext for Shared Worker is created almost same as the Dedicated Worker. The only difference is that CreateWorkerFetchContext() is called in WebSharedWorkerImpl::OnScriptLoaderFinished(). 3. WorkerFetchContext for Service Worker 3.1. WebEmbeddedWorkerImpl::StartWorkerThread() calls ServiceWorkerContextClient::CreateServiceWorkerFetchContext() in the main thread. 3.2. ServiceWorkerContextClient::CreateServiceWorkerFetchContext() gets the interface of WorkerURLLoaderFactoryProvider which is implemented in the browser process as WorkerURLLoaderFactoryProviderImpl, and creates a ServiceWorkerFetchContextImpl with the interface. ServiceWorkerFetchContextImpl keeps the interface as |provider_info_| until InitializeOnWorkerThread() is called in the worker thread. 3.3. In WebEmbeddedWorkerImpl::StartWorkerThread(), the fetch context related information (ex: DataSaverEnabled) is set to the ServiceWorkerFetchContextImpl. (This is not in this CL yet.) 3.4. Same as 1.4. ProvideWorkerFetchContextToWorker() creates a WorkerFetchContextInfo which holds the ServiceWorkerFetchContextImpl and provides it to the WorkerClientsusing the Supplement mechanism. 3.5. Same as 1.5. the WorkerClients is reattached to the worker thread. 3.6. Same as 1.6. 3.7. Same as 1.7. When WorkerGlobalScope::FetchContext() is called for the first time, WorkerFetchContext::Create() is called to get the WorkerFetchContextInfo from the WorkerClients and take the ServiceWorkerFetchContextImpl from it and create the WorkerFetchContext. 3.8. The constructor of WorkerFetchContext calls ServiceWorkerFetchContextImpl ::InitializeOnWorkerThread() which creates a ResourceDispatcher and calls WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactory() in the browser process via mojo IPC with a request of URLLoaderFactory. 3.9. WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactory() binds the request of URLLoaderFactory to ResourceMessageFilter. 3.10. WorkerFetchContext::CreateURLLoader() calls ServiceWorkerFetchContextImpl::CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. BUG=443374 ==========
On 2017/04/19 05:19:27, kinuko wrote: > On 2017/04/19 04:30:22, horo wrote: > > On 2017/04/19 03:50:49, kinuko wrote: > > > > > > https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... > > > File third_party/WebKit/public/platform/WebWorkerFetchContext.h (right): > > > > > > > > > https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... > > > third_party/WebKit/public/platform/WebWorkerFetchContext.h:41: // the main > > > thread. > > > On 2017/04/18 12:53:35, horo wrote: > > > > On 2017/04/18 08:00:31, kinuko wrote: > > > > > Ok I see, I looked at the dependent patch. Hmm... I have a mixed > feeling > > > now. > > > > > > > > > It certainly feels that it might be cleaner to have a separate class at > > > least > > > > > for all the setting part. Maybe have an internal Settings class and > have > > > > > Initialize(Settings) method? What do you think? (Sorry for the back > and > > > > forth) > > > > > > > > > > By the way: > > > > > Do they really need to be called on the main thread, or do they just > need > > to > > > > be > > > > > called *before* InitializeOnWorkerThread is called? > > > > > > > > We don't have the information on the worker thread. > > > > So it must be set on the main thread. > > > > > > > > > > > > > Also reg: RenderFrameImpl::PropagateAuxiliaryFetchContext() (not > included > > in > > > > > this CL)-- do we really need this? Note that we now have > > > > > DocumentLoader::GetServiceWorkerNetworkProvider(), so I think we can > fill > > > the > > > > > info entirely within Blink? > > > > > > > > We don't need to set the provider ID in > > > > RenderFrameImpl::PropagateAuxiliaryFetchContext(). > > > > > > > > But we have to set the parent frame ID to pass > > WorkerTest.WorkerTlsClientAuth > > > > test. The parent frame ID is not available within Blink. > > > > > > I see. CreateWorkerFetchContext might rather be on WebFrameClient then? > > > > I thought that you are decoupling the fetch related code from frames. > > So it shouldn't be on WebFrameClient while you are refactoring it. > > I see what you intended, sorry if I made your choice harder! And I surely do > like to avoid adding Frame dependency if we can, but looks like it's not the > case in this patch, so it seems we at least need some point to associate both. > > Let me try to clarify my stance on that one: I think having some fetch/loading > related context class created in a frame context makes sense, in that way we can > hide all the frame-related dependency inside the context class, and it leaves > the possibility that the context can be created in different path when frame's > not available (e.g. in SW cases we take that path in your patch). What I do > like to avoid is to plumb all the detailed signaling/callbacks directly go > through the frame-related classes (like what current LocalFrameClient does), > that way it's pretty hard to decouple the dependency. Uploaded Patch Set 8 which moved CreateWorkerFetchContext() from Platform to WebFrameClient.
I think this is looking good now. +cc yhirano as we just discussed how we might be adding FetchContext::CreateURLLoader & holding URLLoaderFactory for workers in the browser https://codereview.chromium.org/2804843005/diff/340001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2804843005/diff/340001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:500: void CreateWorkerFetchContext( I still think this could be named like GetURLLoaderFactoryAndRegisterClient or something like that, ...but let's chat offline for this one https://codereview.chromium.org/2804843005/diff/340001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:508: }; nit: no need of semicolon https://codereview.chromium.org/2804843005/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp (right): https://codereview.chromium.org/2804843005/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp:24: class WorkerFetchContextInfo final nit: WorkerFetchContextHolder maybe? https://codereview.chromium.org/2804843005/diff/340001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2804843005/diff/340001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrameClient.h:153: virtual WebWorkerFetchContext* CreateWorkerFetchContext() { return 0; } nit: return nullptr for new code
On 2017/04/19 10:36:32, horo wrote: > On 2017/04/19 05:19:27, kinuko wrote: > > On 2017/04/19 04:30:22, horo wrote: > > > On 2017/04/19 03:50:49, kinuko wrote: > > > > > > > > > > https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... > > > > File third_party/WebKit/public/platform/WebWorkerFetchContext.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2804843005/diff/280001/third_party/WebKit/pub... > > > > third_party/WebKit/public/platform/WebWorkerFetchContext.h:41: // the main > > > > thread. > > > > On 2017/04/18 12:53:35, horo wrote: > > > > > On 2017/04/18 08:00:31, kinuko wrote: > > > > > > Ok I see, I looked at the dependent patch. Hmm... I have a mixed > > feeling > > > > now. > > > > > > > > > > > It certainly feels that it might be cleaner to have a separate class > at > > > > least > > > > > > for all the setting part. Maybe have an internal Settings class and > > have > > > > > > Initialize(Settings) method? What do you think? (Sorry for the back > > and > > > > > forth) > > > > > > > > > > > > By the way: > > > > > > Do they really need to be called on the main thread, or do they just > > need > > > to > > > > > be > > > > > > called *before* InitializeOnWorkerThread is called? > > > > > > > > > > We don't have the information on the worker thread. > > > > > So it must be set on the main thread. > > > > > > > > > > > > > > > > Also reg: RenderFrameImpl::PropagateAuxiliaryFetchContext() (not > > included > > > in > > > > > > this CL)-- do we really need this? Note that we now have > > > > > > DocumentLoader::GetServiceWorkerNetworkProvider(), so I think we can > > fill > > > > the > > > > > > info entirely within Blink? > > > > > > > > > > We don't need to set the provider ID in > > > > > RenderFrameImpl::PropagateAuxiliaryFetchContext(). > > > > > > > > > > But we have to set the parent frame ID to pass > > > WorkerTest.WorkerTlsClientAuth > > > > > test. The parent frame ID is not available within Blink. > > > > > > > > I see. CreateWorkerFetchContext might rather be on WebFrameClient then? > > > > > > I thought that you are decoupling the fetch related code from frames. > > > So it shouldn't be on WebFrameClient while you are refactoring it. > > > > I see what you intended, sorry if I made your choice harder! And I surely do > > like to avoid adding Frame dependency if we can, but looks like it's not the > > case in this patch, so it seems we at least need some point to associate both. > > > > Let me try to clarify my stance on that one: I think having some fetch/loading > > related context class created in a frame context makes sense, in that way we > can > > hide all the frame-related dependency inside the context class, and it leaves > > the possibility that the context can be created in different path when frame's > > not available (e.g. in SW cases we take that path in your patch). What I do > > like to avoid is to plumb all the detailed signaling/callbacks directly go > > through the frame-related classes (like what current LocalFrameClient does), > > that way it's pretty hard to decouple the dependency. > > Uploaded Patch Set 8 which moved CreateWorkerFetchContext() from Platform to > WebFrameClient. Ah, WebFrameClient for SharedWorker is WebSharedWorkerImpl. So Patch Set 8 doesn't work. Uploaded Patch Set 9 which implements EmbeddedSharedWorkerStub::CreateWorkerFetchContext.
The CQ bit was checked by horo@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 checked by horo@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...
https://codereview.chromium.org/2804843005/diff/340001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2804843005/diff/340001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:500: void CreateWorkerFetchContext( On 2017/04/19 11:08:14, kinuko wrote: > I still think this could be named like GetURLLoaderFactoryAndRegisterClient or > something like that, ...but let's chat offline for this one Done. https://codereview.chromium.org/2804843005/diff/340001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:508: }; On 2017/04/19 11:08:14, kinuko wrote: > nit: no need of semicolon Done. https://codereview.chromium.org/2804843005/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp (right): https://codereview.chromium.org/2804843005/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp:24: class WorkerFetchContextInfo final On 2017/04/19 11:08:14, kinuko wrote: > nit: WorkerFetchContextHolder maybe? Done. https://codereview.chromium.org/2804843005/diff/340001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2804843005/diff/340001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrameClient.h:153: virtual WebWorkerFetchContext* CreateWorkerFetchContext() { return 0; } On 2017/04/19 11:08:14, kinuko wrote: > nit: return nullptr for new code Done.
Description was changed from ========== Implement the infrastructure of creating WorkerFetchContext in worker global scope. This CL implements the infrastructure of creating WorkerFetchContext in worker global scope of Dedicated Worker, Shared Worker and Service Worker. 1. WorkerFetchContext for Dedicated Worker 1.1. DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() calls RenderFrameImpl::CreateWorkerFetchContext() in the main thread. 1.2. RenderFrameImpl::CreateWorkerFetchContext() gets the interface of WorkerURLLoaderFactoryProvider which is implemented in the browser process as WorkerURLLoaderFactoryProviderImpl, and creates a WorkerFetchContextImpl with the interface. WorkerFetchContextImpl keeps the interface as |provider_info_| until InitializeOnWorkerThread() is called in the worker thread. 1.3. RenderFrameImpl::CreateWorkerFetchContext() copies the service worker state (ServiceWorkerProviderID and IsControlledByServiceWorker) of the frame to the WorkerFetchContextImpl. 1.4. ProvideWorkerFetchContextToWorker() creates a WorkerFetchContextInfo which holds the WorkerFetchContextImpl and provides it to the WorkerClients using the Supplement mechanism. 1.5. After the worker thread is created, the WorkerClients is reattached to the worker thread in the constructor of WorkerGlobalScope. 1.6. WorkerGlobalScope::FetchContext() will be called to get the WorkerFetchContext in ThreadableLoader::Create() When fetch() is called in the worker thread. (This is not in this CL yet.) 1.7. When WorkerGlobalScope::FetchContext() is called for the first time, WorkerFetchContext::Create() is called to get the WorkerFetchContextInfo from the WorkerClients and take the WorkerFetchContextImpl from it and create the WorkerFetchContext. 1.8. The constructor of WorkerFetchContext calls WorkerFetchContextImpl:: InitializeOnWorkerThread() which creates a ResourceDispatcher and calls WorkerURLLoaderFactoryProviderImpl::CreateWorkerFetchContext() in the browser process via mojo IPC with a request of URLLoaderFactory and the pointer (ServiceWorkerWorkerClient) of itself. 1.9. WorkerURLLoaderFactoryProviderImpl::CreateWorkerFetchContext() binds the request of URLLoaderFactory to ResourceMessageFilter and calls ServiceWorkerContextWrapper::BindWorkerFetchContext() to bind the ServiceWorkerWorkerClient (the pointer to WorkerFetchContextImpl in the worker thread) with the parent frame's ServiceWorkerProviderHost. When the controller service worker of the ServiceWorkerProviderHost changed, WorkerFetchContextImpl::SetControllerServiceWorker() will be called via mojo. So WorkerFetchContextImpl::IsControlledByServiceWorker() can return the correct status. 1.10. WorkerFetchContext::CreateURLLoader() calls WorkerFetchContextImpl:: CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. 2. WorkerFetchContext for Shared Worker is created almost same as the Dedicated Worker. The only difference is that CreateWorkerFetchContext() is called in WebSharedWorkerImpl::OnScriptLoaderFinished(). 3. WorkerFetchContext for Service Worker 3.1. WebEmbeddedWorkerImpl::StartWorkerThread() calls ServiceWorkerContextClient::CreateServiceWorkerFetchContext() in the main thread. 3.2. ServiceWorkerContextClient::CreateServiceWorkerFetchContext() gets the interface of WorkerURLLoaderFactoryProvider which is implemented in the browser process as WorkerURLLoaderFactoryProviderImpl, and creates a ServiceWorkerFetchContextImpl with the interface. ServiceWorkerFetchContextImpl keeps the interface as |provider_info_| until InitializeOnWorkerThread() is called in the worker thread. 3.3. In WebEmbeddedWorkerImpl::StartWorkerThread(), the fetch context related information (ex: DataSaverEnabled) is set to the ServiceWorkerFetchContextImpl. (This is not in this CL yet.) 3.4. Same as 1.4. ProvideWorkerFetchContextToWorker() creates a WorkerFetchContextInfo which holds the ServiceWorkerFetchContextImpl and provides it to the WorkerClientsusing the Supplement mechanism. 3.5. Same as 1.5. the WorkerClients is reattached to the worker thread. 3.6. Same as 1.6. 3.7. Same as 1.7. When WorkerGlobalScope::FetchContext() is called for the first time, WorkerFetchContext::Create() is called to get the WorkerFetchContextInfo from the WorkerClients and take the ServiceWorkerFetchContextImpl from it and create the WorkerFetchContext. 3.8. The constructor of WorkerFetchContext calls ServiceWorkerFetchContextImpl ::InitializeOnWorkerThread() which creates a ResourceDispatcher and calls WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactory() in the browser process via mojo IPC with a request of URLLoaderFactory. 3.9. WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactory() binds the request of URLLoaderFactory to ResourceMessageFilter. 3.10. WorkerFetchContext::CreateURLLoader() calls ServiceWorkerFetchContextImpl::CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. BUG=443374 ========== to ========== Implement the infrastructure of creating WorkerFetchContext in worker global scope. This CL implements the infrastructure of creating WorkerFetchContext in worker global scope of Dedicated Worker, Shared Worker and Service Worker. 1. WorkerFetchContext for Dedicated Worker 1.1. DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() calls RenderFrameImpl::CreateWorkerFetchContext() on the main thread. 1.2. RenderFrameImpl::CreateWorkerFetchContext() gets the interface of WorkerURLLoaderFactoryProvider which is implemented in the browser process as WorkerURLLoaderFactoryProviderImpl, and creates a WorkerFetchContextImpl with the interface. WorkerFetchContextImpl keeps the interface as |provider_info_| until InitializeOnWorkerThread() is called on the worker thread. 1.3. RenderFrameImpl::CreateWorkerFetchContext() copies the service worker state (ServiceWorkerProviderID and IsControlledByServiceWorker) of the frame to the WorkerFetchContextImpl. 1.4. ProvideWorkerFetchContextToWorker() creates a WorkerFetchContextHolder which holds the WorkerFetchContextImpl and provides it to the WorkerClients using the Supplement mechanism. 1.5. After the worker thread is created, the WorkerClients is reattached to the worker thread in the constructor of WorkerGlobalScope. 1.6. WorkerGlobalScope::FetchContext() will be called to get the WorkerFetchContext in ThreadableLoader::Create() When fetch() is called in the worker thread. (This is not in this CL yet.) 1.7. When WorkerGlobalScope::FetchContext() is called for the first time, WorkerFetchContext::Create() is called to get the WorkerFetchContextHolder from the WorkerClients and take the WorkerFetchContextImpl from it and create the WorkerFetchContext. 1.8. The constructor of WorkerFetchContext calls WorkerFetchContextImpl:: InitializeOnWorkerThread() which creates a ResourceDispatcher and calls WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactoryAndRegisterClient() in the browser process via mojo IPC with a request of URLLoaderFactory and the pointer (ServiceWorkerWorkerClient) of itself. 1.9. WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactoryAndRegisterClient() binds the request of URLLoaderFactory to ResourceMessageFilter and calls ServiceWorkerContextWrapper::BindWorkerFetchContext() to bind the ServiceWorkerWorkerClient (the pointer to WorkerFetchContextImpl in the worker thread) with the parent frame's ServiceWorkerProviderHost. When the controller service worker of the ServiceWorkerProviderHost changed, WorkerFetchContextImpl::SetControllerServiceWorker() will be called via mojo. So WorkerFetchContextImpl::IsControlledByServiceWorker() can return the correct status. 1.10. WorkerFetchContext::CreateURLLoader() calls WorkerFetchContextImpl:: CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. 2. WorkerFetchContext for Shared Worker is created almost same as the Dedicated Worker. The only difference is that CreateWorkerFetchContext() is called in WebSharedWorkerImpl::OnScriptLoaderFinished(). 3. WorkerFetchContext for Service Worker 3.1. WebEmbeddedWorkerImpl::StartWorkerThread() calls ServiceWorkerContextClient::CreateServiceWorkerFetchContext() on the main thread. 3.2. ServiceWorkerContextClient::CreateServiceWorkerFetchContext() gets the interface of WorkerURLLoaderFactoryProvider which is implemented in the browser process as WorkerURLLoaderFactoryProviderImpl, and creates a ServiceWorkerFetchContextImpl with the interface. ServiceWorkerFetchContextImpl keeps the interface as |provider_info_| until InitializeOnWorkerThread() is called on the worker thread. 3.3. In WebEmbeddedWorkerImpl::StartWorkerThread(), the fetch context related information (ex: DataSaverEnabled) is set to the ServiceWorkerFetchContextImpl. (This is not in this CL yet.) 3.4. Same as 1.4. ProvideWorkerFetchContextToWorker() creates a WorkerFetchContextHolder which holds the ServiceWorkerFetchContextImpl and provides it to the WorkerClientsusing the Supplement mechanism. 3.5. Same as 1.5. the WorkerClients is reattached to the worker thread. 3.6. Same as 1.6. 3.7. Same as 1.7. When WorkerGlobalScope::FetchContext() is called for the first time, WorkerFetchContext::Create() is called to get the WorkerFetchContextHolder from the WorkerClients and take the ServiceWorkerFetchContextImpl from it and create the WorkerFetchContext. 3.8. The constructor of WorkerFetchContext calls ServiceWorkerFetchContextImpl ::InitializeOnWorkerThread() which creates a ResourceDispatcher and calls WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactory() in the browser process via mojo IPC with a request of URLLoaderFactory. 3.9. WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactory() binds the request of URLLoaderFactory to ResourceMessageFilter. 3.10. WorkerFetchContext::CreateURLLoader() calls ServiceWorkerFetchContextImpl::CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. BUG=443374 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I probably have some more comments but I think this basically lgtm, let's get this reviewed by yhirano/japhet too (as well as SW folks). https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerGlobalScope.h (right): https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerGlobalScope.h:158: WorkerFetchContext* FetchContext(); nit: GetFetchContext() https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right): https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:349: DCHECK(web_worker_fetch_context); We could get ServiceWorkerNetworkProvider here and set necessary info too, is that right? https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrameClient.h:153: virtual WebWorkerFetchContext* CreateWorkerFetchContext() { return nullptr; } Could we make this return std::unique_ptr<> https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebSharedWorkerClient.h (right): https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/pub... third_party/WebKit/public/web/WebSharedWorkerClient.h:101: virtual WebWorkerFetchContext* CreateWorkerFetchContext() { return nullptr; } ditto (unique_ptr)
https://codereview.chromium.org/2804843005/diff/380001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2804843005/diff/380001/content/renderer/rende... content/renderer/render_frame_impl.cc:3008: if (!base::FeatureList::IsEnabled(features::kOffMainThreadFetch)) DCHECK(base::FeatureList::IsEnabled(features::kOffMainThreadFetch)); ? https://codereview.chromium.org/2804843005/diff/380001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2804843005/diff/380001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:1011: ServiceWorkerContextClient::CreateServiceWorkerFetchContext() { DCHECK(main_thread_task_runner_->RunsTasksOnCurrentThread()); ? https://codereview.chromium.org/2804843005/diff/380001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:1012: if (!base::FeatureList::IsEnabled(features::kOffMainThreadFetch)) DCHECK(base::FeatureList::IsEnabled(features::kOffMainThreadFetch)); ? https://codereview.chromium.org/2804843005/diff/380001/content/renderer/servi... File content/renderer/service_worker/worker_fetch_context_impl.h (right): https://codereview.chromium.org/2804843005/diff/380001/content/renderer/servi... content/renderer/service_worker/worker_fetch_context_impl.h:39: void SetIsControlledByServiceWorker(bool flag); Optional: Simple setter/getter can use a naming convention like |set_service_worker_provider_id()|. https://codereview.chromium.org/2804843005/diff/380001/content/renderer/share... File content/renderer/shared_worker/embedded_shared_worker_stub.cc (right): https://codereview.chromium.org/2804843005/diff/380001/content/renderer/share... content/renderer/shared_worker/embedded_shared_worker_stub.cc:263: if (!base::FeatureList::IsEnabled(features::kOffMainThreadFetch)) DCHECK(base::FeatureList::IsEnabled(features::kOffMainThreadFetch)) ? https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp (right): https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp:56: WorkerGlobalScope& worker_global_scope) { Can you add DCHECK(worker_global_scope->IsContextThread()) here? https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp:72: web_context_->InitializeOnWorkerThread(Platform::Current() The GlobalScopeScheduler patch was landed. Can you update this part as I commented before? https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrameClient.h:153: virtual WebWorkerFetchContext* CreateWorkerFetchContext() { return nullptr; } How about returning std::unique_ptr<WebWorkerFetchContext>? https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebSharedWorkerClient.h (right): https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/pub... third_party/WebKit/public/web/WebSharedWorkerClient.h:101: virtual WebWorkerFetchContext* CreateWorkerFetchContext() { return nullptr; } How about returning std::unique_ptr<WebWorkerFetchContext>? https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/pub... File third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h (right): https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/pub... third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h:239: virtual blink::WebWorkerFetchContext* CreateServiceWorkerFetchContext() { How about returning std::unique_ptr<WebWorkerFetchContext>?
lgtm from worker infra pov https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp (right): https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp:72: web_context_->InitializeOnWorkerThread(Platform::Current() On 2017/04/20 04:16:07, nhiroki wrote: > The GlobalScopeScheduler patch was landed. Can you update this part as I > commented before? Hmm..., this was reverted. Please ignore this comment for now.
yhirano@chromium.org changed reviewers: + yhirano@chromium.org
You're adding a code path to create a resource dispatcher in a non-main thread, so I think the resource_dispatcher.cc change in the dependent CL should be included in this CL. https://codereview.chromium.org/2804843005/diff/380001/content/renderer/servi... File content/renderer/service_worker/service_worker_fetch_context_impl.h (right): https://codereview.chromium.org/2804843005/diff/380001/content/renderer/servi... content/renderer/service_worker/service_worker_fetch_context_impl.h:36: std::unique_ptr<ResourceDispatcher> resource_dispatcher_; ResourceDispatcher::main_thread_task_runner_ becomes a very confusing name with this change. I think we should add a TODO comment or rename it. https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.h (right): https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.h:163: WebURLLoader* CreateURLLoader() override; I'm landing https://codereview.chromium.org/2831033002/, can you have this function return a unique_ptr as well?
The CQ bit was checked by horo@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: 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 horo@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...
https://codereview.chromium.org/2804843005/diff/380001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2804843005/diff/380001/content/renderer/rende... content/renderer/render_frame_impl.cc:3008: if (!base::FeatureList::IsEnabled(features::kOffMainThreadFetch)) On 2017/04/20 04:16:06, nhiroki wrote: > DCHECK(base::FeatureList::IsEnabled(features::kOffMainThreadFetch)); ? Done. https://codereview.chromium.org/2804843005/diff/380001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2804843005/diff/380001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:1011: ServiceWorkerContextClient::CreateServiceWorkerFetchContext() { On 2017/04/20 04:16:06, nhiroki wrote: > DCHECK(main_thread_task_runner_->RunsTasksOnCurrentThread()); ? Done. https://codereview.chromium.org/2804843005/diff/380001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:1012: if (!base::FeatureList::IsEnabled(features::kOffMainThreadFetch)) On 2017/04/20 04:16:06, nhiroki wrote: > DCHECK(base::FeatureList::IsEnabled(features::kOffMainThreadFetch)); ? Done. https://codereview.chromium.org/2804843005/diff/380001/content/renderer/servi... File content/renderer/service_worker/service_worker_fetch_context_impl.h (right): https://codereview.chromium.org/2804843005/diff/380001/content/renderer/servi... content/renderer/service_worker/service_worker_fetch_context_impl.h:36: std::unique_ptr<ResourceDispatcher> resource_dispatcher_; On 2017/04/20 04:26:59, yhirano wrote: > ResourceDispatcher::main_thread_task_runner_ becomes a very confusing name with > this change. I think we should add a TODO comment or rename it. Done. https://codereview.chromium.org/2804843005/diff/380001/content/renderer/servi... File content/renderer/service_worker/worker_fetch_context_impl.h (right): https://codereview.chromium.org/2804843005/diff/380001/content/renderer/servi... content/renderer/service_worker/worker_fetch_context_impl.h:39: void SetIsControlledByServiceWorker(bool flag); On 2017/04/20 04:16:07, nhiroki wrote: > Optional: Simple setter/getter can use a naming convention like > |set_service_worker_provider_id()|. Done. https://codereview.chromium.org/2804843005/diff/380001/content/renderer/share... File content/renderer/shared_worker/embedded_shared_worker_stub.cc (right): https://codereview.chromium.org/2804843005/diff/380001/content/renderer/share... content/renderer/shared_worker/embedded_shared_worker_stub.cc:263: if (!base::FeatureList::IsEnabled(features::kOffMainThreadFetch)) On 2017/04/20 04:16:07, nhiroki wrote: > DCHECK(base::FeatureList::IsEnabled(features::kOffMainThreadFetch)) ? Done. https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.h (right): https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.h:163: WebURLLoader* CreateURLLoader() override; On 2017/04/20 04:26:59, yhirano wrote: > I'm landing https://codereview.chromium.org/2831033002/, can you have this > function return a unique_ptr as well? Done. https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp (right): https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp:56: WorkerGlobalScope& worker_global_scope) { On 2017/04/20 04:16:07, nhiroki wrote: > Can you add DCHECK(worker_global_scope->IsContextThread()) here? Done. https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerGlobalScope.h (right): https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerGlobalScope.h:158: WorkerFetchContext* FetchContext(); On 2017/04/20 04:08:14, kinuko wrote: > nit: GetFetchContext() Done. https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right): https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:349: DCHECK(web_worker_fetch_context); On 2017/04/20 04:08:14, kinuko wrote: > We could get ServiceWorkerNetworkProvider here and set necessary info too, is > that right? Done. https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrameClient.h:153: virtual WebWorkerFetchContext* CreateWorkerFetchContext() { return nullptr; } On 2017/04/20 04:16:07, nhiroki wrote: > How about returning std::unique_ptr<WebWorkerFetchContext>? Done. https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebSharedWorkerClient.h (right): https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/pub... third_party/WebKit/public/web/WebSharedWorkerClient.h:101: virtual WebWorkerFetchContext* CreateWorkerFetchContext() { return nullptr; } On 2017/04/20 04:16:07, nhiroki wrote: > How about returning std::unique_ptr<WebWorkerFetchContext>? Done. https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/pub... File third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h (right): https://codereview.chromium.org/2804843005/diff/380001/third_party/WebKit/pub... third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h:239: virtual blink::WebWorkerFetchContext* CreateServiceWorkerFetchContext() { On 2017/04/20 04:16:07, nhiroki wrote: > How about returning std::unique_ptr<WebWorkerFetchContext>? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by horo@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.
https://codereview.chromium.org/2804843005/diff/440001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2804843005/diff/440001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:485: mojo::MakeStrongBinding( When is this destructed? https://codereview.chromium.org/2804843005/diff/440001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/2804843005/diff/440001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:277: for (auto it = worker_clients_.begin(); it != worker_clients_.end(); it++) { optional: for (const auto& pair : worker_clients_) { pair.second.SetControllerServiceWorker(version->version_id()); }
lgtm https://codereview.chromium.org/2804843005/diff/440001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2804843005/diff/440001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:485: mojo::MakeStrongBinding( On 2017/04/21 09:18:41, yhirano wrote: > When is this destructed? Sorry, I was confused. I retract this comment.
The CQ bit was checked by horo@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...
Thank you! https://codereview.chromium.org/2804843005/diff/440001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/2804843005/diff/440001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:277: for (auto it = worker_clients_.begin(); it != worker_clients_.end(); it++) { On 2017/04/21 09:18:41, yhirano wrote: > optional: > > for (const auto& pair : worker_clients_) { > pair.second.SetControllerServiceWorker(version->version_id()); > } Done.
horo@chromium.org changed reviewers: + estark@chromium.org
estark@ Could you please review .mojom files and content_browser_manifest.json?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
estark@chromium.org changed reviewers: + kenrb@chromium.org
The mojom changes mostly lg with a question... but TBH I've been staring at this for a while and feel like I only 10% understand what's going on in this CL. :) kenrb, since I'm new to mojo security reviews and this is a complex CL, do you think you could take a look as well to make sure I'm not missing anything? https://codereview.chromium.org/2804843005/diff/460001/content/renderer/servi... File content/renderer/service_worker/worker_fetch_context_impl.cc (right): https://codereview.chromium.org/2804843005/diff/460001/content/renderer/servi... content/renderer/service_worker/worker_fetch_context_impl.cc:71: controller_version_id_ = controller_version_id; Is there any reason to send the id rather than just a boolean?
https://codereview.chromium.org/2804843005/diff/460001/content/renderer/servi... File content/renderer/service_worker/worker_fetch_context_impl.cc (right): https://codereview.chromium.org/2804843005/diff/460001/content/renderer/servi... content/renderer/service_worker/worker_fetch_context_impl.cc:71: controller_version_id_ = controller_version_id; On 2017/04/25 06:41:08, estark wrote: > Is there any reason to send the id rather than just a boolean? We are using the service worker version ID as a key of MemoryCache in pages. https://docs.google.com/document/d/1eZz3MZ606jQtD2u1SHx8UTguGdMNk6z7iVh54G43a... When we will support MemoryCache in workers, we will use the service worker version ID as a key of MemoryCache same as pages.
https://codereview.chromium.org/2804843005/diff/460001/content/renderer/servi... File content/renderer/service_worker/worker_fetch_context_impl.cc (right): https://codereview.chromium.org/2804843005/diff/460001/content/renderer/servi... content/renderer/service_worker/worker_fetch_context_impl.cc:71: controller_version_id_ = controller_version_id; On 2017/04/25 08:43:44, horo wrote: > On 2017/04/25 06:41:08, estark wrote: > > Is there any reason to send the id rather than just a boolean? > > We are using the service worker version ID as a key of MemoryCache in pages. > > https://docs.google.com/document/d/1eZz3MZ606jQtD2u1SHx8UTguGdMNk6z7iVh54G43a... > > When we will support MemoryCache in workers, we will use the service worker > version ID as a key of MemoryCache same as pages. Thanks for the explanation. LGTM but please wait for kenrb to have a look as well.
lgtm I'm not especially familiar with web/service worker implementations, but I don't see any problems here. The new interfaces are light with very little data being passed, and it is reassuring that the browser process nicely silos providers by renderer process.
The CQ bit was checked by horo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, nhiroki@chromium.org, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2804843005/#ps460001 (title: "incorporated yhirano's comment")
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
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by horo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, kenrb@chromium.org, nhiroki@chromium.org, yhirano@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/2804843005/#ps480001 (title: "rebase")
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
Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by horo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, kenrb@chromium.org, nhiroki@chromium.org, yhirano@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/2804843005/#ps500001 (title: "s/WebScheduler.h/web_scheduler.h/")
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": 500001, "attempt_start_ts": 1493161007648920, "parent_rev": "c36577e5a5e9f4e7a7f089bc7b457fa77f30578b", "commit_rev": "e612058238bb3f549a917d55f94badf32377f85c"}
Message was sent while issue was closed.
Description was changed from ========== Implement the infrastructure of creating WorkerFetchContext in worker global scope. This CL implements the infrastructure of creating WorkerFetchContext in worker global scope of Dedicated Worker, Shared Worker and Service Worker. 1. WorkerFetchContext for Dedicated Worker 1.1. DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() calls RenderFrameImpl::CreateWorkerFetchContext() on the main thread. 1.2. RenderFrameImpl::CreateWorkerFetchContext() gets the interface of WorkerURLLoaderFactoryProvider which is implemented in the browser process as WorkerURLLoaderFactoryProviderImpl, and creates a WorkerFetchContextImpl with the interface. WorkerFetchContextImpl keeps the interface as |provider_info_| until InitializeOnWorkerThread() is called on the worker thread. 1.3. RenderFrameImpl::CreateWorkerFetchContext() copies the service worker state (ServiceWorkerProviderID and IsControlledByServiceWorker) of the frame to the WorkerFetchContextImpl. 1.4. ProvideWorkerFetchContextToWorker() creates a WorkerFetchContextHolder which holds the WorkerFetchContextImpl and provides it to the WorkerClients using the Supplement mechanism. 1.5. After the worker thread is created, the WorkerClients is reattached to the worker thread in the constructor of WorkerGlobalScope. 1.6. WorkerGlobalScope::FetchContext() will be called to get the WorkerFetchContext in ThreadableLoader::Create() When fetch() is called in the worker thread. (This is not in this CL yet.) 1.7. When WorkerGlobalScope::FetchContext() is called for the first time, WorkerFetchContext::Create() is called to get the WorkerFetchContextHolder from the WorkerClients and take the WorkerFetchContextImpl from it and create the WorkerFetchContext. 1.8. The constructor of WorkerFetchContext calls WorkerFetchContextImpl:: InitializeOnWorkerThread() which creates a ResourceDispatcher and calls WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactoryAndRegisterClient() in the browser process via mojo IPC with a request of URLLoaderFactory and the pointer (ServiceWorkerWorkerClient) of itself. 1.9. WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactoryAndRegisterClient() binds the request of URLLoaderFactory to ResourceMessageFilter and calls ServiceWorkerContextWrapper::BindWorkerFetchContext() to bind the ServiceWorkerWorkerClient (the pointer to WorkerFetchContextImpl in the worker thread) with the parent frame's ServiceWorkerProviderHost. When the controller service worker of the ServiceWorkerProviderHost changed, WorkerFetchContextImpl::SetControllerServiceWorker() will be called via mojo. So WorkerFetchContextImpl::IsControlledByServiceWorker() can return the correct status. 1.10. WorkerFetchContext::CreateURLLoader() calls WorkerFetchContextImpl:: CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. 2. WorkerFetchContext for Shared Worker is created almost same as the Dedicated Worker. The only difference is that CreateWorkerFetchContext() is called in WebSharedWorkerImpl::OnScriptLoaderFinished(). 3. WorkerFetchContext for Service Worker 3.1. WebEmbeddedWorkerImpl::StartWorkerThread() calls ServiceWorkerContextClient::CreateServiceWorkerFetchContext() on the main thread. 3.2. ServiceWorkerContextClient::CreateServiceWorkerFetchContext() gets the interface of WorkerURLLoaderFactoryProvider which is implemented in the browser process as WorkerURLLoaderFactoryProviderImpl, and creates a ServiceWorkerFetchContextImpl with the interface. ServiceWorkerFetchContextImpl keeps the interface as |provider_info_| until InitializeOnWorkerThread() is called on the worker thread. 3.3. In WebEmbeddedWorkerImpl::StartWorkerThread(), the fetch context related information (ex: DataSaverEnabled) is set to the ServiceWorkerFetchContextImpl. (This is not in this CL yet.) 3.4. Same as 1.4. ProvideWorkerFetchContextToWorker() creates a WorkerFetchContextHolder which holds the ServiceWorkerFetchContextImpl and provides it to the WorkerClientsusing the Supplement mechanism. 3.5. Same as 1.5. the WorkerClients is reattached to the worker thread. 3.6. Same as 1.6. 3.7. Same as 1.7. When WorkerGlobalScope::FetchContext() is called for the first time, WorkerFetchContext::Create() is called to get the WorkerFetchContextHolder from the WorkerClients and take the ServiceWorkerFetchContextImpl from it and create the WorkerFetchContext. 3.8. The constructor of WorkerFetchContext calls ServiceWorkerFetchContextImpl ::InitializeOnWorkerThread() which creates a ResourceDispatcher and calls WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactory() in the browser process via mojo IPC with a request of URLLoaderFactory. 3.9. WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactory() binds the request of URLLoaderFactory to ResourceMessageFilter. 3.10. WorkerFetchContext::CreateURLLoader() calls ServiceWorkerFetchContextImpl::CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. BUG=443374 ========== to ========== Implement the infrastructure of creating WorkerFetchContext in worker global scope. This CL implements the infrastructure of creating WorkerFetchContext in worker global scope of Dedicated Worker, Shared Worker and Service Worker. 1. WorkerFetchContext for Dedicated Worker 1.1. DedicatedWorkerMessagingProxyProviderImpl::CreateWorkerMessagingProxy() calls RenderFrameImpl::CreateWorkerFetchContext() on the main thread. 1.2. RenderFrameImpl::CreateWorkerFetchContext() gets the interface of WorkerURLLoaderFactoryProvider which is implemented in the browser process as WorkerURLLoaderFactoryProviderImpl, and creates a WorkerFetchContextImpl with the interface. WorkerFetchContextImpl keeps the interface as |provider_info_| until InitializeOnWorkerThread() is called on the worker thread. 1.3. RenderFrameImpl::CreateWorkerFetchContext() copies the service worker state (ServiceWorkerProviderID and IsControlledByServiceWorker) of the frame to the WorkerFetchContextImpl. 1.4. ProvideWorkerFetchContextToWorker() creates a WorkerFetchContextHolder which holds the WorkerFetchContextImpl and provides it to the WorkerClients using the Supplement mechanism. 1.5. After the worker thread is created, the WorkerClients is reattached to the worker thread in the constructor of WorkerGlobalScope. 1.6. WorkerGlobalScope::FetchContext() will be called to get the WorkerFetchContext in ThreadableLoader::Create() When fetch() is called in the worker thread. (This is not in this CL yet.) 1.7. When WorkerGlobalScope::FetchContext() is called for the first time, WorkerFetchContext::Create() is called to get the WorkerFetchContextHolder from the WorkerClients and take the WorkerFetchContextImpl from it and create the WorkerFetchContext. 1.8. The constructor of WorkerFetchContext calls WorkerFetchContextImpl:: InitializeOnWorkerThread() which creates a ResourceDispatcher and calls WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactoryAndRegisterClient() in the browser process via mojo IPC with a request of URLLoaderFactory and the pointer (ServiceWorkerWorkerClient) of itself. 1.9. WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactoryAndRegisterClient() binds the request of URLLoaderFactory to ResourceMessageFilter and calls ServiceWorkerContextWrapper::BindWorkerFetchContext() to bind the ServiceWorkerWorkerClient (the pointer to WorkerFetchContextImpl in the worker thread) with the parent frame's ServiceWorkerProviderHost. When the controller service worker of the ServiceWorkerProviderHost changed, WorkerFetchContextImpl::SetControllerServiceWorker() will be called via mojo. So WorkerFetchContextImpl::IsControlledByServiceWorker() can return the correct status. 1.10. WorkerFetchContext::CreateURLLoader() calls WorkerFetchContextImpl:: CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. 2. WorkerFetchContext for Shared Worker is created almost same as the Dedicated Worker. The only difference is that CreateWorkerFetchContext() is called in WebSharedWorkerImpl::OnScriptLoaderFinished(). 3. WorkerFetchContext for Service Worker 3.1. WebEmbeddedWorkerImpl::StartWorkerThread() calls ServiceWorkerContextClient::CreateServiceWorkerFetchContext() on the main thread. 3.2. ServiceWorkerContextClient::CreateServiceWorkerFetchContext() gets the interface of WorkerURLLoaderFactoryProvider which is implemented in the browser process as WorkerURLLoaderFactoryProviderImpl, and creates a ServiceWorkerFetchContextImpl with the interface. ServiceWorkerFetchContextImpl keeps the interface as |provider_info_| until InitializeOnWorkerThread() is called on the worker thread. 3.3. In WebEmbeddedWorkerImpl::StartWorkerThread(), the fetch context related information (ex: DataSaverEnabled) is set to the ServiceWorkerFetchContextImpl. (This is not in this CL yet.) 3.4. Same as 1.4. ProvideWorkerFetchContextToWorker() creates a WorkerFetchContextHolder which holds the ServiceWorkerFetchContextImpl and provides it to the WorkerClientsusing the Supplement mechanism. 3.5. Same as 1.5. the WorkerClients is reattached to the worker thread. 3.6. Same as 1.6. 3.7. Same as 1.7. When WorkerGlobalScope::FetchContext() is called for the first time, WorkerFetchContext::Create() is called to get the WorkerFetchContextHolder from the WorkerClients and take the ServiceWorkerFetchContextImpl from it and create the WorkerFetchContext. 3.8. The constructor of WorkerFetchContext calls ServiceWorkerFetchContextImpl ::InitializeOnWorkerThread() which creates a ResourceDispatcher and calls WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactory() in the browser process via mojo IPC with a request of URLLoaderFactory. 3.9. WorkerURLLoaderFactoryProviderImpl::GetURLLoaderFactory() binds the request of URLLoaderFactory to ResourceMessageFilter. 3.10. WorkerFetchContext::CreateURLLoader() calls ServiceWorkerFetchContextImpl::CreateURLLoader() which returns a new WebURLLoaderImpl with the pointer of URLLoaderFactory. BUG=443374 Review-Url: https://codereview.chromium.org/2804843005 Cr-Commit-Position: refs/heads/master@{#467200} Committed: https://chromium.googlesource.com/chromium/src/+/e612058238bb3f549a917d55f94b... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:500001) as https://chromium.googlesource.com/chromium/src/+/e612058238bb3f549a917d55f94b... |