[DC] Add DigitalCredentials.setVirtualWalletBehavior CDP command [chromium/src : main]
Tejas Dhagawkar (Gerrit)
Tejas Dhagawkar added 2 comments![]( "Open in Gerrit")
Rarely seen unit test for handler file. Is this really required?
# JSON-encoded response data object returned by the wallet. Parsed by
# the browser when |behavior| is "respond". Required when |behavior| is
# "respond", forbidden otherwise.Make it short
JSON-encoded response data object returned by the wallet.
Required when |behavior| is "respond", forbidden otherwise.
Related details
- Anushka Kulkarni
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. |
Anushka Kulkarni (Gerrit)
Anushka Kulkarni added 2 comments![]( "Open in Gerrit")
Rarely seen unit test for handler file. Is this really required?
Fair question — handler-level unit tests are uncommon, but I added these because:
The handler does non-trivial parameter validation (the respond-only protocol/response contract from the PDL, JSON parsing of response, and behavior dispatch) that isn't covered anywhere else.
WPT/inspector-protocol tests exercise the full BiDi → CDP → handler → wallet flow, but they're slow and a failure there doesn't pinpoint which layer broke. These unit tests give fast, targeted coverage of the handler's branches and would catch regressions in just this file.
There is precedent — webauthn_handler_unittest.cc and tracing_handler_unittest.cc follow the same pattern for handlers with non-trivial validation logic.
Happy to drop them if you feel the WPT coverage is sufficient — but I'd lean toward keeping them. WDYT?
# JSON-encoded response data object returned by the wallet. Parsed by
# the browser when |behavior| is "respond". Required when |behavior| is
# "respond", forbidden otherwise.Make it short
JSON-encoded response data object returned by the wallet.
Required when |behavior| is "respond", forbidden otherwise.
- Tejas Dhagawkar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tejas Dhagawkar (Gerrit)
Tejas Dhagawkar added 2 comments![]( "Open in Gerrit")
const base::Value* response_value = bidi_params->Find("response");
if (response_value) {
if (response_value->is_string()) {
command_params.Set("response", response_value->GetString());
} else if (response_value->is_dict() || response_value->is_list()) {
std::string response_str;
if (base::JSONWriter::Write(*response_value, &response_str)) {
command_params.Set("response", response_str);
} else {
return Status(kInvalidArgument, "Unable to serialize response");
}
} else {
return Status(kInvalidArgument, "Invalid response type");
}
}
We need to test this part. Need unit tests
Related details
- Anushka Kulkarni
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. |
Tejas Dhagawkar (Gerrit)
Tejas Dhagawkar added 2 comments![]( "Open in Gerrit")
EXPECT_EQ(crdtp::DispatchCode::SERVER_ERROR, response.Code());How come this is a server error instead of INVALID_PARAMS
handler_->SetVirtualWalletBehavior("frobnicate", "openid4vp", "{}");Ideally it should say invalid behavior
Related details
- Anushka Kulkarni
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. |
Anushka Kulkarni (Gerrit)
Anushka Kulkarni added 4 comments![]( "Open in Gerrit")
const base::Value* response_value = bidi_params->Find("response");
if (response_value) {
if (response_value->is_string()) {
command_params.Set("response", response_value->GetString());
} else if (response_value->is_dict() || response_value->is_list()) {
std::string response_str;
if (base::JSONWriter::Write(*response_value, &response_str)) {
command_params.Set("response", response_str);
} else {
return Status(kInvalidArgument, "Unable to serialize response");
}
} else {
return Status(kInvalidArgument, "Invalid response type");
}
}
We need to test this part. Need unit tests
Done
Add DCHECK here for wallet
Done
EXPECT_EQ(crdtp::DispatchCode::SERVER_ERROR, response.Code());How come this is a server error instead of INVALID_PARAMS
Good point. You're right — from the client's viewpoint, "no frame host" is a usage error fixable on their side, not an internal browser failure. The BiDi layer guarantees correct routing via BrowsingContextStorage, so this branch only fires for raw-CDP clients that issued the command before attaching to a target. Switching to INVALID_PARAMS with a clearer message. Updated the corresponding unit test as well.
handler_->SetVirtualWalletBehavior("frobnicate", "openid4vp", "{}");Ideally it should say invalid behavior
- Tejas Dhagawkar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |