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

Issue 29410664: Issue 5013 - Use const JsValue and pass reference where applicable (Closed)

Created:
April 12, 2017, 3:24 p.m. by hub
Modified:
April 19, 2017, 11:39 a.m.
Reviewers:
sergei
Base URL:
https://hg.adblockplus.org/libadblockplus/
Visibility:
Public.

Description

Issue 5013 - Use const JsValue and pass reference where applicable

Patch Set 1 #

Total comments: 4

Patch Set 2 : Remove the SetProperty() changes. #

Total comments: 2

Patch Set 3 : Fixed comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+54 lines, -43 lines) Patch
M include/AdblockPlus/FilterEngine.h View 1 chunk +4 lines, -4 lines 0 comments Download
M include/AdblockPlus/JsEngine.h View 1 3 chunks +4 lines, -4 lines 0 comments Download
M include/AdblockPlus/JsValue.h View 1 2 1 chunk +6 lines, -0 lines 0 comments Download
M src/ConsoleJsObject.cpp View 1 2 chunks +2 lines, -2 lines 0 comments Download
M src/FileSystemJsObject.cpp View 1 11 chunks +13 lines, -13 lines 0 comments Download
M src/FilterEngine.cpp View 1 4 chunks +7 lines, -7 lines 0 comments Download
M src/GlobalJsObject.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M src/JsEngine.cpp View 1 2 chunks +3 lines, -3 lines 0 comments Download
M src/JsValue.cpp View 1 1 chunk +5 lines, -0 lines 0 comments Download
M src/WebRequestJsObject.cpp View 1 2 chunks +4 lines, -4 lines 0 comments Download
M test/FilterEngine.cpp View 1 chunk +1 line, -1 line 0 comments Download
M test/JsEngine.cpp View 1 1 chunk +2 lines, -2 lines 0 comments Download
M test/UpdateCheck.cpp View 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 9
hub
April 12, 2017, 3:24 p.m. (2017-04-12 15:24:13 UTC) #1
hub
https://codereview.adblockplus.org/29410664/diff/29410665/src/FilterEngine.cpp File src/FilterEngine.cpp (right): https://codereview.adblockplus.org/29410664/diff/29410665/src/FilterEngine.cpp#newcode536 src/FilterEngine.cpp:536: callback(NotificationPtr(new Notification(params[0]->Clone()))); This is the tricky part. Before that ...
April 12, 2017, 3:28 p.m. (2017-04-12 15:28:21 UTC) #2
sergei
It would be better to make changes related to *JsObject::Setup into a separate commit because ...
April 13, 2017, 10:57 a.m. (2017-04-13 10:57:33 UTC) #3
hub
https://codereview.adblockplus.org/29410664/diff/29410665/include/AdblockPlus/JsValue.h File include/AdblockPlus/JsValue.h (right): https://codereview.adblockplus.org/29410664/diff/29410665/include/AdblockPlus/JsValue.h#newcode142 include/AdblockPlus/JsValue.h:142: * @return Value clone On 2017/04/13 10:57:33, sergei wrote: ...
April 13, 2017, 12:06 p.m. (2017-04-13 12:06:24 UTC) #4
hub
removed the SetProperty() changes as requested.
April 13, 2017, 1:18 p.m. (2017-04-13 13:18:02 UTC) #5
sergei
Good, I would like only comment to be changed. https://codereview.adblockplus.org/29410664/diff/29411619/include/AdblockPlus/JsValue.h File include/AdblockPlus/JsValue.h (right): https://codereview.adblockplus.org/29410664/diff/29411619/include/AdblockPlus/JsValue.h#newcode141 include/AdblockPlus/JsValue.h:141: ...
April 13, 2017, 1:55 p.m. (2017-04-13 13:55:44 UTC) #6
hub
updated patch. https://codereview.adblockplus.org/29410664/diff/29411619/include/AdblockPlus/JsValue.h File include/AdblockPlus/JsValue.h (right): https://codereview.adblockplus.org/29410664/diff/29411619/include/AdblockPlus/JsValue.h#newcode141 include/AdblockPlus/JsValue.h:141: * Clone this value wrapper to new ...
April 13, 2017, 3:46 p.m. (2017-04-13 15:46:01 UTC) #7
sergei
LGTM
April 13, 2017, 4:51 p.m. (2017-04-13 16:51:17 UTC) #8
hub
April 18, 2017, 1:56 p.m. (2017-04-18 13:56:27 UTC) #9 
this is waiting of review https://codereview.adblockplus.org/29409580/ to be
approved and land.

Powered by Google App Engine
This is Rietveld