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

Issue 29531696: Issue 5568 - Improve string handling by splitting read strings into only ASCII and non-ASCII strings (Closed)

Created:
Aug. 30, 2017, 4:55 p.m. by sergei
Modified:
Aug. 31, 2017, 5:15 p.m.
Reviewers:
hub
CC:
Felix Dahlke
Base URL:
https://github.com/adblockplus/libadblockplus.git
Visibility:
Public.

Description

# Since one may not shuffle lines I think we should not over-complicate it and start with the proposed code. IMO any optimizations will be based on moving towards migration from JS to C++ and should be done as another commits.

Patch Set 1 #

Total comments: 7

Patch Set 2 : address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+272 lines, -22 lines) Patch
M lib/io.js View 1 chunk +6 lines, -4 lines 0 comments Download
M src/FileSystemJsObject.cpp View 1 3 chunks +82 lines, -0 lines 0 comments Download
M src/JsError.h View 1 chunk +3 lines, -0 lines 0 comments Download
M src/JsError.cpp View 1 chunk +17 lines, -18 lines 0 comments Download
M test/FileSystemJsObject.cpp View 2 chunks +164 lines, -0 lines 0 comments Download

Messages

Total messages: 6
sergei
https://codereview.adblockplus.org/29531696/diff/29531697/test/FileSystemJsObject.cpp File test/FileSystemJsObject.cpp (right): https://codereview.adblockplus.org/29531696/diff/29531697/test/FileSystemJsObject.cpp#newcode326 test/FileSystemJsObject.cpp:326: readFromFile_Lines({"\n\r\n"}, {""}); The behavior has been changed here a ...
Aug. 30, 2017, 5:14 p.m. (2017-08-30 17:14:08 UTC) #1
hub
just a few remarks LGTM https://codereview.adblockplus.org/29531696/diff/29531697/src/FileSystemJsObject.cpp File src/FileSystemJsObject.cpp (right): https://codereview.adblockplus.org/29531696/diff/29531697/src/FileSystemJsObject.cpp#newcode67 src/FileSystemJsObject.cpp:67: inline bool CReturnOrLineFeed(char c) ...
Aug. 30, 2017, 9:13 p.m. (2017-08-30 21:13:31 UTC) #2
sergei
https://codereview.adblockplus.org/29531696/diff/29531697/src/FileSystemJsObject.cpp File src/FileSystemJsObject.cpp (right): https://codereview.adblockplus.org/29531696/diff/29531697/src/FileSystemJsObject.cpp#newcode67 src/FileSystemJsObject.cpp:67: inline bool CReturnOrLineFeed(char c) On 2017/08/30 21:13:31, hub wrote: ...
Aug. 31, 2017, 10:45 a.m. (2017-08-31 10:45:26 UTC) #3
hub
https://codereview.adblockplus.org/29531696/diff/29531697/src/FileSystemJsObject.cpp File src/FileSystemJsObject.cpp (right): https://codereview.adblockplus.org/29531696/diff/29531697/src/FileSystemJsObject.cpp#newcode67 src/FileSystemJsObject.cpp:67: inline bool CReturnOrLineFeed(char c) On 2017/08/31 10:45:26, sergei wrote: ...
Aug. 31, 2017, 12:59 p.m. (2017-08-31 12:59:44 UTC) #4
sergei
https://codereview.adblockplus.org/29531696/diff/29531697/src/FileSystemJsObject.cpp File src/FileSystemJsObject.cpp (right): https://codereview.adblockplus.org/29531696/diff/29531697/src/FileSystemJsObject.cpp#newcode67 src/FileSystemJsObject.cpp:67: inline bool CReturnOrLineFeed(char c) On 2017/08/31 12:59:43, hub wrote: ...
Aug. 31, 2017, 1:21 p.m. (2017-08-31 13:21:44 UTC) #5
hub
Aug. 31, 2017, 1:30 p.m. (2017-08-31 13:30:59 UTC) #6
LGTM

Powered by Google App Engine
This is Rietveld