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

Issue 4806567450902528: Issue 1794 - add handling of std::vector<std::string> by Communication::{Input,Output}Buffer (Closed)

Created:
Dec. 19, 2014, 9:42 a.m. by sergei
Modified:
Feb. 18, 2015, 3:26 p.m.
Reviewers:
Eric, Oleksandr
Visibility:
Public.

Patch Set 1 #

Total comments: 5

Patch Set 2 : change tests #

Total comments: 2

Patch Set 3 : remove WriteStrings and ReadStrings #

Total comments: 11
Unified diffs Side-by-side diffs Delta from patch set Stats (+85 lines, -29 lines) Patch
M src/engine/Main.cpp View 1 2 3 chunks +2 lines, -11 lines 0 comments Download
M src/plugin/AdblockPlusClient.cpp View 1 2 3 chunks +8 lines, -17 lines 2 comments Download
M src/shared/Communication.h View 5 chunks +29 lines, -1 line 6 comments Download
M src/shared/Utils.h View 1 2 2 chunks +2 lines, -0 lines 2 comments Download
M src/shared/Utils.cpp View 1 2 1 chunk +8 lines, -0 lines 0 comments Download
M test/CommunicationTest.cpp View 1 1 chunk +36 lines, -0 lines 1 comment Download

Messages

Total messages: 17
sergei
Jan. 13, 2015, 2:57 p.m. (2015-01-13 14:57:08 UTC) #1
Eric
http://codereview.adblockplus.org/4806567450902528/diff/5629499534213120/src/shared/Communication.h File src/shared/Communication.h (right): http://codereview.adblockplus.org/4806567450902528/diff/5629499534213120/src/shared/Communication.h#newcode41 src/shared/Communication.h:41: TYPE_PROC, TYPE_STRING, TYPE_WSTRING, TYPE_INT64, TYPE_INT32, TYPE_BOOL, TYPE_STRINGS We should ...
Jan. 13, 2015, 4:20 p.m. (2015-01-13 16:20:45 UTC) #2
sergei
http://codereview.adblockplus.org/4806567450902528/diff/5629499534213120/src/shared/Communication.h File src/shared/Communication.h (right): http://codereview.adblockplus.org/4806567450902528/diff/5629499534213120/src/shared/Communication.h#newcode41 src/shared/Communication.h:41: TYPE_PROC, TYPE_STRING, TYPE_WSTRING, TYPE_INT64, TYPE_INT32, TYPE_BOOL, TYPE_STRINGS On 2015/01/13 ...
Jan. 27, 2015, 3:28 p.m. (2015-01-27 15:28:38 UTC) #3
Eric
On 2015/01/27 15:28:38, sergei wrote: > I'm confused, we recently accepted https://issues.adblockplus.org/ticket/715 . Just saw ...
Feb. 2, 2015, 3:02 p.m. (2015-02-02 15:02:31 UTC) #4
Oleksandr
I might be missing something, but why do we need both ReadStrings in AdblockPlusClient.cpp, WriteStrings ...
Feb. 4, 2015, 8:49 p.m. (2015-02-04 20:49:37 UTC) #5
sergei
On 2015/02/04 20:49:37, Oleksandr wrote: > I might be missing something, but why do we ...
Feb. 5, 2015, 12:54 p.m. (2015-02-05 12:54:50 UTC) #6
Oleksandr
> > I might be missing something, but why do we need both ReadStrings in ...
Feb. 6, 2015, 3:12 p.m. (2015-02-06 15:12:46 UTC) #7
sergei
On 2015/02/06 15:12:46, Oleksandr wrote: > > > I might be missing something, but why ...
Feb. 6, 2015, 3:20 p.m. (2015-02-06 15:20:37 UTC) #8
sergei
Sorry, now I see ReadStrings in AdblockPlusClient.cpp and WriteStrings in Main.cpp. Yes, it makes sense ...
Feb. 9, 2015, 11:48 a.m. (2015-02-09 11:48:55 UTC) #9
sergei
On 2015/02/09 11:48:55, sergei wrote: > Sorry, now I see ReadStrings in AdblockPlusClient.cpp and WriteStrings ...
Feb. 9, 2015, 2:05 p.m. (2015-02-09 14:05:37 UTC) #10
Eric
Now that the code is clearer, it has become clear that there's a design flaw ...
Feb. 9, 2015, 4:18 p.m. (2015-02-09 16:18:57 UTC) #11
sergei
I'm pretty sure it's beyond the scope of this issue, so could you move it ...
Feb. 9, 2015, 4:55 p.m. (2015-02-09 16:55:37 UTC) #12
Eric
On 2015/02/09 16:55:37, sergei wrote: > I'm pretty sure it's beyond the scope of this ...
Feb. 9, 2015, 5:33 p.m. (2015-02-09 17:33:52 UTC) #13
Eric
http://codereview.adblockplus.org/4806567450902528/diff/5700735861784576/src/shared/Communication.h File src/shared/Communication.h (right): http://codereview.adblockplus.org/4806567450902528/diff/5700735861784576/src/shared/Communication.h#newcode87 src/shared/Communication.h:87: buffer.read(reinterpret_cast<char*>(data.get()), sizeof(T::value_type) * length); On 2015/02/09 16:55:37, sergei wrote: ...
Feb. 9, 2015, 5:34 p.m. (2015-02-09 17:34:02 UTC) #14
Oleksandr
On 2015/02/09 17:33:52, Eric wrote: > On 2015/02/09 16:55:37, sergei wrote: > > I'm pretty ...
Feb. 16, 2015, 7:31 a.m. (2015-02-16 07:31:37 UTC) #15
Oleksandr
LGTM
Feb. 17, 2015, 12:23 p.m. (2015-02-17 12:23:10 UTC) #16
Eric
Feb. 17, 2015, 5:18 p.m. (2015-02-17 17:18:35 UTC) #17
For reference, the concerns I had are being addressed in this review:
    http://codereview.adblockplus.org/4882650246414336/

I have one reservation about implementation of a unit test, but we can address
it in the next review.

Neither of these caveats need to block this. LGTM.

http://codereview.adblockplus.org/4806567450902528/diff/5700735861784576/test...
File test/CommunicationTest.cpp (right):

http://codereview.adblockplus.org/4806567450902528/diff/5700735861784576/test...
test/CommunicationTest.cpp:120: Communication::InputBuffer
inputBuffer(outputBuffer.Get());
I don't like the use of Get() here, but I'll save comments for the subsequent
change set.

Powered by Google App Engine
This is Rietveld