[DOM Storage] Replace string source with StorageAreaSource struct [chromium/src : main]
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 1 comment![]( "Open in Gerrit")
Hi folks, please take a look when you have time. Thanks in advance for reviewing!
Related details
- Chromium IPC Reviews
- Evan Stade
- Steve Becker
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: toyo...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): toyo...@chromium.org
Reviewer source(s):
toyo...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Evan Stade
- Steve Becker
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evan Stade (Gerrit)
Evan Stade added 2 comments![]( "Open in Gerrit")
StorageAreaSource source)probably ideal for all of these to be optional, i.e. `StorageAreaSource?`, since there are cases where we're passing an empty value.
String id = String::Number(base::RandUint64());so while we're at it can we make this a `base::Token`? (In some cases one might want to use an `UnguessableToken`, but here we explicitly do not need use this for security, instead relying on the mojo pipes between renderer and browser to correctly direct information, so it's better not to imply the unguessable nature of the token is a security measure.)
Related details
- Steve Becker
- Takashi Toyoshima
- Xiaohan Zhao
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. |
Evan Stade (Gerrit)
Evan Stade added 1 comment![]( "Open in Gerrit")
and thanks for doing this!
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 2 comments![]( "Open in Gerrit")
probably ideal for all of these to be optional, i.e. `StorageAreaSource?`, since there are cases where we're passing an empty value.
Done
so while we're at it can we make this a `base::Token`? (In some cases one might want to use an `UnguessableToken`, but here we explicitly do not need use this for security, instead relying on the mojo pipes between renderer and browser to correctly direct information, so it's better not to imply the unguessable nature of the token is a security measure.)
- Evan Stade
- Steve Becker
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evan Stade (Gerrit)
Evan Stade added 1 comment![]( "Open in Gerrit")
String id = String::Number(base::RandUint64());Xiaohan Zhaoso while we're at it can we make this a `base::Token`? (In some cases one might want to use an `UnguessableToken`, but here we explicitly do not need use this for security, instead relying on the mojo pipes between renderer and browser to correctly direct information, so it's better not to imply the unguessable nature of the token is a security measure.)
Done
Sorry, I meant to use `Token` throughout code, not just to generate a string. `Token` has native mojom support, `mojo_base.mojom.Token`.
Related details
- Steve Becker
- Takashi Toyoshima
- Xiaohan Zhao
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 3 comments![]( "Open in Gerrit")
// null, the key was newly added and had no previous value stored. `source`
// is null when the mutation was not initiated by a specific page (e.g.
// browser-initiated or internal reconciliation).As this seems relevant to other methods below, we may have this part in the interface comment?
// Identifies the source of a storage mutation for observer notifications.This can compile, but it may be nicer to declare before the first use above.
// store.Let's add an explanation on the nullability of the `source` parameter.
Related details
- Steve Becker
- Xiaohan Zhao
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
String id = String(base::Token::CreateRandom().ToString());We may clarify this change in the CL description?
Another question is shall we use base::UnguessableToken that can be mapped to mojom_base.UnguessableToken directly rather than using string in Mojo.
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 6 comments![]( "Open in Gerrit")
// null, the key was newly added and had no previous value stored. `source`
// is null when the mutation was not initiated by a specific page (e.g.
// browser-initiated or internal reconciliation).As this seems relevant to other methods below, we may have this part in the interface comment?
Done
// Identifies the source of a storage mutation for observer notifications.This can compile, but it may be nicer to declare before the first use above.
Done
Let's add an explanation on the nullability of the `source` parameter.
Done
String id = String::Number(base::RandUint64());Xiaohan Zhaoso while we're at it can we make this a `base::Token`? (In some cases one might want to use an `UnguessableToken`, but here we explicitly do not need use this for security, instead relying on the mojo pipes between renderer and browser to correctly direct information, so it's better not to imply the unguessable nature of the token is a security measure.)
Evan StadeDone
Sorry, I meant to use `Token` throughout code, not just to generate a string. `Token` has native mojom support, `mojo_base.mojom.Token`.
Sorry I misunderstood it. Updated. Thank you!
String id = String(base::Token::CreateRandom().ToString());We may clarify this change in the CL description?
Another question is shall we use base::UnguessableToken that can be mapped to mojom_base.UnguessableToken directly rather than using string in Mojo.
I've updated String with `base::Token` since this ID is not a security token and the mojo pipe already provides the security boundary, so the token doesn't need to be unguessable. But thanks for the suggestion!
Related details
- Evan Stade
- Steve Becker
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steve Becker (Gerrit)
Steve Becker added 9 comments![]( "Open in Gerrit")
Thanks for this cleanup. I'm still reviewing the tests, but here are some initial comments.
// StorageAreaObserver events. This map is keyed by storage_area_id (a uniquenit: this does not map to any variable or code in this file.
const base::Token& storage_area_id);nit: I think the new name loses some meaning without source. Including source in the name makes it very clear that it is the source of the mutation.
Same applies to `pending_mutations_by_source_` member.
// An in-process implementation of LocalStorage using a LevelDB Mojo service.Thanks for cleaning up this comment. Can we also remove this reference to LevelDB?
// base::Token() is all-zero, matching the zero-initialized empty slot.
static constexpr bool kEmptyValueIsZero = true;
// Use the all-ones token as the deleted-value sentinel.
static void ConstructDeletedValue(base::Token& slot) {
slot = base::Token(std::numeric_limits<uint64_t>::max(),
std::numeric_limits<uint64_t>::max());
}
static bool IsDeletedValue(const base::Token& token) {
return token.high() == std::numeric_limits<uint64_t>::max() &&
token.low() == std::numeric_limits<uint64_t>::max();
}nit: Can we implement DeletedValue() instead of ConstructDeletedValue() and IsDeletedValue()?
See the comment above DeletedValue() here:
return HashInts(static_cast<unsigned>(token.high()),This cast will truncate the 64-bit token.high(), reducing it to 32-bit. To hash the entire token, we need to call HashInt() on the first 32-bits of the token. Afterwards, we need to call AddIntToHash() on the remaining three 32-bit parts of the token.
See:
[1] HashInt()
[2] AddIntToHash()
auto mojo_source =nit: mojo does not describe this variable since mojo is used everywhere to communicate between processes and threads. Maybe we should use something like `storage_area_source` or `source_url_and_id` instead?
Same applies below.
const base::Token& storage_area_id) {Can/should we check assert that storage_area_id is not 0?
// A zero token means no source (browser-initiated mutation), so there can't
// be a matching pending local mutation. Also, base::Token() is the
// empty-bucket sentinel in the HashMap, so we must not look it up.
if (storage_area_id.is_zero()) {
return nullptr;
}Are these lines necessary? Should searching for a zero token return the end iterator below instead?
Related details
- Evan Stade
- Takashi Toyoshima
- Xiaohan Zhao
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. |
Evan Stade (Gerrit)
Evan Stade added 1 comment![]( "Open in Gerrit")
return HashInts(static_cast<unsigned>(token.high()),This cast will truncate the 64-bit token.high(), reducing it to 32-bit. To hash the entire token, we need to call HashInt() on the first 32-bits of the token. Afterwards, we need to call AddIntToHash() on the remaining three 32-bit parts of the token.
See:
[1] HashInt()
[2] AddIntToHash()
Bit unfortunate we are not allowed to just use absl::flat_hash_map (for reasons that aren't that well explained in the documentation). I see a good deal of `base::flat_map` and `std::map` in Blink, even though they are supposed discouraged? I think it's worth seeking an exception here to avoid this complexity. Would also be good to understand what the tradeoffs actually are.
Failing that, I would still bail on this added complexity and use the stringified token for the key.
Related details
- Takashi Toyoshima
- Xiaohan Zhao
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted Code-Review+1![]( "Open in Gerrit")
- Xiaohan Zhao
Evan Stade (Gerrit)
Evan Stade added 1 comment![]( "Open in Gerrit")
return HashInts(static_cast<unsigned>(token.high()),Evan StadeThis cast will truncate the 64-bit token.high(), reducing it to 32-bit. To hash the entire token, we need to call HashInt() on the first 32-bits of the token. Afterwards, we need to call AddIntToHash() on the remaining three 32-bit parts of the token.
See:
[1] HashInt()
[2] AddIntToHash()
Bit unfortunate we are not allowed to just use absl::flat_hash_map (for reasons that aren't that well explained in the documentation). I see a good deal of `base::flat_map` and `std::map` in Blink, even though they are supposed discouraged? I think it's worth seeking an exception here to avoid this complexity. Would also be good to understand what the tradeoffs actually are.
Failing that, I would still bail on this added complexity and use the stringified token for the key.
I asked this question here: https://chromium.slack.com/archives/CGJNSDHA7/p1772724774596959
Evan Stade (Gerrit)
Evan Stade added 1 comment![]( "Open in Gerrit")
return HashInts(static_cast<unsigned>(token.high()),Evan StadeThis cast will truncate the 64-bit token.high(), reducing it to 32-bit. To hash the entire token, we need to call HashInt() on the first 32-bits of the token. Afterwards, we need to call AddIntToHash() on the remaining three 32-bit parts of the token.
See:
[1] HashInt()
[2] AddIntToHash()
Evan StadeBit unfortunate we are not allowed to just use absl::flat_hash_map (for reasons that aren't that well explained in the documentation). I see a good deal of `base::flat_map` and `std::map` in Blink, even though they are supposed discouraged? I think it's worth seeking an exception here to avoid this complexity. Would also be good to understand what the tradeoffs actually are.
Failing that, I would still bail on this added complexity and use the stringified token for the key.
I asked this question here: https://chromium.slack.com/archives/CGJNSDHA7/p1772724774596959
Seems like folks thinks it's ok to use some non-Blink container such as `absl::flat_hash_map` since the map's value is not a GCed type, although we'll still need to update `audit_non_blink_usage.py` (and also use `ALLOW_DISCOURAGED_TYPE`?) which triggers a final approval.
Steve Becker (Gerrit)
Steve Becker added 1 comment![]( "Open in Gerrit")
return HashInts(static_cast<unsigned>(token.high()),Evan StadeThis cast will truncate the 64-bit token.high(), reducing it to 32-bit. To hash the entire token, we need to call HashInt() on the first 32-bits of the token. Afterwards, we need to call AddIntToHash() on the remaining three 32-bit parts of the token.
See:
[1] HashInt()
[2] AddIntToHash()
Evan StadeBit unfortunate we are not allowed to just use absl::flat_hash_map (for reasons that aren't that well explained in the documentation). I see a good deal of `base::flat_map` and `std::map` in Blink, even though they are supposed discouraged? I think it's worth seeking an exception here to avoid this complexity. Would also be good to understand what the tradeoffs actually are.
Failing that, I would still bail on this added complexity and use the stringified token for the key.
Evan StadeI asked this question here: https://chromium.slack.com/archives/CGJNSDHA7/p1772724774596959
Seems like folks thinks it's ok to use some non-Blink container such as `absl::flat_hash_map` since the map's value is not a GCed type, although we'll still need to update `audit_non_blink_usage.py` (and also use `ALLOW_DISCOURAGED_TYPE`?) which triggers a final approval.
Thanks Evan!
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 8 comments![]( "Open in Gerrit")
// StorageAreaObserver events. This map is keyed by storage_area_id (a uniquenit: this does not map to any variable or code in this file.
Updated the comment.
nit: I think the new name loses some meaning without source. Including source in the name makes it very clear that it is the source of the mutation.
Same applies to `pending_mutations_by_source_` member.
Done
// An in-process implementation of LocalStorage using a LevelDB Mojo service.Thanks for cleaning up this comment. Can we also remove this reference to LevelDB?
Done
// base::Token() is all-zero, matching the zero-initialized empty slot.
static constexpr bool kEmptyValueIsZero = true;
// Use the all-ones token as the deleted-value sentinel.
static void ConstructDeletedValue(base::Token& slot) {
slot = base::Token(std::numeric_limits<uint64_t>::max(),
std::numeric_limits<uint64_t>::max());
}
static bool IsDeletedValue(const base::Token& token) {
return token.high() == std::numeric_limits<uint64_t>::max() &&
token.low() == std::numeric_limits<uint64_t>::max();
}nit: Can we implement DeletedValue() instead of ConstructDeletedValue() and IsDeletedValue()?
See the comment above DeletedValue() here:
Updated the HashMap with absl::flat_hash_map.
Evan StadeThis cast will truncate the 64-bit token.high(), reducing it to 32-bit. To hash the entire token, we need to call HashInt() on the first 32-bits of the token. Afterwards, we need to call AddIntToHash() on the remaining three 32-bit parts of the token.
See:
[1] HashInt()
[2] AddIntToHash()
Evan StadeBit unfortunate we are not allowed to just use absl::flat_hash_map (for reasons that aren't that well explained in the documentation). I see a good deal of `base::flat_map` and `std::map` in Blink, even though they are supposed discouraged? I think it's worth seeking an exception here to avoid this complexity. Would also be good to understand what the tradeoffs actually are.
Failing that, I would still bail on this added complexity and use the stringified token for the key.
Evan StadeI asked this question here: https://chromium.slack.com/archives/CGJNSDHA7/p1772724774596959
Seems like folks thinks it's ok to use some non-Blink container such as `absl::flat_hash_map` since the map's value is not a GCed type, although we'll still need to update `audit_non_blink_usage.py` (and also use `ALLOW_DISCOURAGED_TYPE`?) which triggers a final approval.
Done. Thanks for checking on this!
nit: mojo does not describe this variable since mojo is used everywhere to communicate between processes and threads. Maybe we should use something like `storage_area_source` or `source_url_and_id` instead?
Same applies below.
Done
Can/should we check assert that storage_area_id is not 0?
Done
// A zero token means no source (browser-initiated mutation), so there can't
// be a matching pending local mutation. Also, base::Token() is the
// empty-bucket sentinel in the HashMap, so we must not look it up.
if (storage_area_id.is_zero()) {
return nullptr;
}Are these lines necessary? Should searching for a zero token return the end iterator below instead?
- Evan Stade
- Steve Becker
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted and added 1 comment![]( "Open in Gerrit")
Votes added by Takashi Toyoshima
| Code-Review | +1 |
1 comment
return HashInts(static_cast<unsigned>(token.high()),Evan StadeThis cast will truncate the 64-bit token.high(), reducing it to 32-bit. To hash the entire token, we need to call HashInt() on the first 32-bits of the token. Afterwards, we need to call AddIntToHash() on the remaining three 32-bit parts of the token.
See:
[1] HashInt()
[2] AddIntToHash()
Evan StadeBit unfortunate we are not allowed to just use absl::flat_hash_map (for reasons that aren't that well explained in the documentation). I see a good deal of `base::flat_map` and `std::map` in Blink, even though they are supposed discouraged? I think it's worth seeking an exception here to avoid this complexity. Would also be good to understand what the tradeoffs actually are.
Failing that, I would still bail on this added complexity and use the stringified token for the key.
Evan StadeI asked this question here: https://chromium.slack.com/archives/CGJNSDHA7/p1772724774596959
Xiaohan ZhaoSeems like folks thinks it's ok to use some non-Blink container such as `absl::flat_hash_map` since the map's value is not a GCed type, although we'll still need to update `audit_non_blink_usage.py` (and also use `ALLOW_DISCOURAGED_TYPE`?) which triggers a final approval.
Done. Thanks for checking on this!
Hum... I'm inclined to prefer HashMap with required traits implementation in more general WTF place, but as we already reached a conclusion here. I don't have a strong opinion. absl use sgtm.
Related details
- Evan Stade
- Steve Becker
- Xiaohan Zhao
Steve Becker (Gerrit)
Steve Becker added 5 comments![]( "Open in Gerrit")
Thanks again for the cleanup. I'm still reviewing, but here are some initial comments to work through.
ALLOW_DISCOURAGED_TYPE(
"base::Token has AbslHashValue but no WTF "
"HashTraits; using absl hash map avoids the "
"need for a custom HashTraits specialization.")nit: Let's move `ALLOW_DISCOURAGED_TYPE()` to after the variable name to match usage in the rest of blink.
```
absl::flat_hash_map<base::Token, OwnedPendingMutationQueue> pending_mutations_by_source_ ALLOW_DISCOURAGED_TYPE(...);
```
// corresponding StorageAreaObserver event. See
// |pending_mutations_by_source_| and |pending_mutations_by_key_|
// below.nit: it looks like we can revert this change and restore the original formatting.
const base::Token source_id =
source ? source->storage_area_id : base::Token();
const KURL source_page_url = source ? source->page_url : KURL();nit: For all of these in this file, can we use std::move to avoid a copy? Alternatively, maybe we can make these const references although for this to work, we'd probably need constants defined for the empty cases?
```
const base::Token source_id =
source ? std::move(source->storage_area_id) : base::Token();
```
KURL page_url;
base::Token storage_area_id;Would it simplify to store a mojom::blink::StorageAreaSourcePtr here? mojo::StructPtr<T> defines an == operator. See:
Related details
- Evan Stade
- Xiaohan Zhao
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. |
Steve Becker (Gerrit)
Steve Becker added 4 comments![]( "Open in Gerrit")
Still working on the review, but some more comments.
GURL page_url;
base::Token storage_area_id;Do the tests validate these observations anywhere?
GURL page_url;
base::Token storage_area_id;Should we record the blink::mojom::StorageAreaSourcePtr instead of two separate memebers? blink::mojom::StorageAreaSourcePtr can also record null.
CreateNewMap(NewMapType::EMPTY_FROM_DELETE_ALL, std::nullopt);should this also be use source? We can call source->Clone() or update CreateNewMap to take a const reference to the class (which is probably better since it avoids a copy):
`const blink::mojom::StorageAreaSource& delete_all_source`
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 1 comment![]( "Open in Gerrit")
GURL page_url;
base::Token storage_area_id;Do the tests validate these observations anywhere?
Right now we only validate the type, key and old_value. Should we add also validate the rest of the fields as well?
Related details
- Evan Stade
- Steve Becker
Steve Becker (Gerrit)
Steve Becker added 1 comment![]( "Open in Gerrit")
GURL page_url;
base::Token storage_area_id;Xiaohan ZhaoDo the tests validate these observations anywhere?
Right now we only validate the type, key and old_value. Should we add also validate the rest of the fields as well?
There is no reason to record them in the observation if we don't use them anywhere. Yes, let's please improve this test by adding some verifcation.
Related details
- Evan Stade
- Xiaohan Zhao
Steve Becker (Gerrit)
Steve Becker added 2 comments![]( "Open in Gerrit")
Finished reviewing Patchset 9. Please let me know if you have any questions.
const GURL& kTestPageUrl() {
static const base::NoDestructor<GURL> url("https://example.url");
return *url;
}nit: Let's avoid using NoDestructor for a constant. Instead, let's make a character string constant like the ones below. These will not have a constructor or destructor.
Then we can either create a GURL using the string constant for each use:
`GURL(kTestPageUrl)`
or alternatively, we can construct this constant as part of the test class. For example, see kFakeUrlString below. That way the constructor and destructor will not run globally when the code module loads and unloads but instead when the class constructs and destructs, which only impacts the tests in this file instead of all of the tests in the code module.
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 1 comment![]( "Open in Gerrit")
CreateNewMap(NewMapType::EMPTY_FROM_DELETE_ALL, std::nullopt);should this also be use source? We can call source->Clone() or update CreateNewMap to take a const reference to the class (which is probably better since it avoids a copy):
`const blink::mojom::StorageAreaSource& delete_all_source`
The `delete_all_source` in `CreateNewMap()` looks like a leftover from the old implementation. `CreateNewMap()` used to pass the `source` to observers when handling the `EMPTY_FROM_DELETE_ALL` case. But now it's handled in `NotifyObservesAllDeleted()`. I'll remove the unused parameter.
Related details
- Evan Stade
- Steve Becker
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 1 comment![]( "Open in Gerrit")
CreateNewMap(NewMapType::EMPTY_FROM_DELETE_ALL, std::nullopt);Xiaohan Zhaoshould this also be use source? We can call source->Clone() or update CreateNewMap to take a const reference to the class (which is probably better since it avoids a copy):
`const blink::mojom::StorageAreaSource& delete_all_source`
The `delete_all_source` in `CreateNewMap()` looks like a leftover from the old implementation. `CreateNewMap()` used to pass the `source` to observers when handling the `EMPTY_FROM_DELETE_ALL` case. But now it's handled in `NotifyObservesAllDeleted()`. I'll remove the unused parameter.
Steve Becker (Gerrit)
Steve Becker added 1 comment![]( "Open in Gerrit")
CreateNewMap(NewMapType::EMPTY_FROM_DELETE_ALL, std::nullopt);Xiaohan Zhaoshould this also be use source? We can call source->Clone() or update CreateNewMap to take a const reference to the class (which is probably better since it avoids a copy):
`const blink::mojom::StorageAreaSource& delete_all_source`
Xiaohan ZhaoThe `delete_all_source` in `CreateNewMap()` looks like a leftover from the old implementation. `CreateNewMap()` used to pass the `source` to observers when handling the `EMPTY_FROM_DELETE_ALL` case. But now it's handled in `NotifyObservesAllDeleted()`. I'll remove the unused parameter.
- Evan Stade
- Xiaohan Zhao
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 9 comments![]( "Open in Gerrit")
Hi Steve, comments addressed in the latest patchset. Thanks again for the review!
Xiaohan ZhaoDo the tests validate these observations anywhere?
Steve BeckerRight now we only validate the type, key and old_value. Should we add also validate the rest of the fields as well?
There is no reason to record them in the observation if we don't use them anywhere. Yes, let's please improve this test by adding some verifcation.
I added validations for `source` in tests where we validate the rest elements of Observation. Thanks!
Should we record the blink::mojom::StorageAreaSourcePtr instead of two separate memebers? blink::mojom::StorageAreaSourcePtr can also record null.
Updated to blink::mojom::StorageAreaSourcePtr. Thanks!
CreateNewMap(NewMapType::EMPTY_FROM_DELETE_ALL, std::nullopt);Xiaohan Zhaoshould this also be use source? We can call source->Clone() or update CreateNewMap to take a const reference to the class (which is probably better since it avoids a copy):
`const blink::mojom::StorageAreaSource& delete_all_source`
Xiaohan ZhaoThe `delete_all_source` in `CreateNewMap()` looks like a leftover from the old implementation. `CreateNewMap()` used to pass the `source` to observers when handling the `EMPTY_FROM_DELETE_ALL` case. But now it's handled in `NotifyObservesAllDeleted()`. I'll remove the unused parameter.
Removed the unused parameter. Thanks!
const GURL& kTestPageUrl() {
static const base::NoDestructor<GURL> url("https://example.url");
return *url;
}nit: Let's avoid using NoDestructor for a constant. Instead, let's make a character string constant like the ones below. These will not have a constructor or destructor.
Then we can either create a GURL using the string constant for each use:
`GURL(kTestPageUrl)`
or alternatively, we can construct this constant as part of the test class. For example, see kFakeUrlString below. That way the constructor and destructor will not run globally when the code module loads and unloads but instead when the class constructs and destructs, which only impacts the tests in this file instead of all of the tests in the code module.
Done
ALLOW_DISCOURAGED_TYPE(
"base::Token has AbslHashValue but no WTF "
"HashTraits; using absl hash map avoids the "
"need for a custom HashTraits specialization.")nit: Let's move `ALLOW_DISCOURAGED_TYPE()` to after the variable name to match usage in the rest of blink.
```
absl::flat_hash_map<base::Token, OwnedPendingMutationQueue> pending_mutations_by_source_ ALLOW_DISCOURAGED_TYPE(...);
```
Done
// corresponding StorageAreaObserver event. See
// |pending_mutations_by_source_| and |pending_mutations_by_key_|
// below.nit: it looks like we can revert this change and restore the original formatting.
Done
const base::Token source_id =
source ? source->storage_area_id : base::Token();
const KURL source_page_url = source ? source->page_url : KURL();nit: For all of these in this file, can we use std::move to avoid a copy? Alternatively, maybe we can make these const references although for this to work, we'd probably need constants defined for the empty cases?
```
const base::Token source_id =
source ? std::move(source->storage_area_id) : base::Token();
```
Done
Would it simplify to store a mojom::blink::StorageAreaSourcePtr here? mojo::StructPtr<T> defines an == operator. See:
- Evan Stade
- Steve Becker
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steve Becker (Gerrit)
Steve Becker voted and added 2 comments![]( "Open in Gerrit")
Votes added by Steve Becker
| Code-Review | +1 |
2 comments
ASSERT_EQ(1u, mock_storage_area_.observed_delete_alls().size());
EXPECT_EQ(LocalSource(), mock_storage_area_.observed_delete_alls()[0]);nit: Is EXPECT_THAT a more concise assert for here and below?
`EXPECT_THAT(mock_storage_area_.observed_delete_alls(), ElementsAre(LocalSource()));`
ObservedPut& operator=(const ObservedPut&) = delete;nit: there's no reason to delete the assignment operator when implementing the copy constructor. With copy semantics, we should either provide all or nothing.
Related details
- Evan Stade
- Takashi Toyoshima
- Xiaohan Zhao
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. |
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 1 comment![]( "Open in Gerrit")
ASSERT_EQ(1u, mock_storage_area_.observed_delete_alls().size());
EXPECT_EQ(LocalSource(), mock_storage_area_.observed_delete_alls()[0]);nit: Is EXPECT_THAT a more concise assert for here and below?
`EXPECT_THAT(mock_storage_area_.observed_delete_alls(), ElementsAre(LocalSource()));`
`ElementsAre()` needs to copy the `LocalSource()` into a tuple, but `StorageAreaSourcePtr` is move-only. I deleted the ObservedDeleteAll struct and replace the EXPECT_THAT with single element check.
Related details
- Evan Stade
- Steve Becker
- Takashi Toyoshima
Steve Becker (Gerrit)
Steve Becker added 1 comment![]( "Open in Gerrit")
ASSERT_EQ(1u, mock_storage_area_.observed_delete_alls().size());
EXPECT_EQ(LocalSource(), mock_storage_area_.observed_delete_alls()[0]);Xiaohan Zhaonit: Is EXPECT_THAT a more concise assert for here and below?
`EXPECT_THAT(mock_storage_area_.observed_delete_alls(), ElementsAre(LocalSource()));`
`ElementsAre()` needs to copy the `LocalSource()` into a tuple, but `StorageAreaSourcePtr` is move-only. I deleted the ObservedDeleteAll struct and replace the EXPECT_THAT with single element check.
- Evan Stade
- Takashi Toyoshima
- Xiaohan Zhao
Evan Stade (Gerrit)
Evan Stade voted and added 2 comments![]( "Open in Gerrit")
Votes added by Evan Stade
| Code-Review | +1 |
2 comments
auto source_url_and_id =
mojom::blink::StorageAreaSource::New(page_url, source_id);optional nit: inline this because having a separate local variable (subjectively) reduces readability: it's an additional line, and `auto source_url_and_id` doesn't add any to legibility, and creating a local variable implies that it may be used more than once (wrongly, in this case).
Related details
- Takashi Toyoshima
- Xiaohan Zhao
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted Code-Review+1![]( "Open in Gerrit")
- Xiaohan Zhao
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 2 comments![]( "Open in Gerrit")
auto source_url_and_id =
mojom::blink::StorageAreaSource::New(page_url, source_id);optional nit: inline this because having a separate local variable (subjectively) reduces readability: it's an additional line, and `auto source_url_and_id` doesn't add any to legibility, and creating a local variable implies that it may be used more than once (wrongly, in this case).
Thanks for catching this! I updated it in the latest ps.
ObservedPut& operator=(const ObservedPut&) = delete;Xiaohan Zhaonit: there's no reason to delete the assignment operator when implementing the copy constructor. With copy semantics, we should either provide all or nothing.
Thanks for catching that. I updated that in the latest ps.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 1 comment![]( "Open in Gerrit")
Hi Ayu and Emilia, this CL mainly updates tests after replacing the `source` string with a mojom struct, so the corresponding test strings are now nullptr. PTAL! Thanks!
Related details
- Ayu Ishii
- Emilia Paz
- Evan Stade
- Steve Becker
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emilia Paz (Gerrit)
Emilia Paz voted and added 1 comment![]( "Open in Gerrit")
- Ayu Ishii
- Evan Stade
- Steve Becker
- Takashi Toyoshima
- Xiaohan Zhao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evan Stade (Gerrit)
Evan Stade voted and added 3 comments![]( "Open in Gerrit")
Votes added by Evan Stade
| Code-Review | +1 |
3 comments
const String& url,nit: might as well update this arg to `KURL` while we're at it? I would even say that StorageEvent::url_ should be a KURL but that is a bridge too far for this particular CL.
Related details
- Ayu Ishii
- Steve Becker
- Takashi Toyoshima
- Xiaohan Zhao
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. |
Steve Becker (Gerrit)
Steve Becker voted and added 1 comment![]( "Open in Gerrit")
Votes added by Steve Becker
| Code-Review | +1 |
1 comment
const String& url,nit: might as well update this arg to `KURL` while we're at it? I would even say that StorageEvent::url_ should be a KURL but that is a bridge too far for this particular CL.
Maybe we can do this improvement in a separate change since this is already large?
Related details
- Ayu Ishii
- Takashi Toyoshima
- Xiaohan Zhao
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 1 comment![]( "Open in Gerrit")
Hi Daniel, adding you for approval since this CL updates `audit_non_blink_usage.py` to allow `absl::flat_hash_map` usage in blink. Thanks!
Related details
- Ayu Ishii
- Daniel Cheng
- Takashi Toyoshima
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. |
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 1 comment![]( "Open in Gerrit")
const String& url,Steve Beckernit: might as well update this arg to `KURL` while we're at it? I would even say that StorageEvent::url_ should be a KURL but that is a bridge too far for this particular CL.
Maybe we can do this improvement in a separate change since this is already large?
I'll address this in the relation chained follow-up CL. Thanks!
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 2 comments![]( "Open in Gerrit")
- Ayu Ishii
- Daniel Cheng
- Emilia Paz
- Evan Stade
- Steve Becker
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emilia Paz (Gerrit)
Emilia Paz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Emilia Paz
| Code-Review | +1 |
- Ayu Ishii
- Daniel Cheng
- Evan Stade
- Steve Becker
- Takashi Toyoshima
- Xiaohan Zhao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steve Becker (Gerrit)
Steve Becker voted Code-Review+1![]( "Open in Gerrit")
- Ayu Ishii
- Daniel Cheng
- Evan Stade
- Takashi Toyoshima
- Xiaohan Zhao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Ayu Ishii (Gerrit)
Ayu Ishii voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ayu Ishii
| Code-Review | +1 |
1 comment
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted Code-Review+1![]( "Open in Gerrit")
- Daniel Cheng
- Evan Stade
- Xiaohan Zhao
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
1 comment
- Evan Stade
- Xiaohan Zhao
Code-Coverage
Code-Owners
Xiaohan Zhao (Gerrit)
Xiaohan Zhao voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Evan Stade
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[DOM Storage] Replace string source with StorageAreaSource struct
Replace the opaque string `source` parameter in the StorageArea mojom
interface and StorageAreaObserver with a new StorageAreaSource struct
containing typed `page_url` (url.mojom.Url) and `storage_area_id` (mojo_base.mojom.Token, mapped to base::Token) fields.
Previously, the page URL and storage area ID were packed into a single
newline-delimited string. This was fragile and required manual
pack/unpack helpers (PackSource/UnpackSource). The new struct makes the
API self-documenting and type-safe.
Bug: 477645291
Change-Id: I3cb2a79b41740bad0ca9f8fa60fad807f69c408b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7597837
Commit-Queue: Xiaohan Zhao <xiaoh...@microsoft.com>
Reviewed-by: Steve Becker <ste...@microsoft.com>
Reviewed-by: Emilia Paz <emil...@chromium.org>
Reviewed-by: Takashi Toyoshima <toyo...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Reviewed-by: Ayu Ishii <ay...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1600287}
- M chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
- M chrome/browser/browsing_data/counters/site_data_counting_helper_unittest.cc
- M chrome/browser/extensions/api/browsing_data/browsing_data_test.cc
- M chrome/browser/extensions/extension_service_unittest.cc
- M components/browsing_data/content/local_storage_helper_browsertest.cc
- M components/services/storage/dom_storage/local_storage_impl.cc
- M components/services/storage/dom_storage/local_storage_impl_unittest.cc
- M components/services/storage/dom_storage/session_storage_area_impl.cc
- M components/services/storage/dom_storage/session_storage_area_impl.h
- M components/services/storage/dom_storage/session_storage_area_impl_unittest.cc
- M components/services/storage/dom_storage/session_storage_impl_unittest.cc
- M components/services/storage/dom_storage/session_storage_namespace_impl.cc
- M components/services/storage/dom_storage/session_storage_namespace_impl_unittest.cc
- M components/services/storage/dom_storage/storage_area_impl.cc
- M components/services/storage/dom_storage/storage_area_impl.h
- M components/services/storage/dom_storage/storage_area_impl_unittest.cc
- M components/services/storage/dom_storage/test_support/storage_area_test_util.cc
- M components/services/storage/dom_storage/test_support/storage_area_test_util.h
- M third_party/blink/public/mojom/dom_storage/storage_area.mojom
- M third_party/blink/renderer/modules/storage/cached_storage_area.cc
- M third_party/blink/renderer/modules/storage/cached_storage_area.h
- M third_party/blink/renderer/modules/storage/cached_storage_area_test.cc
- M third_party/blink/renderer/modules/storage/testing/mock_storage_area.cc
- M third_party/blink/renderer/modules/storage/testing/mock_storage_area.h
- M third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
Code-Review: +1 by Emilia Paz, +1 by Ayu Ishii, +1 by Steve Becker, +1 by Daniel Cheng, +1 by Takashi Toyoshima
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |