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

Issue 29524596: Issue 5556 - make C++ implementation of LogSystem manageable only by JsEngine (Closed)

Created:
Aug. 23, 2017, 12:10 p.m. by sergei
Modified:
Aug. 29, 2017, 10:35 a.m.
Reviewers:
diegocarloslima, anton, jens
CC:
Felix Dahlke, René Jeschke
Base URL:
github.com:abby-sergz/libadblockplus-android.git
Visibility:
Public.

Description

In the newer libadblockplus the life time of injected implementations of interfaces is controlled by Platform, namely they are destroyed by Platform (currently the role of platform is played by JsEngine). In this commit we remove a direct control of the life span of C++ implementation of LogSystem from Java, however it's still controlled because LogSystem is being destroyed when JsEngine is being destroyed, namely on Java JsEngine.dispose(). # depends on https://codereview.adblockplus.org/29524565/ COLLABORATOR=anton@adblockplus.org

Patch Set 1 #

Total comments: 2

Patch Set 2 : rebase #

Messages

Total messages: 5
sergei
Aug. 23, 2017, 12:21 p.m. (2017-08-23 12:21:12 UTC) #1
anton
https://codereview.adblockplus.org/29524596/diff/29524597/libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java File libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java (right): https://codereview.adblockplus.org/29524596/diff/29524597/libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java#newcode40 libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java:40: this(appInfo, /* logSystem */null); please remove comment
Aug. 24, 2017, 11:14 a.m. (2017-08-24 11:14:43 UTC) #2
sergei
https://codereview.adblockplus.org/29524596/diff/29524597/libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java File libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java (right): https://codereview.adblockplus.org/29524596/diff/29524597/libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java#newcode40 libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java:40: this(appInfo, /* logSystem */null); On 2017/08/24 11:14:43, anton wrote: ...
Aug. 24, 2017, 1:40 p.m. (2017-08-24 13:40:24 UTC) #3
anton
On 2017/08/24 13:40:24, sergei wrote: > https://codereview.adblockplus.org/29524596/diff/29524597/libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java > File libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java > (right): > > https://codereview.adblockplus.org/29524596/diff/29524597/libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java#newcode40 ...
Aug. 24, 2017, 2:03 p.m. (2017-08-24 14:03:26 UTC) #4
diegocarloslima
Aug. 28, 2017, 8:11 p.m. (2017-08-28 20:11:01 UTC) #5 
On 2017/08/24 14:03:26, anton wrote:
> On 2017/08/24 13:40:24, sergei wrote:
> >
>
https://codereview.adblockplus.org/29524596/diff/29524597/libadblockplus-andr...
> > File libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java
> > (right):
> > 
> >
>
https://codereview.adblockplus.org/29524596/diff/29524597/libadblockplus-andr...
> > libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java:40:
> > this(appInfo, /* logSystem */null);
> > On 2017/08/24 11:14:43, anton wrote:
> > > please remove comment
> > 
> > Done.
> 
> LGTM

LGTM

Powered by Google App Engine
This is Rietveld