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

Issue 29365532: Issue #3594 - Remove `globalJsObject` and add `GetGlobalObject()` (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 2 months ago by Eric
Modified:
3 years, 2 months ago
Visibility:
Public.

Description

Issue #3594 - Remove `globalJsObject` and add `GetGlobalObject()` Remove the global object member variable, which is part of the reference cycle that is the subject of #3594. Replace it with a function that recreates an equivalent JS variable each time. Change the single use of `globalJsObject` to call `GetGlobalObject()` instead. Added member variable and accessor for the v8 context in `JsContext`.

Patch Set 1 #

Total comments: 2

Patch Set 2 : use JsContext #

Total comments: 5

Patch Set 3 : nit fix #

Unified diffs Side-by-side diffs Delta from patch set Stats (+20 lines, -9 lines) Patch
M include/AdblockPlus/JsEngine.h View 2 chunks +2 lines, -1 line 0 comments Download
M src/JsContext.h View 1 1 chunk +6 lines, -0 lines 0 comments Download
M src/JsContext.cpp View 1 1 chunk +2 lines, -1 line 0 comments Download
M src/JsEngine.cpp View 1 2 2 chunks +10 lines, -7 lines 0 comments Download

Messages

Total messages: 11
Eric
This code passes the existing unit test suite. I didn't feel the need to add ...
3 years, 2 months ago (2016-11-28 14:22:32 UTC) #1
sergei
Good change. https://codereview.adblockplus.org/29365532/diff/29365533/src/JsEngine.cpp File src/JsEngine.cpp (right): https://codereview.adblockplus.org/29365532/diff/29365533/src/JsEngine.cpp#newcode100 src/JsEngine.cpp:100: const auto i = GetIsolate(); Why not ...
3 years, 2 months ago (2016-11-28 15:04:33 UTC) #2
Eric
https://codereview.adblockplus.org/29365532/diff/29365533/src/JsEngine.cpp File src/JsEngine.cpp (right): https://codereview.adblockplus.org/29365532/diff/29365533/src/JsEngine.cpp#newcode100 src/JsEngine.cpp:100: const auto i = GetIsolate(); On 2016/11/28 15:04:33, sergei ...
3 years, 2 months ago (2016-11-28 15:33:24 UTC) #3
Eric
Added a line in the commit message about `JsContext`. https://codereview.adblockplus.org/29365532/diff/29365542/src/JsEngine.cpp File src/JsEngine.cpp (right): https://codereview.adblockplus.org/29365532/diff/29365542/src/JsEngine.cpp#newcode101 src/JsEngine.cpp:101: ...
3 years, 2 months ago (2016-11-28 17:51:27 UTC) #4
sergei
LGTM https://codereview.adblockplus.org/29365532/diff/29365542/src/JsEngine.cpp File src/JsEngine.cpp (right): https://codereview.adblockplus.org/29365532/diff/29365542/src/JsEngine.cpp#newcode101 src/JsEngine.cpp:101: return JsValuePtr(new JsValue(shared_from_this(), context.GetV8Context()->Global())); On 2016/11/28 17:51:27, Eric ...
3 years, 2 months ago (2016-11-29 09:47:12 UTC) #5
Eric
https://codereview.adblockplus.org/29365532/diff/29365542/src/JsEngine.cpp File src/JsEngine.cpp (right): https://codereview.adblockplus.org/29365532/diff/29365542/src/JsEngine.cpp#newcode101 src/JsEngine.cpp:101: return JsValuePtr(new JsValue(shared_from_this(), context.GetV8Context()->Global())); On 2016/11/29 09:47:12, sergei wrote: ...
3 years, 2 months ago (2016-11-30 17:59:46 UTC) #6
sergei
lgtm
3 years, 2 months ago (2016-11-30 21:48:58 UTC) #7
Eric
@sergie, when do you want this pushed? Are we waiting on the others?
3 years, 2 months ago (2016-12-02 16:06:09 UTC) #8
sergei
On 2016/12/02 16:06:09, Eric wrote: > @sergie, when do you want this pushed? Are we ...
3 years, 2 months ago (2016-12-02 16:10:12 UTC) #9
Eric
On 2016/12/02 16:10:12, sergei wrote: > Yes, I wanted to give others an opportunity to ...
3 years, 2 months ago (2016-12-02 16:24:27 UTC) #10
Felix Dahlke
3 years, 2 months ago (2016-12-02 16:34:56 UTC) #11
LGTM, but I just had a quick look.
Sign in to reply to this message.

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