[PWA/App Migration] Allow calls to IsInRegistrar to use separate filter [chromium/src : main]
0 views
Skip to first unread message
Dibyajyoti Pal (Gerrit)
Jan 12, 2026, 8:10:14 PM (2 days ago) Jan 12
to Marijn Kruisselbrink, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Simon Hangl, chromium-a...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watchin...@chromium.org, dmurph+wat...@chromium.org, dmurph+watc...@chromium.org, giovax...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mac-r...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, rmcelra...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Attention needed from Marijn Kruisselbrink
Dibyajyoti Pal voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dibyajyoti Pal
| Commit-Queue | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Marijn Kruisselbrink
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marijn Kruisselbrink (Gerrit)
Jan 13, 2026, 12:06:56 AM (yesterday) Jan 13
to Dibyajyoti Pal, Marijn Kruisselbrink, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Simon Hangl, chromium-a...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watchin...@chromium.org, dmurph+wat...@chromium.org, dmurph+watc...@chromium.org, giovax...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mac-r...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, rmcelra...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Attention needed from Dibyajyoti Pal
Marijn Kruisselbrink added 1 comment![]( "Open in Gerrit")
File chrome/browser/web_applications/web_app_filter.h
Line 74, Patchset 3 (Latest):
static WebAppFilter IsAppDeemedToNotBeInRegistry();Marijn Kruisselbrink . unresolved
I feel this naming goes against the general idea that these filters should be for specific capabilities rather than implementation details of the registry. I.e. I'd expect something like IsAppVisibleToUser (or IsAppKnownToUser) (which I guess would both be the reverse of this) to be perhaps a better thing to filter by?
Related details
Attention is currently required from:
- Dibyajyoti Pal
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. |
Dibyajyoti Pal (Gerrit)
Jan 13, 2026, 11:56:42 AM (yesterday) Jan 13
to Marijn Kruisselbrink, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Simon Hangl, chromium-a...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watchin...@chromium.org, dmurph+wat...@chromium.org, dmurph+watc...@chromium.org, giovax...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mac-r...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, rmcelra...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Attention needed from Marijn Kruisselbrink
Dibyajyoti Pal voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dibyajyoti Pal
| Commit-Queue | +1 |
1 comment
File chrome/browser/web_applications/web_app_filter.h
Line 74, Patchset 3:
static WebAppFilter IsAppDeemedToNotBeInRegistry();Marijn Kruisselbrink . unresolved
I feel this naming goes against the general idea that these filters should be for specific capabilities rather than implementation details of the registry. I.e. I'd expect something like IsAppVisibleToUser (or IsAppKnownToUser) (which I guess would both be the reverse of this) to be perhaps a better thing to filter by?
Dibyajyoti Pal
Yeah, naming this function was a pain point for me too, how does `IsAppSurfaceableToUser()` sound? It's closer to `Visible`, and it also feels like it includes the nuances of the registry in it. Wdyt?
Related details
Attention is currently required from:
- Marijn Kruisselbrink
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. |
Dibyajyoti Pal (Gerrit)
Jan 13, 2026, 12:08:48 PM (yesterday) Jan 13
to Marijn Kruisselbrink, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Simon Hangl, chromium-a...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watchin...@chromium.org, dmurph+wat...@chromium.org, dmurph+watc...@chromium.org, giovax...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mac-r...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, rmcelra...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Attention needed from Marijn Kruisselbrink
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
File chrome/browser/web_applications/web_app_filter.h
Line 74, Patchset 3:
static WebAppFilter IsAppDeemedToNotBeInRegistry();Marijn Kruisselbrink . unresolved
Dibyajyoti PalI feel this naming goes against the general idea that these filters should be for specific capabilities rather than implementation details of the registry. I.e. I'd expect something like IsAppVisibleToUser (or IsAppKnownToUser) (which I guess would both be the reverse of this) to be perhaps a better thing to filter by?
Yeah, naming this function was a pain point for me too, how does `IsAppSurfaceableToUser()` sound? It's closer to `Visible`, and it also feels like it includes the nuances of the registry in it. Wdyt?
Dibyajyoti Pal
This will be the "negation" of it, so `IsAppNotSurfaceableToUser()` is what I went with. PTAL!
Related details
Attention is currently required from:
- Marijn Kruisselbrink
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. |
Dibyajyoti Pal (Gerrit)
Jan 13, 2026, 1:50:53 PM (yesterday) Jan 13
to Marijn Kruisselbrink, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Simon Hangl, chromium-a...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watchin...@chromium.org, dmurph+wat...@chromium.org, dmurph+watc...@chromium.org, giovax...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mac-r...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, rmcelra...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
File chrome/browser/apps/app_shim/web_app_shim_manager_delegate_mac.cc
Line 180, Patchset 7 (Latest):
return provider->registrar_unsafe().IsInstallState(Dibyajyoti Pal . resolved
This is just a cleanup. Initially I thought the `profile` was a typo and we had to do `provider && ..` since we exit early if profile is nullptr, but we also have a `CHECK(provider)` before this line anyway which will crash, so checking for provider seemed redundant.
Related details
Attention set is empty
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. |
Marijn Kruisselbrink (Gerrit)
5:29 PM (6 hours ago) 5:29 PM
to Dibyajyoti Pal, Marijn Kruisselbrink, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Simon Hangl, chromium-a...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watchin...@chromium.org, dmurph+wat...@chromium.org, dmurph+watc...@chromium.org, giovax...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mac-r...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, rmcelra...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Attention needed from Dibyajyoti Pal
Marijn Kruisselbrink voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Dibyajyoti Pal
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. |
Dibyajyoti Pal (Gerrit)
5:39 PM (6 hours ago) 5:39 PM
to Marijn Kruisselbrink, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Simon Hangl, chromium-a...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watchin...@chromium.org, dmurph+wat...@chromium.org, dmurph+watc...@chromium.org, giovax...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mac-r...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, rmcelra...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Dibyajyoti Pal added 2 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 7 (Latest):
Dibyajyoti Pal . resolved
tluk@ PTAL at `c/b/ui/views/site_data`, thanks!
File chrome/browser/web_applications/web_app_filter.h
Line 74, Patchset 3:
static WebAppFilter IsAppDeemedToNotBeInRegistry();Marijn Kruisselbrink . resolved
Dibyajyoti PalI feel this naming goes against the general idea that these filters should be for specific capabilities rather than implementation details of the registry. I.e. I'd expect something like IsAppVisibleToUser (or IsAppKnownToUser) (which I guess would both be the reverse of this) to be perhaps a better thing to filter by?
Dibyajyoti PalYeah, naming this function was a pain point for me too, how does `IsAppSurfaceableToUser()` sound? It's closer to `Visible`, and it also feels like it includes the nuances of the registry in it. Wdyt?
This will be the "negation" of it, so `IsAppNotSurfaceableToUser()` is what I went with. PTAL!
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Lukaszewicz (Gerrit)
7:26 PM (4 hours ago) 7:26 PM
to Dibyajyoti Pal, Marijn Kruisselbrink, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Simon Hangl, chromium-a...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watchin...@chromium.org, dmurph+wat...@chromium.org, dmurph+watc...@chromium.org, giovax...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mac-r...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, rmcelra...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Attention needed from Dibyajyoti Pal
Tom Lukaszewicz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tom Lukaszewicz
| Code-Review | +1 |
1 comment
Patchset-level comments
Attention is currently required from:
- Dibyajyoti Pal
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
7:42 PM (4 hours ago) 7:42 PM
to Tom Lukaszewicz, Marijn Kruisselbrink, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Simon Hangl, chromium-a...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watchin...@chromium.org, dmurph+wat...@chromium.org, dmurph+watc...@chromium.org, giovax...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mac-r...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, rmcelra...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Dibyajyoti Pal voted Commit-Queue+2![]( "Open in Gerrit")
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)
8:32 PM (3 hours ago) 8:32 PM
to Dibyajyoti Pal, Tom Lukaszewicz, Marijn Kruisselbrink, chromium...@chromium.org, Andrew Rayskiy, Simon Hangl, chromium-a...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watchin...@chromium.org, dmurph+wat...@chromium.org, dmurph+watc...@chromium.org, giovax...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mac-r...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, rmcelra...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
[PWA/App Migration] Allow calls to IsInRegistrar to use separate filter
This CL updates the production callsites of IsInRegistrar to instead use
a separate WebAppFilter, whose sole purpose is to identify if an app is
considered "installed" for all purposes. This helps replace all
callsites of IsInRegistrar on production, which eventually makes it
easier to deprecate this call site in favor of GetInstallState(), or
WebAppFilters.
This change also allows all apps suggested from migration to be counted
under that same filter, which ensures that all callsites that are
checking whether this app is not in the registry are migrated to also
include suggested apps in a single swoop.
The only callsite that is not migrated is the WebAppIconManager, where
this is used to overwrite the icons in the app's directory with pending
ones. There is a chance for that use-case to be reused for migration.
Bug: 465762217
Include-Ci-Only-Tests: chromium.mac:mac15-x64-rel-tests|browser_tests
Change-Id: I239862532d408664e742517a0f8e740fc70804e2
Reviewed-by: Tom Lukaszewicz <tl...@chromium.org>
Reviewed-by: Marijn Kruisselbrink <m...@chromium.org>
Commit-Queue: Dibyajyoti Pal <diby...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1569460}
Files:
- M chrome/browser/apps/app_shim/web_app_shim_manager_delegate_mac.cc
- M chrome/browser/ui/views/site_data/related_app_row_view.cc
- M chrome/browser/ui/web_applications/sub_apps_service_impl.cc
- M chrome/browser/ui/web_applications/web_app_launch_process.cc
- M chrome/browser/web_applications/ash/migrations/adobe_express_oem_to_default_migration.cc
- M chrome/browser/web_applications/commands/apply_pending_manifest_update_command.cc
- M chrome/browser/web_applications/commands/compute_app_size_command.cc
- M chrome/browser/web_applications/commands/install_app_locally_command.cc
- M chrome/browser/web_applications/commands/launch_web_app_command.cc
- M chrome/browser/web_applications/commands/navigate_and_trigger_install_dialog_command.cc
- M chrome/browser/web_applications/commands/set_user_display_mode_command.cc
- M chrome/browser/web_applications/jobs/get_progressive_web_app_size_job.cc
- M chrome/browser/web_applications/web_app_filter.cc
- M chrome/browser/web_applications/web_app_filter.h
- M chrome/browser/web_applications/web_app_registrar.cc
- M chrome/browser/web_applications/web_app_sync_bridge.cc
Change size: M
Delta: 16 files changed, 52 insertions(+), 46 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Marijn Kruisselbrink, +1 by Tom Lukaszewicz
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages