[<install> element] Increase maximum install PEPCs allowed per page [chromium/src : main]
Lu Huang (Gerrit)
Lu Huang added 5 comments![]( "Open in Gerrit")
// The request is from a <install> element.Nit: an
const int max_elements =
GetMaxElementsPerPageForSource(disconnected_client->source());Nit: declare/define this before the for loop where it's being used.
bool is_geolocation_source = false,
bool is_install_source = false) {Should this be generalized, with an enum maybe? Multiple default parameters is an anti-pattern.
DecoupleInstallFromOtherSources) {Nit: suggest wording this as `InstallIndependentOfOtherSources`
// source. These should also be registered, as the sources are decoupled.Ditto.
Related details
- Lia Hiscock
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. |
Lia Hiscock (Gerrit)
Lia Hiscock added 5 comments![]( "Open in Gerrit")
// The request is from a <install> element.Lia HiscockNit: an
Acknowledged
const int max_elements =
GetMaxElementsPerPageForSource(disconnected_client->source());Nit: declare/define this before the for loop where it's being used.
This is intentionally at the beginning, as the first for loop erases the `disconnected_client` from the map so we need to cache the source first :/ I can add a comment explaining, but also open to other suggestions!
bool is_geolocation_source = false,
bool is_install_source = false) {Should this be generalized, with an enum maybe? Multiple default parameters is an anti-pattern.
Yeah...probably :) I debated since it's just a test but I'll update it
DecoupleInstallFromOtherSources) {Nit: suggest wording this as `InstallIndependentOfOtherSources`
I used `Decouple` for consistency with the existing `DecouplePermissionSources` test in this file. LMK if you still think I should switch though!
// source. These should also be registered, as the sources are decoupled.Lia HiscockDitto.
Acknowledged
Related details
- Lu Huang
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. |
Lu Huang (Gerrit)
Lu Huang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Lu Huang
| Code-Review | +1 |
2 comments
const int max_elements =
GetMaxElementsPerPageForSource(disconnected_client->source());Lia HiscockNit: declare/define this before the for loop where it's being used.
This is intentionally at the beginning, as the first for loop erases the `disconnected_client` from the map so we need to cache the source first :/ I can add a comment explaining, but also open to other suggestions!
Ack. Current comment ("...before erasing the client.") seems sufficient.
DecoupleInstallFromOtherSources) {Lia HiscockNit: suggest wording this as `InstallIndependentOfOtherSources`
I used `Decouple` for consistency with the existing `DecouplePermissionSources` test in this file. LMK if you still think I should switch though!
- Lia Hiscock
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lia Hiscock (Gerrit)
Lia Hiscock added 1 comment![]( "Open in Gerrit")
Hey Mike! This is ready for review when you're back :)
Related details
- Mike West
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lia Hiscock (Gerrit)
Lia Hiscock added 1 comment![]( "Open in Gerrit")
Hi Andy! I noticed you're working on PEPC things, and I'm looking for a review on the content/browser files here. This is part of the <install> element effort we've been working on with mkwst. For context, during our monthly sync he verbally LGTM'd us increasing the element limit, but he hasn't seen any of this CL yet. LMK if this approach seems right, or if I can provide any more context while Mike is OOO. Thanks!
Related details
- Andy Paicu
- Mike West
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West added 2 comments![]( "Open in Gerrit")
I think it's reasonable to allow more `<install>` elements per page than we'd be comfortable with for `<geolocation>` and other more monolithic capabilities. I do wonder whether we should limit the number of `<install>` elements that all point to the same application, as that seems like a more direct analog to the abuse scenario that the limit is meant to deal with for geolocation, cam/mic, etc? That might be worth considering as a followup.
I'll defer to Andy on the implementation, but the broad strokes look reasonable to me, modulo some thoughts about how we communicate the type of request from Blink to //content.
bool install;The pattern this sets up would add a new attribute for each potential type we wanted to make visible to the //content layer. I wonder if it would be more reasonable to either:
- Hold a `source` attribute that represented the permission the element deals with (or the element name or just an enum, no strong opinion). That could simplify the //content side of the implementation as well.
- Hold a `detail` attribute that's a union of `GeolocationEmbeddedPermissionRequestDescriptor`, `InstallEmbeddedPermissionRequestDescriptor`, etc. That's probably a little more verbose, but means we're only looking at one field, rather than a type field and then the relevant detail field.
Either way would make it more clear what exactly this request is communicating.
Related details
- Andy Paicu
- Lia Hiscock
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. |
Andy Paicu (Gerrit)
Andy Paicu added 1 comment![]( "Open in Gerrit")
Hi Thomas, I will defer to you on stuff related to the PEPC control checker.
Related details
- Lia Hiscock
- Thomas Nguyen
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. |
Lia Hiscock (Gerrit)
Lia Hiscock added 2 comments![]( "Open in Gerrit")
I think it's reasonable to allow more `<install>` elements per page than we'd be comfortable with for `<geolocation>` and other more monolithic capabilities. I do wonder whether we should limit the number of `<install>` elements that all point to the same application, as that seems like a more direct analog to the abuse scenario that the limit is meant to deal with for geolocation, cam/mic, etc? That might be worth considering as a followup.
I'll defer to Andy on the implementation, but the broad strokes look reasonable to me, modulo some thoughts about how we communicate the type of request from Blink to //content.
That sounds like a good idea! Opened this issue to track
bool install;The pattern this sets up would add a new attribute for each potential type we wanted to make visible to the //content layer. I wonder if it would be more reasonable to either:
- Hold a `source` attribute that represented the permission the element deals with (or the element name or just an enum, no strong opinion). That could simplify the //content side of the implementation as well.
- Hold a `detail` attribute that's a union of `GeolocationEmbeddedPermissionRequestDescriptor`, `InstallEmbeddedPermissionRequestDescriptor`, etc. That's probably a little more verbose, but means we're only looking at one field, rather than a type field and then the relevant detail field.
Either way would make it more clear what exactly this request is communicating.
Makes sense! I went with the union approach, LMK what you think.
Related details
- Lu Huang
- Mike West
- Thomas Nguyen
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. |