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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 weeks, 6 days ago by hub
Modified:
1 week, 1 day 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. #

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 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: 6
hub
3 weeks, 6 days 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 ...
3 weeks, 6 days 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 ...
3 weeks, 6 days 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 ...
3 weeks, 5 days 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 weeks ago (2018-07-05 08:39:00 UTC) #5
hub
1 week, 1 day ago (2018-07-11 02:11:02 UTC) #6
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 2018/07/05 08:39:00, sergei wrote:
> What about adding something like NOTRYCATCH instead of _NOTHROW? It's easy to
> assume that NOTHROW is related to that macro/function, namely that it does not
> throw, but actually it does throw, the difference is that it does not check
> v8::TryCatch.
> 
> BTW, what do you think about swapping,
> CHECKED_TO_LOCAL_NOTHROW->CHECKED_TO_LOCAL and
CHECKED_TO_LOCAL_WITH_TRY_CATCH?
> If we add something else later, or replace v8::TryCatch then it will be
> reflected in the suffix CHECKED_TO_LOCAL_WITH_SOMETHING_ELSE.

Done.
Sign in to reply to this message.

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