Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1445)

Issue 286790043: False start tweaks (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 2 months ago by mt
Modified:
7 years, 7 months ago
Reviewers:
wtc1, ekr-rietveld
Visibility:
Public.

Description

This adds the test for the bug we discussed (I'll cc you).

Patch Set 1 #

Total comments: 17

Patch Set 2 : Test refactoring, plus bug fix #

Patch Set 3 : Rebased #

Total comments: 3

Patch Set 4 : Filters refactored more cleanly #

Total comments: 26

Patch Set 5 : Now with a fix to ReadBytes #

Total comments: 6

Patch Set 6 : Fix commentary, case statement #

Total comments: 6

Patch Set 7 : Nit corrections #

Patch Set 8 : Don't wait on closed sockets #

Unified diffs Side-by-side diffs Delta from patch set Stats (+551 lines, -320 lines) Patch
M external_tests/common/gtest.mk View 1 chunk +0 lines, -1 line 0 comments Download
M external_tests/ssl_gtest/databuffer.h View 1 2 3 1 chunk +8 lines, -5 lines 0 comments Download
M external_tests/ssl_gtest/ssl_extension_unittest.cc View 1 2 3 2 chunks +101 lines, -98 lines 0 comments Download
M external_tests/ssl_gtest/ssl_loopback_unittest.cc View 1 2 3 4 4 chunks +116 lines, -4 lines 0 comments Download
M external_tests/ssl_gtest/ssl_skip_unittest.cc View 1 2 3 1 chunk +13 lines, -18 lines 0 comments Download
M external_tests/ssl_gtest/test_io.h View 1 2 3 1 chunk +11 lines, -3 lines 0 comments Download
M external_tests/ssl_gtest/test_io.cc View 1 2 3 1 chunk +16 lines, -5 lines 0 comments Download
M external_tests/ssl_gtest/tls_agent.h View 1 5 chunks +8 lines, -2 lines 0 comments Download
M external_tests/ssl_gtest/tls_agent.cc View 1 2 3 4 5 6 7 4 chunks +27 lines, -18 lines 0 comments Download
M external_tests/ssl_gtest/tls_connect.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M external_tests/ssl_gtest/tls_connect.cc View 1 2 3 4 1 chunk +9 lines, -4 lines 0 comments Download
M external_tests/ssl_gtest/tls_filter.h View 1 2 3 4 2 chunks +78 lines, -23 lines 0 comments Download
M external_tests/ssl_gtest/tls_filter.cc View 1 2 3 4 1 chunk +152 lines, -122 lines 0 comments Download
M lib/ssl/ssl3con.c View 1 2 3 4 5 6 3 chunks +6 lines, -7 lines 0 comments Download
M lib/ssl/ssl3gthr.c View 1 chunk +1 line, -1 line 0 comments Download
M lib/ssl/sslimpl.h View 1 2 3 4 5 1 chunk +4 lines, -9 lines 0 comments Download

Messages

Total messages: 16
ekr-rietveld
https://codereview.appspot.com/286790043/diff/1/external_tests/common/gtest.mk File external_tests/common/gtest.mk (left): https://codereview.appspot.com/286790043/diff/1/external_tests/common/gtest.mk#oldcode8 external_tests/common/gtest.mk:8: include ../../cmd/platrules.mk Why was this change needed? https://codereview.appspot.com/286790043/diff/1/external_tests/ssl_gtest/ssl_loopback_unittest.cc File ...
8 years, 2 months ago (2016-01-17 22:02:10 UTC) #1
mt
The changes to the next round are going to look pretty large, but I think ...
8 years, 2 months ago (2016-01-18 06:40:20 UTC) #2
mt
Updated, I pulled in what looked like trunk changes accidentally in the last one, ignore ...
8 years, 2 months ago (2016-01-18 06:49:21 UTC) #3
ekr-rietveld
Partial comments. https://codereview.appspot.com/286790043/diff/1/external_tests/common/gtest.mk File external_tests/common/gtest.mk (left): https://codereview.appspot.com/286790043/diff/1/external_tests/common/gtest.mk#oldcode8 external_tests/common/gtest.mk:8: include ../../cmd/platrules.mk On 2016/01/18 06:40:20, mt wrote: ...
8 years, 2 months ago (2016-01-18 23:39:09 UTC) #4
mt
It turns out that the commented code needed to be removed, though I had to ...
8 years, 2 months ago (2016-01-19 04:04:01 UTC) #5
ekr-rietveld
https://codereview.appspot.com/286790043/diff/60001/external_tests/ssl_gtest/ssl_extension_unittest.cc File external_tests/ssl_gtest/ssl_extension_unittest.cc (left): https://codereview.appspot.com/286790043/diff/60001/external_tests/ssl_gtest/ssl_extension_unittest.cc#oldcode236 external_tests/ssl_gtest/ssl_extension_unittest.cc:236: std::cerr << "L-i:" << ntohs(*len_addr) << std::endl; Any reason ...
8 years, 2 months ago (2016-01-20 01:43:48 UTC) #6
mt
I discovered a pretty big issue with the test harness. The basic false start test ...
8 years, 2 months ago (2016-01-20 08:56:20 UTC) #7
mt
A few notes. https://codereview.appspot.com/286790043/diff/60001/external_tests/ssl_gtest/tls_agent.cc File external_tests/ssl_gtest/tls_agent.cc (right): https://codereview.appspot.com/286790043/diff/60001/external_tests/ssl_gtest/tls_agent.cc#newcode509 external_tests/ssl_gtest/tls_agent.cc:509: if (err != PR_WOULD_BLOCK_ERROR) { The ...
8 years, 2 months ago (2016-01-20 09:00:12 UTC) #8
ekr-rietveld
8 years, 2 months ago (2016-01-20 17:50:21 UTC) #9
wtc1
Review comments on patch set 5: I only reviewed the changes to lib/ssl. I don't ...
8 years, 2 months ago (2016-01-20 22:19:29 UTC) #10
wtc1
https://codereview.appspot.com/286790043/diff/60001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/286790043/diff/60001/lib/ssl/ssl3con.c#newcode7457 lib/ssl/ssl3con.c:7457: case wait_finished: On 2016/01/20 08:56:19, mt wrote: > > ...
8 years, 2 months ago (2016-01-20 22:25:33 UTC) #11
mt
Thanks Wan-Teh. Would you mind checking that the changes to the makefile (gtests.mk) are OK? ...
8 years, 2 months ago (2016-01-20 22:38:15 UTC) #12
wtc1
Patch set 6 LGTM. I only reviewed gtest.mk and lib/ssl. I only verified the patch ...
8 years, 2 months ago (2016-01-20 22:52:17 UTC) #13
mt
Ready for one last look ekr. https://codereview.appspot.com/286790043/diff/100001/external_tests/common/gtest.mk File external_tests/common/gtest.mk (left): https://codereview.appspot.com/286790043/diff/100001/external_tests/common/gtest.mk#oldcode8 external_tests/common/gtest.mk:8: include ../../cmd/platrules.mk On ...
8 years, 2 months ago (2016-01-20 23:00:33 UTC) #14
ekr-rietveld
I didn't find anything major above Wan-teh's comments https://codereview.appspot.com/286790043/diff/60001/external_tests/ssl_gtest/tls_agent.cc File external_tests/ssl_gtest/tls_agent.cc (right): https://codereview.appspot.com/286790043/diff/60001/external_tests/ssl_gtest/tls_agent.cc#newcode509 external_tests/ssl_gtest/tls_agent.cc:509: if ...
8 years, 2 months ago (2016-01-21 00:48:06 UTC) #15
ekr-rietveld
8 years, 2 months ago (2016-01-26 02:19:01 UTC) #16
LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b