Issue 5643 - Make v8::Isolate injectable into JsEngine

Issue 5643 - Make v8::Isolate injectable into JsEngine

[sergei, 2017-09-26 09:44:14]

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
(right):

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:255:
public synchronized boolean retain(boolean asynchronous)
It seems this code and `lastInstance` are not used and unrelated, could you
please remove it from this codereview?

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
File libadblockplus-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:68: static jlong JNICALL
JniGetFilterEnginePtr(JNIEnv* env, jclass clazz, jlong ptr)
It looks strange to have this method in this file and in FilterEngine because it
looks like getting a pointer to an object from the object itself, why not to use
Platform?
BTW, it seems not used, if so, could you please remove it?

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
File libadblockplus-android/jni/JniPlatform.cpp (right):

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
libadblockplus-android/jni/JniPlatform.cpp:34: ~V8IsolateHolder()
I would propose to remove the destructor here.

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
libadblockplus-android/jni/JniPlatform.cpp:106:
std::unique_ptr<AdblockPlus::IV8IsolateProvider> isolateProvider = nullptr;
There is no need in initializing it to nullptr.

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
libadblockplus-android/jni/JniPlatform.cpp:109: isolateProvider =
std::unique_ptr<AdblockPlus::IV8IsolateProvider>(
I think it would be shorter
isolateProvider.reset(new
V8IsolateHolder(static_cast<v8::Isolate*>(v8IsolatePtr)));
and better because of static_cast.

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java
(right):

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java:48:
public void setUpJsEngine(final AppInfo appInfo, final Long v8IsolatePtr)
Why not to use primitive value here and in AdblockHelper.java?

[anton, 2017-09-26 10:55:07]

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
(right):

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:255:
public synchronized boolean retain(boolean asynchronous)
On 2017/09/26 09:44:13, sergei wrote:
> It seems this code and `lastInstance` are not used and unrelated, could you
> please remove it from this codereview?

They are related but i will split it into separate code review

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
File libadblockplus-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:68: static jlong JNICALL
JniGetFilterEnginePtr(JNIEnv* env, jclass clazz, jlong ptr)
On 2017/09/26 09:44:13, sergei wrote:
> It looks strange to have this method in this file and in FilterEngine because
it
> looks like getting a pointer to an object from the object itself, why not to
use
> Platform?
> BTW, it seems not used, if so, could you please remove it?

it's required to pass filter engine to c++ side for integrating into Chromium.
I will move the changes to separate code review.
As for me it sounds good to get filter engine ptr from filter engine class.

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
File libadblockplus-android/jni/JniPlatform.cpp (right):

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
libadblockplus-android/jni/JniPlatform.cpp:34: ~V8IsolateHolder()
On 2017/09/26 09:44:13, sergei wrote:
> I would propose to remove the destructor here.

Acknowledged.

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
libadblockplus-android/jni/JniPlatform.cpp:106:
std::unique_ptr<AdblockPlus::IV8IsolateProvider> isolateProvider = nullptr;
On 2017/09/26 09:44:13, sergei wrote:
> There is no need in initializing it to nullptr.

Acknowledged.

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
libadblockplus-android/jni/JniPlatform.cpp:109: isolateProvider =
std::unique_ptr<AdblockPlus::IV8IsolateProvider>(
On 2017/09/26 09:44:13, sergei wrote:
> I think it would be shorter
> isolateProvider.reset(new
> V8IsolateHolder(static_cast<v8::Isolate*>(v8IsolatePtr)));
> and better because of static_cast.

Acknowledged.

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java
(right):

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java:48:
public void setUpJsEngine(final AppInfo appInfo, final Long v8IsolatePtr)
On 2017/09/26 09:44:13, sergei wrote:
> Why not to use primitive value here and in AdblockHelper.java?

There is almost no difference.
I will use primitive if you insist.

[anton, 2017-09-26 11:04:25]

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
File libadblockplus-android/jni/JniPlatform.cpp (right):

https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
libadblockplus-android/jni/JniPlatform.cpp:109: isolateProvider =
std::unique_ptr<AdblockPlus::IV8IsolateProvider>(
On 2017/09/26 09:44:13, sergei wrote:
> I think it would be shorter
> isolateProvider.reset(new
> V8IsolateHolder(static_cast<v8::Isolate*>(v8IsolatePtr)));
> and better because of static_cast.

"Error:(104, 87) error: invalid static_cast from type 'jlong {aka long long
int}' to type 'v8::Isolate*'"

[anton, 2017-09-26 11:17:04]

On 2017/09/26 11:04:25, anton wrote:
>
https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
> File libadblockplus-android/jni/JniPlatform.cpp (right):
> 
>
https://codereview.adblockplus.org/29556582/diff/29556583/libadblockplus-andr...
> libadblockplus-android/jni/JniPlatform.cpp:109: isolateProvider =
> std::unique_ptr<AdblockPlus::IV8IsolateProvider>(
> On 2017/09/26 09:44:13, sergei wrote:
> > I think it would be shorter
> > isolateProvider.reset(new
> > V8IsolateHolder(static_cast<v8::Isolate*>(v8IsolatePtr)));
> > and better because of static_cast.
> 
> "Error:(104, 87) error: invalid static_cast from type 'jlong {aka long long
> int}' to type 'v8::Isolate*'"

Uploaded patch set #2

[sergei, 2017-09-28 08:35:27]

https://codereview.adblockplus.org/29556582/diff/29556621/libadblockplus-andr...
File libadblockplus-android/jni/JniPlatform.cpp (right):

https://codereview.adblockplus.org/29556582/diff/29556621/libadblockplus-andr...
libadblockplus-android/jni/JniPlatform.cpp:104: isolateProvider.reset(new
V8IsolateHolder(reinterpret_cast<v8::Isolate*>(v8IsolatePtr)));
On 2017/09/26 11:04:25, anton wrote:
> On 2017/09/26 09:44:13, sergei wrote:
> > I think it would be shorter
> > isolateProvider.reset(new
> > V8IsolateHolder(static_cast<v8::Isolate*>(v8IsolatePtr)));
> > and better because of static_cast.
> 
> "Error:(104, 87) error: invalid static_cast from type 'jlong {aka long long
> int}' to type 'v8::Isolate*'"

Acknowledged. Then could please use JniLongToTypePtr. If there is a bug in the
casting it will be fixed everywhere by a fix in JniLongToTypePtr :)

[anton, 2017-09-28 08:56:12]

On 2017/09/28 08:35:27, sergei wrote:
>
https://codereview.adblockplus.org/29556582/diff/29556621/libadblockplus-andr...
> File libadblockplus-android/jni/JniPlatform.cpp (right):
> 
>
https://codereview.adblockplus.org/29556582/diff/29556621/libadblockplus-andr...
> libadblockplus-android/jni/JniPlatform.cpp:104: isolateProvider.reset(new
> V8IsolateHolder(reinterpret_cast<v8::Isolate*>(v8IsolatePtr)));
> On 2017/09/26 11:04:25, anton wrote:
> > On 2017/09/26 09:44:13, sergei wrote:
> > > I think it would be shorter
> > > isolateProvider.reset(new
> > > V8IsolateHolder(static_cast<v8::Isolate*>(v8IsolatePtr)));
> > > and better because of static_cast.
> > 
> > "Error:(104, 87) error: invalid static_cast from type 'jlong {aka long long
> > int}' to type 'v8::Isolate*'"
> 
> Acknowledged. Then could please use JniLongToTypePtr. If there is a bug in the
> casting it will be fixed everywhere by a fix in JniLongToTypePtr :)

Done, see patch set #3

[sergei, 2017-09-28 09:34:50]

LGTM

https://codereview.adblockplus.org/29556582/diff/29558565/libadblockplus-andr...
File
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java
(right):

https://codereview.adblockplus.org/29556582/diff/29558565/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:136:
private Long v8IsolatePtr;
I'm not sure about Long vs long here, but it seems fine.

[diegocarloslima, 2017-09-28 12:07:30]

On 2017/09/28 09:34:50, sergei wrote:
> LGTM
> 
>
https://codereview.adblockplus.org/29556582/diff/29558565/libadblockplus-andr...
> File
>
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java
> (right):
> 
>
https://codereview.adblockplus.org/29556582/diff/29558565/libadblockplus-andr...
>
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:136:
> private Long v8IsolatePtr;
> I'm not sure about Long vs long here, but it seems fine.

LGTM