Issue 29422558: Issue 5167 - Update to use libadblockplus revision dca8df9af1a7

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+96 lines, -127 lines)			Patch
M	dependencies	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	libadblockplus-android/jni/JniCallbacks.h	View	1 2 3 4 5	3 chunks	+3 lines, -3 lines	0 comments	Download
M	libadblockplus-android/jni/JniEventCallback.cpp	View	1 2 3 4 5	2 chunks	+2 lines, -2 lines	0 comments	Download
M	libadblockplus-android/jni/JniFilter.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	libadblockplus-android/jni/JniFilterChangeCallback.cpp	View	1 2 3 4 5	2 chunks	+2 lines, -3 lines	0 comments	Download
M	libadblockplus-android/jni/JniFilterEngine.cpp	View	1 2 3 4 5	13 chunks	+29 lines, -34 lines	0 comments	Download
M	libadblockplus-android/jni/JniJsEngine.cpp	View	1 2 3 4 5	8 chunks	+11 lines, -12 lines	0 comments	Download
M	libadblockplus-android/jni/JniJsValue.h	View	1 2 3 4 5	1 chunk	+4 lines, -4 lines	0 comments	Download
M	libadblockplus-android/jni/JniJsValue.cpp	View	1 2 3 4 5	14 chunks	+25 lines, -33 lines	0 comments	Download
M	libadblockplus-android/jni/JniNotification.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	libadblockplus-android/jni/JniShowNotificationCallback.cpp	View	1 2 3 4 5	2 chunks	+2 lines, -4 lines	0 comments	Download
M	libadblockplus-android/jni/JniSubscription.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	libadblockplus-android/jni/Utils.h	View	1 2 3 4 5	1 chunk	+3 lines, -5 lines	0 comments	Download
M	libadblockplus-android/jni/Utils.cpp	View	1 2 3 4 5	1 chunk	+11 lines, -23 lines	0 comments	Download

Messages

Total messages: 11

Expand All Messages | Collapse All Messages

sergei

I have quickly reviewed the code and paid attention mainly to some more or less ...

April 26, 2017, 9:21 a.m. (2017-04-26 09:21:00 UTC) #1

anton

uploaded patch set #3 https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-android/jni/JniEventCallback.cpp File libadblockplus-android/jni/JniEventCallback.cpp (right): https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-android/jni/JniEventCallback.cpp#newcode50 libadblockplus-android/jni/JniEventCallback.cpp:50: jobject jsList = JniJsValueListToArrayList(*env, params); ...

April 26, 2017, 10:23 a.m. (2017-04-26 10:23:00 UTC) #2

uploaded patch set #3

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
File libadblockplus-android/jni/JniEventCallback.cpp (right):

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniEventCallback.cpp:50: jobject jsList =
JniJsValueListToArrayList(*env, params);
On 2017/04/26 09:20:59, sergei wrote:
> That's a good point, I think libadblockplus should add the callback type
> accepting r-value to params, not sure whether it's possible to all methods,
> though.

Acknowledged.

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
File libadblockplus-android/jni/JniJsEngine.cpp (right):

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniJsEngine.cpp:59: const
AdblockPlus::JsEngine::EventCallback eCallback =
On 2017/04/26 09:20:59, sergei wrote:
> the `const` is not required here.

Acknowledged.

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniJsEngine.cpp:92: return NewJniJsValue(env,
jsValue);
On 2017/04/26 09:20:59, sergei wrote:
> to avoid call of a copy-ctr one has to either
> return NewJniJsValue(env, engine->Evaluate(source, filename));
> or
> return NewJniJsValue(env, move(jsValue));

Acknowledged.

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniJsEngine.cpp:97: static void JNICALL
JniTriggerEvent(JNIEnv* env, jclass clazz, jlong ptr, jstring jEventName, jarray
jJsPtrs)
On 2017/04/26 09:20:59, sergei wrote:
> I wonder we need it for. TriggerEvent is called only from JS, I doubt that
there
> is a need to call it from Java code, I would propose to remove such
> functionality from libadblockplus-android in some future changes.

this was written from the very beginning and i don't do any unrelated (and
undesired) changed for now

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniJsEngine.cpp:130: const AdblockPlus::FileSystemPtr
fileSystem(new AdblockPlus::DefaultFileSystem());
On 2017/04/26 09:20:59, sergei wrote:
> const is not required here.

