ash: Remove VpnService crosapi [chromium/src : main]
Georg Neis (Gerrit)
Georg Neis voted and added 2 comments![]( "Open in Gerrit")
Votes added by Georg Neis
| Commit-Queue | +1 |
2 comments
Patchset 1 contains code moves only. Hopefully this helps a little bit.
if (pepper_vpn_proxy_observer_) {The whole pepper feature was recently removed. See https://chromium-review.googlesource.com/c/chromium/src/+/6667722 and https://chromium-review.googlesource.com/c/chromium/src/+/6677511
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Georg Neis (Gerrit)
Georg Neis added 1 comment![]( "Open in Gerrit")
Patchset 1 contains code moves only. Hopefully this helps a little bit.
Unresolving for visibility.
Related details
- Hidehiko Abe
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe added 1 comment![]( "Open in Gerrit")
Sorry for miscommunication and delayed reply.
Looking at the code in more precise, I still am feeling difficult to say straightforward LG in this CL, and risk of breakage looks non trivial, because even corresponding code pairs look having some minor tweaks after moving the code, and also the size is still huge (1500+LoC edits).
Suggestion:
Can we break down into smaller pieces?
First step can be:
SetParameters, SendPacket, NotifyConnectionStateChanged. They look similar patterns, and has only small dependency in VpnServiceAsh in crosapi, which is active_configuration_. You can temporarily expose the pointer by adding a public method to crosapi::VpnServiceAsh for migration purpose.
Next step can be moving observer method implementation. Some of internal methods may need to be exposed temporarily as back doors, but I think that'd be fine as long as they are explicitly stated so. Or, if you concern more, base::PassKey should help limiting callers.
Related details
- Georg Neis
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Georg Neis (Gerrit)
Georg Neis added 1 comment![]( "Open in Gerrit")
Sorry for miscommunication and delayed reply.
Looking at the code in more precise, I still am feeling difficult to say straightforward LG in this CL, and risk of breakage looks non trivial, because even corresponding code pairs look having some minor tweaks after moving the code, and also the size is still huge (1500+LoC edits).
Suggestion:
Can we break down into smaller pieces?
First step can be:
SetParameters, SendPacket, NotifyConnectionStateChanged. They look similar patterns, and has only small dependency in VpnServiceAsh in crosapi, which is active_configuration_. You can temporarily expose the pointer by adding a public method to crosapi::VpnServiceAsh for migration purpose.Next step can be moving observer method implementation. Some of internal methods may need to be exposed temporarily as back doors, but I think that'd be fine as long as they are explicitly stated so. Or, if you concern more, base::PassKey should help limiting callers.
I don't see what you see regarding "Next step ...".
Let's take VpnServiceForExtensionAsh::OnConfigurationRemoved (ash::NetworkConfigurationObserver) as example. To move this we also need to move configurations and thus basically everything. Moving it without moving configurations would require artificial changes such as exposing DispatchConfigRemovedEvent and DestroyConfigurationInternal and making them take the service path as parameter instead of what they currently take. And then we would still have to do the hard work later.
Related details
- Hidehiko Abe
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. |
Hidehiko Abe (Gerrit)
Hidehiko Abe added 1 comment![]( "Open in Gerrit")
Georg NeisSorry for miscommunication and delayed reply.
Looking at the code in more precise, I still am feeling difficult to say straightforward LG in this CL, and risk of breakage looks non trivial, because even corresponding code pairs look having some minor tweaks after moving the code, and also the size is still huge (1500+LoC edits).
Suggestion:
Can we break down into smaller pieces?
First step can be:
SetParameters, SendPacket, NotifyConnectionStateChanged. They look similar patterns, and has only small dependency in VpnServiceAsh in crosapi, which is active_configuration_. You can temporarily expose the pointer by adding a public method to crosapi::VpnServiceAsh for migration purpose.Next step can be moving observer method implementation. Some of internal methods may need to be exposed temporarily as back doors, but I think that'd be fine as long as they are explicitly stated so. Or, if you concern more, base::PassKey should help limiting callers.
I don't see what you see regarding "Next step ...".
Let's take VpnServiceForExtensionAsh::OnConfigurationRemoved (ash::NetworkConfigurationObserver) as example. To move this we also need to move configurations and thus basically everything. Moving it without moving configurations would require artificial changes such as exposing DispatchConfigRemovedEvent and DestroyConfigurationInternal and making them take the service path as parameter instead of what they currently take. And then we would still have to do the hard work later.
You can break down the operation to smaller ones step by step.
```
void VpnServiceForExtensionAsh::OnConfigurationRemoved(
const std::string& service_path,
const std::string& guid) {
VpnConfiguration* configuration =
base::FindPtrOrNull(service_path_to_configuration_map_, service_path);
if (!configuration) {
// Ignore removal of a configuration unknown to VPN service, which means
// the configuration was created internally by the platform or already
// removed by the extension.
return;
}
DispatchConfigRemovedEvent(configuration->configuration_name());
DestroyConfigurationInternal(configuration);
}
```
So, VpnServiceForExtensionAsh can provide a few thin new APIs like returning configuration name from service_path and destroying some specific configure (equivalent to DestroyConfigurationInternal conceptually, but interface can be somewhat tweakable).
So, with keeping the implementation, observer can be moved to VpnService.
```
void VpnService::OnConfigurationRemoved(const std::string& service_path,
const std::string&) {
auto configration_name = GetVpnServiceForExtension(...)->FindConfigurationName(service_path);
if (!configuration_name) {
return;
}
SendOnConfigRemovedToExtension(...);
GetVpnServiceForExtension(...)->DestroyConfiguration(...);
}
```
So, configuration map can be kep in the old class still, but "sending message part" can be moved out from the class.
The drawback is, as commented above, it breaks some of the encapsulation of VpnServiceForExtensionAsh, but IMHO, it should be fine as long as we can control callers, because anyways we're breaking the class and merge it into the new code in ash.
Thoughts?
Related details
- Georg Neis
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. |
Georg Neis (Gerrit)
Georg Neis added 1 comment![]( "Open in Gerrit")
That's one drawback, but I mentioned more:
Moving it without moving configurations would require artificial changes such as exposing DispatchConfigRemovedEvent and DestroyConfigurationInternal and making them take the service path as parameter instead of what they currently take. And then we would still have to do the hard work later.
Changing the parameter types of the functions is an artificial change only needed for the sake of making these intermediate CLs and something we would most likely want to do undo again later. Moreover, we still have to move the configurations anyways.
I understand you want to have small CLs but in this case I don't think it's the right approach as it creates a lot of overhead.
Related details
- Hidehiko Abe
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. |
Hidehiko Abe (Gerrit)
Hidehiko Abe added 1 comment![]( "Open in Gerrit")
so per offlien chat, you want to use `friend` in order to split this (still) large CL into smaller pieces and migrate one by one.
I'm ok with the approach, too.
Related details
- Georg Neis
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. |