Switch ChildProcessSecurityPolicyImpl to ChildProcessId [chromium/src : main]
Christopher Staite (Gerrit)
Christopher Staite added 1 comment![]( "Open in Gerrit")
Hi all, taking a stab at infiltrating the ChildProcessId a little further. I've done an all target build locally, so I feel confident, but if someone could hit a dry run for me that would be appreciated. Thanks!
Related details
- Dmitry Gozman
- Fergal Daly
- Ken Buchanan
- Tom Sepez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmitry Gozman (Gerrit)
Dmitry Gozman voted and added 4 comments![]( "Open in Gerrit")
Votes added by Dmitry Gozman
| Commit-Queue | +1 |
4 comments
Nice one! I encourage you to push a bit further though 😊
if (base::Contains(security_state_,I'd prefer to see private members of `ChildProcessSecurityPolicyImpl` to be migrated to `ChildProcessId` right away. For example, `ChildProcessSecurityPolicyImpl::security_state_` and `ChildProcessSecurityPolicyImpl::Add`. This will avoid a lot of unnecessary unsafe value conversions, and keep the changes contained to a single file. WDYT?
bool ChildProcessSecurityPolicyImpl::CanReadFile(int child_id,What's the future plan for public APIs like this one? Perhaps we should introduce an overload that takes `ChildProcessId` right away, and immediately use it inside `content/`? Then it would be just a mechanical migration of all callers from the old method to the new one, while impl code will be good from the get go. WDYT?
static RenderFrameHostImpl* FromFrameToken(I like this design with the overload. Let's add the same `TODO(crbug)` to the old method above, for easier search later on?
Related details
- Christopher Staite
- Tom Sepez
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. |
Christopher Staite (Gerrit)
Christopher Staite added 3 comments![]( "Open in Gerrit")
if (base::Contains(security_state_,I'd prefer to see private members of `ChildProcessSecurityPolicyImpl` to be migrated to `ChildProcessId` right away. For example, `ChildProcessSecurityPolicyImpl::security_state_` and `ChildProcessSecurityPolicyImpl::Add`. This will avoid a lot of unnecessary unsafe value conversions, and keep the changes contained to a single file. WDYT?
I did originally try to change `ChildProcessSecurityPolicyImpl::Add`, but the knock on to unit tests was massive and the change went out of control. Hence I've gone as far as I think is appropriate for a single review. I will probably come back and do `Add` next as it seems the logical thing to do, but that will create a very large change set across numerous files.
bool ChildProcessSecurityPolicyImpl::CanReadFile(int child_id,What's the future plan for public APIs like this one? Perhaps we should introduce an overload that takes `ChildProcessId` right away, and immediately use it inside `content/`? Then it would be just a mechanical migration of all callers from the old method to the new one, while impl code will be good from the get go. WDYT?
My plan was to convert one at a time and its usages across the product. I'm happy to take guidance if you think that's a poor approach.
static RenderFrameHostImpl* FromFrameToken(I like this design with the overload. Let's add the same `TODO(crbug)` to the old method above, for easier search later on?
Nice idea, I'll add that in. My TODO's have been a bit hit-and-miss I'll try to make them more reliable.
Related details
- Dmitry Gozman
- Tom Sepez
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. |
Dmitry Gozman (Gerrit)
Dmitry Gozman voted and added 5 comments![]( "Open in Gerrit")
Votes added by Dmitry Gozman
| Commit-Queue | +1 |
5 comments
Sorry, this took me quite a while to review. My only suggestion is to migrate unit tests right away, since you are already touching all the same lines. Also, let me trigger the dry run.
if (base::Contains(security_state_,Christopher StaiteI'd prefer to see private members of `ChildProcessSecurityPolicyImpl` to be migrated to `ChildProcessId` right away. For example, `ChildProcessSecurityPolicyImpl::security_state_` and `ChildProcessSecurityPolicyImpl::Add`. This will avoid a lot of unnecessary unsafe value conversions, and keep the changes contained to a single file. WDYT?
I did originally try to change `ChildProcessSecurityPolicyImpl::Add`, but the knock on to unit tests was massive and the change went out of control. Hence I've gone as far as I think is appropriate for a single review. I will probably come back and do `Add` next as it seems the logical thing to do, but that will create a very large change set across numerous files.
I see, this makes sense. I did not realize the blast radius in the tests.
bool ChildProcessSecurityPolicyImpl::CanReadFile(int child_id,Christopher StaiteWhat's the future plan for public APIs like this one? Perhaps we should introduce an overload that takes `ChildProcessId` right away, and immediately use it inside `content/`? Then it would be just a mechanical migration of all callers from the old method to the new one, while impl code will be good from the get go. WDYT?
My plan was to convert one at a time and its usages across the product. I'm happy to take guidance if you think that's a poor approach.
Hmm... I looked at `CanReadFile()` and it also has a lot of usages across files. Your plan sounds good.
p, ChildProcessId::FromUnsafeValue(kRendererID), granted_file,You can probably introduce something like `kRendererChildId` of the `ChildProcessId` type right away, to avoid touching the same code again.
ChildProcessId::FromUnsafeValue(kTestProcessIdOrigin1),Same here.
Related details
- Christopher Staite
- Tom Sepez
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. |