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

Issue 29465626: Noissue - move work with a raw jni pointer to a separate function (Closed)

Created:
June 14, 2017, 2:45 p.m. by sergei
Modified:
July 6, 2017, 6:36 a.m.
CC:
Felix Dahlke
Base URL:
github.com:abby-sergz/libadblockplus-android.git
Visibility:
Public.

Description

It helps to keep it in one place and reduces the number of changes to only that function if the pointer is being changed. Review: https://codereview.adblockplus.org/29465626/ COLLABORATOR=anton@adblockplus.org

Patch Set 1 #

Total comments: 5

Patch Set 2 : address comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+51 lines, -46 lines) Patch
M libadblockplus-android/jni/JniFilterEngine.cpp View 1 28 chunks +35 lines, -35 lines 0 comments Download
M libadblockplus-android/jni/JniJsEngine.cpp View 12 chunks +16 lines, -11 lines 0 comments Download

Messages

Total messages: 9
sergei
June 14, 2017, 3:09 p.m. (2017-06-14 15:09:13 UTC) #1
anton
https://codereview.adblockplus.org/29465626/diff/29465627/libadblockplus-android/jni/JniFilterEngine.cpp File libadblockplus-android/jni/JniFilterEngine.cpp (right): https://codereview.adblockplus.org/29465626/diff/29465627/libadblockplus-android/jni/JniFilterEngine.cpp#newcode48 libadblockplus-android/jni/JniFilterEngine.cpp:48: namespace can we have the same code style like ...
June 15, 2017, 5:16 a.m. (2017-06-15 05:16:53 UTC) #2
sergei
https://codereview.adblockplus.org/29465626/diff/29465627/libadblockplus-android/jni/JniFilterEngine.cpp File libadblockplus-android/jni/JniFilterEngine.cpp (right): https://codereview.adblockplus.org/29465626/diff/29465627/libadblockplus-android/jni/JniFilterEngine.cpp#newcode48 libadblockplus-android/jni/JniFilterEngine.cpp:48: namespace On 2017/06/15 05:16:53, anton wrote: > can we ...
June 16, 2017, 10:44 a.m. (2017-06-16 10:44:36 UTC) #3
anton
On 2017/06/16 10:44:36, sergei wrote: > https://codereview.adblockplus.org/29465626/diff/29465627/libadblockplus-android/jni/JniFilterEngine.cpp > File libadblockplus-android/jni/JniFilterEngine.cpp (right): > > https://codereview.adblockplus.org/29465626/diff/29465627/libadblockplus-android/jni/JniFilterEngine.cpp#newcode48 > ...
June 16, 2017, 11:41 a.m. (2017-06-16 11:41:59 UTC) #4
sergei
Could you please review it.
July 3, 2017, 7:55 a.m. (2017-07-03 07:55:43 UTC) #5
jens
On 2017/07/03 07:55:43, sergei wrote: > Could you please review it. LGTM
July 5, 2017, 9:27 a.m. (2017-07-05 09:27:40 UTC) #6
jens
https://codereview.adblockplus.org/29465626/diff/29465627/libadblockplus-android/jni/JniFilterEngine.cpp File libadblockplus-android/jni/JniFilterEngine.cpp (right): https://codereview.adblockplus.org/29465626/diff/29465627/libadblockplus-android/jni/JniFilterEngine.cpp#newcode368 libadblockplus-android/jni/JniFilterEngine.cpp:368: AdblockPlus::FilterEnginePtr& engine = Minor one, but why don't have ...
July 5, 2017, 9:27 a.m. (2017-07-05 09:27:53 UTC) #7
anton
https://codereview.adblockplus.org/29465626/diff/29465627/libadblockplus-android/jni/JniFilterEngine.cpp File libadblockplus-android/jni/JniFilterEngine.cpp (right): https://codereview.adblockplus.org/29465626/diff/29465627/libadblockplus-android/jni/JniFilterEngine.cpp#newcode368 libadblockplus-android/jni/JniFilterEngine.cpp:368: AdblockPlus::FilterEnginePtr& engine = On 2017/07/05 09:27:53, jens wrote: > ...
July 5, 2017, 9:30 a.m. (2017-07-05 09:30:32 UTC) #8
sergei
July 5, 2017, 1:46 p.m. (2017-07-05 13:46:20 UTC) #9
https://codereview.adblockplus.org/29465626/diff/29465627/libadblockplus-andr...
File libadblockplus-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29465626/diff/29465627/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:368:
AdblockPlus::FilterEnginePtr& engine =
On 2017/07/05 09:30:32, anton wrote:
> On 2017/07/05 09:27:53, jens wrote:
> > Minor one, but why don't have the definition of engine in only one line,
like
> in
> > all other cases?
> 
> yes, i believe previously the line was too long to fit into 100 characters.
> now we can (like it's done in other places) and i'd say we should.

Right, I guess it's been simply overlooked when I was reverting other changes.

Powered by Google App Engine
This is Rietveld