[remoting] Make SessionOptions strongly typed [chromium/src : main]
Yuwei Huang (Gerrit)
Yuwei Huang added 2 comments![]( "Open in Gerrit")
{"Enable-Sck-Capturer", nullptr},Is there a reason you need to map unsupported keys to `nullptr`? If you remove these then you could also remove null checks and simplify the code.
return std::nullopt;In the old logic, we unsupported keys are technically just ignored. Is there a reason you just short-circuit here and return nullopt? I guess allowing it to be half-parsed could be important for compatibility.
Thinking more about this, maybethis function should not be nullable. In the worse case, it may just return an empty `SessionOptions`. WDYT?
Related details
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. |
Yuwei Huang (Gerrit)
Yuwei Huang added 3 comments![]( "Open in Gerrit")
std::ostream& operator<<(std::ostream& os, const std::optional<T>& opt) {It is generally discouraged to define operators for `std::` types (like `std::optional`), even locally in an anonymous namespace, as it can occasionally lead to ambiguous overload errors if a future header eventually introduces one.
Since this is just for formatting fields, you could format them directly inside `SessionOptions::operator<<` (or use a named helper function like `PrintOptional`) to avoid defining a global-like operator for a standard type.
Is there a reason you need to map unsupported keys to `nullptr`? If you remove these then you could also remove null checks and simplify the code.
Done
In the old logic, we unsupported keys are technically just ignored. Is there a reason you just short-circuit here and return nullopt? I guess allowing it to be half-parsed could be important for compatibility.
Thinking more about this, maybethis function should not be nullable. In the worse case, it may just return an empty `SessionOptions`. WDYT?
Done
Related details
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. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
2 comments
std::ostream& operator<<(std::ostream& os, const std::optional<T>& opt) {It is generally discouraged to define operators for `std::` types (like `std::optional`), even locally in an anonymous namespace, as it can occasionally lead to ambiguous overload errors if a future header eventually introduces one.
Since this is just for formatting fields, you could format them directly inside `SessionOptions::operator<<` (or use a named helper function like `PrintOptional`) to avoid defining a global-like operator for a standard type.
Done
Related details
- Joe Downing
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Downing (Gerrit)
Joe Downing voted and added 4 comments![]( "Open in Gerrit")
Votes added by Joe Downing
| Code-Review | +1 |
4 comments
// Encoder speed for VP9 video codec.
// Corresponding option key: Vp9-Encoder-Speed
std::optional<int> vp9_encoder_speed;Not related to the structure of this file but I feel like a fair number of this options are now the default behavior and could be removed (allow_dirctx, enable_sck, vp9_speed, av1_speed for sure). Please consider removing them in a follow-up CL.
const base::flat_map<std::string_view, std::optional<bool> SessionOptions::*>&optional: This is a mouthful, consider using a typedef for these long types.
LOG(WARNING) << "Unsupported session option key: " << key;If/when we remove support for the outdated session options above, we should probably 'retire' them so they don't trigger warnings.
dict.Set("Detect-Updated-Region", "true");Is it worth putting this in a constant file? It's not critical but we do it in several other places in the code for constants that are used in tests.
Related details
- Yuwei Huang
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. |