Hidehiko Abe (Gerrit)

unread,

Jul 1, 2026, 6:26:38 AM (9 days ago) Jul 1

to Jun Ishiguro, Daniel Murphy, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, rrsilva+wat...@google.com, croissant-...@chromium.org, cros-setti...@google.com, chromium-a...@chromium.org, dmurph+wat...@chromium.org, webap...@microsoft.com

Attention needed from Jun Ishiguro

Hidehiko Abe added 1 comment

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Hidehiko Abe . resolved

PTAL.

Open in Gerrit

Related details

Attention is currently required from:

Jun Ishiguro

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jun Ishiguro (Gerrit)

unread,

Jul 2, 2026, 1:03:31 AM (8 days ago) Jul 2

to Hidehiko Abe, Daniel Murphy, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, rrsilva+wat...@google.com, croissant-...@chromium.org, cros-setti...@google.com, chromium-a...@chromium.org, dmurph+wat...@chromium.org, webap...@microsoft.com

Attention needed from Hidehiko Abe

Jun Ishiguro added 6 comments

File chrome/browser/apps/app_service/app_service_proxy_ash.cc

Line 115, Patchset 5 (Latest): app_service_manager_registrar_.Reset(*account_id, this);

Jun Ishiguro . unresolved

Question: Is there a known case where account_id is empty() in production? Otherwise, add `CHECK(!account_id->empty())`?

Line 123, Patchset 5 (Latest): }

Jun Ishiguro . unresolved

Can we have CHECK_IS_TEST() for `!account_id` case?

File chrome/browser/ash/android_sms/android_sms_app_manager_impl.cc

Line 296, Patchset 5 (Latest): return;

Jun Ishiguro . unresolved

Do we expect this case?

File chrome/browser/ash/android_sms/android_sms_app_manager_impl_unittest.cc

Line 132, Patchset 5 (Latest): ash::AnnotatedAccountId::Set(profile_.get(), kAccountId);

Jun Ishiguro . unresolved

optional: `ASSERT_TRUE(app_service_manager_->Find(kAccountId))`?

File chrome/browser/ui/webui/ash/settings/pages/people/account_manager_ui_handler_browsertest.cc

Line 199, Patchset 5 (Latest): TestingBrowserProcess::GetGlobal()->local_state());

Jun Ishiguro . unresolved

Should this be `g_browser_prcess->local_state()` since this is browser tests?

File components/services/app_service/public/cpp/app_service.h

Line 30, Patchset 5 (Latest): // (e.g. window bounds, display ID). If `window_info` is nullptr, the app

Jun Ishiguro . unresolved

nit: now this part should be "if window_info is not specified"?

Open in Gerrit

Related details

Attention is currently required from:

Hidehiko Abe

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Hidehiko Abe (Gerrit)

unread,

Jul 7, 2026, 10:54:58 AM (3 days ago) Jul 7

to Jun Ishiguro, Daniel Murphy, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, rrsilva+wat...@google.com, croissant-...@chromium.org, cros-setti...@google.com, chromium-a...@chromium.org, dmurph+wat...@chromium.org, webap...@microsoft.com

Attention needed from Jun Ishiguro

Hidehiko Abe added 7 comments

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Hidehiko Abe . resolved

PTAL.
betty failure looks unrelated.
As chatted offline android_sms service is going to be removed soon, so I applied to another case.

File chrome/browser/apps/app_service/app_service_proxy_ash.cc

Line 115, Patchset 5: app_service_manager_registrar_.Reset(*account_id, this);

Jun Ishiguro . resolved

Question: Is there a known case where account_id is empty() in production? Otherwise, add `CHECK(!account_id->empty())`?

Hidehiko Abe

Done

Line 123, Patchset 5: }

Jun Ishiguro . resolved

Can we have CHECK_IS_TEST() for `!account_id` case?

Hidehiko Abe

Done

File chrome/browser/ash/android_sms/android_sms_app_manager_impl.cc

Line 296, Patchset 5: return;

Jun Ishiguro . resolved

Do we expect this case?

Hidehiko Abe

reverted.

File chrome/browser/ash/android_sms/android_sms_app_manager_impl_unittest.cc

Line 132, Patchset 5: ash::AnnotatedAccountId::Set(profile_.get(), kAccountId);

Jun Ishiguro . resolved

optional: `ASSERT_TRUE(app_service_manager_->Find(kAccountId))`?

Hidehiko Abe

reverted.

File chrome/browser/ui/webui/ash/settings/pages/people/account_manager_ui_handler_browsertest.cc

Line 199, Patchset 5: TestingBrowserProcess::GetGlobal()->local_state());

Jun Ishiguro . resolved

