Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29408721: Issue 5002 - Fix potential concurrency issues in WebRequest tests. (Closed)

Created:
April 10, 2017, 12:41 p.m. by hub
Modified:
April 19, 2017, 3:17 p.m.
Reviewers:
sergei
Base URL:
https://hg.adblockplus.org/libadblockplus/
Visibility:
Public.

Description

Issue 5002 - Fix potential concurrency issues in WebRequest tests.

Patch Set 1 #

Total comments: 8

Patch Set 2 : Fixes from review. #

Total comments: 4

Patch Set 3 : Avoid casting the webRequest pointer. #

Total comments: 4

Patch Set 4 : Fixed the last nits. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+85 lines, -30 lines) Patch
M test/WebRequest.cpp View 1 2 3 3 chunks +85 lines, -30 lines 0 comments Download

Messages

Total messages: 8
hub
April 10, 2017, 12:41 p.m. (2017-04-10 12:41:25 UTC) #1
sergei
https://codereview.adblockplus.org/29408721/diff/29408722/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29408721/diff/29408722/test/WebRequest.cpp#newcode33 test/WebRequest.cpp:33: std::set<std::string> r; what about renaming here? r -> headerNames ...
April 19, 2017, 10:38 a.m. (2017-04-19 10:38:31 UTC) #2
hub
updated patch https://codereview.adblockplus.org/29408721/diff/29408722/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29408721/diff/29408722/test/WebRequest.cpp#newcode33 test/WebRequest.cpp:33: std::set<std::string> r; On 2017/04/19 10:38:31, sergei wrote: ...
April 19, 2017, 1:12 p.m. (2017-04-19 13:12:05 UTC) #3
sergei
https://codereview.adblockplus.org/29408721/diff/29417568/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29408721/diff/29417568/test/WebRequest.cpp#newcode64 test/WebRequest.cpp:64: const auto& iter = requestHeaderNames.find(url); const reference keeps the ...
April 19, 2017, 1:17 p.m. (2017-04-19 13:17:29 UTC) #4
hub
updated patch https://codereview.adblockplus.org/29408721/diff/29417568/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29408721/diff/29417568/test/WebRequest.cpp#newcode64 test/WebRequest.cpp:64: const auto& iter = requestHeaderNames.find(url); On 2017/04/19 ...
April 19, 2017, 2:41 p.m. (2017-04-19 14:41:00 UTC) #5
sergei
LGTM https://codereview.adblockplus.org/29408721/diff/29417570/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29408721/diff/29417570/test/WebRequest.cpp#newcode86 test/WebRequest.cpp:86: webRequest = std::shared_ptr<T>(new T()); std::make_shared are usually preferable, ...
April 19, 2017, 2:44 p.m. (2017-04-19 14:44:47 UTC) #6
hub
Update the patch to make things right. https://codereview.adblockplus.org/29408721/diff/29417570/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29408721/diff/29417570/test/WebRequest.cpp#newcode86 test/WebRequest.cpp:86: webRequest = ...
April 19, 2017, 2:55 p.m. (2017-04-19 14:55:06 UTC) #7
sergei
April 19, 2017, 3:01 p.m. (2017-04-19 15:01:55 UTC) #8
LGTM

Powered by Google App Engine
This is Rietveld