Better API contract for QuicCryptoClientStream::CryptoConnect() |
||||||
Issue descriptionThis is filed to follow up on Issue 700617 . QuicCryptoClientStream::CryptoConnect() can fail synchronously. This is unexpected and not documented. This function used to return a bool (crrev.com/fbf7d6b0052f55f5eca633b72c5bb2c5c1e04b9a). We could consider the following: (1) Document this function in QuicCryptoClientStreamBase. (2) Make the function return a bool so we can detect any synchronous error without resorting to an additional "connection()->connected()" check. (3) Make the function additionally take in a callback. This might allow us to simplify caller code and get rid of some delegate callbacks.
,
Mar 24 2017
,
Mar 28 2017
,
Apr 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e23ca12cab688382ed5a3e3350885bed0b9d39b4 commit e23ca12cab688382ed5a3e3350885bed0b9d39b4 Author: xunjieli <xunjieli@chromium.org> Date: Mon Apr 03 22:23:27 2017 Check callback is run in QuicChromiumClientSession's destructor Add a DCHECK to make sure |callback_| has been run in QuicChromiumClientSession's destructor. BUG= 704949 Review-Url: https://codereview.chromium.org/2797443002 Cr-Commit-Position: refs/heads/master@{#461558} [modify] https://crrev.com/e23ca12cab688382ed5a3e3350885bed0b9d39b4/net/quic/chromium/quic_chromium_client_session.cc
,
Apr 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/53b0eeafd14a0a5699ff8850df41ddef696caddc commit 53b0eeafd14a0a5699ff8850df41ddef696caddc Author: xunjieli <xunjieli@chromium.org> Date: Tue Apr 04 16:55:43 2017 Check callback is run in QuicStreamFactory::Job This CL adds a DCHECK in QuicStreamFactory::Job's destructor to make sure |callback_| has been run. This CL clears two related classes's callback instances so we only run them once. BUG= 704949 Review-Url: https://codereview.chromium.org/2794963002 Cr-Commit-Position: refs/heads/master@{#461753} [modify] https://crrev.com/53b0eeafd14a0a5699ff8850df41ddef696caddc/net/http/http_stream_factory_impl_job_controller_unittest.cc [modify] https://crrev.com/53b0eeafd14a0a5699ff8850df41ddef696caddc/net/quic/chromium/quic_stream_factory.cc
,
Apr 4 2017
Alright, I landed two small CLs to add DCHECKs as discussed. If we don't ever trigger them, they will at least make the assumptions obvious. I don't think I will get to the CryptoConnect() part. I will mark this one as Available.
,
Apr 5 2017
,
Apr 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0756f884163fc4b6db4c61d9a06148bec8a0cc20 commit 0756f884163fc4b6db4c61d9a06148bec8a0cc20 Author: zhongyi <zhongyi@chromium.org> Date: Thu Apr 06 23:15:49 2017 relnote: change QuicCryptoClientStreamBase::CryptoConnect API to return a boolean so that synchronous error can be detected. chromium: Remove QuicChromiumClientSession::ResumeCryptoConnect as well, which is no longer used. Merge internal change: 152398936 BUG= 704949 Review-Url: https://codereview.chromium.org/2806553004 Cr-Commit-Position: refs/heads/master@{#462671} [modify] https://crrev.com/0756f884163fc4b6db4c61d9a06148bec8a0cc20/net/quic/chromium/quic_chromium_client_session.cc [modify] https://crrev.com/0756f884163fc4b6db4c61d9a06148bec8a0cc20/net/quic/chromium/quic_chromium_client_session.h [modify] https://crrev.com/0756f884163fc4b6db4c61d9a06148bec8a0cc20/net/quic/core/quic_crypto_client_stream.cc [modify] https://crrev.com/0756f884163fc4b6db4c61d9a06148bec8a0cc20/net/quic/core/quic_crypto_client_stream.h [modify] https://crrev.com/0756f884163fc4b6db4c61d9a06148bec8a0cc20/net/quic/test_tools/mock_crypto_client_stream.cc [modify] https://crrev.com/0756f884163fc4b6db4c61d9a06148bec8a0cc20/net/quic/test_tools/mock_crypto_client_stream.h
,
Apr 6 2017
So I have changed a API a little bit per #1: documented the function in QuicCryptoClientStreamBase, and make the function returns a boolean to indicate whether the connection is still connected. As rch@ has suggested, we might want to rename CryptoConnect to StartCryptoConnect which sounds more suitable for this function.
,
Apr 7 2017
,
Apr 10 2017
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by rch@chromium.org
, Mar 24 2017