remoting: PeerSessionFactory [chromium/src : main]
Yuwei Huang (Gerrit)
Yuwei Huang added 6 comments![]( "Open in Gerrit")
PeerSessionFactory* peer_session_factory() const {Yuwei HuangIs this getter actually being used? If not, please remove it.
Done
std::make_shared<std::unique_ptr<protocol::ConnectionToClient>>(Yuwei HuangThis is a very weird pattern. Are you just trying to pass `ConnectionToClient` into the lambda?
There is the `Return(ByMove())` pattern as demonstrated in remoting/host/desktop_process_unittest.cc. Can it be used instead?
Done
.Times(testing::AtMost(1))Yuwei HuangThere is `using testing::AtMost` above, so just use `AtMost(1)` here.
Done
std::make_shared<std::unique_ptr<MockPeerSession>>(std::move(mock_peer));Yuwei HuangAgain. Very bizarre construct. See if `Return(ByMove())` works.
Done
// Factory implementation for creating `PeerSessionImpl` instances.Yuwei HuangShould probably say "Factory for creating..." and get rid of "implementation".
Done
context_->audio_task_runner(), /*pairing_registry=*/nullptr);Yuwei HuangLooks like you always pass `nullptr` for `pairing_registry` then later call `set_pairing_registry`. What about just getting rid of this argument?
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
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 2 comments![]( "Open in Gerrit")
Votes added by Joe Downing
| Code-Review | +1 |
2 comments
event_handler_ = event_handler;
client_jid_ = std::string(client_jid);Would it be useful to CHECK here that event_handler_ is null (meaning Start was called once) and that event_handler and client_jid are valid?
LOG_IF(WARNING, !get_ice_config_fetcher_cb_)
<< "Missing Ice Config Fetcher callback.";I know this code was copied but I'm trying to recall whether there is a valid production code use case where ice_config_fethcer_cb is null. IIRC we used to pass the IceConfig down from the website through the NMH but that code is gone since we always pass down an OAuth access token.
Actually that is the case and you just removed the constants. Can we require that get_ice_config_fetcher_cb_ is valid now (since we're close to branch point, feel free to do this in a follow-up if you want this CL to be a pure refactoring effort).
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. |