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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 4 weeks ago by hub
Modified:
1 month, 2 weeks ago
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
2 months, 4 weeks ago (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 ...
2 months, 4 weeks ago (2018-06-22 00:34:31 UTC) #2
sergei
I think we need several tests to better understand what is appropriate when a value ...
2 months, 4 weeks ago (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 ...
2 months, 4 weeks ago (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): ...
2 months, 2 weeks ago (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 ...
2 months, 1 week ago (2018-07-11 02:11:02 UTC) #6
sergei
It seems LGTM, though could you please add to the issue description - revisit ThrowExceptionInJS ...
1 month, 2 weeks ago (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 ...
1 month, 2 weeks ago (2018-08-06 13:25:48 UTC) #8
sergei
1 month, 2 weeks ago (2018-08-06 13:53:46 UTC) #9
LGTM
Sign in to reply to this message.

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