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

Issue 29809555: Issue #6526 - pass v8::Isolate to more functions because old approach is deprecated (Closed)

Created:
June 18, 2018, 10:22 a.m. by sergei
Modified:
June 20, 2018, 2:55 p.m.
Reviewers:
hub
CC:
Oleksandr
Base URL:
https://github.com/adblockplus/libadblockplus@4d9bcc12e77369cbc4bc04bace9a3e7fa03de17b
Visibility:
Public.

Description

Beside v8::Isolate some places now start to pass current context, though the implementation is not complete yet because there is no check of the return value. Despite it can seem that static JsError::ExceptionToString should return an optinal/maybe value if there is no value, IMO it would be better to get as much as possible information, even just a string "unknown line" when it is about an exception. Perhaps even such information can already help to find out the bug, notice it or debug it.

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+40 lines, -32 lines) Patch
M src/ConsoleJsObject.cpp View 2 chunks +3 lines, -3 lines 0 comments Download
M src/FileSystemJsObject.cpp View 1 chunk +6 lines, -4 lines 2 comments Download
M src/JsEngine.cpp View 2 chunks +7 lines, -6 lines 0 comments Download
M src/JsError.h View 1 chunk +3 lines, -2 lines 0 comments Download
M src/JsError.cpp View 1 chunk +11 lines, -6 lines 0 comments Download
M src/JsValue.cpp View 3 chunks +4 lines, -5 lines 0 comments Download
M src/Utils.h View 1 chunk +2 lines, -2 lines 0 comments Download
M src/Utils.cpp View 1 chunk +4 lines, -4 lines 0 comments Download

Messages

Total messages: 5
sergei
Since it has some changes what can be a source of bugs I'm not sure ...
June 18, 2018, 10:34 a.m. (2018-06-18 10:34:06 UTC) #1
hub
https://codereview.adblockplus.org/29809555/diff/29809556/src/FileSystemJsObject.cpp File src/FileSystemJsObject.cpp (right): https://codereview.adblockplus.org/29809555/diff/29809556/src/FileSystemJsObject.cpp#newcode191 src/FileSystemJsObject.cpp:191: processFunc->Call(v8Context, globalContext, 1, &jsLine); I get this warning here: ...
June 18, 2018, 12:59 p.m. (2018-06-18 12:59:02 UTC) #2
sergei
https://codereview.adblockplus.org/29809555/diff/29809556/src/FileSystemJsObject.cpp File src/FileSystemJsObject.cpp (right): https://codereview.adblockplus.org/29809555/diff/29809556/src/FileSystemJsObject.cpp#newcode191 src/FileSystemJsObject.cpp:191: processFunc->Call(v8Context, globalContext, 1, &jsLine); On 2018/06/18 12:59:02, hub wrote: ...
June 18, 2018, 1 p.m. (2018-06-18 13:00:19 UTC) #3
hub
if you are good with it, LGTM.
June 18, 2018, 1:01 p.m. (2018-06-18 13:01:04 UTC) #4
sergei
June 18, 2018, 1:19 p.m. (2018-06-18 13:19:55 UTC) #5
On 2018/06/18 13:01:04, hub wrote:
> if you are good with it, LGTM.

yes, I'm good with it, the return value is not checked and it will be addressed
in the next codereviews.

Powered by Google App Engine
This is Rietveld