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

Issue 29812649: Issue 6526 - *ToV8String() return MaybeLocal<> and check Call() return value (Closed)

Created:
June 21, 2018, 11:17 p.m. by hub
Modified:
Aug. 6, 2018, 3:36 p.m.
Reviewers:
sergei
Base URL:
https://hg.adblockplus.org/libadblockplus/
Visibility:
Public.

Description

Issue 6526 - *ToV8String() return MaybeLocal<> and check Call() return value

Patch Set 1 #

Total comments: 15

Patch Set 2 : Introduce CHECKED_TO_LOCAL_NOTHROW and use it. #

Total comments: 2

Patch Set 3 : Added tests. Renamed CHECKED_TO_LOCAL. #

Total comments: 2

Patch Set 4 : Wrap macro arguments in brackets. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+76 lines, -31 lines) Patch
M src/FileSystemJsObject.cpp View 1 2 1 chunk +10 lines, -4 lines 0 comments Download
M src/JsEngine.cpp View 1 2 3 chunks +13 lines, -6 lines 0 comments Download
M src/JsValue.cpp View 1 2 2 chunks +21 lines, -10 lines 0 comments Download
M src/Utils.h View 1 2 3 1 chunk +10 lines, -6 lines 0 comments Download
M src/Utils.cpp View 1 chunk +12 lines, -5 lines 0 comments Download
M test/JsValue.cpp View 1 2 2 chunks +10 lines, -0 lines 0 comments Download

Messages

Total messages: 9
hub
June 21, 2018, 11:17 p.m. (2018-06-21 23:17:11 UTC) #1
hub
https://codereview.adblockplus.org/29812649/diff/29812650/src/JsEngine.cpp File src/JsEngine.cpp (right): https://codereview.adblockplus.org/29812649/diff/29812650/src/JsEngine.cpp#newcode242 src/JsEngine.cpp:242: return JsValue(shared_from_this(), v8::Undefined(GetIsolate())); This is where I think we ...
June 22, 2018, 12:34 a.m. (2018-06-22 00:34:31 UTC) #2
sergei
I think we need several tests to better understand what is appropriate when a value ...
June 22, 2018, 6:57 a.m. (2018-06-22 06:57:47 UTC) #3
hub
Updated patch. Didn't yet attempt to write tests for ThrowExceptionInJS() https://codereview.adblockplus.org/29812649/diff/29812650/src/JsEngine.cpp File src/JsEngine.cpp (right): https://codereview.adblockplus.org/29812649/diff/29812650/src/JsEngine.cpp#newcode242 ...
June 22, 2018, 3:50 p.m. (2018-06-22 15:50:56 UTC) #4
sergei
I think the only stopper here is the absence of tests. https://codereview.adblockplus.org/29812649/diff/29812650/src/Utils.cpp File src/Utils.cpp (right): ...
July 5, 2018, 8:39 a.m. (2018-07-05 08:39:00 UTC) #5
hub
updated patch with tests. https://codereview.adblockplus.org/29812649/diff/29813585/src/Utils.h File src/Utils.h (right): https://codereview.adblockplus.org/29812649/diff/29813585/src/Utils.h#newcode58 src/Utils.h:58: #define CHECKED_TO_LOCAL_NOTHROW(isolate, value) \ On ...
July 11, 2018, 2:11 a.m. (2018-07-11 02:11:02 UTC) #6
sergei
It seems LGTM, though could you please add to the issue description - revisit ThrowExceptionInJS ...
Aug. 6, 2018, 12:18 p.m. (2018-08-06 12:18:31 UTC) #7
hub
Updated patch https://codereview.adblockplus.org/29812649/diff/29826601/src/Utils.h File src/Utils.h (right): https://codereview.adblockplus.org/29812649/diff/29826601/src/Utils.h#newcode56 src/Utils.h:56: AdblockPlus::Utils::CheckedToLocal(isolate, value, &tryCatch, __FILE__, __LINE__) On 2018/08/06 ...
Aug. 6, 2018, 1:25 p.m. (2018-08-06 13:25:48 UTC) #8
sergei
Aug. 6, 2018, 1:53 p.m. (2018-08-06 13:53:46 UTC) #9
LGTM

Powered by Google App Engine
This is Rietveld