Make DedicatedWorkerHost inherit BackForwardCacheHostControllerHost [chromium/src : main]
Hajime Hoshi (Gerrit)
Attention is currently required from: Daniel Cheng, Alexander Timin, Hiroki Nakagawa.
Hajime Hoshi would like Daniel Cheng, Alexander Timin and Hiroki Nakagawa to review this change.
Make DedicatedWorkerHost inherit BackForwardCacheHostControllerHost
This is a preparation to notify feature usages and/or evict the frame
from dedicated workers.
This CL also passes a BackForwardCacheHostControllerHost to a renderer,
but the renderer doesn't call any functions of the host yet. I will
update this in later CLs.
Design Doc: https://docs.google.com/document/d/1CJfDNqA2tEp_dmxK5kaBHHqGjx7cK2_jhTpsCwSCa3A/edit?usp=sharing
Bug: 1146955
Change-Id: Ieea9c9013e52b7e0ffb8d9b4386903dd32fe82bb
---
M content/browser/worker_host/dedicated_worker_service_impl_unittest.cc
M content/renderer/worker/dedicated_worker_host_factory_client.h
M third_party/blink/renderer/core/workers/dedicated_worker_messaging_proxy.cc
M third_party/blink/renderer/core/workers/dedicated_worker_thread.cc
M third_party/blink/renderer/core/workers/dedicated_worker.h
M third_party/blink/renderer/core/workers/dedicated_worker_global_scope.h
M content/browser/worker_host/dedicated_worker_host.cc
M third_party/blink/renderer/core/workers/dedicated_worker_messaging_proxy.h
M content/renderer/worker/dedicated_worker_host_factory_client.cc
M third_party/blink/renderer/core/workers/dedicated_worker_test.cc
M third_party/blink/public/mojom/worker/dedicated_worker_host_factory.mojom
M third_party/blink/public/platform/web_dedicated_worker.h
M content/browser/worker_host/dedicated_worker_host.h
M third_party/blink/renderer/core/workers/dedicated_worker.cc
M third_party/blink/renderer/core/workers/dedicated_worker_thread.h
M third_party/blink/renderer/core/workers/dedicated_worker_global_scope.cc
16 files changed, 168 insertions(+), 38 deletions(-)
To view, visit change 3220891. To unsubscribe, or for help writing mail filters, visit settings.
Hajime Hoshi (Gerrit)
Attention is currently required from: Daniel Cheng, Alexander Timin, Hiroki Nakagawa.
Patch set 12:Auto-Submit +1Commit-Queue +1
1 comment:
Patchset:
PTAL
To view, visit change 3220891. To unsubscribe, or for help writing mail filters, visit settings.
Hiroki Nakagawa (Gerrit)
Attention is currently required from: Daniel Cheng, Hajime Hoshi, Alexander Timin.
Patch set 14:Code-Review +1
4 comments:
Patchset:
LGTM
File content/renderer/worker/dedicated_worker_host_factory_client.cc:
Patch Set #14, Line 18:
#include "third_party/blink/public/mojom/frame/back_forward_cache_controller.mojom.h"This is already included in the header.
File third_party/blink/public/mojom/worker/dedicated_worker_host_factory.mojom:
Patch Set #14, Line 67:
pending_remote<BackForwardCacheControllerHost> back_forward_cache_controller_host);Can you wrap these lines at the 80th char?
File third_party/blink/renderer/core/workers/dedicated_worker_messaging_proxy.cc:
Patch Set #14, Line 74:
|dedicated_worker_host|"These must be stored before InitializeWorkerThread." ?
To view, visit change 3220891. To unsubscribe, or for help writing mail filters, visit settings.
Hajime Hoshi (Gerrit)
Attention is currently required from: Daniel Cheng, Alexander Timin.
Patch set 14:Auto-Submit +1
4 comments:
Patchset:
Thank you!
File content/renderer/worker/dedicated_worker_host_factory_client.cc:
Patch Set #14, Line 18:
#include "third_party/blink/public/mojom/frame/back_forward_cache_controller.mojom.h"This is already included in the header.
Done
File third_party/blink/public/mojom/worker/dedicated_worker_host_factory.mojom:
Patch Set #14, Line 67:
pending_remote<BackForwardCacheControllerHost> back_forward_cache_controller_host);Can you wrap these lines at the 80th char?
Done
File third_party/blink/renderer/core/workers/dedicated_worker_messaging_proxy.cc:
Patch Set #14, Line 74:
|dedicated_worker_host|
"These must be stored before InitializeWorkerThread. […]
Done
To view, visit change 3220891. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: Hajime Hoshi, Alexander Timin.
2 comments:
File content/browser/worker_host/dedicated_worker_host.cc:
Patch Set #15, Line 807:
// The frame may have already been closed.Does this imply that the dedicated worker host will go away "soon" too?
Patch Set #15, Line 818:
// The frame may have already been closed.Similar question here. Does a DedicatedWorkerHost observe the lifetime of the RFH in any way?
To view, visit change 3220891. To unsubscribe, or for help writing mail filters, visit settings.
Hajime Hoshi (Gerrit)
Attention is currently required from: Daniel Cheng, Alexander Timin.
2 comments:
File content/browser/worker_host/dedicated_worker_host.cc:
Patch Set #15, Line 807:
// The frame may have already been closed.Does this imply that the dedicated worker host will go away "soon" too?
Yes, I followed the similar checks like [1].
Patch Set #15, Line 818:
// The frame may have already been closed.
Similar question here. […]
ditto
To view, visit change 3220891. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: Hajime Hoshi, Alexander Timin.
Patch set 15:Code-Review +1
1 comment:
Patchset:
LGTM
(I would suggest that it might be helpful to centralize this lifetime in a class-level comment, but I see this is already a TODO in https://source.chromium.org/chromium/chromium/src/+/main:content/browser/worker_host/dedicated_worker_host.h;l=57;drc=5539ecff898c79b0771340051d62bf81649e448d so I guess this is OK. Thank you for the explanation!)
To view, visit change 3220891. To unsubscribe, or for help writing mail filters, visit settings.
Hajime Hoshi (Gerrit)
Attention is currently required from: Alexander Timin.
1 comment:
Patchset:
This CL adds a new argument back_forward_cache_controller_host to DedicatedWorkerHostFactoryClient::OnScriptLoaded, but I started to think that we should add this to OnWorkerHostCreated instead, as a worker thread is created. There are some cases that OnScriptLoaded is not invoked (e.g., DedicatedWorker::OnHostCreated is invoked with type module), but I'm not 100% sure.
nhiroki@, what do you think?
To view, visit change 3220891. To unsubscribe, or for help writing mail filters, visit settings.
Hajime Hoshi (Gerrit)
Attention is currently required from: Alexander Timin.
Patch set 15:Commit-Queue +2
1 comment:
Patchset:
This CL adds a new argument back_forward_cache_controller_host to DedicatedWorkerHostFactoryClient:: […]
As we discussed offline, the path to call ContinueStart will be integrated to OnScriptLoaded from the current three paths (OnScriptLoaded, OnWorkerHostCreated, OnFinished) in the very near future. (= PlzDedicatedWorker will be used by default) Then, let's go with this CL.
Note that we can switch this behavior by the feature flag "PlzDedicatedWorker" [1]. We can write browser tests for feature detection from dedicated workers with this flag.
To view, visit change 3220891. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Make DedicatedWorkerHost inherit BackForwardCacheHostControllerHost
This is a preparation to notify feature usages and/or evict the frame
from dedicated workers.
This CL also passes a BackForwardCacheHostControllerHost to a renderer,
but the renderer doesn't call any functions of the host yet. I will
update this in later CLs.
Design Doc: https://docs.google.com/document/d/1CJfDNqA2tEp_dmxK5kaBHHqGjx7cK2_jhTpsCwSCa3A/edit?usp=sharing
Bug: 1146955
Change-Id: Ieea9c9013e52b7e0ffb8d9b4386903dd32fe82bb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3220891
Auto-Submit: Hajime Hoshi <hajim...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhi...@chromium.org>
Commit-Queue: Hajime Hoshi <hajim...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#934878}
---
M content/browser/worker_host/dedicated_worker_service_impl_unittest.cc
M content/renderer/worker/dedicated_worker_host_factory_client.h
M third_party/blink/renderer/core/workers/dedicated_worker_messaging_proxy.cc
M third_party/blink/renderer/core/workers/dedicated_worker_thread.cc
M third_party/blink/renderer/core/workers/dedicated_worker.h
M third_party/blink/renderer/core/workers/dedicated_worker_global_scope.h
M content/browser/worker_host/dedicated_worker_host.cc
M third_party/blink/renderer/core/workers/dedicated_worker_messaging_proxy.h
M content/renderer/worker/dedicated_worker_host_factory_client.cc
M third_party/blink/renderer/core/workers/dedicated_worker_test.cc
M third_party/blink/public/mojom/worker/dedicated_worker_host_factory.mojom
M third_party/blink/public/platform/web_dedicated_worker.h
M content/browser/worker_host/dedicated_worker_host.h
M third_party/blink/renderer/core/workers/dedicated_worker.cc
M third_party/blink/renderer/core/workers/dedicated_worker_thread.h
M third_party/blink/renderer/core/workers/dedicated_worker_global_scope.cc
16 files changed, 177 insertions(+), 39 deletions(-)
Fergal Daly (Gerrit)
Attention is currently required from: Hajime Hoshi.
Hajime Hoshi has uploaded this change for review.
Fergal Daly (Gerrit)
Attention is currently required from: Hajime Hoshi.
2 comments:
File content/browser/worker_host/dedicated_worker_host.cc:
Patch Set #16, Line 821:
ancestor_render_frame_host->DidChangeBackForwardCacheDisablingFeatures(Very late but this is going to just overwrite the frame's existing reasons (or if the frame changes its reasons later, it will overwrite this).
Also, if the DW is destroyed, its reasons are no longer relevant.
We need the DWHost to store its own reasons. I think we should factor out a class/struct with
BackForwardCacheDisablingFeatures
renderer_reported_bfcache_disabling_features_;// Count the usage of BackForwardCacheDisablingFeature.
base::flat_map<BackForwardCacheDisablingFeature, int>
browser_reported_bfcache_disabling_features_counts_;
and related code and include it in RFH and DWH and iterate over them all when we are trying to decide if we can cache or not.We will also want this later because we would like to display each frame's and reasons separately and it makes sense to display each worker's reasons separately too.
Patch Set #16, Line 821:
ancestor_render_frame_host->DidChangeBackForwardCacheDisablingFeatures(Very late but this is going to just overwrite the frame's existing reasons (or if the frame changes its reasons later, it will overwrite this).
Also, if the DW is destroyed, its reasons are no longer relevant.
We need the DWHost to store its own reasons. I think we should factor out a class/struct with
BackForwardCacheDisablingFeatures
renderer_reported_bfcache_disabling_features_;// Count the usage of BackForwardCacheDisablingFeature.
base::flat_map<BackForwardCacheDisablingFeature, int>
browser_reported_bfcache_disabling_features_counts_;
and related code and include it in RFH and DWH and iterate over them all when we are trying to decide if we can cache or not.We will also want this later because we would like to display each frame's and reasons separately and it makes sense to display each worker's reasons separately too.
To view, visit change 3220891. To unsubscribe, or for help writing mail filters, visit settings.
Hajime Hoshi (Gerrit)
1 comment:
File content/browser/worker_host/dedicated_worker_host.cc:
Patch Set #16, Line 821:
ancestor_render_frame_host->DidChangeBackForwardCacheDisablingFeatures(
Very late but this is going to just overwrite the frame's existing reasons (or if the frame changes […]
Yeah that's a good point... I'll take care of them in another CL. Thanks,
To view, visit change 3220891. To unsubscribe, or for help writing mail filters, visit settings.