Commit 3d3bd2b2 authored by Christian Sadilek's avatar Christian Sadilek
Browse files

Closes #1912: Call speculativeConnect from toolbar and awesomebar

parent 1e5bfb16
......@@ -465,7 +465,7 @@ private class AsyncAutocompleteDelegate(
override val coroutineContext: CoroutineContext,
private val logger: Logger = Logger("AsyncAutocompleteDelegate")
) : AutocompleteDelegate, CoroutineScope {
override fun applyAutocompleteResult(result: AutocompleteResult) {
override fun applyAutocompleteResult(result: AutocompleteResult, onApplied: () -> Unit) {
// Bail out if we were cancelled already.
if (!parentScope.isActive) {
logger.debug("Autocomplete request cancelled. Discarding results.")
......@@ -483,6 +483,7 @@ private class AsyncAutocompleteDelegate(
totalItems = result.totalItems
)
)
onApplied()
} else {
logger.debug("Discarding stale autocomplete result.")
}
......
......@@ -11,8 +11,11 @@ package mozilla.components.concept.toolbar
interface AutocompleteDelegate {
/**
* @param result Apply result of autocompletion.
* @param onApplied a lambda/callback invoked if (and only if) the result has been
* applied. A result may be discarded by implementations because it is stale or
* the autocomplete request has been cancelled.
*/
fun applyAutocompleteResult(result: AutocompleteResult)
fun applyAutocompleteResult(result: AutocompleteResult, onApplied: () -> Unit = { })
/**
* Autocompletion was invoked and no match was returned.
......
......@@ -12,8 +12,10 @@ import mozilla.components.browser.search.SearchEngine
import mozilla.components.browser.search.SearchEngineManager
import mozilla.components.browser.session.SessionManager
import mozilla.components.concept.awesomebar.AwesomeBar
import mozilla.components.concept.engine.Engine
import mozilla.components.concept.engine.EngineView
import mozilla.components.concept.fetch.Client
import mozilla.components.concept.storage.BookmarksStorage
import mozilla.components.concept.storage.HistoryStorage
import mozilla.components.concept.toolbar.Toolbar
import mozilla.components.feature.awesomebar.provider.ClipboardSuggestionProvider
......@@ -62,15 +64,25 @@ class AwesomeBarFeature(
/**
* Adds a [AwesomeBar.SuggestionProvider] for search engine suggestions to the [AwesomeBar].
*
* @param searchEngine The search engine to request suggestions from.
* @param searchUseCase The use case to invoke for searches.
* @param fetchClient The HTTP client for requesting suggestions from the search engine.
* @param limit The maximum number of suggestions that should be returned. It needs to be >= 1.
* @param mode Whether to return a single search suggestion (with chips) or one suggestion per item.
* @param engine optional [Engine] instance to call [Engine.speculativeConnect] for the
* highest scored search suggestion URL.
*/
@Suppress("LongParameterList")
fun addSearchProvider(
searchEngine: SearchEngine,
searchUseCase: SearchUseCases.SearchUseCase,
fetchClient: Client,
limit: Int = 15,
mode: SearchSuggestionProvider.Mode = SearchSuggestionProvider.Mode.SINGLE_SUGGESTION
mode: SearchSuggestionProvider.Mode = SearchSuggestionProvider.Mode.SINGLE_SUGGESTION,
engine: Engine? = null
): AwesomeBarFeature {
awesomeBar.addProviders(SearchSuggestionProvider(searchEngine, searchUseCase, fetchClient, limit, mode))
awesomeBar.addProviders(SearchSuggestionProvider(searchEngine, searchUseCase, fetchClient, limit, mode, engine))
return this
}
......@@ -79,6 +91,15 @@ class AwesomeBarFeature(
* If the default search engine is to be used for fetching search engine suggestions then
* this method is preferable over [addSearchProvider], as it will lazily load the default
* search engine using the provided [SearchEngineManager].
*
* @param context the activity or application context, required to load search engines.
* @param searchEngineManager The search engine manager to look up search engines.
* @param searchUseCase The use case to invoke for searches.
* @param fetchClient The HTTP client for requesting suggestions from the search engine.
* @param limit The maximum number of suggestions that should be returned. It needs to be >= 1.
* @param mode Whether to return a single search suggestion (with chips) or one suggestion per item.
* @param engine optional [Engine] instance to call [Engine.speculativeConnect] for the
* highest scored search suggestion URL.
*/
@Suppress("LongParameterList")
fun addSearchProvider(
......@@ -87,30 +108,47 @@ class AwesomeBarFeature(
searchUseCase: SearchUseCases.SearchUseCase,
fetchClient: Client,
limit: Int = 15,
mode: SearchSuggestionProvider.Mode = SearchSuggestionProvider.Mode.SINGLE_SUGGESTION
mode: SearchSuggestionProvider.Mode = SearchSuggestionProvider.Mode.SINGLE_SUGGESTION,
engine: Engine? = null
): AwesomeBarFeature {
awesomeBar.addProviders(
SearchSuggestionProvider(context, searchEngineManager, searchUseCase, fetchClient, limit, mode)
SearchSuggestionProvider(context, searchEngineManager, searchUseCase, fetchClient, limit, mode, engine)
)
return this
}
/**
* Add a [AwesomeBar.SuggestionProvider] for browsing history to the [AwesomeBar].
*
* @param historyStorage and instance of the [BookmarksStorage] used to query matching bookmarks.
* @param loadUrlUseCase the use case invoked to load the url when the user clicks on the suggestion.
* @param engine optional [Engine] instance to call [Engine.speculativeConnect] for the
* highest scored suggestion URL.
*/
fun addHistoryProvider(
historyStorage: HistoryStorage,
loadUrlUseCase: SessionUseCases.LoadUrlUseCase
loadUrlUseCase: SessionUseCases.LoadUrlUseCase,
engine: Engine? = null
): AwesomeBarFeature {
awesomeBar.addProviders(HistoryStorageSuggestionProvider(historyStorage, loadUrlUseCase, icons))
awesomeBar.addProviders(HistoryStorageSuggestionProvider(historyStorage, loadUrlUseCase, icons, engine))
return this
}
/**
* Add a [AwesomeBar.SuggestionProvider] for clipboard items to the [AwesomeBar].
*
* @param context the activity or application context, required to look up the clipboard manager.
* @param loadUrlUseCase the use case invoked to load the url when
* the user clicks on the suggestion.
* @param engine optional [Engine] instance to call [Engine.speculativeConnect] for the
* highest scored suggestion URL.
*/
fun addClipboardProvider(
context: Context,
loadUrlUseCase: SessionUseCases.LoadUrlUseCase
loadUrlUseCase: SessionUseCases.LoadUrlUseCase,
engine: Engine? = null
): AwesomeBarFeature {
awesomeBar.addProviders(ClipboardSuggestionProvider(context, loadUrlUseCase))
awesomeBar.addProviders(ClipboardSuggestionProvider(context, loadUrlUseCase, engine = engine))
return this
}
......
......@@ -7,6 +7,7 @@ package mozilla.components.feature.awesomebar.provider
import mozilla.components.browser.icons.BrowserIcons
import mozilla.components.browser.icons.IconRequest
import mozilla.components.concept.awesomebar.AwesomeBar
import mozilla.components.concept.engine.Engine
import mozilla.components.concept.storage.BookmarkNode
import mozilla.components.concept.storage.BookmarksStorage
import mozilla.components.feature.session.SessionUseCases
......@@ -17,11 +18,21 @@ private const val BOOKMARKS_SUGGESTION_LIMIT = 20
/**
* A [AwesomeBar.SuggestionProvider] implementation that provides suggestions based on the bookmarks
* stored in the [BookmarksStorage].
*
* @property bookmarksStorage and instance of the [BookmarksStorage] used
* to query matching bookmarks.
* @property loadUrlUseCase the use case invoked to load the url when the
* user clicks on the suggestion.
* @property icons optional instance of [BrowserIcons] to load fav icons
* for bookmarked URLs.
* @param engine optional [Engine] instance to call [Engine.speculativeConnect] for the
* highest scored suggestion URL.
*/
class BookmarksStorageSuggestionProvider(
private val bookmarksStorage: BookmarksStorage,
private val loadUrlUseCase: SessionUseCases.LoadUrlUseCase,
private val icons: BrowserIcons? = null
private val icons: BrowserIcons? = null,
private val engine: Engine? = null
) : AwesomeBar.SuggestionProvider {
override val id: String = UUID.randomUUID().toString()
......@@ -32,8 +43,13 @@ class BookmarksStorageSuggestionProvider(
}
val suggestions = bookmarksStorage.searchBookmarks(text, BOOKMARKS_SUGGESTION_LIMIT)
return suggestions.filter { it.url != null }.distinctBy { it.url }.sortedBy { it.guid }
.into()
.filter { it.url != null }
.distinctBy { it.url }
.sortedBy { it.guid }
suggestions.firstOrNull()?.url?.let { url -> engine?.speculativeConnect(url) }
return suggestions.into()
}
/**
......
......@@ -10,6 +10,7 @@ import android.content.Context
import android.graphics.Bitmap
import androidx.core.graphics.drawable.toBitmap
import mozilla.components.concept.awesomebar.AwesomeBar
import mozilla.components.concept.engine.Engine
import mozilla.components.feature.awesomebar.R
import mozilla.components.feature.session.SessionUseCases
import mozilla.components.support.utils.WebURLFinder
......@@ -20,13 +21,24 @@ private const val MIME_TYPE_TEXT_PLAIN = "text/plain"
/**
* An [AwesomeBar.SuggestionProvider] implementation that returns a suggestions for an URL in the clipboard (if there's
* any).
*
* @property context the activity or application context, required to look up the clipboard manager.
* @property loadUrlUseCase the use case invoked to load the url when
* the user clicks on the suggestion.
* @property icon optional icon used for the [AwesomeBar.Suggestion].
* @property title optional title used for the [AwesomeBar.Suggestion].
* @property requireEmptyText whether or no the input text must be empty for a
* clipboard suggestion to be provided, defaults to true.
* @property engine optional [Engine] instance to call [Engine.speculativeConnect] for the
* highest scored suggestion URL.
*/
class ClipboardSuggestionProvider(
private val context: Context,
private val loadUrlUseCase: SessionUseCases.LoadUrlUseCase,
private val icon: Bitmap? = null,
private val title: String? = null,
private val requireEmptyText: Boolean = true
private val requireEmptyText: Boolean = true,
internal val engine: Engine? = null
) : AwesomeBar.SuggestionProvider {
override val id: String = UUID.randomUUID().toString()
......@@ -42,6 +54,8 @@ class ClipboardSuggestionProvider(
findUrl(it)
} ?: return emptyList()
engine?.speculativeConnect(url)
return listOf(AwesomeBar.Suggestion(
provider = this,
id = url,
......
......@@ -7,6 +7,8 @@ package mozilla.components.feature.awesomebar.provider
import mozilla.components.browser.icons.BrowserIcons
import mozilla.components.browser.icons.IconRequest
import mozilla.components.concept.awesomebar.AwesomeBar
import mozilla.components.concept.engine.Engine
import mozilla.components.concept.storage.BookmarksStorage
import mozilla.components.concept.storage.HistoryStorage
import mozilla.components.concept.storage.SearchResult
import mozilla.components.feature.session.SessionUseCases
......@@ -17,11 +19,21 @@ internal const val HISTORY_SUGGESTION_LIMIT = 20
/**
* A [AwesomeBar.SuggestionProvider] implementation that provides suggestions based on the browsing
* history stored in the [HistoryStorage].
*
* @property historyStorage and instance of the [BookmarksStorage] used
* to query matching bookmarks.
* @property loadUrlUseCase the use case invoked to load the url when the
* user clicks on the suggestion.
* @property icons optional instance of [BrowserIcons] to load fav icons
* for bookmarked URLs.
* @param engine optional [Engine] instance to call [Engine.speculativeConnect] for the
* highest scored suggestion URL.
*/
class HistoryStorageSuggestionProvider(
private val historyStorage: HistoryStorage,
private val loadUrlUseCase: SessionUseCases.LoadUrlUseCase,
private val icons: BrowserIcons? = null
private val icons: BrowserIcons? = null,
internal val engine: Engine? = null
) : AwesomeBar.SuggestionProvider {
override val id: String = UUID.randomUUID().toString()
......@@ -31,10 +43,15 @@ class HistoryStorageSuggestionProvider(
return emptyList()
}
val suggestions = historyStorage.getSuggestions(text, HISTORY_SUGGESTION_LIMIT)
// In case of duplicates we want to pick the suggestion with the highest score.
// See: https://github.com/mozilla/application-services/issues/970
return suggestions.sortedByDescending { it.score }.distinctBy { it.id }.into()
val suggestions = historyStorage.getSuggestions(text, HISTORY_SUGGESTION_LIMIT)
.sortedByDescending { it.score }
.distinctBy { it.id }
suggestions.firstOrNull()?.url?.let { url -> engine?.speculativeConnect(url) }
return suggestions.into()
}
override val shouldClearSuggestions: Boolean
......
......@@ -11,6 +11,7 @@ import mozilla.components.browser.search.SearchEngine
import mozilla.components.browser.search.SearchEngineManager
import mozilla.components.browser.search.suggestions.SearchSuggestionClient
import mozilla.components.concept.awesomebar.AwesomeBar
import mozilla.components.concept.engine.Engine
import mozilla.components.concept.fetch.Client
import mozilla.components.concept.fetch.Request
import mozilla.components.concept.fetch.isSuccess
......@@ -36,11 +37,15 @@ class SearchSuggestionProvider : AwesomeBar.SuggestionProvider {
private val icon: Bitmap?
private val showDescription: Boolean
@VisibleForTesting
internal val engine: Engine?
private constructor(
client: SearchSuggestionClient,
searchUseCase: SearchUseCases.SearchUseCase,
limit: Int = 15,
mode: Mode = Mode.SINGLE_SUGGESTION,
engine: Engine? = null,
icon: Bitmap? = null,
showDescription: Boolean = true
) {
......@@ -52,6 +57,7 @@ class SearchSuggestionProvider : AwesomeBar.SuggestionProvider {
this.mode = mode
this.icon = icon
this.showDescription = showDescription
this.engine = engine
}
/**
......@@ -62,7 +68,10 @@ class SearchSuggestionProvider : AwesomeBar.SuggestionProvider {
* @param fetchClient The HTTP client for requesting suggestions from the search engine.
* @param limit The maximum number of suggestions that should be returned. It needs to be >= 1.
* @param mode Whether to return a single search suggestion (with chips) or one suggestion per item.
* @param icon The image to display next to the result. If not specified, the engine icon is used
* @param engine optional [Engine] instance to call [Engine.speculativeConnect] for the
* highest scored search suggestion URL.
* @param icon The image to display next to the result. If not specified, the engine icon is used.
* @param showDescription whether or not to add the search engine name as description.
*/
constructor(
searchEngine: SearchEngine,
......@@ -70,6 +79,7 @@ class SearchSuggestionProvider : AwesomeBar.SuggestionProvider {
fetchClient: Client,
limit: Int = 15,
mode: Mode = Mode.SINGLE_SUGGESTION,
engine: Engine? = null,
icon: Bitmap? = null,
showDescription: Boolean = true
) : this (
......@@ -77,6 +87,7 @@ class SearchSuggestionProvider : AwesomeBar.SuggestionProvider {
searchUseCase,
limit,
mode,
engine,
icon,
showDescription
)
......@@ -91,7 +102,10 @@ class SearchSuggestionProvider : AwesomeBar.SuggestionProvider {
* @param fetchClient The HTTP client for requesting suggestions from the search engine.
* @param limit The maximum number of suggestions that should be returned. It needs to be >= 1.
* @param mode Whether to return a single search suggestion (with chips) or one suggestion per item.
* @param icon The image to display next to the result. If not specified, the engine icon is used
* @param engine optional [Engine] instance to call [Engine.speculativeConnect] for the
* highest scored search suggestion URL.
* @param icon The image to display next to the result. If not specified, the engine icon is used.
* @param showDescription whether or not to add the search engine name as description.
*/
constructor(
context: Context,
......@@ -100,6 +114,7 @@ class SearchSuggestionProvider : AwesomeBar.SuggestionProvider {
fetchClient: Client,
limit: Int = 15,
mode: Mode = Mode.SINGLE_SUGGESTION,
engine: Engine? = null,
icon: Bitmap? = null,
showDescription: Boolean = true
) : this (
......@@ -107,6 +122,7 @@ class SearchSuggestionProvider : AwesomeBar.SuggestionProvider {
searchUseCase,
limit,
mode,
engine,
icon,
showDescription
)
......@@ -120,8 +136,20 @@ class SearchSuggestionProvider : AwesomeBar.SuggestionProvider {
val suggestions = fetchSuggestions(text)
return when (mode) {
Mode.MULTIPLE_SUGGESTIONS -> createMultipleSuggestions(text, suggestions)
Mode.SINGLE_SUGGESTION -> createSingleSearchSuggestion(text, suggestions)
Mode.MULTIPLE_SUGGESTIONS -> createMultipleSuggestions(text, suggestions).also {
// Call speculativeConnect for URL of first (highest scored) suggestion
it.firstOrNull()?.title?.let { searchTerms -> maybeCallSpeculativeConnect(searchTerms) }
}
Mode.SINGLE_SUGGESTION -> createSingleSearchSuggestion(text, suggestions).also {
// Call speculativeConnect for URL of first (highest scored) chip
it.firstOrNull()?.chips?.firstOrNull()?.let { chip -> maybeCallSpeculativeConnect(chip.title) }
}
}
}
private fun maybeCallSpeculativeConnect(searchTerms: String) {
client.searchEngine?.let { searchEngine ->
engine?.speculativeConnect(searchEngine.buildSearchUrl(searchTerms))
}
}
......
......@@ -10,7 +10,10 @@ import androidx.test.ext.junit.runners.AndroidJUnit4
import mozilla.components.browser.search.SearchEngine
import mozilla.components.browser.search.SearchEngineManager
import mozilla.components.concept.awesomebar.AwesomeBar
import mozilla.components.concept.engine.Engine
import mozilla.components.concept.toolbar.Toolbar
import mozilla.components.feature.awesomebar.provider.ClipboardSuggestionProvider
import mozilla.components.feature.awesomebar.provider.HistoryStorageSuggestionProvider
import mozilla.components.feature.awesomebar.provider.SearchSuggestionProvider
import mozilla.components.support.test.any
import mozilla.components.support.test.argumentCaptor
......@@ -26,6 +29,7 @@ import org.mockito.Mockito
import org.mockito.Mockito.`when`
import org.mockito.Mockito.doReturn
import org.mockito.Mockito.never
import org.mockito.Mockito.times
import org.mockito.Mockito.verify
@RunWith(AndroidJUnit4::class)
......@@ -130,6 +134,23 @@ class AwesomeBarFeatureTest {
assertSame(searchEngineManager, provider.value.client.searchEngineManager)
}
@Test
fun `addSearchProvider adds browser engine to suggestion provider`() {
val engine: Engine = mock()
val awesomeBar: AwesomeBar = mock()
val feature = AwesomeBarFeature(awesomeBar, mock())
feature.addSearchProvider(testContext, mock(), mock(), mock(), engine = engine)
val provider = argumentCaptor<SearchSuggestionProvider>()
verify(awesomeBar).addProviders(provider.capture())
assertSame(engine, provider.value.engine)
feature.addSearchProvider(mock(), mock(), mock(), engine = engine)
verify(awesomeBar, times(2)).addProviders(provider.capture())
assertSame(engine, provider.allValues.last().engine)
}
@Test
fun `addHistoryProvider adds provider`() {
val awesomeBar: AwesomeBar = mock()
......@@ -143,6 +164,19 @@ class AwesomeBarFeatureTest {
verify(awesomeBar).addProviders(any())
}
@Test
fun `addHistoryProvider adds browser engine to suggestion provider`() {
val engine: Engine = mock()
val awesomeBar: AwesomeBar = mock()
val feature = AwesomeBarFeature(awesomeBar, mock())
feature.addHistoryProvider(mock(), mock(), engine = engine)
val provider = argumentCaptor<HistoryStorageSuggestionProvider>()
verify(awesomeBar).addProviders(provider.capture())
assertSame(engine, provider.value.engine)
}
@Test
fun `addClipboardProvider adds provider`() {
val awesomeBar: AwesomeBar = mock()
......@@ -156,6 +190,19 @@ class AwesomeBarFeatureTest {
verify(awesomeBar).addProviders(any())
}
@Test
fun `addClipboardProvider adds browser engine to suggestion provider`() {
val engine: Engine = mock()
val awesomeBar: AwesomeBar = mock()
val feature = AwesomeBarFeature(awesomeBar, mock())
feature.addClipboardProvider(testContext, mock(), engine = engine)
val provider = argumentCaptor<ClipboardSuggestionProvider>()
verify(awesomeBar).addProviders(provider.capture())
assertSame(engine, provider.value.engine)
}
@Test
fun `Feature invokes custom start and complete hooks`() {
val toolbar: Toolbar = mock()
......
......@@ -6,16 +6,22 @@ package mozilla.components.feature.awesomebar.provider
import androidx.test.ext.junit.runners.AndroidJUnit4
import kotlinx.coroutines.runBlocking
import mozilla.components.concept.engine.Engine
import mozilla.components.concept.storage.BookmarkInfo
import mozilla.components.concept.storage.BookmarkNode
import mozilla.components.concept.storage.BookmarkNodeType
import mozilla.components.concept.storage.BookmarksStorage
import mozilla.components.support.test.eq
import mozilla.components.support.test.mock
import mozilla.components.support.utils.StorageUtils.levenshteinDistance
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.ArgumentMatchers.anyString
import org.mockito.Mockito.never
import org.mockito.Mockito.times
import org.mockito.Mockito.verify
import java.util.UUID
@RunWith(AndroidJUnit4::class)
......@@ -88,6 +94,23 @@ class BookmarksStorageSuggestionProviderTest {
assertTrue(provider.shouldClearSuggestions)
}
@Test
fun `provider calls speculative connect for URL of first suggestion`() = runBlocking {
val engine: Engine = mock()
val provider = BookmarksStorageSuggestionProvider(bookmarks, mock(), engine = engine)
var suggestions = provider.onInputChanged("")
assertTrue(suggestions.isEmpty())
verify(engine, never()).speculativeConnect(anyString())
val id = bookmarks.addItem("Mobile", newItem.url!!, newItem.title!!, null)
suggestions = provider.onInputChanged("moz")
assertEquals(1, suggestions.size)
assertEquals(id, suggestions[0].id)
assertEquals("http://www.mozilla.org", suggestions[0].description)
verify(engine, times(1)).speculativeConnect(eq(suggestions[0].description!!))
}
@SuppressWarnings
class testableBookmarksStorage : BookmarksStorage {
val bookmarkMap: HashMap<String, BookmarkNode> = hashMapOf()
......
......@@ -13,6 +13,7 @@ import kotlinx.coroutines.runBlocking
import mozilla.components.browser.session.Session
import mozilla.components.browser.session.SessionManager
import mozilla.components.concept.awesomebar.AwesomeBar
import mozilla.components.concept.engine.Engine
import mozilla.components.concept.engine.EngineSession
import mozilla.components.feature.session.SessionUseCases
import mozilla.components.support.test.any
......@@ -26,9 +27,11 @@ import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.ArgumentMatchers.anyString
import org.mockito.Mockito.`when`
import org.mockito.Mockito.never
import org.mockito.Mockito.spy
import org.mockito.Mockito.times
import org.mockito.Mockito.verify
@RunWith(AndroidJUnit4::class)
......@@ -203,6 +206,24 @@ class ClipboardSuggestionProviderTest {
assertTrue(suggestions.isEmpty())
}
@Test
fun `provider calls speculative connect for URL of suggestion`() {
val engine: Engine = mock()
val provider = ClipboardSuggestionProvider(testContext, mock(), engine = engine)
var suggestions = runBlocking { provider.onInputStarted() }
assertTrue(suggestions.isEmpty())
verify(engine, never()).speculativeConnect(anyString())
clipboardManager.setPrimaryClip(ClipData.newPlainText("Test label", "https://www.mozilla.org"))
suggestions = runBlocking { provider.onInputStarted() }
assertEquals(1, suggestions.size)
verify(engine, times(1)).speculativeConnect(eq("https://www.mozilla.org"))
val suggestion = suggestions.firstOrNull()
assertNotNull(suggestion!!)
assertEquals("https://www.mozilla.org", suggestion.description)