Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2477743002:
Various cleanups to QUIC end to end test to sync up with the internal version. (Closed)
|
Created:
4 years, 1 month ago by Ryan Hamilton Modified:
4 years, 1 month ago CC:
chromium-reviews, cbentzel+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionVarious cleanups to QUIC end to end test to sync up with the internal version.
Merge internal change 138066022
Committed: https://crrev.com/664c9f34a300d44e95851105abbef710c473ea16
Cr-Commit-Position: refs/heads/master@{#429677}
Patch Set 1 #Patch Set 2 : More #Patch Set 3 : size_t #Patch Set 4 : Rebase #Patch Set 5 : Rebase #
Total comments: 4
Messages
Total messages: 22 (16 generated)
rch@chromium.org changed reviewers: + zhongyi@chromium.org
The CQ bit was checked by rch@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 checked by rch@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 checked by rch@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 checked by rch@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...
Description was changed from ========== Various cleanups to QUIC end to end test to sync up with the internal version. ========== to ========== Various cleanups to QUIC end to end test to sync up with the internal version. Merge internal change 138066022 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jri@chromium.org changed reviewers: + jri@chromium.org
lgtm LGTM, modulo a couple of nits/clarifications https://codereview.chromium.org/2477743002/diff/80001/net/tools/quic/end_to_e... File net/tools/quic/end_to_end_test.cc (left): https://codereview.chromium.org/2477743002/diff/80001/net/tools/quic/end_to_e... net/tools/quic/end_to_end_test.cc:1991: server_thread_->Pause(); Assuming that this doesn't cause races now? https://codereview.chromium.org/2477743002/diff/80001/net/tools/quic/end_to_e... net/tools/quic/end_to_end_test.cc:2545: // Invalid content-length so the request will receive an early 500 response. nit: move this comment to the comment block above instead of deleting?
Thanks! https://codereview.chromium.org/2477743002/diff/80001/net/tools/quic/end_to_e... File net/tools/quic/end_to_end_test.cc (left): https://codereview.chromium.org/2477743002/diff/80001/net/tools/quic/end_to_e... net/tools/quic/end_to_end_test.cc:1991: server_thread_->Pause(); On 2016/11/03 17:51:40, Jana wrote: > Assuming that this doesn't cause races now? Right. To be safe, I took the chrome code and attempted to use it internally but that causes hangs! Calling Pause() is actually a terrible idea, as it turns out, because if the client has not yet received a response, then stopping the server guarantees that it will *never* get a response :> And the code here doesn't actually tickle any races, as far as I can tell. perhaps this code did something different in a previous incarnation? https://codereview.chromium.org/2477743002/diff/80001/net/tools/quic/end_to_e... net/tools/quic/end_to_end_test.cc:2545: // Invalid content-length so the request will receive an early 500 response. On 2016/11/03 17:51:40, Jana wrote: > nit: move this comment to the comment block above instead of deleting? meh. the comment above headers already talks about generating an early response on this matches the internal code.
The CQ bit was checked by rch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Various cleanups to QUIC end to end test to sync up with the internal version. Merge internal change 138066022 ========== to ========== Various cleanups to QUIC end to end test to sync up with the internal version. Merge internal change 138066022 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Various cleanups to QUIC end to end test to sync up with the internal version. Merge internal change 138066022 ========== to ========== Various cleanups to QUIC end to end test to sync up with the internal version. Merge internal change 138066022 Committed: https://crrev.com/664c9f34a300d44e95851105abbef710c473ea16 Cr-Commit-Position: refs/heads/master@{#429677} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/664c9f34a300d44e95851105abbef710c473ea16 Cr-Commit-Position: refs/heads/master@{#429677} |
