Commit 28da6a5c authored by Grisha Kruglov's avatar Grisha Kruglov Committed by Grisha Kruglov
Browse files

Perform browser-toolbar autocompletion off the UI thread

At this point in the stack, we're not in control over what our
autocomplete providers are, what actions they'll do in order to
field our queries, etc. For example, some providers may hit the disk
and perform expensive DB queries internally. Some may even hit the
network, in theory!

In order to keep things perceptively speedy, let's run the actual work
off the main thread. This patch sets up a new pool thread to process
autocomplete requests. More than one thread is selected so that we maintain
liveliness during quick user input. Background tasks are cancelled as new
queries come in, and stale results are discarded.
parent 1f942bd7
......@@ -11,13 +11,14 @@ import android.util.AttributeSet
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Job
import kotlinx.coroutines.asCoroutineDispatcher
import kotlinx.coroutines.async
import kotlinx.coroutines.launch
import kotlinx.coroutines.newFixedThreadPoolContext
import mozilla.components.browser.awesomebar.layout.SuggestionLayout
import mozilla.components.browser.awesomebar.transform.SuggestionTransformer
import mozilla.components.concept.awesomebar.AwesomeBar
import mozilla.components.support.ktx.android.content.res.pxToDp
import java.util.concurrent.Executors
private const val PROVIDER_QUERY_THREADS = 3
......@@ -35,7 +36,7 @@ class BrowserAwesomeBar @JvmOverloads constructor(
attrs: AttributeSet? = null,
defStyleAttr: Int = 0
) : RecyclerView(context, attrs, defStyleAttr), AwesomeBar {
private val jobDispatcher = newFixedThreadPoolContext(PROVIDER_QUERY_THREADS, "AwesomeBarProviders")
private val jobDispatcher = Executors.newFixedThreadPool(PROVIDER_QUERY_THREADS).asCoroutineDispatcher()
private val providers: MutableList<AwesomeBar.SuggestionProvider> = mutableListOf()
internal var suggestionsAdapter = SuggestionsAdapter(this)
internal var scope = CoroutineScope(Dispatchers.Main)
......
......@@ -73,6 +73,7 @@ abstract class BaseDomainAutocompleteProvider(private val list: DomainList) :
val wwwDomain = "www.${it.host}"
if (wwwDomain.startsWith(searchText)) {
return DomainAutocompleteResult(
input = searchText,
text = getResultText(query, wwwDomain),
url = it.url,
source = list.listName,
......@@ -82,6 +83,7 @@ abstract class BaseDomainAutocompleteProvider(private val list: DomainList) :
if (it.host.startsWith(searchText)) {
return DomainAutocompleteResult(
input = searchText,
text = getResultText(query, it.host),
url = it.url,
source = list.listName,
......@@ -106,12 +108,14 @@ abstract class BaseDomainAutocompleteProvider(private val list: DomainList) :
/**
* Describes an autocompletion result against a list of domains.
* @property input Input for which this result is being provided.
* @property text Result of autocompletion, text to be displayed.
* @property url Result of autocompletion, full matching url.
* @property source Name of the autocompletion source.
* @property totalItems A total number of results also available.
*/
class DomainAutocompleteResult(
val input: String,
val text: String,
val url: String,
val source: String,
......
......@@ -85,7 +85,7 @@ class InMemoryHistoryStorage : HistoryStorage {
override fun getAutocompleteSuggestion(query: String): HistoryAutocompleteResult? = synchronized(pages) {
segmentAwareDomainMatch(query, pages.keys)?.let { urlMatch ->
return HistoryAutocompleteResult(
urlMatch.matchedSegment, urlMatch.url, AUTOCOMPLETE_SOURCE_NAME, pages.size)
query, urlMatch.matchedSegment, urlMatch.url, AUTOCOMPLETE_SOURCE_NAME, pages.size)
}
}
......
......@@ -93,7 +93,13 @@ open class PlacesHistoryStorage(context: Context) : HistoryStorage, SyncableStor
val resultText = segmentAwareDomainMatch(query, arrayListOf(url))
return resultText?.let {
HistoryAutocompleteResult(it.matchedSegment, it.url, AUTOCOMPLETE_SOURCE_NAME, 1)
HistoryAutocompleteResult(
input = query,
text = it.matchedSegment,
url = it.url,
source = AUTOCOMPLETE_SOURCE_NAME,
totalItems = 1
)
}
}
......
......@@ -34,6 +34,7 @@ dependencies {
implementation Dependencies.support_appcompat
implementation Dependencies.support_design
implementation Dependencies.kotlin_stdlib
implementation Dependencies.kotlin_coroutines
testImplementation project(':support-test')
......
......@@ -14,6 +14,14 @@ import android.view.View
import android.view.View.OnFocusChangeListener
import android.view.ViewGroup
import android.widget.ImageButton
import kotlinx.coroutines.CoroutineExceptionHandler
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.asCoroutineDispatcher
import kotlinx.coroutines.cancelChildren
import kotlinx.coroutines.isActive
import kotlinx.coroutines.launch
import mozilla.components.browser.menu.BrowserMenuBuilder
import mozilla.components.browser.toolbar.display.DisplayToolbar
import mozilla.components.browser.toolbar.edit.EditToolbar
......@@ -21,10 +29,17 @@ import mozilla.components.concept.toolbar.AutocompleteDelegate
import mozilla.components.concept.toolbar.AutocompleteResult
import mozilla.components.concept.toolbar.Toolbar
import mozilla.components.support.base.android.Padding
import mozilla.components.support.base.log.logger.Logger
import mozilla.components.support.ktx.android.content.res.pxToDp
import mozilla.components.support.ktx.android.view.forEach
import mozilla.components.support.ktx.android.view.isVisible
import mozilla.components.ui.autocomplete.AutocompleteView
import mozilla.components.ui.autocomplete.InlineAutocompleteEditText
import mozilla.components.ui.autocomplete.OnFilterListener
import java.util.concurrent.Executors
import kotlin.coroutines.CoroutineContext
private const val AUTOCOMPLETE_QUERY_THREADS = 3
/**
* A customizable toolbar for browsers.
......@@ -50,11 +65,22 @@ class BrowserToolbar @JvmOverloads constructor(
attrs: AttributeSet? = null,
defStyleAttr: Int = 0
) : ViewGroup(context, attrs, defStyleAttr), Toolbar {
private val logger = Logger("BrowserToolbar")
// displayToolbar and editToolbar are only visible internally and mutable so that we can mock
// them in tests.
@VisibleForTesting internal var displayToolbar = DisplayToolbar(context, this)
@VisibleForTesting internal var editToolbar = EditToolbar(context, this)
private val autocompleteExceptionHandler = CoroutineExceptionHandler { _, throwable ->
logger.error("Error while processing autocomplete input", throwable)
}
private val autocompleteSupervisorJob = SupervisorJob()
private val autocompleteDispatcher = autocompleteSupervisorJob +
Executors.newFixedThreadPool(AUTOCOMPLETE_QUERY_THREADS).asCoroutineDispatcher() +
autocompleteExceptionHandler
/**
* Set/Get whether a site security icon (usually a lock or globe icon) should be next to the URL.
*/
......@@ -157,24 +183,14 @@ class BrowserToolbar @JvmOverloads constructor(
editToolbar.editListener = listener
}
override fun setAutocompleteListener(filter: (String, AutocompleteDelegate) -> Unit) {
override fun setAutocompleteListener(filter: suspend (String, AutocompleteDelegate) -> Unit) {
// Our 'filter' knows how to autocomplete, and the 'urlView' knows how to apply results of
// autocompletion. Which gives us a lovely delegate chain!
// urlView decides when it's appropriate to ask for autocompletion, and in turn we invoke
// our 'filter' and send results back to 'urlView'.
editToolbar.urlView.setOnFilterListener { text ->
filter.invoke(text, object : AutocompleteDelegate {
override fun applyAutocompleteResult(result: AutocompleteResult) {
editToolbar.urlView.applyAutocompleteResult(InlineAutocompleteEditText.AutocompleteResult(
text = result.text, source = result.source, totalItems = result.totalItems
))
}
override fun noAutocompleteResult() {
editToolbar.urlView.noAutocompleteResult()
}
})
}
editToolbar.urlView.setOnFilterListener(
AsyncFilterListener(editToolbar.urlView, autocompleteDispatcher, filter)
)
}
/**
......@@ -504,3 +520,77 @@ class BrowserToolbar @JvmOverloads constructor(
Padding(ACTION_PADDING_DP, ACTION_PADDING_DP, ACTION_PADDING_DP, ACTION_PADDING_DP)
}
}
/**
* Wraps [filter] execution in a coroutine context, cancelling prior executions on every invocation.
* [coroutineContext] must be of type that doesn't propagate cancellation of its children upwards.
*/
class AsyncFilterListener(
private val urlView: AutocompleteView,
override val coroutineContext: CoroutineContext,
private val filter: suspend (String, AutocompleteDelegate) -> Unit,
private val uiContext: CoroutineContext = Dispatchers.Main
) : OnFilterListener, CoroutineScope {
override fun invoke(text: String) {
// We got a new input, so whatever past autocomplete queries we still have running are
// irrelevant. We cancel them, but do not depend on cancellation to take place.
coroutineContext.cancelChildren()
CoroutineScope(coroutineContext).launch {
filter(text, AsyncAutocompleteDelegate(urlView, this, uiContext))
}
}
}
/**
* An autocomplete delegate which is aware of its parent scope (to check for cancellations).
* Responsible for processing autocompletion results and discarding stale results when [urlView] moved on.
*/
class AsyncAutocompleteDelegate(
private val urlView: AutocompleteView,
private val parentScope: CoroutineScope,
override val coroutineContext: CoroutineContext,
private val logger: Logger = Logger("AsyncAutocompleteDelegate")
) : AutocompleteDelegate, CoroutineScope {
override fun applyAutocompleteResult(result: AutocompleteResult) {
// Bail out if we were cancelled already.
if (!parentScope.isActive) {
logger.debug("Autocomplete request cancelled. Discarding results.")
return
}
// Process results on the UI dispatcher.
CoroutineScope(coroutineContext).launch {
// Ignore this result if the query is stale.
if (result.input == urlView.originalText) {
urlView.applyAutocompleteResult(
InlineAutocompleteEditText.AutocompleteResult(
text = result.text,
source = result.source,
totalItems = result.totalItems
)
)
} else {
logger.debug("Discarding stale autocomplete result.")
}
}
}
override fun noAutocompleteResult(input: String) {
// Bail out if we were cancelled already.
if (!parentScope.isActive) {
logger.debug("Autocomplete request cancelled. Discarding 'noAutocompleteResult'.")
return
}
// Process results on the UI thread.
CoroutineScope(coroutineContext).launch {
// Ignore this result if the query is stale.
if (input == urlView.originalText) {
urlView.noAutocompleteResult()
} else {
logger.debug("Discarding stale lack of autocomplete results.")
}
}
}
}
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
package mozilla.components.browser.toolbar
import kotlinx.coroutines.asCoroutineDispatcher
import kotlinx.coroutines.async
import kotlinx.coroutines.cancelChildren
import kotlinx.coroutines.isActive
import kotlinx.coroutines.runBlocking
import mozilla.components.concept.toolbar.AutocompleteDelegate
import mozilla.components.concept.toolbar.AutocompleteResult
import mozilla.components.support.test.mock
import mozilla.components.ui.autocomplete.AutocompleteView
import mozilla.components.ui.autocomplete.InlineAutocompleteEditText
import org.junit.Assert.assertEquals
import org.junit.Assert.fail
import org.junit.Test
import org.mockito.Mockito.atLeast
import org.mockito.Mockito.atLeastOnce
import org.mockito.Mockito.never
import org.mockito.Mockito.spy
import org.mockito.Mockito.verify
import java.util.concurrent.Executor
class AsyncFilterListenerTest {
@Test
fun `filter listener cancels prior filter executions`() = runBlocking {
val urlView: AutocompleteView = mock()
val filter: suspend (String, AutocompleteDelegate) -> Unit = mock()
val dispatcher = spy(Executor {
it.run()
}.asCoroutineDispatcher())
val listener = AsyncFilterListener(urlView, dispatcher, filter)
verify(dispatcher, never()).cancelChildren()
listener("test")
verify(dispatcher, atLeastOnce()).cancelChildren()
}
@Test
fun `filter delegate checks for cancellations before it runs, passes results to autocomplete view`() = runBlocking {
var filter: suspend (String, AutocompleteDelegate) -> Unit = { query, delegate ->
assertEquals("test", query)
delegate.applyAutocompleteResult(AutocompleteResult(
input = "test",
text = "testing.com",
url = "http://www.testing.com",
source = "asyncTest",
totalItems = 1
))
}
val dispatcher = spy(Executor {
it.run()
}.asCoroutineDispatcher())
var didCallApply = 0
var listener = AsyncFilterListener(object : AutocompleteView {
override val originalText: String = "test"
override fun applyAutocompleteResult(result: InlineAutocompleteEditText.AutocompleteResult) {
assertEquals("asyncTest", result.source)
assertEquals("testing.com", result.text)
assertEquals(1, result.totalItems)
didCallApply += 1
}
override fun noAutocompleteResult() {
fail()
}
}, dispatcher, filter, this.coroutineContext)
verify(dispatcher, never()).isActive
async { listener("test") }.await()
// Checked if parent scope is still active. Somehow, each access to 'isActive' registers as 4?
verify(dispatcher, atLeast(4)).isActive
// Passed the result to the view's apply method exactly once.
assertEquals(1, didCallApply)
filter = { query, delegate ->
assertEquals("moz", query)
delegate.applyAutocompleteResult(AutocompleteResult(
input = "moz",
text = "mozilla.com",
url = "http://www.mozilla.com",
source = "asyncTestTwo",
totalItems = 2
))
}
listener = AsyncFilterListener(object : AutocompleteView {
override val originalText: String = "moz"
override fun applyAutocompleteResult(result: InlineAutocompleteEditText.AutocompleteResult) {
assertEquals("asyncTestTwo", result.source)
assertEquals("mozilla.com", result.text)
assertEquals(2, result.totalItems)
didCallApply += 1
}
override fun noAutocompleteResult() {
fail()
}
}, dispatcher, filter, this.coroutineContext)
async { listener("moz") }.await()
verify(dispatcher, atLeast(8)).isActive
assertEquals(2, didCallApply)
}
@Test
fun `delegate discards stale results`() = runBlocking {
val filter: suspend (String, AutocompleteDelegate) -> Unit = { query, delegate ->
assertEquals("test", query)
delegate.applyAutocompleteResult(AutocompleteResult(
input = "test",
text = "testing.com",
url = "http://www.testing.com",
source = "asyncTest",
totalItems = 1
))
}
val dispatcher = Executor {
it.run()
}.asCoroutineDispatcher()
val listener = AsyncFilterListener(object : AutocompleteView {
override val originalText: String = "nolongertest"
override fun applyAutocompleteResult(result: InlineAutocompleteEditText.AutocompleteResult) {
fail()
}
override fun noAutocompleteResult() {
fail()
}
}, dispatcher, filter, this.coroutineContext)
listener("test")
}
@Test
fun `delegate discards stale lack of results`() = runBlocking {
val filter: suspend (String, AutocompleteDelegate) -> Unit = { query, delegate ->
assertEquals("test", query)
delegate.noAutocompleteResult("test")
}
val dispatcher = Executor {
it.run()
}.asCoroutineDispatcher()
val listener = AsyncFilterListener(object : AutocompleteView {
override val originalText: String = "nolongertest"
override fun applyAutocompleteResult(result: InlineAutocompleteEditText.AutocompleteResult) {
fail()
}
override fun noAutocompleteResult() {
fail()
}
}, dispatcher, filter, this.coroutineContext)
listener("test")
}
@Test
fun `delegate passes through non-stale lack of results`() = runBlocking {
val filter: suspend (String, AutocompleteDelegate) -> Unit = { query, delegate ->
assertEquals("test", query)
delegate.noAutocompleteResult("test")
}
val dispatcher = Executor {
it.run()
}.asCoroutineDispatcher()
var calledNoResults = 0
val listener = AsyncFilterListener(object : AutocompleteView {
override val originalText: String = "test"
override fun applyAutocompleteResult(result: InlineAutocompleteEditText.AutocompleteResult) {
fail()
}
override fun noAutocompleteResult() {
calledNoResults += 1
}
}, dispatcher, filter, this.coroutineContext)
async { listener("test") }.await()
assertEquals(1, calledNoResults)
}
@Test
fun `delegate discards results if parent scope was cancelled`() = runBlocking {
var preservedDelegate: AutocompleteDelegate? = null
val filter: suspend (String, AutocompleteDelegate) -> Unit = { query, delegate ->
preservedDelegate = delegate
assertEquals("test", query)
delegate.applyAutocompleteResult(AutocompleteResult(
input = "test",
text = "testing.com",
url = "http://www.testing.com",
source = "asyncTest",
totalItems = 1
))
}
val dispatcher = Executor {
it.run()
}.asCoroutineDispatcher()
var calledResults = 0
val listener = AsyncFilterListener(object : AutocompleteView {
override val originalText: String = "test"
override fun applyAutocompleteResult(result: InlineAutocompleteEditText.AutocompleteResult) {
assertEquals("asyncTest", result.source)
assertEquals("testing.com", result.text)
assertEquals(1, result.totalItems)
calledResults += 1
}
override fun noAutocompleteResult() {
fail()
}
}, dispatcher, filter, this.coroutineContext)
async {
listener("test")
listener("test")
}.await()
// This result application should be discarded, because scope has been cancelled by the second
// 'listener' call above.
preservedDelegate!!.applyAutocompleteResult(AutocompleteResult(
input = "test",
text = "testing.com",
url = "http://www.testing.com",
source = "asyncCancelled",
totalItems = 1
))
assertEquals(2, calledResults)
}
@Test
fun `delegate discards lack of results if parent scope was cancelled`() = runBlocking {
var preservedDelegate: AutocompleteDelegate? = null
val filter: suspend (String, AutocompleteDelegate) -> Unit = { query, delegate ->
preservedDelegate = delegate
assertEquals("test", query)
delegate.noAutocompleteResult("test")
}
val dispatcher = Executor {
it.run()
}.asCoroutineDispatcher()
var calledResults = 0
val listener = AsyncFilterListener(object : AutocompleteView {
override val originalText: String = "test"
override fun applyAutocompleteResult(result: InlineAutocompleteEditText.AutocompleteResult) {
fail()
}
override fun noAutocompleteResult() {
calledResults += 1
}
}, dispatcher, filter, this.coroutineContext)
async {
listener("test")
listener("test")
}.await()
// This "no results" call should be discarded, because scope has been cancelled by the second
// 'listener' call above.
preservedDelegate!!.noAutocompleteResult("test")
assertEquals(2, calledResults)
}
}
......@@ -4,6 +4,7 @@
package mozilla.components.browser.toolbar.edit
import kotlinx.coroutines.runBlocking
import mozilla.components.browser.toolbar.BrowserToolbar