Should this be `g_browser_prcess->local_state()` since this is browser tests?

Hidehiko Abe

Done

File components/services/app_service/public/cpp/app_service.h

Line 30, Patchset 5: // (e.g. window bounds, display ID). If `window_info` is nullptr, the app

Jun Ishiguro . resolved

nit: now this part should be "if window_info is not specified"?

Hidehiko Abe

Done

Open in Gerrit

Related details

Attention is currently required from:

Jun Ishiguro

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jun Ishiguro (Gerrit)

unread,

Jul 7, 2026, 9:13:53 PM (3 days ago) Jul 7

to Hidehiko Abe, Daniel Murphy, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, rrsilva+wat...@google.com, croissant-...@chromium.org, cros-setti...@google.com, chromium-a...@chromium.org, dmurph+wat...@chromium.org, webap...@microsoft.com

Attention needed from Hidehiko Abe

Jun Ishiguro voted and added 3 comments

Votes added by Jun Ishiguro

Code-Review

+1

3 comments

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Jun Ishiguro . resolved

LGTM with a question and an option.

File chrome/browser/apps/app_service/app_service_proxy_ash.h

Line 206, Patchset 6 (Latest): // If there was old registration, unregister the registered app_service

Jun Ishiguro . unresolved

BTW, do we plan to use this class for other places e.g. unit tests? This seems a bit complicated if it's dedicated to AppServiceProxyAsh's ctor.

Line 197, Patchset 6 (Latest): class ScopedAppServiceManagerRegistrar {

Jun Ishiguro . unresolved

Optional naming proposal:

`AppServiceManager` -> `AppServiceRegistry`
`ScopedAppServiceManagerRegistrar` -> `ScopedAppServiceRegistration`

The current `AppServiceManager` only set/unset/lookup pointers of AppService, so `AppServiceRegistry` might be more intuitive.

Open in Gerrit

Related details

Attention is currently required from:

Hidehiko Abe

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Hidehiko Abe (Gerrit)

unread,

4:03 AM (7 hours ago) 4:03 AM

to Tiago Vignatti, Jun Ishiguro, Daniel Murphy, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, rrsilva+wat...@google.com, croissant-...@chromium.org, cros-setti...@google.com, chromium-a...@chromium.org, dmurph+wat...@chromium.org, webap...@microsoft.com

Attention needed from Jun Ishiguro

Hidehiko Abe added 3 comments

Patchset-level comments

File-level comment, Patchset 9 (Latest):

Hidehiko Abe . resolved

updated according to our offline discussion. PTAL again?

File chrome/browser/apps/app_service/app_service_proxy_ash.h

Line 206, Patchset 6: // If there was old registration, unregister the registered app_service

Jun Ishiguro . resolved

BTW, do we plan to use this class for other places e.g. unit tests? This seems a bit complicated if it's dedicated to AppServiceProxyAsh's ctor.

Hidehiko Abe

Done

Line 197, Patchset 6: class ScopedAppServiceManagerRegistrar {

Jun Ishiguro . resolved

Optional naming proposal:
`AppServiceManager` -> `AppServiceRegistry`
`ScopedAppServiceManagerRegistrar` -> `ScopedAppServiceRegistration`
The current `AppServiceManager` only set/unset/lookup pointers of AppService, so `AppServiceRegistry` might be more intuitive.

Hidehiko Abe

Done

Open in Gerrit

Related details

Attention is currently required from:

Jun Ishiguro

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jun Ishiguro (Gerrit)

unread,

4:23 AM (7 hours ago) 4:23 AM

to Hidehiko Abe, Tiago Vignatti, Daniel Murphy, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, rrsilva+wat...@google.com, croissant-...@chromium.org, cros-setti...@google.com, chromium-a...@chromium.org, dmurph+wat...@chromium.org, webap...@microsoft.com

Attention needed from Hidehiko Abe

Jun Ishiguro voted and added 2 comments

Votes added by Jun Ishiguro

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 9 (Latest):

Jun Ishiguro . resolved

Thank you for applying my suggestions!
LGTM with a final optional nit.

File components/services/app_service/public/cpp/app_service_registry.cc

Line 31, Patchset 9 (Latest):AppService* AppServiceRegistry::Find(const AccountId& account_id) {

Jun Ishiguro . unresolved

optional: might be good to have `CHECK(!account_id.empty())` in Find/Register/Unregsiter?

Open in Gerrit

Related details

Attention is currently required from:

Hidehiko Abe

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Hidehiko Abe (Gerrit)

unread,

5:35 AM (5 hours ago) 5:35 AM

to Jun Ishiguro, Tiago Vignatti, Daniel Murphy, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, rrsilva+wat...@google.com, croissant-...@chromium.org, cros-setti...@google.com, chromium-a...@chromium.org, dmurph+wat...@chromium.org, webap...@microsoft.com

Hidehiko Abe voted and added 2 comments

Votes added by Hidehiko Abe

Commit-Queue

+2

2 comments

Patchset-level comments

File-level comment, Patchset 10 (Latest):

Hidehiko Abe . resolved

Thank you for review!

File components/services/app_service/public/cpp/app_service_registry.cc

Line 31, Patchset 9:AppService* AppServiceRegistry::Find(const AccountId& account_id) {

Jun Ishiguro . resolved

optional: might be good to have `CHECK(!account_id.empty())` in Find/Register/Unregsiter?

Hidehiko Abe

Done

Open in Gerrit

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.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

6:36 AM (4 hours ago) 6:36 AM

to Hidehiko Abe, Jun Ishiguro, Tiago Vignatti, Daniel Murphy, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, rrsilva+wat...@google.com, croissant-...@chromium.org, cros-setti...@google.com, chromium-a...@chromium.org, dmurph+wat...@chromium.org, webap...@microsoft.com

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

9 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: components/services/app_service/public/cpp/app_service_registry.cc
Insertions: 3, Deletions: 0.

@@ -29,17 +29,20 @@


 }
 
 AppService* AppServiceRegistry::Find(const AccountId& account_id) {

+  CHECK(!account_id.empty());
   auto it = service_map_.find(account_id);
   return it == service_map_.end() ? nullptr : it->second;
 }
 
 void AppServiceRegistry::Register(const AccountId& account_id,
                                   AppService* app_service) {
+  CHECK(!account_id.empty());
   CHECK(app_service);
   CHECK(service_map_.try_emplace(account_id, app_service).second);
 }
 
 void AppServiceRegistry::Unregister(const AccountId& account_id) {
+  CHECK(!account_id.empty());
   CHECK_GT(service_map_.erase(account_id), 0u);
 }
 
```

Change information

Commit message:

Introduce AppService interface and AppServiceManager for ChromeOS

AppService interface abstracts AppServiceProxy, living in
components.
AppServiceManager now maintains the AppService instances tied to
the AccountId, so classes in ChromeOS can reduce the code dependencies
to chrome/browser/apps.

BUG=477191550
TEST=Tryjob

Change-Id: Ic96c1b78588dda0534e65709f3bfb601db7cf0cb

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8017963

Commit-Queue: Hidehiko Abe <hide...@chromium.org>

Reviewed-by: Jun Ishiguro <ju...@google.com>

Cr-Commit-Position: refs/heads/main@{#1660160}

Files:

M chrome/browser/apps/app_service/app_service_proxy_ash.cc
M chrome/browser/apps/app_service/app_service_proxy_ash.h
M chrome/browser/apps/app_service/app_service_proxy_base.h
M chrome/browser/ash/main_parts/chrome_browser_main_parts_ash.cc
M chrome/browser/ash/main_parts/chrome_browser_main_parts_ash.h
M chrome/browser/ui/webui/ash/app_install/BUILD.gn
M chrome/browser/ui/webui/ash/app_install/app_install_page_handler.cc
M chrome/browser/ui/webui/ash/settings/pages/people/DEPS
M chrome/browser/ui/webui/ash/settings/pages/people/account_manager_ui_handler_browsertest.cc
M components/services/app_service/BUILD.gn
M components/services/app_service/public/cpp/BUILD.gn
A components/services/app_service/public/cpp/app_service.cc
A components/services/app_service/public/cpp/app_service.h
A components/services/app_service/public/cpp/app_service_registry.cc
A components/services/app_service/public/cpp/app_service_registry.h

Change size: L

Delta: 15 files changed, 323 insertions(+), 78 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Jun Ishiguro

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

Introduce AppService interface and AppServiceManager for ChromeOS [chromium/src : main]

Hidehiko Abe (Gerrit)

Hidehiko Abe added 1 comment

Related details

Jun Ishiguro (Gerrit)

Jun Ishiguro added 6 comments

Related details

Hidehiko Abe (Gerrit)

Hidehiko Abe added 7 comments

Related details

Jun Ishiguro (Gerrit)

Jun Ishiguro voted and added 3 comments

Votes added by Jun Ishiguro

3 comments

Related details

Hidehiko Abe (Gerrit)

Hidehiko Abe added 3 comments

Related details

Jun Ishiguro (Gerrit)

Jun Ishiguro voted and added 2 comments

Votes added by Jun Ishiguro

2 comments

Related details

Hidehiko Abe (Gerrit)

Hidehiko Abe voted and added 2 comments

Votes added by Hidehiko Abe

2 comments

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information