Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(558)

Issue 353713005: Implements new, more robust design for communicating between SSLConnectJobs. (Closed)

Created:
6 years, 6 months ago by mshelley
Modified:
6 years, 4 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

This CL is a better implementation of https://codereview.chromium.org/328903004/ . It allows SSLConnectJob waiting even in the case of false start. Additionally, it groups all of the functions for communicating between SSLConnectJobs into one object, the SSLConnectJobMessenger. R=mmenke@chromium.org,rsleevi@chromium.org,wtc@chromium.org TBR=mek@chromium.org,asargent@chormium.org,thestig@chromium.org BUG=398967 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287408

Patch Set 1 : Implements new, more robust design for communicating between SSLConectJobs. #

Total comments: 63

Patch Set 2 : Fixed logical errors regarding connecting sockets, made cache communication more consistent, and ch… #

Patch Set 3 : Adds test confirming that jobs wait for the leader to connect before connecting. #

Patch Set 4 : Added a unit test for the case in which the leader job fails. #

Patch Set 5 : Added checks to determine if false start connections fail, and moved location of enable_job_waiting… #

Total comments: 92

Patch Set 6 : Changed the structure of pending_sockets_and_callbacks_ and made cl compatible with nss. #

Patch Set 7 : Changed the structure of pending_sockets_and_callbacks_ and made cl compatible with nss. (Fixed one… #

Total comments: 14

Patch Set 8 : Changed location of enable_connect_job_waiting_ and added documentation. #

Total comments: 95

Patch Set 9 : Implemented better memory management and callback handling. #

Total comments: 61

Patch Set 10 : Redesigned tests and fixed various other issues. #

Total comments: 28

Patch Set 11 : Moved OnHandshakeFailureCheck and fixed comments. #

Patch Set 12 : Fixed error in SSLSessionIsInCache. #

Total comments: 95

Patch Set 13 : Fixed comment issues, addressed case where session is NULL #

Total comments: 42

Patch Set 14 : Changed Connect to use a state loop, and moved location of OnHandshakeFailure calls. #

Total comments: 24

Patch Set 15 : Accidentally compiled w/o use_openssl=true before; fixed compiler errors #

Total comments: 33

Patch Set 16 : Combined OnSocketFailure/Success callbacks and Added tests. #

Patch Set 17 : Fixed comment I missed in the last patch. #

Total comments: 47

Patch Set 18 : Added a new state to SSLClientSocket Connect, fixed various comment issues. #

Total comments: 40

Patch Set 19 : Restructured Connect for MockSSLClientSockets, fixed tests. #

Patch Set 20 : Rewrote the MockSSLClientSocket connect state loop to use the typical Chrome structure & fixed bugs… #

Total comments: 30

Patch Set 21 : Reordered MockSSLClientSocket methods, fixed other comments #

Patch Set 22 : Prevent certain tests from running when USE_OPENSSL is not defined. #

Total comments: 26

Patch Set 23 : Updated tests for client sockets to confirm use of completion callback and switched messenger to us… #

Total comments: 8

Patch Set 24 : Updated SSLClientSocket tests & fixed bug in SSLSessionCacheOpenSSL #

Total comments: 31

Patch Set 25 : Rebase #

Patch Set 26 : Moved tests back to ssl_client_socket_unittest.cc, fixed various other issues. #

Total comments: 46

Patch Set 27 : Removed additional arg from GetNextProto (was added in by rebase (?)) and fixed other small bugs. #

Total comments: 16

Patch Set 28 : Removed test from SSLClientSocketOpenSSL, removed false start test, inlined CreateAndConnectSSLClie… #

Total comments: 6

Patch Set 29 : Fixed nits from previous patchset. #

Total comments: 4

Patch Set 30 : Fixed typo & removed CapturingNetLog arg. #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+1314 lines, -151 lines) Patch
M chrome/browser/extensions/api/socket/tls_socket_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/io_thread.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/io_thread.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 3 chunks +5 lines, -0 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +4 lines, -0 lines 0 comments Download
M net/http/http_network_session.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +1 line, -0 lines 0 comments Download
M net/http/http_network_session.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 2 chunks +4 lines, -3 lines 1 comment Download
M net/http/http_network_transaction_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +3 lines, -1 line 0 comments Download
M net/http/http_proxy_client_socket_pool_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +1 line, -0 lines 0 comments Download
M net/http/http_stream_factory_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 2 chunks +6 lines, -4 lines 0 comments Download
M net/socket/client_socket_pool_manager_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 2 chunks +2 lines, -0 lines 0 comments Download
M net/socket/client_socket_pool_manager_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 5 chunks +20 lines, -15 lines 0 comments Download
M net/socket/socket_test_util.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 8 chunks +51 lines, -3 lines 3 comments Download
M net/socket/socket_test_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 8 chunks +122 lines, -29 lines 1 comment Download
M net/socket/ssl_client_socket.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 2 chunks +43 lines, -1 line 0 comments Download
M net/socket/ssl_client_socket.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 2 chunks +11 lines, -0 lines 0 comments Download
M net/socket/ssl_client_socket_nss.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +3 lines, -0 lines 0 comments Download
M net/socket/ssl_client_socket_nss.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +9 lines, -0 lines 0 comments Download
M net/socket/ssl_client_socket_openssl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 26 3 chunks +13 lines, -0 lines 0 comments Download
M net/socket/ssl_client_socket_openssl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 13 chunks +63 lines, -13 lines 0 comments Download
M net/socket/ssl_client_socket_pool.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 9 chunks +125 lines, -39 lines 0 comments Download
M net/socket/ssl_client_socket_pool.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 26 18 chunks +181 lines, -37 lines 1 comment Download
M net/socket/ssl_client_socket_pool_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 5 chunks +462 lines, -1 line 0 comments Download
M net/socket/ssl_client_socket_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 3 chunks +88 lines, -1 line 0 comments Download
M net/socket/ssl_session_cache_openssl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +11 lines, -0 lines 0 comments Download
M net/socket/ssl_session_cache_openssl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 6 chunks +82 lines, -4 lines 0 comments Download

Messages

Total messages: 74 (0 generated)
mshelley1
This is the first version of the new design that Ryan and I discussed. I ...
6 years, 6 months ago (2014-06-25 18:09:56 UTC) #1
Ryan Sleevi
Quick drive-by comments; I haven't really reviewed in depth yet. https://codereview.chromium.org/353713005/diff/20001/net/socket/ssl_client_socket_openssl.h File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/353713005/diff/20001/net/socket/ssl_client_socket_openssl.h#newcode62 ...
6 years, 6 months ago (2014-06-26 01:47:17 UTC) #2
wtc
Review comments on patch set 1: I haven't reviewed ssl_client_socket_pool.*. This version seems harder to ...
6 years, 6 months ago (2014-06-27 00:36:50 UTC) #3
mshelley
https://codereview.chromium.org/353713005/diff/20001/net/socket/ssl_client_socket.h File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/353713005/diff/20001/net/socket/ssl_client_socket.h#newcode87 net/socket/ssl_client_socket.h:87: // The cache key consists of a host_and_poart concatenated ...
6 years, 5 months ago (2014-07-01 02:35:24 UTC) #4
mshelley
6 years, 5 months ago (2014-07-07 18:33:41 UTC) #5
wtc
Review comments on patch set 5: I need to review this carefully again. It seems ...
6 years, 5 months ago (2014-07-08 01:25:44 UTC) #6
mmenke
A couple quick comments, still need to get up to speed on the code. Is ...
6 years, 5 months ago (2014-07-08 21:02:05 UTC) #7
Ryan Sleevi
https://codereview.chromium.org/353713005/diff/20001/net/socket/ssl_client_socket_pool.h File net/socket/ssl_client_socket_pool.h (right): https://codereview.chromium.org/353713005/diff/20001/net/socket/ssl_client_socket_pool.h#newcode113 net/socket/ssl_client_socket_pool.h:113: SSLPendingSocketsAndCallbacks(SSLPendingSocketsAndCallbacks& ref) { On 2014/07/01 02:35:22, mshelley wrote: > ...
6 years, 5 months ago (2014-07-08 22:34:01 UTC) #8
mshelley
On 2014/07/08 22:34:01, Ryan Sleevi wrote: > https://codereview.chromium.org/353713005/diff/20001/net/socket/ssl_client_socket_pool.h > File net/socket/ssl_client_socket_pool.h (right): > > https://codereview.chromium.org/353713005/diff/20001/net/socket/ssl_client_socket_pool.h#newcode113 ...
6 years, 5 months ago (2014-07-09 19:38:58 UTC) #9
mshelley
https://codereview.chromium.org/353713005/diff/160001/net/socket/socket_test_util.cc File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/353713005/diff/160001/net/socket/socket_test_util.cc#newcode1455 net/socket/socket_test_util.cc:1455: return false; On 2014/07/08 01:25:41, wtc wrote: > > ...
6 years, 5 months ago (2014-07-09 19:51:03 UTC) #10
Ryan Sleevi
With fresh eyes comes an unfortunate set of new comments of things that I've missed ...
6 years, 5 months ago (2014-07-10 19:51:06 UTC) #11
mshelley
https://codereview.chromium.org/353713005/diff/240001/net/socket/socket_test_util.h File net/socket/socket_test_util.h (right): https://codereview.chromium.org/353713005/diff/240001/net/socket/socket_test_util.h#newcode338 net/socket/socket_test_util.h:338: bool is_in_session_cache_; On 2014/07/10 19:51:05, Ryan Sleevi wrote: > ...
6 years, 5 months ago (2014-07-10 22:07:15 UTC) #12
wtc
Review comments on patch set 8: I decided to skip the tests in this review ...
6 years, 5 months ago (2014-07-11 00:48:56 UTC) #13
mshelley
https://codereview.chromium.org/353713005/diff/260001/net/socket/ssl_client_socket.h File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/353713005/diff/260001/net/socket/ssl_client_socket.h#newcode84 net/socket/ssl_client_socket.h:84: // Format a unique key for the SSL session ...
6 years, 5 months ago (2014-07-11 23:26:28 UTC) #14
Ryan Sleevi
I have mostly tried to focus on the tests, and haven't reviewed as much of ...
6 years, 5 months ago (2014-07-12 00:12:16 UTC) #15
wtc
Review comments on patch set 9: https://codereview.chromium.org/353713005/diff/300001/net/socket/socket_test_util.h File net/socket/socket_test_util.h (right): https://codereview.chromium.org/353713005/diff/300001/net/socket/socket_test_util.h#newcode711 net/socket/socket_test_util.h:711: virtual void SetIsLeader() ...
6 years, 5 months ago (2014-07-15 19:28:01 UTC) #16
mshelley
https://codereview.chromium.org/353713005/diff/300001/net/socket/socket_test_util.h File net/socket/socket_test_util.h (right): https://codereview.chromium.org/353713005/diff/300001/net/socket/socket_test_util.h#newcode341 net/socket/socket_test_util.h:341: bool is_in_session_cache_; On 2014/07/12 00:12:15, Ryan Sleevi wrote: > ...
6 years, 5 months ago (2014-07-17 00:28:46 UTC) #17
mmenke
Mostly a bunch of nits, still have a lot of code to go through. https://codereview.chromium.org/353713005/diff/360001/net/socket/ssl_client_socket.h ...
6 years, 5 months ago (2014-07-17 19:21:32 UTC) #18
mshelley
https://codereview.chromium.org/353713005/diff/360001/net/socket/ssl_client_socket.h File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/353713005/diff/360001/net/socket/ssl_client_socket.h#newcode87 net/socket/ssl_client_socket.h:87: // TODO(mshelley) this method will be deleted soon. On ...
6 years, 5 months ago (2014-07-17 21:12:49 UTC) #19
mmenke
https://codereview.chromium.org/353713005/diff/360001/net/socket/ssl_client_socket_pool.cc File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/353713005/diff/360001/net/socket/ssl_client_socket_pool.cc#newcode276 net/socket/ssl_client_socket_pool.cc:276: rv = DoCreateSSLSocket(); On 2014/07/17 21:12:49, mshelley wrote: > ...
6 years, 5 months ago (2014-07-17 21:27:55 UTC) #20
wtc
Patch set 12 LGTM. 1. I didn't have time to review the test code, so ...
6 years, 5 months ago (2014-07-18 01:17:16 UTC) #21
wtc
More review comments on patch set 12: I reviewed the test code in socket_test_util.*. I ...
6 years, 5 months ago (2014-07-18 15:39:17 UTC) #22
mshelley
https://codereview.chromium.org/353713005/diff/400001/chrome/common/chrome_switches.cc File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/353713005/diff/400001/chrome/common/chrome_switches.cc#newcode618 chrome/common/chrome_switches.cc:618: const char kEnableSSLConnectJobWaiting[] = "enable-ssl-connect-job-waiting"; On 2014/07/18 01:17:13, wtc ...
6 years, 5 months ago (2014-07-18 21:08:33 UTC) #23
Ryan Sleevi
https://codereview.chromium.org/353713005/diff/460001/net/socket/socket_test_util.cc File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/353713005/diff/460001/net/socket/socket_test_util.cc#newcode1369 net/socket/socket_test_util.cc:1369: if (!data_->blocked_in_connect) { When would this ever be true ...
6 years, 5 months ago (2014-07-18 22:01:18 UTC) #24
mshelley
https://codereview.chromium.org/353713005/diff/460001/net/socket/socket_test_util.cc File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/353713005/diff/460001/net/socket/socket_test_util.cc#newcode1383 net/socket/socket_test_util.cc:1383: return ERR_IO_PENDING; On 2014/07/18 22:01:16, Ryan Sleevi wrote: > ...
6 years, 5 months ago (2014-07-21 23:00:09 UTC) #25
wtc
Review comments on patch set 14: I can go over my review comments in person ...
6 years, 5 months ago (2014-07-22 02:25:22 UTC) #26
mmenke
https://codereview.chromium.org/353713005/diff/520001/net/socket/socket_test_util.h File net/socket/socket_test_util.h (right): https://codereview.chromium.org/353713005/diff/520001/net/socket/socket_test_util.h#newcode339 net/socket/socket_test_util.h:339: bool should_block_in_connect; nit: Maybe in -> on? https://codereview.chromium.org/353713005/diff/520001/net/socket/ssl_client_socket_openssl.cc File ...
6 years, 5 months ago (2014-07-22 16:24:12 UTC) #27
mmenke
More test suggestions - noticed this while looking at one of the followups CLs. https://codereview.chromium.org/353713005/diff/520001/net/socket/ssl_client_socket_pool.cc ...
6 years, 5 months ago (2014-07-22 18:03:54 UTC) #28
wtc
Review comments on patch set 15: mostly responding to Matt's comments. https://codereview.chromium.org/353713005/diff/520001/net/socket/ssl_client_socket_openssl.cc File net/socket/ssl_client_socket_openssl.cc (right): ...
6 years, 5 months ago (2014-07-23 02:03:27 UTC) #29
mshelley
https://codereview.chromium.org/353713005/diff/500001/net/socket/socket_test_util.h File net/socket/socket_test_util.h (right): https://codereview.chromium.org/353713005/diff/500001/net/socket/socket_test_util.h#newcode989 net/socket/socket_test_util.h:989: // this method. On 2014/07/22 02:25:21, wtc wrote: > ...
6 years, 5 months ago (2014-07-23 03:49:57 UTC) #30
wtc
Review comments on patch set 17: The changes to the non-testing code are good in ...
6 years, 5 months ago (2014-07-23 22:53:33 UTC) #31
mshelley
https://codereview.chromium.org/353713005/diff/560001/chrome/browser/chrome_browser_main.cc File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/353713005/diff/560001/chrome/browser/chrome_browser_main.cc#newcode136 chrome/browser/chrome_browser_main.cc:136: #include "net/socket/ssl_client_socket_pool.h" On 2014/07/23 22:53:32, wtc wrote: > > ...
6 years, 5 months ago (2014-07-24 20:37:49 UTC) #32
Ryan Sleevi
Looking so much better! A few comments, but if things are done a certain way ...
6 years, 5 months ago (2014-07-25 01:36:37 UTC) #33
Ryan Sleevi
https://codereview.chromium.org/353713005/diff/580001/net/socket/ssl_client_socket_pool.cc File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/353713005/diff/580001/net/socket/ssl_client_socket_pool.cc#newcode121 net/socket/ssl_client_socket_pool.cc:121: // TODO(mshelley): Both of these callbacks will use WeakPtr ...
6 years, 5 months ago (2014-07-25 21:31:22 UTC) #34
Ryan Sleevi
6 years, 5 months ago (2014-07-25 21:31:26 UTC) #35
Ryan Sleevi
6 years, 5 months ago (2014-07-25 21:31:29 UTC) #36
mshelley
https://codereview.chromium.org/353713005/diff/580001/net/socket/socket_test_util.cc File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/353713005/diff/580001/net/socket/socket_test_util.cc#newcode770 net/socket/socket_test_util.cc:770: return true; On 2014/07/25 01:36:34, Ryan Sleevi wrote: > ...
6 years, 5 months ago (2014-07-26 00:58:28 UTC) #37
Ryan Sleevi
https://codereview.chromium.org/353713005/diff/580001/net/socket/socket_test_util.cc File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/353713005/diff/580001/net/socket/socket_test_util.cc#newcode770 net/socket/socket_test_util.cc:770: return true; On 2014/07/26 00:58:27, mshelley wrote: > On ...
6 years, 5 months ago (2014-07-26 02:22:32 UTC) #38
mshelley
6 years, 4 months ago (2014-07-29 17:48:51 UTC) #39
wtc
Patch set 20 LGTM. I have reviewed everything carefully, including the testing code. I suggest ...
6 years, 4 months ago (2014-07-29 20:59:35 UTC) #40
mshelley
https://codereview.chromium.org/353713005/diff/680001/net/socket/socket_test_util.cc File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/353713005/diff/680001/net/socket/socket_test_util.cc#newcode799 net/socket/socket_test_util.cc:799: void MockSSLClientSocket::RestartPausedConnect() { On 2014/07/29 20:59:33, wtc wrote: > ...
6 years, 4 months ago (2014-07-29 21:40:13 UTC) #41
wtc
Patch set 21 LGTM. Please edit the CL's description 1. Add line breaks. 2. Add ...
6 years, 4 months ago (2014-07-29 22:15:31 UTC) #42
Ryan Sleevi
Yes, to echo wtc, we want to make sure there is a BUG= filed. I ...
6 years, 4 months ago (2014-07-29 23:19:23 UTC) #43
mshelley
https://codereview.chromium.org/353713005/diff/710001/net/socket/socket_test_util.cc File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/353713005/diff/710001/net/socket/socket_test_util.cc#newcode1471 net/socket/socket_test_util.cc:1471: DCHECK(!connect_callback_.is_null()); On 2014/07/29 23:19:22, Ryan Sleevi wrote: > You ...
6 years, 4 months ago (2014-07-30 18:33:55 UTC) #44
wtc
Patch set 23 LGTM. https://codereview.chromium.org/353713005/diff/710001/net/socket/socket_test_util.cc File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/353713005/diff/710001/net/socket/socket_test_util.cc#newcode1471 net/socket/socket_test_util.cc:1471: DCHECK(!connect_callback_.is_null()); On 2014/07/29 23:19:22, Ryan ...
6 years, 4 months ago (2014-07-30 21:56:58 UTC) #45
mshelley
https://codereview.chromium.org/353713005/diff/730001/net/socket/ssl_client_socket_openssl_unittest.cc File net/socket/ssl_client_socket_openssl_unittest.cc (right): https://codereview.chromium.org/353713005/diff/730001/net/socket/ssl_client_socket_openssl_unittest.cc#newcode212 net/socket/ssl_client_socket_openssl_unittest.cc:212: bool ran_completion_callback_; On 2014/07/30 21:56:57, wtc wrote: > > ...
6 years, 4 months ago (2014-07-31 00:51:21 UTC) #46
wtc
Review comments on patch set 24: I will review ssl_client_socket_openssl_unittest.cc tomorrow. Here are some suggested ...
6 years, 4 months ago (2014-07-31 02:02:25 UTC) #47
mshelley
https://codereview.chromium.org/353713005/diff/770001/net/socket/socket_test_util.cc File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/353713005/diff/770001/net/socket/socket_test_util.cc#newcode1524 net/socket/socket_test_util.cc:1524: reached_connect_ = true; On 2014/07/31 02:02:24, wtc wrote: > ...
6 years, 4 months ago (2014-07-31 20:11:38 UTC) #48
wtc
Review comments on patch set 26: 1. There seem to be several merge errors related ...
6 years, 4 months ago (2014-07-31 23:05:25 UTC) #49
mshelley
https://codereview.chromium.org/353713005/diff/810001/net/cert/multi_threaded_cert_verifier.cc File net/cert/multi_threaded_cert_verifier.cc (left): https://codereview.chromium.org/353713005/diff/810001/net/cert/multi_threaded_cert_verifier.cc#oldcode619 net/cert/multi_threaded_cert_verifier.cc:619: On 2014/07/31 23:05:24, wtc wrote: > > Please add ...
6 years, 4 months ago (2014-08-02 23:59:15 UTC) #50
wtc
Patch set 27 LGTM. Please make the changes I suggested below and upload a new ...
6 years, 4 months ago (2014-08-03 01:49:11 UTC) #51
mshelley
https://codereview.chromium.org/353713005/diff/850001/net/socket/socket_test_util.h File net/socket/socket_test_util.h (right): https://codereview.chromium.org/353713005/diff/850001/net/socket/socket_test_util.h#newcode734 net/socket/socket_test_util.h:734: bool reached_connect_; On 2014/08/03 01:49:10, wtc wrote: > > ...
6 years, 4 months ago (2014-08-03 23:37:10 UTC) #52
wtc
Patch set 28 LGTM. Please fix the following nits, upload a new patch set, and ...
6 years, 4 months ago (2014-08-04 00:04:17 UTC) #53
mshelley
The CQ bit was checked by mshelley@chromium.org
6 years, 4 months ago (2014-08-04 00:18:31 UTC) #54
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mshelley@chromium.org/353713005/930001
6 years, 4 months ago (2014-08-04 00:19:02 UTC) #55
wtc
Patch set 29 LGTM. You need to fix a compilation error caused by a typo. ...
6 years, 4 months ago (2014-08-04 02:39:47 UTC) #56
mshelley
The CQ bit was checked by mshelley@chromium.org
6 years, 4 months ago (2014-08-04 02:50:56 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mshelley@chromium.org/353713005/950001
6 years, 4 months ago (2014-08-04 02:51:38 UTC) #58
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium.linux ...
6 years, 4 months ago (2014-08-04 04:36:57 UTC) #59
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 4 months ago (2014-08-04 04:40:03 UTC) #60
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/1772)
6 years, 4 months ago (2014-08-04 04:40:04 UTC) #61
wtc
Patch set 30 LGTM. Patch set 30 could not be committed because of this presubmit ...
6 years, 4 months ago (2014-08-04 15:35:34 UTC) #62
mshelley
mek@ and asargent@, can you please review the changes in chrome/browser/extensions/api/socket/tls_socket_unittest.cc made due to changes ...
6 years, 4 months ago (2014-08-04 16:41:59 UTC) #63
Marijn Kruisselbrink
chrome/browser/extensions/api/socket/tls_socket_unittest.cc lgtm
6 years, 4 months ago (2014-08-04 16:46:30 UTC) #64
mmenke
Sorry for disappearing. Not need to delay landing for my sake, but one question... Looks ...
6 years, 4 months ago (2014-08-04 17:55:38 UTC) #65
mshelley
On 2014/08/04 17:55:38, mmenke wrote: > Sorry for disappearing. Not need to delay landing for ...
6 years, 4 months ago (2014-08-04 18:02:53 UTC) #66
mmenke
On 2014/08/04 18:02:53, mshelley wrote: > On 2014/08/04 17:55:38, mmenke wrote: > > Sorry for ...
6 years, 4 months ago (2014-08-04 18:04:08 UTC) #67
mshelley
On 2014/08/04 18:04:08, mmenke wrote: > On 2014/08/04 18:02:53, mshelley wrote: > > On 2014/08/04 ...
6 years, 4 months ago (2014-08-04 18:10:43 UTC) #68
Lei Zhang
chrome/ lgtm
6 years, 4 months ago (2014-08-04 18:32:04 UTC) #69
wtc
The CQ bit was checked by wtc@chromium.org
6 years, 4 months ago (2014-08-04 20:30:04 UTC) #70
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mshelley@chromium.org/353713005/950001
6 years, 4 months ago (2014-08-04 20:32:23 UTC) #71
commit-bot: I haz the power
Change committed as 287408
6 years, 4 months ago (2014-08-05 00:02:21 UTC) #72
Lei Zhang
https://codereview.chromium.org/353713005/diff/950001/net/http/http_network_session.cc File net/http/http_network_session.cc (right): https://codereview.chromium.org/353713005/diff/950001/net/http/http_network_session.cc#newcode71 net/http/http_network_session.cc:71: ignore_certificate_errors(false), You forgot to initialize |enable_ssl_connect_job_waiting| here. Now a ...
6 years, 4 months ago (2014-08-05 02:37:45 UTC) #73
wtc
6 years, 4 months ago (2014-08-06 19:01:27 UTC) #74
Message was sent while issue was closed.
Mackenzie: now that this CL has been committed, please make the following
cleanup changes. The ones marked with "Nit" are optional.

https://codereview.chromium.org/353713005/diff/950001/net/socket/socket_test_...
File net/socket/socket_test_util.cc (right):

https://codereview.chromium.org/353713005/diff/950001/net/socket/socket_test_...
net/socket/socket_test_util.cc:1352: next_connect_state_ =
STATE_TRANSPORT_CONNECT;

Here we should set next_connect_state_ to STATE_SSL_CONNECT instead.

https://codereview.chromium.org/353713005/diff/950001/net/socket/socket_test_...
File net/socket/socket_test_util.h (right):

https://codereview.chromium.org/353713005/diff/950001/net/socket/socket_test_...
net/socket/socket_test_util.h:337: // Indicates that the socket should block in
the Connect method.

Nit: I suggest changing "block" to "pause". This wording implies a link to the
related RestartPausedConnect() method.

https://codereview.chromium.org/353713005/diff/950001/net/socket/socket_test_...
net/socket/socket_test_util.h:338: bool should_block_on_connect;

Nit: similarly, rename this member "should_pause_on_connect".

https://codereview.chromium.org/353713005/diff/950001/net/socket/socket_test_...
net/socket/socket_test_util.h:994: STATE_TRANSPORT_CONNECT_COMPLETE,

Please delete the STATE_TRANSPORT_CONNECT and STATE_TRANSPORT_CONNECT_COMPLETE
states and the DoTransportConnect() and DoTransportConnectComplete() methods.
The transport socket should be already connected when it is passed to the
SSLClientSocket constructor.

Powered by Google App Engine
This is Rietveld 408576698