Acknowledged.

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniJsEngine.cpp:146: const AdblockPlus::WebRequestPtr
webRequest(new AdblockPlus::DefaultWebRequest());
On 2017/04/26 09:20:59, sergei wrote:
> const is not required here.

Acknowledged.

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniJsEngine.cpp:171: const AdblockPlus::LogSystemPtr
logSystem = *JniLongToTypePtr<AdblockPlus::LogSystemPtr>(logSystemPtr);
On 2017/04/26 09:20:59, sergei wrote:
> const is not required here.

Acknowledged.

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniJsEngine.cpp:184: const
AdblockPlus::WebRequestPtr& webRequest =
On 2017/04/26 09:20:59, sergei wrote:
> const is not required here.

Acknowledged.

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniJsEngine.cpp:198: AdblockPlus::JsValue jsValue =
engine->NewValue(static_cast<int64_t>(value));
On 2017/04/26 09:20:59, sergei wrote:
> here and blow, see comment above to avoid copy-ctr.

Acknowledged.

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
File libadblockplus-android/jni/JniJsValue.h (right):

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniJsValue.h:34: AdblockPlus::JsValue*
JniGetJsValuePtr(jlong ptr);
as for me it's more logical "..Ptr()" to return pointer and ".." to return an
instance

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
File libadblockplus-android/jni/JniNotification.cpp (right):

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniNotification.cpp:40: return
JniLongToTypePtr<AdblockPlus::Notification>(ptr);
On 2017/04/26 09:21:00, sergei wrote:
> as far as I remember we store the pointer in Java JsValue.
> 
> Strictly speaking before passing the pointer to JsValue it should be converted
> to `JsValue*` (the best way would be without cutting corners, namely accept a
> pointer to Notification in Java Notification and before passing it to base
Java
> JsValue it should be casted to pointer to JsValue*) and here we need to cast
it
> back as
> static_cast<AdblockPlus::Notification*>(JniGetJsValuePtr(ptr));
> 
> It's also valid for Filter and Subscription.
> 
> To tell the truth I don't like the idea when we publicly derive from JsValue,
so
> I would rather propose to think in advance and minimize the usage of this
fact.

No, we do create the same object class as it was passed to NewObject() and now
it's concrete class (not base JsValue).
Same for Filter and Subscriptions

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
File libadblockplus-android/jni/Utils.cpp (right):

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/Utils.cpp:167: static jobject NewJniObject(JNIEnv*
env, const T& value, jclass clazz, jmethodID ctor)
On 2017/04/26 09:21:00, sergei wrote:
> `const T& value` -> `T&& value`

Acknowledged.

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/Utils.cpp:169: return env->NewObject(clazz, ctor,
JniPtrToLong(new T(value)));
On 2017/04/26 09:21:00, sergei wrote:
> return env->NewObject(clazz, ctor, JniPtrToLong(new
T(std::forward<T>(value))));

then we will have compile error: "Error:(169, 80) error: new cannot be applied
to a reference type"

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
File libadblockplus-android/jni/Utils.h (right):

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/Utils.h:190: jobject NewJniFilter(JNIEnv* env,
AdblockPlus::Filter& filter);
On 2017/04/26 09:21:00, sergei wrote:
> here and below the argument types should be const references, e.g. `const
> Filter&`. However, I think it's very important to actually add methods
accepting
> r-value references and move the value in the implementation (see comments for
> cpp file).
> 
> e.g.
> jobject NewJniFilter(JNIEnv* env, AdblockPlus::Filter&& filter);
> 
> PS:
> I would recommend to comment methods, accepting const reference, while making
> these changes and see where they are required from, most likely we can get rid
> of copying values. It will improve the efficiency.

I agree to have optimized methods but following is unclear:
engine returns instances (not pointers) then the content is moved to the new
instance (created by jni layer). why filter engine can't just return pointers
instead of objects? eg. `Subscription subscription =
filterEngine->getSubscription(); Subscription *copy = .. move(subscription)` -> 
`Subscription*`

sergei

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-android/jni/JniJsEngine.cpp File libadblockplus-android/jni/JniJsEngine.cpp (right): https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-android/jni/JniJsEngine.cpp#newcode97 libadblockplus-android/jni/JniJsEngine.cpp:97: static void JNICALL JniTriggerEvent(JNIEnv* env, jclass clazz, jlong ptr, ...

April 26, 2017, 2:12 p.m. (2017-04-26 14:12:57 UTC) #4

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
File libadblockplus-android/jni/JniJsEngine.cpp (right):

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniJsEngine.cpp:97: static void JNICALL
JniTriggerEvent(JNIEnv* env, jclass clazz, jlong ptr, jstring jEventName, jarray
jJsPtrs)
On 2017/04/26 10:22:59, anton wrote:
> On 2017/04/26 09:20:59, sergei wrote:
> > I wonder we need it for. TriggerEvent is called only from JS, I doubt that
> there
> > is a need to call it from Java code, I would propose to remove such
> > functionality from libadblockplus-android in some future changes.
> 
> this was written from the very beginning and i don't do any unrelated (and
> undesired) changed for now

I totally agree that it should not be in this codereview, but in general it
should be removed, maybe in some future codereview. I think we will soon make
method JsEngine::TriggerEvent not available for others, so compiler will help to
ensure that it's not used in an undesired way even accidentally.

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
File libadblockplus-android/jni/JniJsValue.cpp (right):

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniJsValue.cpp:171: jobject NewJniJsValue(JNIEnv*
env, const AdblockPlus::JsValue& jsValue, jclass jsValueClassArg)
On 2017/04/26 09:20:59, sergei wrote:
> The same (see Utils) regarding r-value is here. For the case with `JsValue&&
> jsValue` the call below should be `new JsValue(std::move(jsValue))`.

What about it?

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniJsValue.cpp:186: jobject
JniJsValueListToArrayList(JNIEnv* env, const AdblockPlus::JsValueList& list)
On 2017/04/26 09:21:00, sergei wrote:
> Here it would be also better to use `JsValueList& list` and below
> for (auto& entry : list)
> {
>   JniAddObjectToList(env, arrayList, *JniLocalReference<jobject>(env,
> NewJniJsValue(env, move(entry))));
> }

What about it?

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
File libadblockplus-android/jni/JniJsValue.h (right):

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniJsValue.h:34: AdblockPlus::JsValue*
JniGetJsValuePtr(jlong ptr);
On 2017/04/26 10:22:59, anton wrote:
> as for me it's more logical "..Ptr()" to return pointer and ".." to return an
> instance
I agree, why is this comment?

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
File libadblockplus-android/jni/JniNotification.cpp (right):

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniNotification.cpp:40: return
JniLongToTypePtr<AdblockPlus::Notification>(ptr);
On 2017/04/26 10:22:59, anton wrote:
> On 2017/04/26 09:21:00, sergei wrote:
> > as far as I remember we store the pointer in Java JsValue.
> > 
> > Strictly speaking before passing the pointer to JsValue it should be
converted
> > to `JsValue*` (the best way would be without cutting corners, namely accept
a
> > pointer to Notification in Java Notification and before passing it to base
> Java
> > JsValue it should be casted to pointer to JsValue*) and here we need to cast
> it
> > back as
> > static_cast<AdblockPlus::Notification*>(JniGetJsValuePtr(ptr));
> > 
> > It's also valid for Filter and Subscription.
> > 
> > To tell the truth I don't like the idea when we publicly derive from
JsValue,
> so
> > I would rather propose to think in advance and minimize the usage of this
> fact.
> 
> No, we do create the same object class as it was passed to NewObject() and now
> it's concrete class (not base JsValue).
> Same for Filter and Subscriptions

Let's consider an example with Notification.
Java class Notification derives from JsValue,
https://github.com/adblockplus/libadblockplus-android/blob/master/libadblockp...
It accepts a pointer to C++ Notification and passes it to Java JsValue as a
pointer to JsValue
https://github.com/adblockplus/libadblockplus-android/blob/master/libadblockp....
Here I think it should not pass pointer to Notification as a pointer to JsValue
even if it works right now.
Instead of `super(ptr);` it should be
`super(CastNotificationPtrToJsValuePtr(ptr));` and when we get this pointer back
from base Java JsValue we should cast JsValue* to Notification* using
static_cast. However, it would be good if you could tell me what methods of
JsValue are actually required to use Notification, Filter and Subscription, so
we can implement them on the these concrete classes and hide/remove the
inheritance from JsValue.

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
File libadblockplus-android/jni/Utils.cpp (right):

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/Utils.cpp:169: return env->NewObject(clazz, ctor,
JniPtrToLong(new T(value)));
On 2017/04/26 10:22:59, anton wrote:
> On 2017/04/26 09:21:00, sergei wrote:
> > return env->NewObject(clazz, ctor, JniPtrToLong(new
> T(std::forward<T>(value))));
> 
> then we will have compile error: "Error:(169, 80) error: new cannot be applied
> to a reference type"

That is strange, is it when the type of value arg is `T&&`?

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
File libadblockplus-android/jni/Utils.h (right):

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/Utils.h:190: jobject NewJniFilter(JNIEnv* env,
AdblockPlus::Filter& filter);
On 2017/04/26 10:22:59, anton wrote:
> On 2017/04/26 09:21:00, sergei wrote:
> > here and below the argument types should be const references, e.g. `const
> > Filter&`. However, I think it's very important to actually add methods
> accepting
> > r-value references and move the value in the implementation (see comments
for
> > cpp file).
> > 
> > e.g.
> > jobject NewJniFilter(JNIEnv* env, AdblockPlus::Filter&& filter);
> > 
> > PS:
> > I would recommend to comment methods, accepting const reference, while
making
> > these changes and see where they are required from, most likely we can get
rid
> > of copying values. It will improve the efficiency.
> 
> I agree to have optimized methods but following is unclear:
> engine returns instances (not pointers) then the content is moved to the new
> instance (created by jni layer). why filter engine can't just return pointers
> instead of objects? eg. `Subscription subscription =
> filterEngine->getSubscription(); Subscription *copy = .. move(subscription)`
-> 
> `Subscription*`

Because it's a bad design to return a pointer when we know that the value of the
pointer never can be nullptr, and in addition returning the pointer means an
additional memory allocation on a heap, however it's not required. If someone,
like this android code, has to move the object to the heap then it should do it.

What about methods accepting r-value references?

https://codereview.adblockplus.org/29422558/diff/29422601/libadblockplus-andr...
File libadblockplus-android/jni/JniJsEngine.cpp (right):

https://codereview.adblockplus.org/29422558/diff/29422601/libadblockplus-andr...
libadblockplus-android/jni/JniJsEngine.cpp:170: AdblockPlus::LogSystemPtr
logSystem =
it seems these changes are unrelated, do you mind to revert them?

anton

+ uploaded new patch set https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-android/jni/JniJsEngine.cpp File libadblockplus-android/jni/JniJsEngine.cpp (right): https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-android/jni/JniJsEngine.cpp#newcode97 libadblockplus-android/jni/JniJsEngine.cpp:97: static void JNICALL JniTriggerEvent(JNIEnv* ...

April 27, 2017, 6:21 a.m. (2017-04-27 06:21:01 UTC) #5

+ uploaded new patch set

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
File libadblockplus-android/jni/JniJsEngine.cpp (right):

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniJsEngine.cpp:97: static void JNICALL
JniTriggerEvent(JNIEnv* env, jclass clazz, jlong ptr, jstring jEventName, jarray
jJsPtrs)
On 2017/04/26 14:12:56, sergei wrote:
> On 2017/04/26 10:22:59, anton wrote:
> > On 2017/04/26 09:20:59, sergei wrote:
> > > I wonder we need it for. TriggerEvent is called only from JS, I doubt that
> > there
> > > is a need to call it from Java code, I would propose to remove such
> > > functionality from libadblockplus-android in some future changes.
> > 
> > this was written from the very beginning and i don't do any unrelated (and
> > undesired) changed for now
> 
> I totally agree that it should not be in this codereview, but in general it
> should be removed, maybe in some future codereview. I think we will soon make
> method JsEngine::TriggerEvent not available for others, so compiler will help
to
> ensure that it's not used in an undesired way even accidentally.

feel free to add a ticket to libadblockplus-android

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
File libadblockplus-android/jni/JniJsValue.cpp (right):

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniJsValue.cpp:186: jobject
JniJsValueListToArrayList(JNIEnv* env, const AdblockPlus::JsValueList& list)
On 2017/04/26 14:12:56, sergei wrote:
> On 2017/04/26 09:21:00, sergei wrote:
> > Here it would be also better to use `JsValueList& list` and below
> > for (auto& entry : list)
> > {
> >   JniAddObjectToList(env, arrayList, *JniLocalReference<jobject>(env,
> > NewJniJsValue(env, move(entry))));
> > }
> 
> What about it?

Acknowledged. Now moving in NewJniObject(..){ ... std::move() ... }

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniJsValue.cpp:208: //JsValue Call(const JsValueList&
params = JsValueList(), AdblockPlus::JsValue thisPtr = AdblockPlus::JsValue())
const;
On 2017/04/26 09:21:00, sergei wrote:
> I think the types of JsValue now are slightly different, but it's fine me.

i wil revert this

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
File libadblockplus-android/jni/JniJsValue.h (right):

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniJsValue.h:34: AdblockPlus::JsValue*
JniGetJsValuePtr(jlong ptr);
On 2017/04/26 14:12:56, sergei wrote:
> On 2017/04/26 10:22:59, anton wrote:
> > as for me it's more logical "..Ptr()" to return pointer and ".." to return
an
> > instance
> I agree, why is this comment?

It's just explanation why 2 methods changed the names to each other

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
File libadblockplus-android/jni/JniNotification.cpp (right):

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/JniNotification.cpp:40: return
JniLongToTypePtr<AdblockPlus::Notification>(ptr);
On 2017/04/26 14:12:56, sergei wrote:
> On 2017/04/26 10:22:59, anton wrote:
> > On 2017/04/26 09:21:00, sergei wrote:
> > > as far as I remember we store the pointer in Java JsValue.
> > > 
> > > Strictly speaking before passing the pointer to JsValue it should be
> converted
> > > to `JsValue*` (the best way would be without cutting corners, namely
accept
> a
> > > pointer to Notification in Java Notification and before passing it to base
> > Java
> > > JsValue it should be casted to pointer to JsValue*) and here we need to
cast
> > it
> > > back as
> > > static_cast<AdblockPlus::Notification*>(JniGetJsValuePtr(ptr));
> > > 
> > > It's also valid for Filter and Subscription.
> > > 
> > > To tell the truth I don't like the idea when we publicly derive from
> JsValue,
> > so
> > > I would rather propose to think in advance and minimize the usage of this
> > fact.
> > 
> > No, we do create the same object class as it was passed to NewObject() and
now
> > it's concrete class (not base JsValue).
> > Same for Filter and Subscriptions
> 
> Let's consider an example with Notification.
> Java class Notification derives from JsValue,
>
https://github.com/adblockplus/libadblockplus-android/blob/master/libadblockp...
> It accepts a pointer to C++ Notification and passes it to Java JsValue as a
> pointer to JsValue
>
https://github.com/adblockplus/libadblockplus-android/blob/master/libadblockp....
> Here I think it should not pass pointer to Notification as a pointer to
JsValue
> even if it works right now.
> Instead of `super(ptr);` it should be
> `super(CastNotificationPtrToJsValuePtr(ptr));` and when we get this pointer
back
> from base Java JsValue we should cast JsValue* to Notification* using
> static_cast. However, it would be good if you could tell me what methods of
> JsValue are actually required to use Notification, Filter and Subscription, so
> we can implement them on the these concrete classes and hide/remove the
> inheritance from JsValue.

We pass just `long` value to Java side without any type specification and it's
up to c++ jni side to cast it to correct type.
So we do create Notification* ptr and then cast it as Notification* ptr later:
https://github.com/adblockplus/libadblockplus-android/blob/8c60759bc1df5a0b04....

Same for Filter and Subscription.

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
File libadblockplus-android/jni/Utils.h (right):

https://codereview.adblockplus.org/29422558/diff/29422573/libadblockplus-andr...
libadblockplus-android/jni/Utils.h:190: jobject NewJniFilter(JNIEnv* env,
AdblockPlus::Filter& filter);
On 2017/04/26 14:12:56, sergei wrote:
> On 2017/04/26 10:22:59, anton wrote:
> > On 2017/04/26 09:21:00, sergei wrote:
> > > here and below the argument types should be const references, e.g. `const
> > > Filter&`. However, I think it's very important to actually add methods
> > accepting
> > > r-value references and move the value in the implementation (see comments
> for
> > > cpp file).
> > > 
> > > e.g.
> > > jobject NewJniFilter(JNIEnv* env, AdblockPlus::Filter&& filter);
> > > 
> > > PS:
> > > I would recommend to comment methods, accepting const reference, while
> making
> > > these changes and see where they are required from, most likely we can get
> rid
> > > of copying values. It will improve the efficiency.
> > 
> > I agree to have optimized methods but following is unclear:
> > engine returns instances (not pointers) then the content is moved to the new
> > instance (created by jni layer). why filter engine can't just return
pointers
> > instead of objects? eg. `Subscription subscription =
> > filterEngine->getSubscription(); Subscription *copy = .. move(subscription)`
> -> 
> > `Subscription*`
> 
> Because it's a bad design to return a pointer when we know that the value of
the
> pointer never can be nullptr, and in addition returning the pointer means an
> additional memory allocation on a heap, however it's not required. If someone,
> like this android code, has to move the object to the heap then it should do
it.
> 
> What about methods accepting r-value references?

i will use r-values and move ctors

https://codereview.adblockplus.org/29422558/diff/29422601/libadblockplus-andr...
File libadblockplus-android/jni/JniJsEngine.cpp (right):

https://codereview.adblockplus.org/29422558/diff/29422601/libadblockplus-andr...
libadblockplus-android/jni/JniJsEngine.cpp:170: AdblockPlus::LogSystemPtr
logSystem =
On 2017/04/26 14:12:56, sergei wrote:
> it seems these changes are unrelated, do you mind to revert them?

Acknowledged.

sergei

https://codereview.adblockplus.org/29422558/diff/29430560/libadblockplus-android/jni/JniFilterEngine.cpp File libadblockplus-android/jni/JniFilterEngine.cpp (right): https://codereview.adblockplus.org/29422558/diff/29430560/libadblockplus-android/jni/JniFilterEngine.cpp#newcode328 libadblockplus-android/jni/JniFilterEngine.cpp:328: return filterPtr.get() ? NewJniFilter(env, std::move(*filterPtr)) : 0; Actually here, ...

May 5, 2017, 1:05 p.m. (2017-05-05 13:05:14 UTC) #7

sergei

https://codereview.adblockplus.org/29422558/diff/29430560/dependencies File dependencies (right): https://codereview.adblockplus.org/29422558/diff/29430560/dependencies#newcode4 dependencies:4: libadblockplus-android/jni/libadblockplus-binaries = libadblockplus-binaries hg:a40587693658 git:a528bf6 BTW, I think it ...

May 8, 2017, 8:15 a.m. (2017-05-08 08:15:15 UTC) #8

diegocarloslima

On 2017/05/08 08:15:15, sergei wrote: > https://codereview.adblockplus.org/29422558/diff/29430560/dependencies > File dependencies (right): > > https://codereview.adblockplus.org/29422558/diff/29430560/dependencies#newcode4 > ...

May 8, 2017, 9:54 p.m. (2017-05-08 21:54:40 UTC) #9

anton

May 10, 2017, 6:56 a.m. (2017-05-10 06:56:31 UTC) #10

sergei

May 10, 2017, 11:06 a.m. (2017-05-10 11:06:49 UTC) #11

Message was sent while issue was closed.

On 2017/05/10 06:56:31, anton wrote:
> On 2017/05/08 08:15:15, sergei wrote:
> > https://codereview.adblockplus.org/29422558/diff/29430560/dependencies
> > File dependencies (right):
> > 
> >
>
https://codereview.adblockplus.org/29422558/diff/29430560/dependencies#newcode4
> > dependencies:4: libadblockplus-android/jni/libadblockplus-binaries =
> > libadblockplus-binaries hg:a40587693658 git:a528bf6
> > BTW, I think it makes a little sense to use some short version of a commit
ID
> > instead of a full version. It's maybe fine so far but I would recommend to
use
> > the full one.
> 
> Well, it's processed by our scripts and it was meant to be used by human,  so
i
> believe it does not make sense.
> BTW i've uploaded new patch set

Our script finally feeds it to git which can use even 3-4 first chars but it
will very soon result in a conflict, the probability of conflict for first 7
chars is also pretty high BTW. What concerns designed for human, for me it's
easier to verify first 3 characters and last 3 characters instead of first 7-10
characters. I think it's also easier to work with full length commit ID because
one does not have to edit it in addition, just double click (what automatically
fully selects it) and copy-paste and every one can be sure there are no weird
things.

Expand All Messages | Collapse All Messages