ash: Dismantle the document_scan crosapi service, part 2 [chromium/src : main]
Georg Neis (Gerrit)
Georg Neis voted and added 6 comments![]( "Open in Gerrit")
Votes added by Georg Neis
| Commit-Queue | +1 |
6 comments
lorgnette::OPERATION_RESULT_SUCCESS, std::move(config));See the deleted FakeDocumentScanAsh::GetOptionGroups.
lorgnette::OperationResult result);This was accidentally still using the mojo machinery and in the mojo namespace.
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 voted and added 4 comments![]( "Open in Gerrit")
Votes added by Hidehiko Abe
| Code-Review | +1 |
4 comments
std::optional<lorgnette::OperationResult> result,
std::optional<lorgnette::ScannerConfig> config) {
get_current_config_result_ = std::move(result);
get_current_config_config_ = std::move(config);nit: `SetGetCurrentConfigResponse` now sounds a bit tricky name, because this does not directly set GetCurrentConfigResponse (but set some values to construct the response).
Could you either:
- change the name to reflect your conceptual change, i.e., configure the (fake) manager with the given scanne rconfig, or
- undo to have optional<GetCurrentConfigResponse> as an argument, and construct the protobuf in callers?
class CancelScanResponse;Hidehiko AbeThis was unused.
Acknowledged
lorgnette::OPERATION_RESULT_SUCCESS, std::move(config));See the deleted FakeDocumentScanAsh::GetOptionGroups.
- 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")
std::optional<lorgnette::OperationResult> result,
std::optional<lorgnette::ScannerConfig> config) {
get_current_config_result_ = std::move(result);
get_current_config_config_ = std::move(config);nit: `SetGetCurrentConfigResponse` now sounds a bit tricky name, because this does not directly set GetCurrentConfigResponse (but set some values to construct the response).
Could you either:
- change the name to reflect your conceptual change, i.e., configure the (fake) manager with the given scanne rconfig, or
- undo to have optional<GetCurrentConfigResponse> as an argument, and construct the protobuf in callers?
Hmm I don't understand the concern. Yes we don't pass the whole response but we do pass the components that fully determine the response, because by contract the scanner handle is always the one from the corresponding request: https://source.chromium.org/chromium/chromium/src/+/main:third_party/cros_system_api/dbus/lorgnette/lorgnette_service.proto;l=725;drc=c6a2b6946d14688c4e54506227e03714e374b000
(I.e. the previous SetGetCurrentConfigResponse unnecessarily allowed constructing invalid responses.)
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Hidehiko Abe
| Code-Review | +1 |
1 comment
std::optional<lorgnette::OperationResult> result,
std::optional<lorgnette::ScannerConfig> config) {
get_current_config_result_ = std::move(result);
get_current_config_config_ = std::move(config);Georg Neisnit: `SetGetCurrentConfigResponse` now sounds a bit tricky name, because this does not directly set GetCurrentConfigResponse (but set some values to construct the response).
Could you either:
- change the name to reflect your conceptual change, i.e., configure the (fake) manager with the given scanne rconfig, or
- undo to have optional<GetCurrentConfigResponse> as an argument, and construct the protobuf in callers?
Hmm I don't understand the concern. Yes we don't pass the whole response but we do pass the components that fully determine the response, because by contract the scanner handle is always the one from the corresponding request: https://source.chromium.org/chromium/chromium/src/+/main:third_party/cros_system_api/dbus/lorgnette/lorgnette_service.proto;l=725;drc=c6a2b6946d14688c4e54506227e03714e374b000
(I.e. the previous SetGetCurrentConfigResponse unnecessarily allowed constructing invalid responses.)
Sorry for miscommunication. My point was:
The method name looks like a simple setter, as its name is prefixed with "Set", and actually, the original code did so. The simple setter is to take the value directly, and store it in the member.
Now, with your change, it's no longer true. It does not take the value to be directly set to "get_current_config_resposne_".
Now, I'd interpret the method is to configure the fake manager to hold the specified scanner config. I think it is fine, moreover a good change to a good direction, assuming "all" fake manager will respect the scanner config when needed later (not necessary in this CL) . GetCurrentConfig() in this CL looks like a first step towards to the direction to me. It is a simple method, returning the scanner config of the configured scanner config, so it makes sense to return the value set from here.
In the sense, it no longer makes sense to keep the name "SetGetCurrentConfigResponse".
My suggestion is:
- If you'd like to keep this as a simple setter to configure the return value of GetCurrentConfig() directly, it should be the original form.
- If you'd like to move towards to the direction of my interpretation described above, please do not name it "SetGetCurrentConfigResponse" because it should no longer be so.
- If you have different design, please let me know. I'm open for the discussion of course :-)
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 voted and added 5 comments![]( "Open in Gerrit")
Votes added by Georg Neis
| Commit-Queue | +2 |
5 comments
std::optional<lorgnette::OperationResult> result,
std::optional<lorgnette::ScannerConfig> config) {
get_current_config_result_ = std::move(result);
get_current_config_config_ = std::move(config);Georg Neisnit: `SetGetCurrentConfigResponse` now sounds a bit tricky name, because this does not directly set GetCurrentConfigResponse (but set some values to construct the response).
Could you either:
- change the name to reflect your conceptual change, i.e., configure the (fake) manager with the given scanne rconfig, or
- undo to have optional<GetCurrentConfigResponse> as an argument, and construct the protobuf in callers?
Hidehiko AbeHmm I don't understand the concern. Yes we don't pass the whole response but we do pass the components that fully determine the response, because by contract the scanner handle is always the one from the corresponding request: https://source.chromium.org/chromium/chromium/src/+/main:third_party/cros_system_api/dbus/lorgnette/lorgnette_service.proto;l=725;drc=c6a2b6946d14688c4e54506227e03714e374b000
(I.e. the previous SetGetCurrentConfigResponse unnecessarily allowed constructing invalid responses.)
Sorry for miscommunication. My point was:
The method name looks like a simple setter, as its name is prefixed with "Set", and actually, the original code did so. The simple setter is to take the value directly, and store it in the member.
Now, with your change, it's no longer true. It does not take the value to be directly set to "get_current_config_resposne_".
Now, I'd interpret the method is to configure the fake manager to hold the specified scanner config. I think it is fine, moreover a good change to a good direction, assuming "all" fake manager will respect the scanner config when needed later (not necessary in this CL) . GetCurrentConfig() in this CL looks like a first step towards to the direction to me. It is a simple method, returning the scanner config of the configured scanner config, so it makes sense to return the value set from here.
In the sense, it no longer makes sense to keep the name "SetGetCurrentConfigResponse".My suggestion is:
- If you'd like to keep this as a simple setter to configure the return value of GetCurrentConfig() directly, it should be the original form.
- If you'd like to move towards to the direction of my interpretation described above, please do not name it "SetGetCurrentConfigResponse" because it should no longer be so.
- If you have different design, please let me know. I'm open for the discussion of course :-)
Renamed to Configure*
const lorgnette::CancelScanResponse& response);Georg Neisdito
Acknowledged
lorgnette::OperationResult result);Georg NeisThis was accidentally still using the mojo machinery and in the mojo namespace.
Acknowledged
moved only, no code change
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/extensions/api/document_scan/document_scan_apitest.cc
Insertions: 1, Deletions: 1.
@@ -128,7 +128,7 @@
group->set_title("title");
group->add_members("item1");
group->add_members("item2");
- lorgnette_manager->SetGetCurrentConfigResponse(
+ lorgnette_manager->ConfigureGetCurrentConfigResponse(
lorgnette::OPERATION_RESULT_SUCCESS, std::move(config));
}
```
```
The name of the file: chrome/browser/ash/scanning/fake_lorgnette_scanner_manager.h
Insertions: 5, Deletions: 4.
@@ -80,10 +80,11 @@
void SetSetOptionsResponse(
const std::optional<lorgnette::SetOptionsResponse>& response);
- // Sets the response returned by GetCurrentConfig(), modulo the scanner, which
- // will be taken from the request. If `result` has no value, the response
- // will be nullopt (that's the default).
- void SetGetCurrentConfigResponse(
+ // Configures the response returned by GetCurrentConfig().
+ // If `result` has no value, the response will be nullopt (that's the
+ // default). Otherwise, the response will consist of the given values and the
+ // scanner from the request.
+ void ConfigureGetCurrentConfigResponse(
std::optional<lorgnette::OperationResult> result,
std::optional<lorgnette::ScannerConfig> config);
```
```
The name of the file: chrome/browser/extensions/api/document_scan/document_scan_api_handler_unittest.cc
Insertions: 2, Deletions: 2.
@@ -750,7 +750,7 @@
const std::string scanner_id = CreateScannerIdForExtension(extension_);
ASSERT_FALSE(scanner_id.empty());
- GetLorgnetteScannerManager()->SetGetCurrentConfigResponse(
+ GetLorgnetteScannerManager()->ConfigureGetCurrentConfigResponse(
lorgnette::OPERATION_RESULT_SUCCESS, std::nullopt);
// The first open succeeds because the scanner is not open.
@@ -819,7 +819,7 @@
lorgnette::OptionGroup* group = config.add_option_groups();
group->set_title("group-title");
group->add_members("group-member");
- GetLorgnetteScannerManager()->SetGetCurrentConfigResponse(
+ GetLorgnetteScannerManager()->ConfigureGetCurrentConfigResponse(
lorgnette::OPERATION_RESULT_SUCCESS, std::move(config));
GetOptionGroupsFuture future;
```
```
The name of the file: chrome/browser/ash/scanning/fake_lorgnette_scanner_manager.cc
Insertions: 1, Deletions: 1.
@@ -353,7 +353,7 @@
set_options_response_ = response;
}
-void FakeLorgnetteScannerManager::SetGetCurrentConfigResponse(
+void FakeLorgnetteScannerManager::ConfigureGetCurrentConfigResponse(
std::optional<lorgnette::OperationResult> result,
std::optional<lorgnette::ScannerConfig> config) {
get_current_config_result_ = std::move(result);
```
Change information
ash: Dismantle the document_scan crosapi service, part 2
Migrate DocumentScanAPIHandler::GetOptionGroups to call
ash::LorgnetteScannerManager directly instead of using the crosapi
service, and remove the GetOptionGroups from the latter.
This is an incremental step in eliminating the document_scan crosapi
service.
- M chrome/browser/ash/crosapi/document_scan_ash.cc
- M chrome/browser/ash/crosapi/document_scan_ash.h
- M chrome/browser/ash/crosapi/document_scan_ash_type_converters.cc
- M chrome/browser/ash/crosapi/document_scan_ash_type_converters.h
- M chrome/browser/ash/crosapi/document_scan_ash_type_converters_unittest.cc
- M chrome/browser/ash/crosapi/document_scan_ash_unittest.cc
- M chrome/browser/ash/scanning/fake_lorgnette_scanner_manager.cc
- M chrome/browser/ash/scanning/fake_lorgnette_scanner_manager.h
- M chrome/browser/extensions/api/document_scan/document_scan_api_handler.cc
- M chrome/browser/extensions/api/document_scan/document_scan_api_handler.h
- M chrome/browser/extensions/api/document_scan/document_scan_api_handler_unittest.cc
- M chrome/browser/extensions/api/document_scan/document_scan_apitest.cc
- M chrome/browser/extensions/api/document_scan/document_scan_type_converters.cc
- M chrome/browser/extensions/api/document_scan/document_scan_type_converters.h
- M chrome/browser/extensions/api/document_scan/document_scan_type_converters_unittest.cc
- M chrome/browser/extensions/api/document_scan/fake_document_scan_ash.cc
- M chrome/browser/extensions/api/document_scan/fake_document_scan_ash.h
- M chromeos/crosapi/mojom/document_scan.mojom
Code-Review: +1 by Hidehiko Abe
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |