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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 4 months ago by sergei
Modified:
1 year, 3 months ago
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 ...
1 year, 4 months ago (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: ...
1 year, 4 months ago (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: ...
1 year, 4 months ago (2018-06-18 13:00:19 UTC) #3
hub
if you are good with it, LGTM.
1 year, 4 months ago (2018-06-18 13:01:04 UTC) #4
sergei
1 year, 4 months ago (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.
Sign in to reply to this message.

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