[Extensions] Move chrome_settings_overrides handler into //extensions/ [chromium/src : main]
Anton Bershanskyi (Gerrit)
Anton Bershanskyi added 9 comments![]( "Open in Gerrit")
Hi Devlin,
this is a minor refactor with no functional changes, it just moves some extension manifest code from //chrome into //extensions. I left explanations in inline resolved comments explaining most changes.
Would you be interested in reviewing this change? I'll add the second reviewer after your review. Thanks!
},Moved verbatim to `extensions/common/api/_manifest_features.json`.
{Moved verbatim to `extensions/common/api/extensions_manifest_types.json`.
APIPermissionInfo::kFlagDoesNotRequireManagedSessionFullLoginWarning},Moved verbatim to `extensions/common/permissions/extensions_api_permissions.cc`.
}],Moved verbatim from `chrome/common/extensions/api/_manifest_features.json`.
{Moved verbatim from `chrome/common/extensions/api/manifest_types.json`.
~SettingsOverrides() override;I updated signature of `SettingsOverrides::Get()` to accept a const-reference `const&` instead of const-pointer `const*` because the code never calls it with null pointers and function assumes parameter to be non-null and dereferences it without checking.
// Copyright 2013 The Chromium AuthorsI just moved/renamed this file, formatted it, and updated `manifest_version` from `2` to `3`.
{APIPermissionID::kWmDesksPrivate, "wmDesksPrivate"},Moved verbatim from `chrome/common/extensions/permissions/chrome_api_permissions.cc`.
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anton Bershanskyi (Gerrit)
Anton Bershanskyi added 1 comment![]( "Open in Gerrit")
Hi Devlin,
this is a minor refactor with no functional changes, it just moves some extension manifest code from //chrome into //extensions. I left explanations in inline resolved comments explaining most changes.
Would you be interested in reviewing this change? I'll add the second reviewer after your review. Thanks!
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 1 comment![]( "Open in Gerrit")
Hi Devlin,
this is a minor refactor with no functional changes, it just moves some extension manifest code from //chrome into //extensions. I left explanations in inline resolved comments explaining most changes.
Would you be interested in reviewing this change? I'll add the second reviewer after your review. Thanks!
Thanks, Anton! But I think this one might be one that we keep at the chrome layer. Changing chrome settings is pretty linked to the chrome browser, IMO, and doesn't have much value for a generic extensions runtime. And, I don't think this is something that other components in //extensions are likely to rely on.
In general, it's okay to have APIs (/ manifest keys) at either //chrome or //extensions; we'll likely always have a split. We should be moving the ones to //extensions when they are more independent of the browser and browser concepts, and we should be consistent for individual APIs when possible (i.e., the manifest keys, API schemas, feature declarations, API implementations, etc, should all live at the same layer).
Is this needed for any particular work, or is this one that it'd make sense to leave at //chrome? I'm not dead-set on keeping it there if there's compelling reason to bring it up to //extensions, but for now, I think I'd lean towards it being in the //chrome layer.
(I'm also happy to run through future ones with you, if you want to discuss them before moving them around!)
Related details
- Anton Bershanskyi
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. |
Anton Bershanskyi (Gerrit)
Anton Bershanskyi added 1 comment![]( "Open in Gerrit")
Devlin CroninHi Devlin,
this is a minor refactor with no functional changes, it just moves some extension manifest code from //chrome into //extensions. I left explanations in inline resolved comments explaining most changes.
Would you be interested in reviewing this change? I'll add the second reviewer after your review. Thanks!
Thanks, Anton! But I think this one might be one that we keep at the chrome layer. Changing chrome settings is pretty linked to the chrome browser, IMO, and doesn't have much value for a generic extensions runtime. And, I don't think this is something that other components in //extensions are likely to rely on.
In general, it's okay to have APIs (/ manifest keys) at either //chrome or //extensions; we'll likely always have a split. We should be moving the ones to //extensions when they are more independent of the browser and browser concepts, and we should be consistent for individual APIs when possible (i.e., the manifest keys, API schemas, feature declarations, API implementations, etc, should all live at the same layer).
Is this needed for any particular work, or is this one that it'd make sense to leave at //chrome? I'm not dead-set on keeping it there if there's compelling reason to bring it up to //extensions, but for now, I think I'd lean towards it being in the //chrome layer.
(I'm also happy to run through future ones with you, if you want to discuss them before moving them around!)
I think this one might be one that we keep at the chrome layer.
OK, let's leave it in `//chrome`.
Is this needed for any particular work
No, I don't need to move this manifest key handler in particular. I just would like to move all manifest key handlers to their final place before some refactoring. In particular, I want to make `ManifestHandlerRegistry` more static (e.g., avoid handler priority sorting during execution, add more guarantees of handler for a particular key, etc.). For this, I would prefer to move handlers to their long-term locations in `//extensions` and `//chrome`.
I'm also happy to run through future ones with you, if you want to discuss them before moving them around!
- Devlin Cronin
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. |
Devlin Cronin (Gerrit)
Devlin Cronin added 1 comment![]( "Open in Gerrit")
Devlin CroninHi Devlin,
this is a minor refactor with no functional changes, it just moves some extension manifest code from //chrome into //extensions. I left explanations in inline resolved comments explaining most changes.
Would you be interested in reviewing this change? I'll add the second reviewer after your review. Thanks!
Anton BershanskyiThanks, Anton! But I think this one might be one that we keep at the chrome layer. Changing chrome settings is pretty linked to the chrome browser, IMO, and doesn't have much value for a generic extensions runtime. And, I don't think this is something that other components in //extensions are likely to rely on.
In general, it's okay to have APIs (/ manifest keys) at either //chrome or //extensions; we'll likely always have a split. We should be moving the ones to //extensions when they are more independent of the browser and browser concepts, and we should be consistent for individual APIs when possible (i.e., the manifest keys, API schemas, feature declarations, API implementations, etc, should all live at the same layer).
Is this needed for any particular work, or is this one that it'd make sense to leave at //chrome? I'm not dead-set on keeping it there if there's compelling reason to bring it up to //extensions, but for now, I think I'd lean towards it being in the //chrome layer.
(I'm also happy to run through future ones with you, if you want to discuss them before moving them around!)
I think this one might be one that we keep at the chrome layer.
OK, let's leave it in `//chrome`.
Is this needed for any particular work
No, I don't need to move this manifest key handler in particular. I just would like to move all manifest key handlers to their final place before some refactoring. In particular, I want to make `ManifestHandlerRegistry` more static (e.g., avoid handler priority sorting during execution, add more guarantees of handler for a particular key, etc.). For this, I would prefer to move handlers to their long-term locations in `//extensions` and `//chrome`.
I'm also happy to run through future ones with you, if you want to discuss them before moving them around!
That would be awesome, thanks!
Sounds good! I'm fully on board with this work, but we'll have some that remain in //chrome.
Do you want to drop the ones you're looking at or think should be moved into a shared doc, and I can comment there on any that seem like they belong more at the //chrome layer?
Related details
- Anton Bershanskyi
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. |
Anton Bershanskyi (Gerrit)
Anton Bershanskyi added 1 comment![]( "Open in Gerrit")
Devlin CroninHi Devlin,
this is a minor refactor with no functional changes, it just moves some extension manifest code from //chrome into //extensions. I left explanations in inline resolved comments explaining most changes.
Would you be interested in reviewing this change? I'll add the second reviewer after your review. Thanks!
Anton BershanskyiThanks, Anton! But I think this one might be one that we keep at the chrome layer. Changing chrome settings is pretty linked to the chrome browser, IMO, and doesn't have much value for a generic extensions runtime. And, I don't think this is something that other components in //extensions are likely to rely on.
In general, it's okay to have APIs (/ manifest keys) at either //chrome or //extensions; we'll likely always have a split. We should be moving the ones to //extensions when they are more independent of the browser and browser concepts, and we should be consistent for individual APIs when possible (i.e., the manifest keys, API schemas, feature declarations, API implementations, etc, should all live at the same layer).
Is this needed for any particular work, or is this one that it'd make sense to leave at //chrome? I'm not dead-set on keeping it there if there's compelling reason to bring it up to //extensions, but for now, I think I'd lean towards it being in the //chrome layer.
(I'm also happy to run through future ones with you, if you want to discuss them before moving them around!)
Devlin CroninI think this one might be one that we keep at the chrome layer.
OK, let's leave it in `//chrome`.
Is this needed for any particular work
No, I don't need to move this manifest key handler in particular. I just would like to move all manifest key handlers to their final place before some refactoring. In particular, I want to make `ManifestHandlerRegistry` more static (e.g., avoid handler priority sorting during execution, add more guarantees of handler for a particular key, etc.). For this, I would prefer to move handlers to their long-term locations in `//extensions` and `//chrome`.
I'm also happy to run through future ones with you, if you want to discuss them before moving them around!
That would be awesome, thanks!
Sounds good! I'm fully on board with this work, but we'll have some that remain in //chrome.
Do you want to drop the ones you're looking at or think should be moved into a shared doc, and I can comment there on any that seem like they belong more at the //chrome layer?
I already moved all manifest handlers which I really wanted to move, I believe that the rest should stay in `//chrome/` as they are. The only two handlers I'm ambivalent about are:
- `StorageSchemaManifestHandler` - this could benefit from being agnostic from browser UI ("the browser chrome"). On the other hand, the schema distribution mechanism is very Enterprise-y and not Web-y at all.
- `UrlHandlersParser` - since `"url_handlers"` is literally just a URL pattern matching registry for launching an app, it's pretty distant from "the browser chrome" too. But this handler is a part of deprecated platform apps feature and I'm not sure if it's worth touching. It'll probably go away on its own after October 2028, if the current timeline is maintained.
Related details
- Devlin Cronin
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. |
Devlin Cronin (Gerrit)
Devlin Cronin added 1 comment![]( "Open in Gerrit")
I'm okay with both of those staying put in //chrome (along with all the others that are there, too). I could see an argument to pull StorageSchemaManifestHandler up, especially since most of the storage API is in //extensions, but the _managed_ aspect of the storage API is actually still in //chrome -- so I think it makes sense to keep those together.
So, I think at this point, we can call it good : )
Thank you for all your work here! : )
Related details
- Anton Bershanskyi
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. |