Opened 22 months ago

Closed 22 months ago

Last modified 22 months ago

#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):

https://codereview.adblockplus.org/29671734/

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.

Change History (9)

comment:1 follow-up: Changed 22 months ago by sergz

  • Cc sergz 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.

comment:2 in reply to: ↑ 1 Changed 22 months ago 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.

Last edited 22 months ago by asmirnov (previous) (diff)

comment:3 Changed 22 months ago by asmirnov

  • Description modified (diff)

comment:4 Changed 22 months ago by vickyyu

  • Cc vickyyu added

comment:5 Changed 22 months ago by asmirnov

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:7 Changed 22 months ago by asmirnov

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:8 Changed 22 months ago by asmirnov

  • Blocking 6299 added

comment:9 Changed 22 months ago by asmirnov

  • Blocking 6301 added
Note: See TracTickets for help on using tickets.