[permissions] Refactor PermissionRequestData [chromium/src : main]
0 views
Skip to first unread message
Antonio Sartori (Gerrit)
Apr 21, 2026, 11:26:39 AM (2 days ago) Apr 21
to Judith Hemp, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Andrew Rayskiy, Javier Fernandez, Permissions Reviews, Peter Beverloo, Rijubrata Bhaumik, Simon Hangl, cfredri...@chromium.org, chfreme...@chromium.org, dibyapal+wa...@chromium.org, droger+w...@chromium.org, dtraino...@chromium.org, dullweb...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, iclella...@chromium.org, mattreyno...@chromium.org, msrame...@chromium.org, nator...@chromium.org, npm+...@chromium.org, rayanka...@chromium.org, sloboda...@chromium.org, storage...@chromium.org, toyosh...@chromium.org, yigu+...@chromium.org
Attention needed from Judith Hemp
Antonio Sartori added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Judith Hemp
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Judith Hemp (Gerrit)
Apr 22, 2026, 7:37:16 AM (yesterday) Apr 22
to Antonio Sartori, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Andrew Rayskiy, Javier Fernandez, Permissions Reviews, Peter Beverloo, Rijubrata Bhaumik, Simon Hangl, cfredri...@chromium.org, chfreme...@chromium.org, dibyapal+wa...@chromium.org, droger+w...@chromium.org, dtraino...@chromium.org, dullweb...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, iclella...@chromium.org, mattreyno...@chromium.org, msrame...@chromium.org, nator...@chromium.org, npm+...@chromium.org, rayanka...@chromium.org, sloboda...@chromium.org, storage...@chromium.org, toyosh...@chromium.org, yigu+...@chromium.org
Attention needed from Antonio Sartori
Judith Hemp added 13 comments![]( "Open in Gerrit")
Patchset-level comments
Judith Hemp . resolved
This is a very nice change, thank you for doing this refactoring!
File chrome/browser/geolocation/geolocation_permission_context_delegate_android.cc
Line 67, Patchset 4 (Parent):
ContentSettingsType type = permissions::RequestTypeToContentSettingsType(
request_data.request_type.value())
.value();
CHECK(type == ContentSettingsType::GEOLOCATION ||
type == ContentSettingsType::GEOLOCATION_WITH_OPTIONS);Judith Hemp . unresolved
You completely removed that retrieving and checking step of the type from the request_data and directly used the geolocationcontentsettingstype. Is it safe to assume here? It probably is since its a geolocation delegate - just checking.
File chrome/browser/idle/idle_detection_permission_context_unittest.cc
Line 90, Patchset 4 (Latest):
blink::mojom::PermissionName::IDLE_DETECTION, nullptr),Judith Hemp . unresolved
[optional] nit: /*extension=*/ nullptr . same below
File chrome/browser/payments/payment_handler_permission_context_unittest.cc
Line 95, Patchset 4 (Latest):
nullptr, id, /*user_gesture=*/true, url),Judith Hemp . unresolved
OK, this one was confusing, I was thinking how can a nullptr bind to a reference, but it seems like there is an implicit constructor for nullptr inside this weird mojom structptr thing. And then it relies upon PaymentHandlerPermissionContext being a single purpose class, knowing how to deduce its own permission type. Whatever, works - I find it not easy to understand why, however.
nit: Use /*permission_descriptor=*/ here .
File chrome/browser/permissions/permission_context_base_permissions_policy_unittest.cc
Line 24, Patchset 4 (Latest):
#include "components/permissions/request_type.h"Judith Hemp . unresolved
is this addition needed?
File chrome/browser/storage/persistent_storage_permission_context.cc
Line 55, Patchset 4 (Latest):
CreatePermissionResolver(nullptr);Judith Hemp . unresolved
nit /*permission_descriptor=*/nullptr
just to be sure, that this will create and empty descriptor struct (because of it being a nullptr) is not an issue here?
File chrome/browser/storage/persistent_storage_permission_context_unittest.cc
Line 106, Patchset 4 (Latest):
nullptr, id, /*user_gesture=*/true, url, url),Judith Hemp . unresolved
[optional] nit: /*permission_descriptor=*/ here and below
File chrome/browser/top_level_storage_access_api/top_level_storage_access_permission_context_unittest.cc
File components/background_sync/background_sync_permission_context_unittest.cc
Line 53, Patchset 4 (Latest):
blink::mojom::PermissionName::BACKGROUND_SYNC, nullptr),Judith Hemp . unresolved
[optional] nit /*extension=*/
File components/permissions/content_setting_permission_context_base_unittest.cc
Line 379, Patchset 4 (Latest):
blink::mojom::PermissionDescriptor::New(permission_name, nullptr);Judith Hemp . unresolved
[optional] nit: /*extension=*/, here and below
Line 488, Patchset 4 (Latest):
url, blink::mojom::PermissionName::GEOLOCATION, 3);Judith Hemp . unresolved
[optional] nit: /*iterations=*/ (was there before but since you are on it)
File components/permissions/contexts/geolocation_permission_context_android.cc
Line 414, Patchset 4 (Latest):
*request_data, std::move(callback),
/*persist=*/result_decision == PermissionDecision::kAllow,Judith Hemp . unresolved
You change persist behavior here. It was always false, and now it depends on the `result_decision`. Is this intended?
File components/permissions/permission_context_base.cc
Line 284, Patchset 4 (Latest):
CHECK(requested_geolocation_accuracy.has_value());Judith Hemp . unresolved
This was a if before - I am sure you have your reasons why this will be set here always, just want to confirm it.
Related details
Attention is currently required from:
- Antonio Sartori
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. |
Antonio Sartori (Gerrit)
7:34 AM (8 hours ago) 7:34 AM
to Judith Hemp, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Andrew Rayskiy, Javier Fernandez, Permissions Reviews, Peter Beverloo, Rijubrata Bhaumik, Simon Hangl, cfredri...@chromium.org, chfreme...@chromium.org, dibyapal+wa...@chromium.org, droger+w...@chromium.org, dtraino...@chromium.org, dullweb...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, iclella...@chromium.org, mattreyno...@chromium.org, msrame...@chromium.org, nator...@chromium.org, npm+...@chromium.org, rayanka...@chromium.org, sloboda...@chromium.org, storage...@chromium.org, toyosh...@chromium.org, yigu+...@chromium.org
Attention needed from Judith Hemp
Antonio Sartori added 13 comments![]( "Open in Gerrit")
Patchset-level comments
Antonio Sartori . resolved
Thanks for the thorough review!
File chrome/browser/geolocation/geolocation_permission_context_delegate_android.cc
Line 67, Patchset 4 (Parent):
ContentSettingsType type = permissions::RequestTypeToContentSettingsType(
request_data.request_type.value())
.value();
CHECK(type == ContentSettingsType::GEOLOCATION ||
type == ContentSettingsType::GEOLOCATION_WITH_OPTIONS);Judith Hemp . resolved
You completely removed that retrieving and checking step of the type from the request_data and directly used the geolocationcontentsettingstype. Is it safe to assume here? It probably is since its a geolocation delegate - just checking.
Antonio Sartori
Yes. I also added a check to make sure of this.
File chrome/browser/idle/idle_detection_permission_context_unittest.cc
Line 90, Patchset 4 (Latest):
blink::mojom::PermissionName::IDLE_DETECTION, nullptr),Judith Hemp . resolved
[optional] nit: /*extension=*/ nullptr . same below
Antonio Sartori
Done
File chrome/browser/payments/payment_handler_permission_context_unittest.cc
Line 95, Patchset 4 (Latest):
nullptr, id, /*user_gesture=*/true, url),Judith Hemp . resolved
OK, this one was confusing, I was thinking how can a nullptr bind to a reference, but it seems like there is an implicit constructor for nullptr inside this weird mojom structptr thing. And then it relies upon PaymentHandlerPermissionContext being a single purpose class, knowing how to deduce its own permission type. Whatever, works - I find it not easy to understand why, however.
nit: Use /*permission_descriptor=*/ here .
Antonio Sartori
A mojo::StructPtr is basically a unique_ptr, so it should be relatively clear that we can pass a nullptr in, no?
For the rest, this thing is weird since it seems to reuse some of the permission logic even if it is not a permission (and there is no permission for payments, so..)
Anyway, added the comment :)
File chrome/browser/permissions/permission_context_base_permissions_policy_unittest.cc
Line 24, Patchset 4 (Latest):
#include "components/permissions/request_type.h"Judith Hemp . resolved
Antonio Sartoriis this addition needed?
Done
File chrome/browser/storage/persistent_storage_permission_context.cc
Line 55, Patchset 4 (Latest):
CreatePermissionResolver(nullptr);Judith Hemp . resolved
nit /*permission_descriptor=*/nullptr
just to be sure, that this will create and empty descriptor struct (because of it being a nullptr) is not an issue here?
Antonio Sartori
Added the comment.
Yes, I also don't particularly like it, but on the other hand it's used only in a dcheck, so...
File chrome/browser/storage/persistent_storage_permission_context_unittest.cc
Line 106, Patchset 4 (Latest):
nullptr, id, /*user_gesture=*/true, url, url),Judith Hemp . resolved
[optional] nit: /*permission_descriptor=*/ here and below
Antonio Sartori
Done
File chrome/browser/top_level_storage_access_api/top_level_storage_access_permission_context_unittest.cc
Line 100, Patchset 4 (Latest):
nullptr),Judith Hemp . resolved
Antonio Sartori[optional] nit: /*extension=*/
Done
File components/background_sync/background_sync_permission_context_unittest.cc
Line 53, Patchset 4 (Latest):
blink::mojom::PermissionName::BACKGROUND_SYNC, nullptr),Judith Hemp . resolved
Antonio Sartori[optional] nit /*extension=*/
Done
File components/permissions/content_setting_permission_context_base_unittest.cc
Line 379, Patchset 4 (Latest):
blink::mojom::PermissionDescriptor::New(permission_name, nullptr);Judith Hemp . resolved
[optional] nit: /*extension=*/, here and below
Antonio Sartori
Done
Line 488, Patchset 4 (Latest):
url, blink::mojom::PermissionName::GEOLOCATION, 3);Judith Hemp . resolved
[optional] nit: /*iterations=*/ (was there before but since you are on it)
Antonio Sartori
I would leave as is. I'm not sure it's helpful to add parameter names everywhere tbh.
File components/permissions/contexts/geolocation_permission_context_android.cc
Line 414, Patchset 4 (Latest):
*request_data, std::move(callback),
/*persist=*/result_decision == PermissionDecision::kAllow,Judith Hemp . resolved
You change persist behavior here. It was always false, and now it depends on the `result_decision`. Is this intended?
Antonio Sartori
Thanks for noticing this! Fixed.
File components/permissions/permission_context_base.cc
Line 284, Patchset 4 (Latest):
CHECK(requested_geolocation_accuracy.has_value());Judith Hemp . resolved
This was a if before - I am sure you have your reasons why this will be set here always, just want to confirm it.
Antonio Sartori
Yes. With the new logic, GetRequestedGeolocationAccuracy always returns a valid value for a GEOLOCATION_WITH_OPTIONS request.
Related details
Attention is currently required from:
- Judith Hemp
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Judith Hemp (Gerrit)
8:34 AM (7 hours ago) 8:34 AM
to Antonio Sartori, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Andrew Rayskiy, Javier Fernandez, Permissions Reviews, Peter Beverloo, Rijubrata Bhaumik, Simon Hangl, cfredri...@chromium.org, chfreme...@chromium.org, dibyapal+wa...@chromium.org, droger+w...@chromium.org, dtraino...@chromium.org, dullweb...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, iclella...@chromium.org, mattreyno...@chromium.org, msrame...@chromium.org, nator...@chromium.org, npm+...@chromium.org, rayanka...@chromium.org, sloboda...@chromium.org, storage...@chromium.org, toyosh...@chromium.org, yigu+...@chromium.org
Attention needed from Antonio Sartori
Judith Hemp voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Antonio Sartori
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Antonio Sartori (Gerrit)
8:44 AM (6 hours ago) 8:44 AM
to Javier Fernandez, Marc Treib, Judith Hemp, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Peter Beverloo, Rijubrata Bhaumik, Simon Hangl, cfredri...@chromium.org, chfreme...@chromium.org, dibyapal+wa...@chromium.org, droger+w...@chromium.org, dtraino...@chromium.org, dullweb...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, iclella...@chromium.org, mattreyno...@chromium.org, msrame...@chromium.org, nator...@chromium.org, npm+...@chromium.org, rayanka...@chromium.org, sloboda...@chromium.org, storage...@chromium.org, toyosh...@chromium.org, yigu+...@chromium.org
Attention needed from Javier Fernandez and Marc Treib
Antonio Sartori added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 5 (Latest):
Antonio Sartori . resolved
+jfernandez for register_protocol_handler_permission_request.cc
Marc could you stamp the remaining //chrome files?
Related details
Attention is currently required from:
- Javier Fernandez
- Marc Treib
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Fernandez (Gerrit)
9:26 AM (6 hours ago) 9:26 AM
to Antonio Sartori, Marc Treib, Judith Hemp, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Peter Beverloo, Rijubrata Bhaumik, Simon Hangl, cfredri...@chromium.org, chfreme...@chromium.org, dibyapal+wa...@chromium.org, droger+w...@chromium.org, dtraino...@chromium.org, dullweb...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, iclella...@chromium.org, mattreyno...@chromium.org, msrame...@chromium.org, nator...@chromium.org, npm+...@chromium.org, rayanka...@chromium.org, sloboda...@chromium.org, storage...@chromium.org, toyosh...@chromium.org, yigu+...@chromium.org
Attention needed from Antonio Sartori and Marc Treib
Javier Fernandez voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Antonio Sartori
- Marc Treib
Marc Treib (Gerrit)
10:27 AM (5 hours ago) 10:27 AM
to Antonio Sartori, Marc Treib, Javier Fernandez, Judith Hemp, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Peter Beverloo, Rijubrata Bhaumik, Simon Hangl, cfredri...@chromium.org, chfreme...@chromium.org, dibyapal+wa...@chromium.org, droger+w...@chromium.org, dtraino...@chromium.org, dullweb...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, iclella...@chromium.org, mattreyno...@chromium.org, msrame...@chromium.org, nator...@chromium.org, npm+...@chromium.org, rayanka...@chromium.org, sloboda...@chromium.org, storage...@chromium.org, toyosh...@chromium.org, yigu+...@chromium.org
Attention needed from Antonio Sartori
Marc Treib voted and added 2 comments![]( "Open in Gerrit")
Votes added by Marc Treib
| Code-Review | +1 |
2 comments
Patchset-level comments
Antonio Sartori . resolved
+jfernandez for register_protocol_handler_permission_request.cc
Marc could you stamp the remaining //chrome files?
Marc Treib
Marc could you stamp the remaining //chrome files?
rslgtm % one question
File chrome/browser/payments/payment_handler_permission_context_unittest.cc
Line 96, Patchset 5 (Parent):
ContentSettingsType::PAYMENT_HANDLER),Marc Treib . unresolved
Was this part not important/relevant?
Related details
Attention is currently required from:
- Antonio Sartori
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. |
Antonio Sartori (Gerrit)
11:23 AM (4 hours ago) 11:23 AM
to Marc Treib, Javier Fernandez, Judith Hemp, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Peter Beverloo, Rijubrata Bhaumik, Simon Hangl, cfredri...@chromium.org, chfreme...@chromium.org, dibyapal+wa...@chromium.org, droger+w...@chromium.org, dtraino...@chromium.org, dullweb...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, iclella...@chromium.org, mattreyno...@chromium.org, msrame...@chromium.org, nator...@chromium.org, npm+...@chromium.org, rayanka...@chromium.org, sloboda...@chromium.org, storage...@chromium.org, toyosh...@chromium.org, yigu+...@chromium.org
Antonio Sartori voted and added 2 comments![]( "Open in Gerrit")
Votes added by Antonio Sartori
| Commit-Queue | +2 |
2 comments
Patchset-level comments
Antonio Sartori . resolved
Thanks!
File chrome/browser/payments/payment_handler_permission_context_unittest.cc
Line 96, Patchset 5 (Parent):
ContentSettingsType::PAYMENT_HANDLER),Marc Treib . resolved
Was this part not important/relevant?
Antonio Sartori
There is no RequestType for PAYMENT_HANDLER (https://source.chromium.org/chromium/chromium/src/+/main:components/permissions/request_type.cc;l=212;drc=84fb307ed868ca8903dba11a75a1f0afc34bffb3). The correct PermissionResolver is anyway instantiated using the content_settings_type of the PermissionContext when needed (https://source.chromium.org/chromium/chromium/src/+/main:components/permissions/permission_context_base.cc;l=733;drc=01f3fea318f80b5541e1d5eb8dfeb174820926be)
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)
1:37 PM (1 hour ago) 1:37 PM
to Antonio Sartori, Marc Treib, Javier Fernandez, Judith Hemp, Christian Biesinger, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Peter Beverloo, Rijubrata Bhaumik, Simon Hangl, cfredri...@chromium.org, chfreme...@chromium.org, dibyapal+wa...@chromium.org, droger+w...@chromium.org, dtraino...@chromium.org, dullweb...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, iclella...@chromium.org, mattreyno...@chromium.org, msrame...@chromium.org, nator...@chromium.org, npm+...@chromium.org, rayanka...@chromium.org, sloboda...@chromium.org, storage...@chromium.org, toyosh...@chromium.org, yigu+...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
[permissions] Refactor PermissionRequestData
This CL refactors PermissionRequestData so that it becomes again a
data-only struct (which can be e.g. cloned) by removing the
PermissionResolver member. This allows simplifying
GeolocationPermissionContextAndroid, where we don't need to recreate
fake PermissionRequestData for the callbacks anymore.
The refactoring unfortunately implies adapting all callsites, which
however become more natural as they don't need to instantiate a
PermissionResolver anymore.
R=hempj...@google.com
Change-Id: Ib7883e9f7cac32ef4039ce3098275ad9018f47d3
Bug: 443898320
Commit-Queue: Antonio Sartori <antonio...@chromium.org>
Reviewed-by: Javier Fernandez <jfern...@igalia.com>
Reviewed-by: Judith Hemp <hempj...@google.com>
Reviewed-by: Marc Treib <tr...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1619624}
Files:
- M chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
- M chrome/browser/download/download_permission_request.cc
- M chrome/browser/geolocation/geolocation_permission_context_delegate_android.cc
- M chrome/browser/idle/idle_detection_permission_context_unittest.cc
- M chrome/browser/media/android/cdm/per_device_provisioning_permission.cc
- M chrome/browser/media/webrtc/media_stream_device_permission_context.cc
- M chrome/browser/notifications/notification_permission_context.cc
- M chrome/browser/notifications/notification_permission_context_unittest.cc
- M chrome/browser/payments/payment_handler_permission_context_unittest.cc
- M chrome/browser/permissions/permission_context_base_permissions_policy_unittest.cc
- M chrome/browser/smart_card/smart_card_permission_request.cc
- M chrome/browser/storage/persistent_storage_permission_context.cc
- M chrome/browser/storage/persistent_storage_permission_context_unittest.cc
- M chrome/browser/storage_access_api/storage_access_grant_permission_context.cc
- M chrome/browser/storage_access_api/storage_access_grant_permission_context_unittest.cc
- M chrome/browser/top_level_storage_access_api/top_level_storage_access_permission_context_unittest.cc
- M chrome/browser/ui/views/frame/top_controls_slide_controller_chromeos_browsertest.cc
- M chrome/browser/ui/views/permissions/exclusive_access_permission_prompt_browsertest.cc
- M chrome/browser/webid/identity_provider_permission_request.cc
- M chrome/test/permissions/permission_request_manager_test_api.cc
- M components/background_sync/background_sync_permission_context_unittest.cc
- M components/custom_handlers/register_protocol_handler_permission_request.cc
- M components/permissions/DEPS
- M components/permissions/content_setting_permission_context_base_unittest.cc
- M components/permissions/contexts/geolocation_permission_context_android.cc
- M components/permissions/contexts/geolocation_permission_context_android.h
- M components/permissions/contexts/geolocation_permission_context_unittest.cc
- M components/permissions/contexts/midi_sysex_permission_context_unittest.cc
- M components/permissions/contexts/nfc_permission_context_android.cc
- M components/permissions/contexts/nfc_permission_context_unittest.cc
- M components/permissions/contexts/webxr_permission_context.cc
- M components/permissions/permission_context_base.cc
- M components/permissions/permission_manager.cc
- M components/permissions/permission_request_data.cc
- M components/permissions/permission_request_data.h
- M components/permissions/permission_request_data_unittest.cc
- M components/permissions/permission_request_manager_unittest.cc
- M components/permissions/permission_uma_util_unittest.cc
- M components/permissions/test/mock_permission_request.cc
Change size: L
Delta: 39 files changed, 419 insertions(+), 343 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Marc Treib, +1 by Javier Fernandez, +1 by Judith Hemp
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages