[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.