[Extensions] Update signature of *ManifestData() [chromium/src : main]
Anton Bershanskyi (Gerrit)
Anton Bershanskyi added 2 comments![]( "Open in Gerrit")
Anton BershanskyiThanks, Anton! I like the idea of flattening this structure, but a couple comments:
1) We shouldn't store this in extension.h. If we keep this structure, we should definitely make this a new file. But, see below.
2) I'm not sold on having a centralized enum for this. We've been (slowly, gradually) pulling different APIs into their own targets that could be included / excluded from the build on a per-API basis. This has lots of practical applications, as different OSes may have different sets of APIs, or they may be otherwise controlled by gn build flags, etc. Being able to isolate all the logic for "API Foo" (including its manifest data) in a single leaf node without the rest of the system knowing about it makes that modularity better, and is also typically better architecture because it encourages scalable API development (the core system shouldn't need to know about each individual API). This seems like a bit of a step back from that, since it inherently moves towards a single, centralized location for all of these.As an alternative, there's significant prior art (predominantly with [base::SupportsUserData](https://source.chromium.org/chromium/chromium/src/+/main:base/supports_user_data.h;l=42;drc=a1a66ee6a75f6c98e925e72c58923e9e8cd5a9fe)) of using a void* as a key in the map. This is similarly flat to an enum, but wouldn't necessitate having a single centralized list of entries, and would also avoid the need for most of the code changes here. And then, as a future step, we could work towards pulling those keys into the ManifestHandlers themselves (again, similar to SupportsUserData::Data), which helps with our modularization efforts.
WDYT?
Thank you for the detailed review. I see the programming pattern used in `base::SupportsUserData` with `void*` and implemented it here.
On a personal level, I'm not sure if I really like `void*` being used for a substitute for `enum` values, but it definitely looks better than `std::string`. Matters of code taste aside, I updated the CL to move forward with the change.
---------
The rest of the comment is a bit off-topic.
This is similarly flat to an enum, but wouldn't necessitate having a single centralized list of entries,
This is a bit counter-intuitive to me. If the data is stored in a centralized place (private attribute `manifest_data_` of `extensions::Extension`), I would think that we would want to assure uniqueness of the map keys while reducing the key sizes as much as possible. A single centralized enum would seem like the ideal solution, perhaps even with macro-flags `#if BUILDFLAG(IS_PLATFORM)` ensuring flag values for other platforms are not compiled in. I understand that allocating a `void*` and setting it to its own address also ensures uniqueness, but it just seems like an extra variable allocation for generation of a unique value.
and would also avoid the need for most of the code changes here.
The original version of this CL just updates all the call sites of `GetExtensionData()` and `SetExtensionData()` which we will need to update anyway to create the auxiliary `void*` variables. We'll likely end up with a very similar-looking changeset, but may be just split across multiple CLs.
using ManifestDataMap = std::map<const void*, std::unique_ptr<ManifestData>>;Migrating from `std::map` to `absl::flat_hash_map` requires inclusion of `third_party/abseil-cpp/absl/container/flat_hash_map.h` which is a new import for many dependencies of `e/c/extension.h` and causes failure of `compile-size` test[1].
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 4 comments![]( "Open in Gerrit")
Anton BershanskyiThanks, Anton! I like the idea of flattening this structure, but a couple comments:
1) We shouldn't store this in extension.h. If we keep this structure, we should definitely make this a new file. But, see below.
2) I'm not sold on having a centralized enum for this. We've been (slowly, gradually) pulling different APIs into their own targets that could be included / excluded from the build on a per-API basis. This has lots of practical applications, as different OSes may have different sets of APIs, or they may be otherwise controlled by gn build flags, etc. Being able to isolate all the logic for "API Foo" (including its manifest data) in a single leaf node without the rest of the system knowing about it makes that modularity better, and is also typically better architecture because it encourages scalable API development (the core system shouldn't need to know about each individual API). This seems like a bit of a step back from that, since it inherently moves towards a single, centralized location for all of these.As an alternative, there's significant prior art (predominantly with [base::SupportsUserData](https://source.chromium.org/chromium/chromium/src/+/main:base/supports_user_data.h;l=42;drc=a1a66ee6a75f6c98e925e72c58923e9e8cd5a9fe)) of using a void* as a key in the map. This is similarly flat to an enum, but wouldn't necessitate having a single centralized list of entries, and would also avoid the need for most of the code changes here. And then, as a future step, we could work towards pulling those keys into the ManifestHandlers themselves (again, similar to SupportsUserData::Data), which helps with our modularization efforts.
WDYT?
Thank you for the detailed review. I see the programming pattern used in `base::SupportsUserData` with `void*` and implemented it here.
On a personal level, I'm not sure if I really like `void*` being used for a substitute for `enum` values, but it definitely looks better than `std::string`. Matters of code taste aside, I updated the CL to move forward with the change.
---------
The rest of the comment is a bit off-topic.
This is similarly flat to an enum, but wouldn't necessitate having a single centralized list of entries,
This is a bit counter-intuitive to me. If the data is stored in a centralized place (private attribute `manifest_data_` of `extensions::Extension`), I would think that we would want to assure uniqueness of the map keys while reducing the key sizes as much as possible. A single centralized enum would seem like the ideal solution, perhaps even with macro-flags `#if BUILDFLAG(IS_PLATFORM)` ensuring flag values for other platforms are not compiled in. I understand that allocating a `void*` and setting it to its own address also ensures uniqueness, but it just seems like an extra variable allocation for generation of a unique value.
and would also avoid the need for most of the code changes here.
The original version of this CL just updates all the call sites of `GetExtensionData()` and `SetExtensionData()` which we will need to update anyway to create the auxiliary `void*` variables. We'll likely end up with a very similar-looking changeset, but may be just split across multiple CLs.
which we will need to update anyway to create the auxiliary void* variables.
We shouldn't need to create auxiliary void* variables, though, right? We can just use the manifest constants?
const void* const kIconVariantsManifestDataKey = &kIconVariantsManifestDataKey;could we instead use manifest_keys::kIconVariants, similar to what the other handlers will do?
const void* const kFileHandlersManifestDataKey = &kFileHandlersManifestDataKey;ditto
Related details
- Anton Bershanskyi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anton Bershanskyi (Gerrit)
Anton Bershanskyi added 3 comments![]( "Open in Gerrit")
Anton BershanskyiThanks, Anton! I like the idea of flattening this structure, but a couple comments:
1) We shouldn't store this in extension.h. If we keep this structure, we should definitely make this a new file. But, see below.
2) I'm not sold on having a centralized enum for this. We've been (slowly, gradually) pulling different APIs into their own targets that could be included / excluded from the build on a per-API basis. This has lots of practical applications, as different OSes may have different sets of APIs, or they may be otherwise controlled by gn build flags, etc. Being able to isolate all the logic for "API Foo" (including its manifest data) in a single leaf node without the rest of the system knowing about it makes that modularity better, and is also typically better architecture because it encourages scalable API development (the core system shouldn't need to know about each individual API). This seems like a bit of a step back from that, since it inherently moves towards a single, centralized location for all of these.As an alternative, there's significant prior art (predominantly with [base::SupportsUserData](https://source.chromium.org/chromium/chromium/src/+/main:base/supports_user_data.h;l=42;drc=a1a66ee6a75f6c98e925e72c58923e9e8cd5a9fe)) of using a void* as a key in the map. This is similarly flat to an enum, but wouldn't necessitate having a single centralized list of entries, and would also avoid the need for most of the code changes here. And then, as a future step, we could work towards pulling those keys into the ManifestHandlers themselves (again, similar to SupportsUserData::Data), which helps with our modularization efforts.
WDYT?
Devlin CroninThank you for the detailed review. I see the programming pattern used in `base::SupportsUserData` with `void*` and implemented it here.
On a personal level, I'm not sure if I really like `void*` being used for a substitute for `enum` values, but it definitely looks better than `std::string`. Matters of code taste aside, I updated the CL to move forward with the change.
---------
The rest of the comment is a bit off-topic.
This is similarly flat to an enum, but wouldn't necessitate having a single centralized list of entries,
This is a bit counter-intuitive to me. If the data is stored in a centralized place (private attribute `manifest_data_` of `extensions::Extension`), I would think that we would want to assure uniqueness of the map keys while reducing the key sizes as much as possible. A single centralized enum would seem like the ideal solution, perhaps even with macro-flags `#if BUILDFLAG(IS_PLATFORM)` ensuring flag values for other platforms are not compiled in. I understand that allocating a `void*` and setting it to its own address also ensures uniqueness, but it just seems like an extra variable allocation for generation of a unique value.
and would also avoid the need for most of the code changes here.
The original version of this CL just updates all the call sites of `GetExtensionData()` and `SetExtensionData()` which we will need to update anyway to create the auxiliary `void*` variables. We'll likely end up with a very similar-looking changeset, but may be just split across multiple CLs.
which we will need to update anyway to create the auxiliary void* variables.
We shouldn't need to create auxiliary void* variables, though, right? We can just use the manifest constants?
OK, I updated CL to re-use `manifest_keys` constants. Another round of reviews?
const void* const kIconVariantsManifestDataKey = &kIconVariantsManifestDataKey;could we instead use manifest_keys::kIconVariants, similar to what the other handlers will do?
Done
const void* const kFileHandlersManifestDataKey = &kFileHandlersManifestDataKey;Anton Bershanskyiditto
Done
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |