Issue 29678590: Issue 6307 - Introduce external engine created callback (Closed)
|
Created:
Jan. 24, 2018, 12:46 p.m. by anton Modified:
Jan. 26, 2018, 1:22 p.m. CC:
René Jeschke, sergei Visibility:
Public. |
DescriptionIssue 6307 - Introduce external engine created callback
Patch Set 1 #
Total comments: 9
Patch Set 2 : added 'clear..()' methods, updated README #
Total comments: 1
Patch Set 3 : introduced custom listener interfaces #
Total comments: 1
MessagesTotal messages: 16
https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:51: private final Runnable engineCreatedCallback = new Runnable() We could accept Runnable in some `AdblockHelper` method and fire it after this one is fired. But since we already have callback in `SingleInstanceEngineProvider`, i preferred to convert callback ref to list of callbacks instead.
https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java (right): https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java:100: public void removeEngineCreatedCallback(Runnable callback) "remove.." methods are not used anywhere but could be used. also we could add "clear.." methods too for consistency. What do you think?
looks good https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java (right): https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java:100: public void removeEngineCreatedCallback(Runnable callback) On 2018/01/24 12:51:08, anton wrote: > "remove.." methods are not used anywhere but could be used. > also we could add "clear.." methods too for consistency. > What do you think? I would say if someone wants to remove or clear then just create a new instance of SingleInstanceEngineProvider and abandon the present one.
On 2018/01/24 13:33:02, sergei wrote: > looks good > > https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... > File > libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java > (right): > > https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... > libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java:100: > public void removeEngineCreatedCallback(Runnable callback) > On 2018/01/24 12:51:08, anton wrote: > > "remove.." methods are not used anywhere but could be used. > > also we could add "clear.." methods too for consistency. > > What do you think? > > I would say if someone wants to remove or clear then just create a new instance > of SingleInstanceEngineProvider and abandon the present one. If it's created in AdblockHelper it can be undesired to clear new instance just because of missing methods.
On 2018/01/24 13:34:44, anton wrote: > If it's created in AdblockHelper it can be undesired to clear new instance just > because of missing methods. *clear -> create
https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... File libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java (right): https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java:38: // put your Adblock FilterEngine init here Should we provide some more information/context here or in the README (sample code)? https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java:47: // put your Adblock FilterEngine deinit here Should we provide some more information/context here or in the README (sample code)? https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java (right): https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java:100: public void removeEngineCreatedCallback(Runnable callback) On 2018/01/24 12:51:08, anton wrote: > "remove.." methods are not used anywhere but could be used. > also we could add "clear.." methods too for consistency. > What do you think? Even if Sergejs idea is also fine I would prefer to be consistent and add a clear method too.
https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... File libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java (right): https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java:38: // put your Adblock FilterEngine init here On 2018/01/26 08:15:22, jens wrote: > Should we provide some more information/context here or in the README (sample > code)? Acknowledged. https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java:47: // put your Adblock FilterEngine deinit here On 2018/01/26 08:15:22, jens wrote: > Should we provide some more information/context here or in the README (sample > code)? Acknowledged. https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java (right): https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java:100: public void removeEngineCreatedCallback(Runnable callback) On 2018/01/26 08:15:22, jens wrote: > On 2018/01/24 12:51:08, anton wrote: > > "remove.." methods are not used anywhere but could be used. > > also we could add "clear.." methods too for consistency. > > What do you think? > > Even if Sergejs idea is also fine I would prefer to be consistent and add a > clear method too. Acknowledged.
On 2018/01/26 08:22:59, anton wrote: > https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... > File > libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java > (right): > > https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... > libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java:38: > // put your Adblock FilterEngine init here > On 2018/01/26 08:15:22, jens wrote: > > Should we provide some more information/context here or in the README (sample > > code)? > > Acknowledged. > > https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... > libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java:47: > // put your Adblock FilterEngine deinit here > On 2018/01/26 08:15:22, jens wrote: > > Should we provide some more information/context here or in the README (sample > > code)? > > Acknowledged. > > https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... > File > libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java > (right): > > https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... > libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java:100: > public void removeEngineCreatedCallback(Runnable callback) > On 2018/01/26 08:15:22, jens wrote: > > On 2018/01/24 12:51:08, anton wrote: > > > "remove.." methods are not used anywhere but could be used. > > > also we could add "clear.." methods too for consistency. > > > What do you think? > > > > Even if Sergejs idea is also fine I would prefer to be consistent and add a > > clear method too. > > Acknowledged. See patch set #2
On 2018/01/26 08:25:02, anton wrote: > On 2018/01/26 08:22:59, anton wrote: > > > https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... > > File > > > libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java > > (right): > > > > > https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... > > > libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java:38: > > // put your Adblock FilterEngine init here > > On 2018/01/26 08:15:22, jens wrote: > > > Should we provide some more information/context here or in the README > (sample > > > code)? > > > > Acknowledged. > > > > > https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... > > > libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/Application.java:47: > > // put your Adblock FilterEngine deinit here > > On 2018/01/26 08:15:22, jens wrote: > > > Should we provide some more information/context here or in the README > (sample > > > code)? > > > > Acknowledged. > > > > > https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... > > File > > > libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java > > (right): > > > > > https://codereview.adblockplus.org/29678590/diff/29678591/libadblockplus-andr... > > > libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java:100: > > public void removeEngineCreatedCallback(Runnable callback) > > On 2018/01/26 08:15:22, jens wrote: > > > On 2018/01/24 12:51:08, anton wrote: > > > > "remove.." methods are not used anywhere but could be used. > > > > also we could add "clear.." methods too for consistency. > > > > What do you think? > > > > > > Even if Sergejs idea is also fine I would prefer to be consistent and add a > > > clear method too. > > > > Acknowledged. > > See patch set #2 LGTM
https://codereview.adblockplus.org/29678590/diff/29680632/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java (right): https://codereview.adblockplus.org/29678590/diff/29680632/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java:48: private List<Runnable> engineDisposedCallbacks = new LinkedList<Runnable>(); I would prefer the callbacks to be interfaces instead of Runnable instances. Also, for convenience I think that the OnEngineCreated(Listener/Callback) interface should pass an engine instance as parameter
On 2018/01/26 12:30:01, diegocarloslima wrote: > https://codereview.adblockplus.org/29678590/diff/29680632/libadblockplus-andr... > File > libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java > (right): > > https://codereview.adblockplus.org/29678590/diff/29680632/libadblockplus-andr... > libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java:48: > private List<Runnable> engineDisposedCallbacks = new LinkedList<Runnable>(); > I would prefer the callbacks to be interfaces instead of Runnable instances. > Also, for convenience I think that the OnEngineCreated(Listener/Callback) > interface should pass an engine instance as parameter see new patch set #3
https://codereview.adblockplus.org/29678590/diff/29680662/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java (right): https://codereview.adblockplus.org/29678590/diff/29680662/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java:60: new LinkedList<EngineDisposedListener>(); Just one minor thing, the lists could be final
On 2018/01/26 13:04:28, diegocarloslima wrote: > https://codereview.adblockplus.org/29678590/diff/29680662/libadblockplus-andr... > File > libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java > (right): > > https://codereview.adblockplus.org/29678590/diff/29680662/libadblockplus-andr... > libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java:60: > new LinkedList<EngineDisposedListener>(); > Just one minor thing, the lists could be final LGTM with a small remark regarding the listeners list declaration that could be final.
On 2018/01/26 13:05:47, diegocarloslima wrote: > On 2018/01/26 13:04:28, diegocarloslima wrote: > > > https://codereview.adblockplus.org/29678590/diff/29680662/libadblockplus-andr... > > File > > > libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java > > (right): > > > > > https://codereview.adblockplus.org/29678590/diff/29680662/libadblockplus-andr... > > > libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java:60: > > new LinkedList<EngineDisposedListener>(); > > Just one minor thing, the lists could be final > > LGTM with a small remark regarding the listeners list declaration that could be > final. created separate ticket for it: https://issues.adblockplus.org/ticket/6319
On 2018/01/26 13:07:33, anton wrote: > On 2018/01/26 13:05:47, diegocarloslima wrote: > > On 2018/01/26 13:04:28, diegocarloslima wrote: > > > > > > https://codereview.adblockplus.org/29678590/diff/29680662/libadblockplus-andr... > > > File > > > > > > libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java > > > (right): > > > > > > > > > https://codereview.adblockplus.org/29678590/diff/29680662/libadblockplus-andr... > > > > > > libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.java:60: > > > new LinkedList<EngineDisposedListener>(); > > > Just one minor thing, the lists could be final > > > > LGTM with a small remark regarding the listeners list declaration that could > be > > final. > > created separate ticket for it: https://issues.adblockplus.org/ticket/6319 LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
