Add SaasUsageReportScheduler to ChromeBrowserCloudManagementController [chromium/src : main]
Jan Zarzycki (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Igor Ruvinov
- Nicolas Ouellet-Payeur
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolas Ouellet-Payeur (Gerrit)
Nicolas Ouellet-Payeur added 3 comments![]( "Open in Gerrit")
I'm not sure if this is the best approach... I don't necessarily want to block this CL, but let's ask zmin@ what he thinks since he's familiar with this `ReportingDelegateFactory` class
+CC zmin@
if (saas_usage_report_scheduler_) {nit: no need for an if-statement here. IMO you can remove the one for `report_scheduler_` while you're here
virtual std::unique_ptr<SaasUsageReportFactory::Delegate>
GetSaasUsageReportFactoryDelegate() const = 0;
virtual std::unique_ptr<SaasUsageReportUploader> GetSaasUsageReportUploader()
const = 0;
virtual std::unique_ptr<SaasUsageReportScheduler::Delegate>
GetSaasUsageReportSchedulerDelegate() const = 0;This class is getting bigger and bigger, and I feel like this is the wrong pattern.
We're always(?) calling these 3 methods together, and they're always all null or all non-null. One option is to return an `optional<Something>`, where `Something` is a struct of 3 objects, always non-null.
But beyond that, I wonder if there's a better way to prevent the explosion of methods here. For instance, maybe we could "delegate" more logic to this factory?
We could for instance move some code, and change this:
```
auto saas_report_factory_delegate =
reporting_delegate_factory->GetSaasUsageReportFactoryDelegate();
auto saas_report_uploader =
reporting_delegate_factory->GetSaasUsageReportUploader();
auto saas_report_scheduler_delegate =
reporting_delegate_factory->GetSaasUsageReportSchedulerDelegate();
if (saas_report_factory_delegate && saas_report_uploader &&
saas_report_scheduler_delegate) {
saas_usage_report_scheduler_ =
std::make_unique<enterprise_reporting::SaasUsageReportScheduler>(
local_state,
std::make_unique<enterprise_reporting::SaasUsageReportFactory>(
local_state, std::move(saas_report_factory_delegate)),
std::move(saas_report_uploader),
std::move(saas_report_scheduler_delegate));
}
```
to this:
```
saas_usage_report_scheduler_ =
reporting_delegate_factory->CreateSaasUsageReportScheduler(local_state);
```
It also has the nice side-effect of keeping (some) SAAS-specific stuff away from the `CBCMController` class.
I suppose that's not a "Delegate Factory" anymore, so maybe this code should go somewhere else. But I don't think this trajectory is sustainable, if we keep adding types of reporting. Ideally we'd refactor this whole class, but that's way out of scope for this CL.
@zmin: you're the last person to touch this code, do you have a preference?
Related details
- Igor Ruvinov
- Jan Zarzycki
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. |
Owen Min (Gerrit)
Owen Min added 1 comment![]( "Open in Gerrit")
I agree with the approach of reducing the number of APIs of report delegate factory by creating secondary factory class.
I don't want to put "Create" class directory under reporting delegate factory. As you said, it exceeds the original purpose of delegate factory.
To simpfiles the logic in CBCM controller, we could create a new factory function. But it could be either a free function or under SaasReportScheduler. And DelegateFactory flass should focus on providing delegate.
Related details
- Jan Zarzycki
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. |
Jan Zarzycki (Gerrit)
Jan Zarzycki added 2 comments![]( "Open in Gerrit")
if (saas_usage_report_scheduler_) {nit: no need for an if-statement here. IMO you can remove the one for `report_scheduler_` while you're here
I'm worried that there can be edge case when it can be null, for example:
- Shutdown may be called before scheduler is initialized
- Shutdown can be called twice
Are you sure that this won't happen? I'd prefer to leave this check until I have time to investigate further.
virtual std::unique_ptr<SaasUsageReportFactory::Delegate>
GetSaasUsageReportFactoryDelegate() const = 0;
virtual std::unique_ptr<SaasUsageReportUploader> GetSaasUsageReportUploader()
const = 0;
virtual std::unique_ptr<SaasUsageReportScheduler::Delegate>
GetSaasUsageReportSchedulerDelegate() const = 0;Thanks for suggestions. I updated CL:
- Added `SaasUsageReportingDelegateFactory`
- Added desktop implementation for the new delegate: `SaasUsageReportingDelegateFactoryDesktop`.
- Added factory method in `SaasUsageReportScheduler` which takes `SaasUsageReportingDelegateFactory`
- Added virtual function to CBCMController delegate which returns platform specific delegate factory.
- CBCM controller now gets SaaS delegate factory from its delegate and instantiates scheduler using factory method.
I think that it makes sense to separate SaaS reporting delegate factory from existing reporting delegate factory, as those are two separate features and they don't need to be coupled.
It will also simplify adding profile specific factory, because I will be able to easily create SaasReportingDelegateFactory with profile as member. For ReportingDelegateFactory this would require growing it further and having two separate methods to get delegates for browser and profile. Without getting into much details - this is because there is single factory class used for both desktop and ChromeOS. It would probably require refactoring separate ChromeOS delegate factory to untangle it.
Related details
- Igor Ruvinov
- Nicolas Ouellet-Payeur
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. |
Nicolas Ouellet-Payeur (Gerrit)
Nicolas Ouellet-Payeur voted and added 4 comments![]( "Open in Gerrit")
Votes added by Nicolas Ouellet-Payeur
| Code-Review | +1 |
4 comments
if (saas_usage_report_scheduler_) {Jan Zarzyckinit: no need for an if-statement here. IMO you can remove the one for `report_scheduler_` while you're here
I'm worried that there can be edge case when it can be null, for example:
- Shutdown may be called before scheduler is initialized
- Shutdown can be called twice
Are you sure that this won't happen? I'd prefer to leave this check until I have time to investigate further.
It doesn't matter. Even if we call `Shutdown()` before `Init()`, it shouldn't cause any issues:
- `unique_ptr` default-constructs to null
- `unique_ptr::reset()` on null is a no-op
There's already an [if-statement inside `unique_ptr::reset()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/libc++/src/include/__memory/unique_ptr.h;l=287-288;drc=44c3d61b466d13e43fd307e40b0c64d95be1e793), so you're checking the same thing twice
auto saas_usage_reporting_delegate_factory =
delegate_->GetSaasUsageReportingDelegateFactory();
if (saas_usage_reporting_delegate_factory) {nit:
```
if (auto saas_usage_reporting_delegate_factory =
delegate_->GetSaasUsageReportingDelegateFactory()) {
...
}
```
I think that it makes sense to separate SaaS reporting delegate factory from existing reporting delegate factory, as those are two separate features and they don't need to be coupled.
- Igor Ruvinov
- Jan Zarzycki
- Owen Min
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. |
Igor Ruvinov (Gerrit)
Igor Ruvinov voted Code-Review+1![]( "Open in Gerrit")
- Jan Zarzycki
- Owen Min
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. |
Jan Zarzycki (Gerrit)
Jan Zarzycki added 2 comments![]( "Open in Gerrit")
if (saas_usage_report_scheduler_) {Jan Zarzyckinit: no need for an if-statement here. IMO you can remove the one for `report_scheduler_` while you're here
Nicolas Ouellet-PayeurI'm worried that there can be edge case when it can be null, for example:
- Shutdown may be called before scheduler is initialized
- Shutdown can be called twice
Are you sure that this won't happen? I'd prefer to leave this check until I have time to investigate further.
It doesn't matter. Even if we call `Shutdown()` before `Init()`, it shouldn't cause any issues:
- `unique_ptr` default-constructs to null
- `unique_ptr::reset()` on null is a no-op
There's already an [if-statement inside `unique_ptr::reset()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/libc++/src/include/__memory/unique_ptr.h;l=287-288;drc=44c3d61b466d13e43fd307e40b0c64d95be1e793), so you're checking the same thing twice
TIL, thanks! I removed ifs.
auto saas_usage_reporting_delegate_factory =
delegate_->GetSaasUsageReportingDelegateFactory();
if (saas_usage_reporting_delegate_factory) {nit:
```
if (auto saas_usage_reporting_delegate_factory =
delegate_->GetSaasUsageReportingDelegateFactory()) {
...
}
```
- Igor Ruvinov
- Nicolas Ouellet-Payeur
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Ruvinov (Gerrit)
Igor Ruvinov voted Code-Review+1![]( "Open in Gerrit")
- Nicolas Ouellet-Payeur
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolas Ouellet-Payeur (Gerrit)
Nicolas Ouellet-Payeur voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Jan Zarzycki
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |