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

Issue 29449592: Issue 5183 - Provide async interface for FileSystem (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 4 months ago by hub
Modified:
2 years, 3 months ago
Reviewers:
sergei
CC:
Felix Dahlke
Base URL:
https://hg.adblockplus.org/libadblockplus/
Visibility:
Public.

Description

Issue 5183 - Provide async interface for FileSystem

Patch Set 1 #

Total comments: 10

Patch Set 2 : Updated implementation. #

Total comments: 1

Patch Set 3 : Remove a #include Utils.h from test. #

Total comments: 42

Patch Set 4 : Updated patch after review. #

Total comments: 15

Patch Set 5 : Rebased. Corrected most of the review issues. #

Total comments: 8

Patch Set 6 : Make read write deal with binary buffers. #

Total comments: 13

Patch Set 7 : Rebased on https://codereview.adblockplus.org/29481704 #

Total comments: 1

Patch Set 8 : Rebase on master. Last changes. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+692 lines, -424 lines) Patch
M include/AdblockPlus/DefaultFileSystem.h View 1 2 3 4 5 6 1 chunk +25 lines, -4 lines 0 comments Download
M include/AdblockPlus/FileSystem.h View 1 2 3 4 5 6 7 2 chunks +6 lines, -40 lines 0 comments Download
M include/AdblockPlus/IFileSystem.h View 1 2 3 4 5 6 7 2 chunks +44 lines, -15 lines 0 comments Download
M include/AdblockPlus/JsEngine.h View 1 2 3 4 5 6 4 chunks +16 lines, -7 lines 0 comments Download
M include/AdblockPlus/JsValue.h View 1 2 3 4 5 6 7 3 chunks +12 lines, -0 lines 0 comments Download
M libadblockplus.gyp View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M src/DefaultFileSystem.cpp View 1 2 3 4 5 6 4 chunks +144 lines, -12 lines 0 comments Download
M src/FileSystemJsObject.h View 1 1 chunk +0 lines, -2 lines 0 comments Download
M src/FileSystemJsObject.cpp View 1 2 3 4 5 6 7 1 chunk +105 lines, -227 lines 0 comments Download
M src/FilterEngine.cpp View 1 2 3 4 1 chunk +0 lines, -31 lines 0 comments Download
M src/JsEngine.cpp View 1 2 3 4 5 6 3 chunks +15 lines, -7 lines 0 comments Download
M src/JsValue.cpp View 1 2 3 4 5 6 7 2 chunks +12 lines, -0 lines 0 comments Download
M src/Thread.h View 1 2 3 4 1 chunk +49 lines, -0 lines 0 comments Download
M src/Utils.h View 1 2 3 4 5 6 1 chunk +5 lines, -0 lines 0 comments Download
M src/Utils.cpp View 1 2 3 4 5 6 1 chunk +16 lines, -0 lines 0 comments Download
M test/BaseJsTest.h View 1 2 3 4 5 6 2 chunks +76 lines, -5 lines 0 comments Download
M test/BaseJsTest.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M test/DefaultFileSystem.cpp View 1 2 3 4 5 6 1 chunk +87 lines, -28 lines 0 comments Download
M test/FileSystemJsObject.cpp View 1 2 3 4 5 6 7 chunks +55 lines, -32 lines 0 comments Download
M test/FilterEngine.cpp View 1 2 3 4 3 chunks +19 lines, -4 lines 0 comments Download
M test/JsEngine.cpp View 1 2 3 4 5 2 chunks +1 line, -7 lines 0 comments Download
M test/Prefs.cpp View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 20
hub
2 years, 4 months ago (2017-05-26 12:43:13 UTC) #1
hub
the DefaultFileSystem implementation of the asynchronous API is still synchronous. Also I wasn't sure if ...
2 years, 4 months ago (2017-05-26 12:45:29 UTC) #2
sergei
On 2017/05/26 12:45:29, hub wrote: > the DefaultFileSystem implementation of the asynchronous API is still ...
2 years, 4 months ago (2017-05-26 13:32:04 UTC) #3
hub
Reworked the whole thing. Including the tests. https://codereview.adblockplus.org/29449592/diff/29449593/include/AdblockPlus/FileSystem.h File include/AdblockPlus/FileSystem.h (right): https://codereview.adblockplus.org/29449592/diff/29449593/include/AdblockPlus/FileSystem.h#newcode70 include/AdblockPlus/FileSystem.h:70: typedef std::function<void ...
2 years, 4 months ago (2017-06-02 16:04:17 UTC) #4
hub
we can wait that you land the update of v8 first as it will need ...
2 years, 4 months ago (2017-06-02 19:36:15 UTC) #5
sergei
https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus/IFileSystem.h File include/AdblockPlus/IFileSystem.h (right): https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus/IFileSystem.h#newcode82 include/AdblockPlus/IFileSystem.h:82: typedef std::function<void(std::string&&, const std::string&)> ReadCallback; I think that the ...
2 years, 4 months ago (2017-06-16 15:05:56 UTC) #6
hub
Patch updated. https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus/IFileSystem.h File include/AdblockPlus/IFileSystem.h (right): https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus/IFileSystem.h#newcode82 include/AdblockPlus/IFileSystem.h:82: typedef std::function<void(std::string&&, const std::string&)> ReadCallback; On 2017/06/16 ...
2 years, 4 months ago (2017-06-16 21:52:56 UTC) #7
sergei
I have quickly gone through the changes in last patch set, it seems almost ready. ...
2 years, 3 months ago (2017-07-03 09:25:56 UTC) #8
hub
Beside the read / write API comments, I have dealt with the rest. https://codereview.adblockplus.org/29449592/diff/29467603/include/AdblockPlus/JsEngine.h File ...
2 years, 3 months ago (2017-07-04 19:58:28 UTC) #9
hub
https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus/IFileSystem.h File include/AdblockPlus/IFileSystem.h (right): https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus/IFileSystem.h#newcode82 include/AdblockPlus/IFileSystem.h:82: typedef std::function<void(std::string&&, const std::string&)> ReadCallback; On 2017/07/03 09:25:53, sergei ...
2 years, 3 months ago (2017-07-04 21:20:14 UTC) #10
sergei
https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus/IFileSystem.h File include/AdblockPlus/IFileSystem.h (right): https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus/IFileSystem.h#newcode82 include/AdblockPlus/IFileSystem.h:82: typedef std::function<void(std::string&&, const std::string&)> ReadCallback; On 2017/07/04 21:20:14, hub ...
2 years, 3 months ago (2017-07-05 10:03:22 UTC) #11
hub
Patch updated. Still one comment unaddressed. https://codereview.adblockplus.org/29449592/diff/29479644/src/FilterEngine.cpp File src/FilterEngine.cpp (left): https://codereview.adblockplus.org/29449592/diff/29479644/src/FilterEngine.cpp#oldcode185 src/FilterEngine.cpp:185: class Sync On ...
2 years, 3 months ago (2017-07-06 12:21:53 UTC) #12
sergei
https://codereview.adblockplus.org/29449592/diff/29479644/src/JsEngine.cpp File src/JsEngine.cpp (right): https://codereview.adblockplus.org/29449592/diff/29479644/src/JsEngine.cpp#newcode332 src/JsEngine.cpp:332: fileSystemLegacy = val; On 2017/07/06 12:21:52, hub wrote: > ...
2 years, 3 months ago (2017-07-06 14:14:06 UTC) #13
hub
https://codereview.adblockplus.org/29449592/diff/29481630/include/AdblockPlus/FileSystem.h File include/AdblockPlus/FileSystem.h (right): https://codereview.adblockplus.org/29449592/diff/29481630/include/AdblockPlus/FileSystem.h#newcode52 include/AdblockPlus/FileSystem.h:52: std::ostream& data) = 0; On 2017/07/06 14:14:04, sergei wrote: ...
2 years, 3 months ago (2017-07-06 14:33:32 UTC) #14
sergei
https://codereview.adblockplus.org/29449592/diff/29481630/include/AdblockPlus/FileSystem.h File include/AdblockPlus/FileSystem.h (right): https://codereview.adblockplus.org/29449592/diff/29481630/include/AdblockPlus/FileSystem.h#newcode52 include/AdblockPlus/FileSystem.h:52: std::ostream& data) = 0; On 2017/07/06 14:33:31, hub wrote: ...
2 years, 3 months ago (2017-07-06 14:53:35 UTC) #15
hub
I will need to rebase this patch on top of https://codereview.adblockplus.org/29481704 Eventually https://codereview.adblockplus.org/29449592/diff/29479644/src/JsEngine.cpp File src/JsEngine.cpp ...
2 years, 3 months ago (2017-07-06 21:24:30 UTC) #16
hub
Now it requires https://codereview.adblockplus.org/29481704
2 years, 3 months ago (2017-07-06 22:43:03 UTC) #17
sergei
https://codereview.adblockplus.org/29449592/diff/29481630/src/JsValue.cpp File src/JsValue.cpp (right): https://codereview.adblockplus.org/29449592/diff/29481630/src/JsValue.cpp#newcode212 src/JsValue.cpp:212: void AdblockPlus::JsValue::SetProperty(const std::string& name, const std::vector<char>& val) On 2017/07/06 ...
2 years, 3 months ago (2017-07-07 13:29:28 UTC) #18
hub
updated patch https://codereview.adblockplus.org/29449592/diff/29481630/src/JsValue.cpp File src/JsValue.cpp (right): https://codereview.adblockplus.org/29449592/diff/29481630/src/JsValue.cpp#newcode212 src/JsValue.cpp:212: void AdblockPlus::JsValue::SetProperty(const std::string& name, const std::vector<char>& val) ...
2 years, 3 months ago (2017-07-07 13:39:20 UTC) #19
sergei
2 years, 3 months ago (2017-07-07 13:41:02 UTC) #20
LGTM

https://codereview.adblockplus.org/29449592/diff/29481725/src/DefaultFileSyst...
File src/DefaultFileSystem.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29481725/src/DefaultFileSyst...
src/DefaultFileSystem.cpp:72: DefaultFileSystemSync::Read(const std::string&
path) const
It's actually not our coding style, but I think it's fine. I would even like to
have such coding style (return value on a separate line for a function
implementation).
Sign in to reply to this message.

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