Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2596393002:
Update IPC message handling for SecurityKeyIpcClient class (Closed)
DescriptionUpdate IPC message handling for SecurityKeyIpcClient class
Per discussion with DCheng, we should not be DCHECK'ing if we receive
the connection IPC message out of order or when not expected. This
change will now call the error callback if this situation occurs which
will gracefully close the IPC connection. I've also added unit tests
for these scenarios.
BUG=675818
Committed: https://crrev.com/d383cc3285b2d20ed583d74a50f6dc019dc8bb3a
Cr-Commit-Position: refs/heads/master@{#440563}
Patch Set 1 #Patch Set 2 : Removing unnecessary patchset dependency #
Total comments: 4
Patch Set 3 : Addressing CR Feedback #
Messages
Total messages: 24 (18 generated)
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
joedow@chromium.org changed reviewers: + dcheng@chromium.org, sergeyu@chromium.org
PTAL!
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM https://codereview.chromium.org/2596393002/diff/20001/remoting/host/security_... File remoting/host/security_key/security_key_ipc_client.cc (right): https://codereview.chromium.org/2596393002/diff/20001/remoting/host/security_... remoting/host/security_key/security_key_ipc_client.cc:157: } else { nit: It would be a bit more readable if you handle the error case and remove else: if (!connected_callback_) { LOG(ERROR)... ... return; } base::ResetAndReturn(&connected_callback_).Run(/*connection_usable=*/true); https://codereview.chromium.org/2596393002/diff/20001/remoting/host/security_... remoting/host/security_key/security_key_ipc_client.cc:159: if (!connection_error_callback_.is_null()) { if (connection_error_callback_)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! https://codereview.chromium.org/2596393002/diff/20001/remoting/host/security_... File remoting/host/security_key/security_key_ipc_client.cc (right): https://codereview.chromium.org/2596393002/diff/20001/remoting/host/security_... remoting/host/security_key/security_key_ipc_client.cc:157: } else { On 2016/12/22 23:01:36, Sergey Ulanov wrote: > nit: It would be a bit more readable if you handle the error case and remove > else: > if (!connected_callback_) { > LOG(ERROR)... > ... > return; > } > > base::ResetAndReturn(&connected_callback_).Run(/*connection_usable=*/true); Done. https://codereview.chromium.org/2596393002/diff/20001/remoting/host/security_... remoting/host/security_key/security_key_ipc_client.cc:159: if (!connection_error_callback_.is_null()) { On 2016/12/22 23:01:36, Sergey Ulanov wrote: > if (connection_error_callback_) I've updated this line and the rest of the file to use that style.
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by joedow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2596393002/#ps40001 (title: "Addressing CR Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482453186084330,
"parent_rev": "b45d4c132af4162b282c8405e1ce890394a661cd", "commit_rev":
"2754eaf6dd10ab898baecfccaf8c456339849073"}
Message was sent while issue was closed.
Description was changed from ========== Update IPC message handling for SecurityKeyIpcClient class Per discussion with DCheng, we should not be DCHECK'ing if we receive the connection IPC message out of order or when not expected. This change will now call the error callback if this situation occurs which will gracefully close the IPC connection. I've also added unit tests for these scenarios. BUG=675818 ========== to ========== Update IPC message handling for SecurityKeyIpcClient class Per discussion with DCheng, we should not be DCHECK'ing if we receive the connection IPC message out of order or when not expected. This change will now call the error callback if this situation occurs which will gracefully close the IPC connection. I've also added unit tests for these scenarios. BUG=675818 Review-Url: https://codereview.chromium.org/2596393002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Update IPC message handling for SecurityKeyIpcClient class Per discussion with DCheng, we should not be DCHECK'ing if we receive the connection IPC message out of order or when not expected. This change will now call the error callback if this situation occurs which will gracefully close the IPC connection. I've also added unit tests for these scenarios. BUG=675818 Review-Url: https://codereview.chromium.org/2596393002 ========== to ========== Update IPC message handling for SecurityKeyIpcClient class Per discussion with DCheng, we should not be DCHECK'ing if we receive the connection IPC message out of order or when not expected. This change will now call the error callback if this situation occurs which will gracefully close the IPC connection. I've also added unit tests for these scenarios. BUG=675818 Committed: https://crrev.com/d383cc3285b2d20ed583d74a50f6dc019dc8bb3a Cr-Commit-Position: refs/heads/master@{#440563} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d383cc3285b2d20ed583d74a50f6dc019dc8bb3a Cr-Commit-Position: refs/heads/master@{#440563} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
