Ensure WakeEventPage works for service worker based extensions [chromium/src : main]
0 views
Skip to first unread message
Shari Trewin (Gerrit)
Oct 17, 2025, 11:24:25 AM (3 days ago) Oct 17
to Kristi Saney, Devlin Cronin, Emilia Paz, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Devlin Cronin, Emilia Paz and Kristi Saney
Shari Trewin added 2 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 2 (Latest):
Shari Trewin . resolved
LGTM! I tested on ChromeOS and can confirm this does resolve the P0 issue of language installs not happening. Language installs still take much longer than previously (~30 sec), which we should look into.
Commit Message
Line 15, Patchset 2 (Latest):
Fixed: b/448034422Shari Trewin . unresolved
Please fix this WARNING reported by No short links in OSS: Do not use b/<number> on the Fixed: line in OSS code. Prefer `Fixed: <number>` i...
Do not use b/<number> on the Fixed: line in OSS code. Prefer `Fixed: <number>` instead. While bugs should be public by default, bugs that must remain internal may use b:<number>, ignoring this warning; see https://www.chromium.org/issue-tracking/googler-guidelines/.
Related details
Attention is currently required from:
- Devlin Cronin
- Emilia Paz
- Kristi Saney
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kristi Saney (Gerrit)
Oct 17, 2025, 12:59:35 PM (3 days ago) Oct 17
to Devlin Cronin, Emilia Paz, Shari Trewin, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Devlin Cronin, Emilia Paz and Shari Trewin
Kristi Saney voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kristi Saney
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Please fix this WARNING reported by No short links in OSS: Do not use b/<number> on the Fixed: line in OSS code. Prefer `Fixed: <number>` i...
Do not use b/<number> on the Fixed: line in OSS code. Prefer `Fixed: <number>` instead. While bugs should be public by default, bugs that must remain internal may use b:<number>, ignoring this warning; see https://www.chromium.org/issue-tracking/googler-guidelines/.
Attention is currently required from:
- Devlin Cronin
- Emilia Paz
- Shari Trewin
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emilia Paz (Gerrit)
Oct 17, 2025, 2:24:35 PM (3 days ago) Oct 17
to Kristi Saney, Chromium LUCI CQ, Devlin Cronin, Shari Trewin, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Devlin Cronin, Kristi Saney and Shari Trewin
Emilia Paz added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 4 (Latest):
Emilia Paz . resolved
Thanks for the fix! Could we add a regression test?
Related details
Attention is currently required from:
- Devlin Cronin
- Kristi Saney
- Shari Trewin
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kristi Saney (Gerrit)
Oct 17, 2025, 3:45:40 PM (3 days ago) Oct 17
to Chromium LUCI CQ, Devlin Cronin, Emilia Paz, Shari Trewin, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Emilia Paz
Kristi Saney voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kristi Saney
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Emilia Paz . resolved
Thanks for the fix! Could we add a regression test?
Attention is currently required from:
- Emilia Paz
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kristi Saney (Gerrit)
Oct 17, 2025, 7:23:30 PM (3 days ago) Oct 17
to Chromium LUCI CQ, Devlin Cronin, Emilia Paz, Shari Trewin, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Emilia Paz
Kristi Saney added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 6 (Latest):
Kristi Saney . resolved
PTAL as this is blocking stable. Thanks.
Emilia Paz (Gerrit)
Oct 17, 2025, 7:36:17 PM (3 days ago) Oct 17
to Kristi Saney, Chromium LUCI CQ, Devlin Cronin, Shari Trewin, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Kristi Saney
Emilia Paz voted and added 3 comments![]( "Open in Gerrit")
Votes added by Emilia Paz
| Code-Review | +1 |
3 comments
Patchset-level comments
Emilia Paz . resolved
Thanks! Just one small improvement
File chrome/browser/extensions/service_worker_apitest.cc
Line 1972, Patchset 6 (Latest):
IN_PROC_BROWSER_TEST_F(ServiceWorkerBasedBackgroundTest,
WakeEventPage_WakesServiceWorkerBasedExtension) {Emilia Paz . unresolved
nit: Lets add a comment that explains the test and links to the regression bug
File extensions/browser/process_manager.cc
Line 402, Patchset 6 (Latest):
const Extension* extension =
extension_registry_->enabled_extensions().GetByID(extension_id);
if ((extension && BackgroundInfo::IsServiceWorkerBased(extension) &&
!GetServiceWorkersForExtension(extension_id).empty()) ||
GetBackgroundHostForExtension(extension_id)) {Emilia Paz . unresolved
nit: If there is no extension, cannot wake the event page regardless of whether it's service worker or background page. The background host is removed if the extension is unloaded [here](https://source.chromium.org/chromium/chromium/src/+/main:extensions/browser/process_manager.cc;l=635-643;drc=240825a2c96b5bd0ae6d124919de323054ac6080;bpv=1;bpt=1)
I think it's more correct to exit early if there is no extension:
```
if (!extension) {
// No extension to wake up.
return;
}
if (BackgroundInfo::IsServiceWorkerBased(extension) &&
!GetServiceWorkersForExtension(extension_id).empty()) ||
GetBackgroundHostForExtension(extension_id)) {
// The extension is already awake
return;
}
```
Sorry I missed this on the previous review
Related details
Attention is currently required from:
- Kristi Saney
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kristi Saney (Gerrit)
Oct 17, 2025, 7:48:28 PM (3 days ago) Oct 17
to Emilia Paz, Chromium LUCI CQ, Devlin Cronin, Shari Trewin, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Kristi Saney voted and added 2 comments![]( "Open in Gerrit")
Votes added by Kristi Saney
| Auto-Submit | +1 |
2 comments
File chrome/browser/extensions/service_worker_apitest.cc
Line 1972, Patchset 6:
IN_PROC_BROWSER_TEST_F(ServiceWorkerBasedBackgroundTest,
WakeEventPage_WakesServiceWorkerBasedExtension) {Emilia Paz . resolved
nit: Lets add a comment that explains the test and links to the regression bug
Kristi Saney
Done
File extensions/browser/process_manager.cc
Line 402, Patchset 6:
const Extension* extension =
extension_registry_->enabled_extensions().GetByID(extension_id);
if ((extension && BackgroundInfo::IsServiceWorkerBased(extension) &&
!GetServiceWorkersForExtension(extension_id).empty()) ||
GetBackgroundHostForExtension(extension_id)) {Emilia Paz . resolved
nit: If there is no extension, cannot wake the event page regardless of whether it's service worker or background page. The background host is removed if the extension is unloaded [here](https://source.chromium.org/chromium/chromium/src/+/main:extensions/browser/process_manager.cc;l=635-643;drc=240825a2c96b5bd0ae6d124919de323054ac6080;bpv=1;bpt=1)
I think it's more correct to exit early if there is no extension:
```
if (!extension) {
// No extension to wake up.
return;
}if (BackgroundInfo::IsServiceWorkerBased(extension) &&
!GetServiceWorkersForExtension(extension_id).empty()) ||
GetBackgroundHostForExtension(extension_id)) {
// The extension is already awake
return;
}
```Sorry I missed this on the previous review
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kristi Saney (Gerrit)
Oct 17, 2025, 7:48:34 PM (3 days ago) Oct 17
to Emilia Paz, Chromium LUCI CQ, Devlin Cronin, Shari Trewin, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Kristi Saney voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
4:25 PM (4 hours ago) 4:25 PM
to Kristi Saney, Eitan Goldberger, Emilia Paz, Devlin Cronin, Shari Trewin, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/extensions/service_worker_apitest.cc
Insertions: 3, Deletions: 0.
@@ -1970,6 +1970,9 @@
EXPECT_TRUE(finished_listener.WaitUntilSatisfied());
}
+// Regression test for crbug.com/c/448034422. Other tests already test that a
+// background page is wakened via WakeEventPage, but this tests that service
+// worker based extensions wake too.
IN_PROC_BROWSER_TEST_F(ServiceWorkerBasedBackgroundTest,
WakeEventPage_WakesServiceWorkerBasedExtension) {
ExtensionTestMessageListener event_listener_added("ready");
```
Change information
Commit message:
Ensure WakeEventPage works for service worker based extensions
On ChromeOS we have a launch blocking P0 that language downloads do not
work in read aloud. This is because we are waiting for the tts engine to
wake before requesting install but it never wakes in the manifest v3
version. See where this is used at
https://source.corp.google.com/h/chromium/chromium/src/+/main:chrome/browser/extensions/component_loader.cc;l=753?q=chrome%2Fbrowser%2Fextensions%2Fcomponent_loader.cc
Fixed: 448034422
Change-Id: Id034ac898b29326f1b7c15163026a57f62f05091
Reviewed-by: Emilia Paz <emil...@chromium.org>
Commit-Queue: Eitan Goldberger <eit...@google.com>
Auto-Submit: Kristi Saney <krist...@google.com>
Cr-Commit-Position: refs/heads/main@{#1532500}
Files:
- M chrome/browser/extensions/service_worker_apitest.cc
- M extensions/browser/process_manager.cc
Change size: M
Delta: 2 files changed, 67 insertions(+), 2 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Emilia Paz
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages