Issue 29523679: Issue 5556 - use JsEngine& instead of JsEnginePtr because JsEnginePtr is not available anymore (Closed)
|
Created:
Aug. 22, 2017, 2:37 p.m. by sergei Modified:
Aug. 25, 2017, 7:33 a.m. CC:
René Jeschke, Felix Dahlke Base URL:
github.com:abby-sergz/libadblockplus-android.git Visibility:
Public. |
Patch Set 1 #
Total comments: 2
Patch Set 2 : revert jsEngine<>engine #MessagesTotal messages: 6
It's a small self-contained step in order to accomplish #5556.
https://codereview.adblockplus.org/29523679/diff/29523680/libadblockplus-andr... File libadblockplus-android/jni/JniJsEngine.cpp (right): https://codereview.adblockplus.org/29523679/diff/29523680/libadblockplus-andr... libadblockplus-android/jni/JniJsEngine.cpp:66: AdblockPlus::JsEngine& jsEngine = GetJsEngineRef(ptr); in code review https://codereview.adblockplus.org/29523682/ you did not rename variable 'engine' to 'filterEngine' but here you did it. actually i don't see any [significant] reason as there are no other engines (eg. filter engine instance) in methods. Let's have consistent code style and revert renaming here or rename vars there.
https://codereview.adblockplus.org/29523679/diff/29523680/libadblockplus-andr... File libadblockplus-android/jni/JniJsEngine.cpp (right): https://codereview.adblockplus.org/29523679/diff/29523680/libadblockplus-andr... libadblockplus-android/jni/JniJsEngine.cpp:66: AdblockPlus::JsEngine& jsEngine = GetJsEngineRef(ptr); On 2017/08/23 05:46:47, anton wrote: > in code review https://codereview.adblockplus.org/29523682/ you did not rename > variable 'engine' to 'filterEngine' but here you did it. > actually i don't see any [significant] reason as there are no other engines (eg. > filter engine instance) in methods. > > Let's have consistent code style and revert renaming here or rename vars there. Done.
On 2017/08/23 08:24:18, sergei wrote: > https://codereview.adblockplus.org/29523679/diff/29523680/libadblockplus-andr... > File libadblockplus-android/jni/JniJsEngine.cpp (right): > > https://codereview.adblockplus.org/29523679/diff/29523680/libadblockplus-andr... > libadblockplus-android/jni/JniJsEngine.cpp:66: AdblockPlus::JsEngine& jsEngine = > GetJsEngineRef(ptr); > On 2017/08/23 05:46:47, anton wrote: > > in code review https://codereview.adblockplus.org/29523682/ you did not rename > > variable 'engine' to 'filterEngine' but here you did it. > > actually i don't see any [significant] reason as there are no other engines > (eg. > > filter engine instance) in methods. > > > > Let's have consistent code style and revert renaming here or rename vars > there. > > Done. LGTM
On 2017/08/23 08:43:26, anton wrote: > On 2017/08/23 08:24:18, sergei wrote: > > > https://codereview.adblockplus.org/29523679/diff/29523680/libadblockplus-andr... > > File libadblockplus-android/jni/JniJsEngine.cpp (right): > > > > > https://codereview.adblockplus.org/29523679/diff/29523680/libadblockplus-andr... > > libadblockplus-android/jni/JniJsEngine.cpp:66: AdblockPlus::JsEngine& jsEngine > = > > GetJsEngineRef(ptr); > > On 2017/08/23 05:46:47, anton wrote: > > > in code review https://codereview.adblockplus.org/29523682/ you did not > rename > > > variable 'engine' to 'filterEngine' but here you did it. > > > actually i don't see any [significant] reason as there are no other engines > > (eg. > > > filter engine instance) in methods. > > > > > > Let's have consistent code style and revert renaming here or rename vars > > there. > > > > Done. > > LGTM LGTM
On 2017/08/23 08:43:26, anton wrote: > On 2017/08/23 08:24:18, sergei wrote: > > > https://codereview.adblockplus.org/29523679/diff/29523680/libadblockplus-andr... > > File libadblockplus-android/jni/JniJsEngine.cpp (right): > > > > > https://codereview.adblockplus.org/29523679/diff/29523680/libadblockplus-andr... > > libadblockplus-android/jni/JniJsEngine.cpp:66: AdblockPlus::JsEngine& jsEngine > = > > GetJsEngineRef(ptr); > > On 2017/08/23 05:46:47, anton wrote: > > > in code review https://codereview.adblockplus.org/29523682/ you did not > rename > > > variable 'engine' to 'filterEngine' but here you did it. > > > actually i don't see any [significant] reason as there are no other engines > > (eg. > > > filter engine instance) in methods. > > > > > > Let's have consistent code style and revert renaming here or rename vars > > there. > > > > Done. > > LGTM Pushed 
|
