[PermissionOptions] Move to using PermissionSetting in permission infra [chromium/src : main]
0 views
Skip to first unread message
Christian Dullweber (Gerrit)
Jul 1, 2025, 9:42:13 AM7/1/25
to Florian Jacky, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Rijubrata Bhaumik, Simon Hangl, cfredri...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, rmcelra...@chromium.org
Attention needed from Florian Jacky
Christian Dullweber voted and added 2 comments![]( "Open in Gerrit")
Votes added by Christian Dullweber
| Code-Review | +1 |
2 comments
Patchset-level comments
File components/permissions/permission_context_base.cc
Line 728, Patchset 3 (Latest):
if (content_settings::CanBeAutoRevoked(content_settings_type(), value,Christian Dullweber . unresolved
Should we change the CanBeAutoRevoked method that takes a ContentSetting to take a PermissionSetting instead?
The base::Value method seems to be for chooser permissions
Related details
Attention is currently required from:
- Florian Jacky
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Dullweber (Gerrit)
Jul 3, 2025, 6:31:42 AM7/3/25
to Florian Jacky, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Rijubrata Bhaumik, Simon Hangl, droger+w...@chromium.org, dullweb...@chromium.org, msrame...@chromium.org, cfredri...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, rmcelra...@chromium.org
Attention needed from Florian Jacky
Christian Dullweber added 2 comments![]( "Open in Gerrit")
File components/content_settings/core/browser/content_settings_utils.cc
Line 219, Patchset 10 (Latest):
blink::mojom::PermissionStatus basic_permission_status,Christian Dullweber . unresolved
we could move this method to the permissions code base. You can't include permissions code in content_settings
Line 240, Patchset 10 (Latest):
return IsChooserPermissionEligibleForAutoRevocation(type) && !value.is_none();Christian Dullweber . unresolved
This method now only works for chooser permissions. Could we CHECK that something is a chooser permission?
Related details
Attention is currently required from:
- Florian Jacky
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florian Jacky (Gerrit)
Jul 3, 2025, 7:45:45 AM7/3/25
to Christian Dullweber, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Rijubrata Bhaumik, Simon Hangl, droger+w...@chromium.org, dullweb...@chromium.org, msrame...@chromium.org, cfredri...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, rmcelra...@chromium.org
Attention needed from Christian Dullweber and Florian Jacky
Message from Florian Jacky![]( "Open in Gerrit")
Attention is currently required from:
- Christian Dullweber
- Florian Jacky
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florian Jacky (Gerrit)
Jul 3, 2025, 2:20:16 PM7/3/25
to Christian Dullweber, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Rijubrata Bhaumik, Simon Hangl, droger+w...@chromium.org, dullweb...@chromium.org, msrame...@chromium.org, cfredri...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, rmcelra...@chromium.org
Attention needed from Christian Dullweber
Florian Jacky added 3 comments![]( "Open in Gerrit")
File components/content_settings/core/browser/content_settings_utils.cc
Line 219, Patchset 10:
blink::mojom::PermissionStatus basic_permission_status,Christian Dullweber . resolved
we could move this method to the permissions code base. You can't include permissions code in content_settings
Florian Jacky
As mentioned, moving the method to permissions code is quite challenging as it requires moving quite a bit of safety hub related stuff there as we still can't depend on permissions code from content_settings. I figured out how to resolve the dependency issue in the current state in content settings, however I also figured out why the tests failed: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/safety_hub/revoked_permissions_service.cc;l=860;drc=17b4c42cfb3512836655464b0d41d319e819c0b6 --> the method using base::Value is not used for chooser based settings only. So the best solution is to add this to the delegate. I've special cased ContentSettings and added TODOs where the full content setting migration to PermissionSetting will lead to simplifications.
Line 240, Patchset 10:
return IsChooserPermissionEligibleForAutoRevocation(type) && !value.is_none();Christian Dullweber . resolved
This method now only works for chooser permissions. Could we CHECK that something is a chooser permission?
Florian Jacky
No longer fully relevant, but we don't have a utility method for this. However, I think it will be sufficient to check it's not a permission setting (once content settings have been migrated to it), since IsChooserPermissionEligibleForAutoRevocation should return false in that case anyway.
File components/permissions/permission_context_base.cc
Line 728, Patchset 3:
if (content_settings::CanBeAutoRevoked(content_settings_type(), value,Christian Dullweber . resolved
Should we change the CanBeAutoRevoked method that takes a ContentSetting to take a PermissionSetting instead?
The base::Value method seems to be for chooser permissions
Florian Jacky
No longer relevant.
Related details
Attention is currently required from:
- Christian Dullweber
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Dullweber (Gerrit)
Jul 4, 2025, 9:16:36 AM7/4/25
to Florian Jacky, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Rijubrata Bhaumik, Simon Hangl, droger+w...@chromium.org, dullweb...@chromium.org, msrame...@chromium.org, cfredri...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, rmcelra...@chromium.org
Attention needed from Florian Jacky
Christian Dullweber added 6 comments![]( "Open in Gerrit")
File chrome/browser/ui/safety_hub/revoked_permissions_service.cc
Line 863, Patchset 15:
if (auto* info =Christian Dullweber . unresolved
Could you add a TODO to remove the if/else when contentsettings are also registered as permission settings?
File components/content_settings/core/browser/content_settings_utils.cc
Line 240, Patchset 10:
return IsChooserPermissionEligibleForAutoRevocation(type) && !value.is_none();Christian Dullweber . resolved
Florian JackyThis method now only works for chooser permissions. Could we CHECK that something is a chooser permission?
No longer fully relevant, but we don't have a utility method for this. However, I think it will be sufficient to check it's not a permission setting (once content settings have been migrated to it), since IsChooserPermissionEligibleForAutoRevocation should return false in that case anyway.
Christian Dullweber
Ideally we have a method that handles all possible contentsettings. But we can follow up on this
File components/content_settings/core/browser/geolocation_setting_delegate.h
Line 19, Patchset 16 (Latest):
// Return whether the setting is valid.Christian Dullweber . unresolved
I removed these comments since this is the implementation of the delegate and the original definition has comments
File components/content_settings/core/browser/geolocation_setting_delegate.cc
Line 80, Patchset 15:
auto* geolocation_setting = std::get_if<GeolocationSetting>(&setting);Christian Dullweber . unresolved
I'd use std::get here. If this is called for the wrong type, then it is ok to crash
File components/content_settings/core/browser/host_content_settings_map.cc
Line 285, Patchset 16 (Latest):
void CheckPermissionTypeRegistration(ContentSettingsType content_type) {Christian Dullweber . unresolved
It looks like this was duplicated?
File components/content_settings/core/browser/permission_settings_info.h
Line 40, Patchset 16 (Latest):
// Returns whether the permission setting can be auto-revoked.Christian Dullweber . unresolved
maybe add "by safety hub"?
Related details
Attention is currently required from:
- Florian Jacky
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florian Jacky (Gerrit)
Jul 4, 2025, 9:31:09 AM7/4/25
to Christian Dullweber, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Rijubrata Bhaumik, Simon Hangl, droger+w...@chromium.org, dullweb...@chromium.org, msrame...@chromium.org, cfredri...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, rmcelra...@chromium.org
Attention needed from Christian Dullweber
Florian Jacky added 5 comments![]( "Open in Gerrit")
File chrome/browser/ui/safety_hub/revoked_permissions_service.cc
Line 863, Patchset 15:
if (auto* info =Christian Dullweber . resolved
Could you add a TODO to remove the if/else when contentsettings are also registered as permission settings?
Florian Jacky
Done, they are currently handled in the other block. Updated the comment and added an update todo.
File components/content_settings/core/browser/geolocation_setting_delegate.h
I removed these comments since this is the implementation of the delegate and the original definition has comments
Florian Jacky
Done
File components/content_settings/core/browser/geolocation_setting_delegate.cc
Line 80, Patchset 15:
auto* geolocation_setting = std::get_if<GeolocationSetting>(&setting);Christian Dullweber . resolved
I'd use std::get here. If this is called for the wrong type, then it is ok to crash
Florian Jacky
Done
File components/content_settings/core/browser/host_content_settings_map.cc
Line 285, Patchset 16:
void CheckPermissionTypeRegistration(ContentSettingsType content_type) {Christian Dullweber . resolved
It looks like this was duplicated?
Florian Jacky
Ah yes, that was a rebase error, fixed.
File components/content_settings/core/browser/permission_settings_info.h
Line 40, Patchset 16:
// Returns whether the permission setting can be auto-revoked.Christian Dullweber . resolved
maybe add "by safety hub"?
Attention is currently required from:
- Christian Dullweber
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Dullweber (Gerrit)
Jul 4, 2025, 9:56:23 AM7/4/25
to Florian Jacky, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Rijubrata Bhaumik, Simon Hangl, droger+w...@chromium.org, dullweb...@chromium.org, msrame...@chromium.org, cfredri...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, rmcelra...@chromium.org
Attention needed from Florian Jacky
Christian Dullweber added 1 comment![]( "Open in Gerrit")
File components/content_settings/core/browser/content_settings_utils.h
Line 87, Patchset 18 (Latest):
// Returns whether ContentSettingPermissionContextBase-based or
// ObjectPermissionContextBase-based permission's ContentSettingsType is an
// eligible permission for auto-revocation. Directly use theChristian Dullweber . unresolved
Actually, why don't we just move the code from revoked_permissions_service.cc here already? Then the method handles all types of settings and callers don't need to worry about the type of setting they are dealing with
Related details
Attention is currently required from:
- Florian Jacky
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florian Jacky (Gerrit)
Jul 7, 2025, 4:39:55 AM7/7/25
to Christian Dullweber, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Rijubrata Bhaumik, Simon Hangl, droger+w...@chromium.org, dullweb...@chromium.org, msrame...@chromium.org, cfredri...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, rmcelra...@chromium.org
Attention needed from Christian Dullweber
Florian Jacky added 1 comment![]( "Open in Gerrit")
File components/content_settings/core/browser/content_settings_utils.h
Line 87, Patchset 18:
// Returns whether ContentSettingPermissionContextBase-based or
// ObjectPermissionContextBase-based permission's ContentSettingsType is an
// eligible permission for auto-revocation. Directly use theChristian Dullweber . unresolved
Actually, why don't we just move the code from revoked_permissions_service.cc here already? Then the method handles all types of settings and callers don't need to worry about the type of setting they are dealing with
Florian Jacky
The service isn't always triggered from the PermissionContext. I think it makes sense to have its own service instead of a global method. This particular method is used to set a constraint in the context that will later be processed.
However, I agree that callers shouldn't need to worry abotu this. I've packed the logic for which type should be used (and calling the delegate for settings) into this method.
Related details
Attention is currently required from:
- Christian Dullweber
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Dullweber (Gerrit)
Jul 7, 2025, 7:26:29 AM7/7/25
to Florian Jacky, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Rijubrata Bhaumik, Simon Hangl, droger+w...@chromium.org, dullweb...@chromium.org, msrame...@chromium.org, cfredri...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, rmcelra...@chromium.org
Attention needed from Florian Jacky
Christian Dullweber voted and added 3 comments![]( "Open in Gerrit")
Votes added by Christian Dullweber
| Code-Review | +1 |
Christian Dullweber . resolved
lgtm
File chrome/browser/ui/safety_hub/revoked_permissions_service.cc
Line 863, Patchset 20 (Latest):
if (auto* info =
content_settings::PermissionSettingsRegistry::GetInstance()->Get(
entry.type)) {
can_be_autorevoked = info->delegate().CanBeAutoRevoked(
/*setting=*/info->delegate()
.FromValue(entry.source.setting_value)
.value_or(info->GetInitialDefaultSetting()),
/*is_one_time=*/false);
} else {Christian Dullweber . unresolved
this code can be removed now
File components/content_settings/core/browser/content_settings_utils.h
Line 87, Patchset 18:
// Returns whether ContentSettingPermissionContextBase-based or
// ObjectPermissionContextBase-based permission's ContentSettingsType is an
// eligible permission for auto-revocation. Directly use theChristian Dullweber . resolved
Florian JackyActually, why don't we just move the code from revoked_permissions_service.cc here already? Then the method handles all types of settings and callers don't need to worry about the type of setting they are dealing with
The service isn't always triggered from the PermissionContext. I think it makes sense to have its own service instead of a global method. This particular method is used to set a constraint in the context that will later be processed.
However, I agree that callers shouldn't need to worry abotu this. I've packed the logic for which type should be used (and calling the delegate for settings) into this method.
Attention is currently required from:
- Florian Jacky
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florian Jacky (Gerrit)
Jul 7, 2025, 8:22:20 AM7/7/25
to Side YILMAZ, Dave Tapuska, Christian Dullweber, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Rijubrata Bhaumik, Simon Hangl, droger+w...@chromium.org, dullweb...@chromium.org, msrame...@chromium.org, cfredri...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, rmcelra...@chromium.org
Attention needed from Dave Tapuska and Side YILMAZ
Florian Jacky voted and added 2 comments![]( "Open in Gerrit")
Votes added by Florian Jacky
| Auto-Submit | +1 |
2 comments
Patchset-level comments
File-level comment, Patchset 22 (Latest):
Florian Jacky . resolved
@dtap...@chromium.org could you PTAL at the minor change in content/public/browser/ permission_result.h ?
@sidey...@chromium.org could you PTAL at the minor change in chrome/browser/ui/safety_hub/revoked_permissions_service.cc ?
File chrome/browser/ui/safety_hub/revoked_permissions_service.cc
Line 863, Patchset 20:
if (auto* info =
content_settings::PermissionSettingsRegistry::GetInstance()->Get(
entry.type)) {
can_be_autorevoked = info->delegate().CanBeAutoRevoked(
/*setting=*/info->delegate()
.FromValue(entry.source.setting_value)
.value_or(info->GetInitialDefaultSetting()),
/*is_one_time=*/false);
} else {Christian Dullweber . resolved
this code can be removed now
Attention is currently required from:
- Dave Tapuska
- Side YILMAZ
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florian Jacky (Gerrit)
Jul 7, 2025, 8:46:10 AM7/7/25
to Side YILMAZ, Dave Tapuska, Christian Dullweber, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Rijubrata Bhaumik, Simon Hangl, droger+w...@chromium.org, dullweb...@chromium.org, msrame...@chromium.org, cfredri...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, rmcelra...@chromium.org
Attention needed from Dave Tapuska and Side YILMAZ
Florian Jacky removed a vote from this change![]( "Open in Gerrit")
Removed Auto-Submit+1 by Florian Jacky <fja...@chromium.org>
Related details
Attention is currently required from:
- Dave Tapuska
- Side YILMAZ
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Dullweber (Gerrit)
Jul 7, 2025, 10:57:49 AM7/7/25
to Florian Jacky, Side YILMAZ, Dave Tapuska, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Rijubrata Bhaumik, Simon Hangl, droger+w...@chromium.org, dullweb...@chromium.org, msrame...@chromium.org, cfredri...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, rmcelra...@chromium.org
Attention needed from Dave Tapuska, Florian Jacky and Side YILMAZ
Christian Dullweber voted and added 1 comment![]( "Open in Gerrit")
Votes added by Christian Dullweber
| Code-Review | +1 |
1 comment
File content/public/browser/permission_result.h
Line 8, Patchset 22 (Latest):
#include "base/values.h"Christian Dullweber . unresolved
can we remove this include?
Related details
Attention is currently required from:
- Dave Tapuska
- Florian Jacky
- Side YILMAZ
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Dullweber (Gerrit)
Jul 7, 2025, 10:58:29 AM7/7/25
to Florian Jacky, Side YILMAZ, Dave Tapuska, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Rijubrata Bhaumik, Simon Hangl, droger+w...@chromium.org, dullweb...@chromium.org, msrame...@chromium.org, cfredri...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, rmcelra...@chromium.org
Attention needed from Dave Tapuska, Florian Jacky and Side YILMAZ
Christian Dullweber added 1 comment![]( "Open in Gerrit")
File chrome/browser/ui/safety_hub/revoked_permissions_service.cc
Line 862, Patchset 22 (Latest):
// TODO(crbug.com/425642101): Update comment When ContentSettings have
// been migrated to PermissionSettings, they can also use
// PermissionSettingsInfo::Delegate.Christian Dullweber . unresolved
I think this TODO can be removed now?
Florian Jacky (Gerrit)
Jul 7, 2025, 11:12:38 AM7/7/25
to Side YILMAZ, Dave Tapuska, Christian Dullweber, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Rijubrata Bhaumik, Simon Hangl, droger+w...@chromium.org, dullweb...@chromium.org, msrame...@chromium.org, cfredri...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, rmcelra...@chromium.org
Attention needed from Christian Dullweber, Dave Tapuska and Side YILMAZ
Florian Jacky added 2 comments![]( "Open in Gerrit")
File chrome/browser/ui/safety_hub/revoked_permissions_service.cc
Line 862, Patchset 22:
// TODO(crbug.com/425642101): Update comment When ContentSettings have
// been migrated to PermissionSettings, they can also use
// PermissionSettingsInfo::Delegate.Christian Dullweber . resolved
I think this TODO can be removed now?
Florian Jacky
Done
File content/public/browser/permission_result.h
Line 8, Patchset 22:
#include "base/values.h"Christian Dullweber . unresolved
can we remove this include?
Florian Jacky
I don't think so, it would require declaring the variant again which requires a forward declaration of ContentSetting.
Related details
Attention is currently required from:
- Christian Dullweber
- Dave Tapuska
- Side YILMAZ
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Dullweber (Gerrit)
Jul 7, 2025, 11:16:01 AM7/7/25
to Florian Jacky, Side YILMAZ, Dave Tapuska, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Rijubrata Bhaumik, Simon Hangl, droger+w...@chromium.org, dullweb...@chromium.org, msrame...@chromium.org, cfredri...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, rmcelra...@chromium.org
Attention needed from Dave Tapuska, Florian Jacky and Side YILMAZ
Christian Dullweber voted and added 1 comment![]( "Open in Gerrit")
Votes added by Christian Dullweber
| Code-Review | +1 |
1 comment
File content/public/browser/permission_result.h
Line 8, Patchset 22:
#include "base/values.h"Christian Dullweber . unresolved
Florian Jackycan we remove this include?
I don't think so, it would require declaring the variant again which requires a forward declaration of ContentSetting.
Attention is currently required from:
- Dave Tapuska
- Florian Jacky
- Side YILMAZ
Dave Tapuska (Gerrit)
Jul 7, 2025, 11:19:15 AM7/7/25
to Florian Jacky, Side YILMAZ, Christian Dullweber, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Rijubrata Bhaumik, Simon Hangl, droger+w...@chromium.org, dullweb...@chromium.org, msrame...@chromium.org, cfredri...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, rmcelra...@chromium.org
Attention needed from Florian Jacky and Side YILMAZ
Dave Tapuska voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dave Tapuska
| Code-Review | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Florian Jacky
- Side YILMAZ
Florian Jacky (Gerrit)
Jul 8, 2025, 2:45:11 AM7/8/25
to Dave Tapuska, Side YILMAZ, Christian Dullweber, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Rijubrata Bhaumik, Simon Hangl, droger+w...@chromium.org, dullweb...@chromium.org, msrame...@chromium.org, cfredri...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, rmcelra...@chromium.org
Attention needed from Side YILMAZ
Florian Jacky added 1 comment![]( "Open in Gerrit")
File content/public/browser/permission_result.h
Line 8, Patchset 22:
#include "base/values.h"Christian Dullweber . resolved
Florian Jackycan we remove this include?
Christian DullweberI don't think so, it would require declaring the variant again which requires a forward declaration of ContentSetting.
I mean base/values.h
Attention is currently required from:
- Side YILMAZ
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Side YILMAZ (Gerrit)
Jul 8, 2025, 5:33:14 AM7/8/25
to Florian Jacky, Dave Tapuska, Christian Dullweber, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Rijubrata Bhaumik, Simon Hangl, droger+w...@chromium.org, dullweb...@chromium.org, msrame...@chromium.org, cfredri...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, rmcelra...@chromium.org
Attention needed from Florian Jacky
Side YILMAZ voted and added 1 comment![]( "Open in Gerrit")
Votes added by Side YILMAZ
| Code-Review | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Florian Jacky
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Jul 8, 2025, 5:37:28 AM7/8/25
to Florian Jacky, Side YILMAZ, Dave Tapuska, Christian Dullweber, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Rijubrata Bhaumik, Simon Hangl, droger+w...@chromium.org, dullweb...@chromium.org, msrame...@chromium.org, cfredri...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, rmcelra...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
[PermissionOptions] Move to using PermissionSetting in permission infra
Bug: 425642101
Change-Id: I65d1bf5aab8d76831f31f811af13ef47edec7254
Reviewed-by: Christian Dullweber <dull...@chromium.org>
Commit-Queue: Florian Jacky <fja...@chromium.org>
Reviewed-by: Dave Tapuska <dtap...@chromium.org>
Reviewed-by: Side YILMAZ <sidey...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1483639}
Files:
- M chrome/browser/media/webrtc/media_stream_device_permission_context.cc
- M chrome/browser/storage_access_api/storage_access_grant_permission_context.cc
- M chrome/browser/ui/safety_hub/revoked_permissions_service.cc
- M components/content_settings/core/browser/content_settings_utils.cc
- M components/content_settings/core/browser/content_settings_utils.h
- M components/content_settings/core/browser/content_settings_utils_unittest.cc
- M components/content_settings/core/browser/geolocation_delegate_unittest.cc
- M components/content_settings/core/browser/geolocation_setting_delegate.cc
- M components/content_settings/core/browser/geolocation_setting_delegate.h
- M components/content_settings/core/browser/host_content_settings_map.cc
- M components/content_settings/core/browser/host_content_settings_map.h
- M components/content_settings/core/browser/permission_settings_info.h
- M components/content_settings/core/browser/permission_settings_registry.cc
- M components/permissions/content_setting_permission_context_base.cc
- M components/permissions/content_setting_permission_context_base.h
- M components/permissions/contexts/webxr_permission_context.cc
- M components/permissions/permission_context_base.cc
- M components/permissions/permission_context_base.h
- M components/permissions/resolvers/content_setting_permission_resolver.cc
- M components/permissions/resolvers/content_setting_permission_resolver.h
- M components/permissions/resolvers/content_setting_permission_resolver_unittest.cc
- M components/permissions/resolvers/permission_resolver.h
- M content/public/browser/permission_result.cc
- M content/public/browser/permission_result.h
Change size: L
Delta: 24 files changed, 200 insertions(+), 171 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Christian Dullweber, +1 by Dave Tapuska, +1 by Side YILMAZ
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages