Opened on 01/15/2018 at 06:54:12 AM
Closed on 01/22/2018 at 12:27:01 PM
Last modified on 01/23/2018 at 07:24:10 AM
#6265 closed change (fixed)
Create shared AdblockEngine instance in AdblockWebView in background
Reported by: | asmirnov | Assignee: | |
---|---|---|---|
Priority: | P2 | Milestone: | |
Module: | Libadblockplus-Android | Keywords: | |
Cc: | sergz, vickyyu | Blocked By: | |
Blocking: | #6299, #6301 | Platform: | Android |
Ready: | no | Confidential: | no |
Tester: | Unknown | Verified working: | no |
Review URL(s): |
Description (last modified by asmirnov)
Background
Creating AdblockEngine can take lot's of time (see #4432) and it blocks the thread. AdblockWebView can create it's own AdblockEngine instance to work as drop-in replacement for default Android WebView but every FilterEngine instance will take significant amount of memory so for our use-case AdblockWebView should:
- create instance of AdblockEngine on request (from background thread)
- share the (only) instance of AdblockEngine in AdblockWebView instances.
What to change
The way AdblockEngine is created and shared between AdblockWebView instances.
Attachments (0)
Change History (9)
comment:2 in reply to: ↑ 1 Changed on 01/15/2018 at 10:42:31 AM by asmirnov
Replying to sergz:
JIC I would like to leave a couple notes because some places sounds suspicious. IMO creation of AdblockWebView on request
Creating of "AdblockEngine", not "AdblockWebView" (sorry for confusion)
... (from background thread) can be not desired or even not possible in some circumstances. So, supposing that there is a factory which creates instances of WebView,
There is no factory, that creates AdblockWebView instances. If AdblockWebView is created from XML, it's up to android to create it while invoking setContentView(...)
in the synchronous case we should implement such factory which also manages the instance of AdblockEngine and injects the same AdblockEngine into each instance of AdblockWebView on creation. However, since we need an asynchronous initialization of AdblockEngine, the factory should be able to create AdblockEngine in a background thread when the factory is initialized and should either inject e.g. Future with AdblockEngine or send it when it's ready to all already created and newly created AdblockWebViews. The former looks much more attractive for me. In addition AdblockWebView should be apparently adjusted to operate with such asynchronously available AdblockEngine.
I will add you to code review reviewer, i brief it will look like that: AdblockWebView will accept AdblockEngineProvider instance that will create AdblockEngine synchronously or asynchronously (pretty close to AdblockHelper API at the moment). So actual instantiation will be delegated to AdblockEngineProvider.
comment:4 Changed on 01/15/2018 at 11:59:31 AM by vickyyu
- Cc vickyyu added
comment:5 Changed on 01/17/2018 at 11:57:34 AM by asmirnov
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:6 Changed on 01/22/2018 at 12:26:17 PM by abpbot
Some commits referencing this issue have landed:
comment:7 Changed on 01/22/2018 at 12:27:01 PM by asmirnov
- Resolution set to fixed
- Status changed from reviewing to closed
comment:8 Changed on 01/23/2018 at 06:09:58 AM by asmirnov
- Blocking 6299 added
comment:9 Changed on 01/23/2018 at 07:24:10 AM by asmirnov
- Blocking 6301 added
JIC I would like to leave a couple notes because some places sounds suspicious. IMO creation of AdblockWebView on request (from background thread) can be not desired or even not possible in some circumstances. So, supposing that there is a factory which creates instances of WebView, in the synchronous case we should implement such factory which also manages the instance of AdblockEngine and injects the same AdblockEngine into each instance of AdblockWebView on creation. However, since we need an asynchronous initialization of AdblockEngine, the factory should be able to create AdblockEngine in a background thread when the factory is initialized and should either inject e.g. Future with AdblockEngine or send it when it's ready to all already created and newly created AdblockWebViews. The former looks much more attractive for me. In addition AdblockWebView should be apparently adjusted to operate with such asynchronously available AdblockEngine.