From 90496d3c4dbdeccdd266d2f43712e0ed299f0b08 Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Tue, 3 Dec 2019 18:53:18 +0100 Subject: [PATCH 01/22] Recover BrowserTabFragment after WebView crashes. * Error forwarded from WebViewClient to ViewModel * Introduced a new command to show an error with recovery action. * GlobalLayoutViewState as sealed class to model invalidated state or Browser. * Fragment invalidates WebView after crash destroying it. --- .../app/browser/BrowserTabFragment.kt | 26 +++++++++++++--- .../app/browser/BrowserTabViewModel.kt | 30 +++++++++++++++---- .../app/browser/BrowserWebViewClient.kt | 4 ++- .../app/browser/WebViewClientListener.kt | 1 + 4 files changed, 50 insertions(+), 11 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt index 72becfcce787..fd2f4b72c234 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt @@ -89,6 +89,7 @@ import com.duckduckgo.app.tabs.model.TabEntity import com.duckduckgo.app.tabs.ui.TabSwitcherActivity import com.duckduckgo.app.widget.ui.AddWidgetInstructionsActivity import com.duckduckgo.widget.SearchWidgetLight +import com.google.android.material.snackbar.BaseTransientBottomBar import com.google.android.material.snackbar.Snackbar import dagger.android.support.AndroidSupportInjection import kotlinx.android.synthetic.main.fragment_browser_tab.* @@ -481,6 +482,14 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { is Command.SaveCredentials -> saveBasicAuthCredentials(it.request, it.credentials) is Command.GenerateWebViewPreviewImage -> generateWebViewPreviewImage() is Command.LaunchTabSwitcher -> launchTabSwitcher() + is Command.ShowErrorWithAction -> { + Snackbar + .make(rootView, "The webpage could not be displayed.", Snackbar.LENGTH_INDEFINITE) + .setBehavior(object : BaseTransientBottomBar.Behavior() { + override fun canSwipeDismissView(child: View) = false + }) + .setAction("Reload") { _ -> it.action }.show() + } } } @@ -1125,12 +1134,21 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { fun renderGlobalViewState(viewState: GlobalLayoutViewState) { renderIfChanged(viewState, lastSeenGlobalViewState) { + if (lastSeenGlobalViewState is GlobalLayoutViewState.Invalidated) { + throw IllegalStateException("Can't recover from Invalidated State") + } + lastSeenGlobalViewState = viewState - if (viewState.isNewTabState) { - browserLayout.hide() - } else { - browserLayout.show() + when (viewState) { + is GlobalLayoutViewState.Browser -> { + if (viewState.isNewTabState) { + browserLayout.hide() + } else { + browserLayout.show() + } + } + is GlobalLayoutViewState.Invalidated -> destroyWebView() } } } diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt index be30e2b2932b..2fbad9fbe1c4 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt @@ -104,9 +104,10 @@ class BrowserTabViewModel( private var buildingSiteFactoryJob: Job? = null - data class GlobalLayoutViewState( - val isNewTabState: Boolean = true - ) + sealed class GlobalLayoutViewState { + data class Browser(val isNewTabState: Boolean = true) : GlobalLayoutViewState() + object Invalidated : GlobalLayoutViewState() + } data class BrowserViewState( val browserShowing: Boolean = false, @@ -182,6 +183,7 @@ class BrowserTabViewModel( class SaveCredentials(val request: BasicAuthenticationRequest, val credentials: BasicAuthenticationCredentials) : Command() object GenerateWebViewPreviewImage : Command() object LaunchTabSwitcher : Command() + class ShowErrorWithAction(val action: (BrowserTabViewModel) -> Unit) : Command() } val autoCompleteViewState: MutableLiveData = MutableLiveData() @@ -339,7 +341,7 @@ class BrowserTabViewModel( command.value = Navigate(queryUrlConverter.convertQueryToUrl(trimmedInput)) } - globalLayoutState.value = GlobalLayoutViewState(isNewTabState = false) + globalLayoutState.value = GlobalLayoutViewState.Browser(isNewTabState = false) findInPageViewState.value = FindInPageViewState(visible = false, canFindInPage = true) omnibarViewState.value = currentOmnibarViewState().copy(omnibarText = trimmedInput, shouldMoveCaretToEnd = false) browserViewState.value = currentBrowserViewState().copy(browserShowing = true, showClearButton = false) @@ -583,6 +585,7 @@ class BrowserTabViewModel( command.value = ShowFileChooser(filePathCallback, fileChooserParams) } + private fun currentGlobalLayoutState(): GlobalLayoutViewState = globalLayoutState.value!! private fun currentAutoCompleteViewState(): AutoCompleteViewState = autoCompleteViewState.value!! private fun currentBrowserViewState(): BrowserViewState = browserViewState.value!! private fun currentFindInPageViewState(): FindInPageViewState = findInPageViewState.value!! @@ -728,7 +731,7 @@ class BrowserTabViewModel( } fun onWebSessionRestored() { - globalLayoutState.value = GlobalLayoutViewState(isNewTabState = false) + globalLayoutState.value = GlobalLayoutViewState.Browser(isNewTabState = false) } fun onDesktopSiteModeToggled(desktopSiteRequested: Boolean) { @@ -747,7 +750,7 @@ class BrowserTabViewModel( } private fun initializeViewStates() { - globalLayoutState.value = GlobalLayoutViewState() + globalLayoutState.value = GlobalLayoutViewState.Browser() browserViewState.value = BrowserViewState().copy(addToHomeVisible = addToHomeCapabilityDetector.isAddToHomeSupported()) loadingViewState.value = LoadingViewState() autoCompleteViewState.value = AutoCompleteViewState() @@ -856,6 +859,11 @@ class BrowserTabViewModel( command.value = HandleExternalAppLink(appLink) } + override fun recoverFromRenderProcessGone() { + invalidateUserNavigation() + command.value = ShowErrorWithAction { it.onUserSubmittedQuery(url.orEmpty()) } + } + override fun requiresAuthentication(request: BasicAuthenticationRequest) { command.value = RequiresAuthentication(request) } @@ -872,4 +880,14 @@ class BrowserTabViewModel( fun userLaunchingTabSwitcher() { command.value = LaunchTabSwitcher } + + private fun invalidateUserNavigation() { + globalLayoutState.value = GlobalLayoutViewState.Invalidated + browserViewState.value = currentBrowserViewState().copy( + canGoBack = false, + canGoForward = false + ) + loadingViewState.value = LoadingViewState() + findInPageViewState.value = FindInPageViewState() + } } \ No newline at end of file diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt index a86b91361c75..ebcd640df92a 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt @@ -175,7 +175,9 @@ class BrowserWebViewClient( } else { offlinePixelCountDataStore.webRendererGoneKilledCount += 1 } - return super.onRenderProcessGone(view, detail) + + webViewClientListener?.recoverFromRenderProcessGone() + return true } @UiThread diff --git a/app/src/main/java/com/duckduckgo/app/browser/WebViewClientListener.kt b/app/src/main/java/com/duckduckgo/app/browser/WebViewClientListener.kt index 71bf15a3f9a6..61fc538bfa23 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/WebViewClientListener.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/WebViewClientListener.kt @@ -40,6 +40,7 @@ interface WebViewClientListener { fun exitFullScreen() fun showFileChooser(filePathCallback: ValueCallback>, fileChooserParams: WebChromeClient.FileChooserParams) fun externalAppLinkClicked(appLink: SpecialUrlDetector.UrlType.IntentType) + fun recoverFromRenderProcessGone() fun requiresAuthentication(request: BasicAuthenticationRequest) } \ No newline at end of file From 94a28b28fefd5764ca567ad86b884c982040b0bf Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Wed, 4 Dec 2019 21:33:35 +0100 Subject: [PATCH 02/22] Improving how to show snackbar adding isVisible property to GlobalLayoutViewState. Detected some issues when having multiple tabs crashed. Snackbar is like a singleton view that will be added only to one view hierarchy. We need to avoid adding the error snackbar in a non visible instance of BrowserTabFragment. --- .../app/browser/BrowserTabFragment.kt | 34 ++++++++------- .../app/browser/BrowserTabViewModel.kt | 41 ++++++++++++++----- .../app/global/view/NonDismissibleBehavior.kt | 24 +++++++++++ 3 files changed, 74 insertions(+), 25 deletions(-) create mode 100644 app/src/main/java/com/duckduckgo/app/global/view/NonDismissibleBehavior.kt diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt index fd2f4b72c234..057af25c4b91 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt @@ -89,7 +89,6 @@ import com.duckduckgo.app.tabs.model.TabEntity import com.duckduckgo.app.tabs.ui.TabSwitcherActivity import com.duckduckgo.app.widget.ui.AddWidgetInstructionsActivity import com.duckduckgo.widget.SearchWidgetLight -import com.google.android.material.snackbar.BaseTransientBottomBar import com.google.android.material.snackbar.Snackbar import dagger.android.support.AndroidSupportInjection import kotlinx.android.synthetic.main.fragment_browser_tab.* @@ -204,6 +203,11 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { private var webView: WebView? = null + private val errorSnackbar: Snackbar by lazy { + Snackbar.make(rootView, "The webpage could not be displayed.", Snackbar.LENGTH_INDEFINITE) + .setBehavior(NonDismissibleBehavior()) + } + private val findInPageTextWatcher = object : TextChangedWatcher() { override fun afterTextChanged(editable: Editable) { viewModel.userFindingInPage(findInPageInput.text.toString()) @@ -295,7 +299,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { super.onResume() addTextChangedListeners() appBarLayout.setExpanded(true) - viewModel.onViewVisible() + viewModel.onViewVisible(isVisible) logoHidingListener.onResume() } @@ -482,14 +486,13 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { is Command.SaveCredentials -> saveBasicAuthCredentials(it.request, it.credentials) is Command.GenerateWebViewPreviewImage -> generateWebViewPreviewImage() is Command.LaunchTabSwitcher -> launchTabSwitcher() - is Command.ShowErrorWithAction -> { - Snackbar - .make(rootView, "The webpage could not be displayed.", Snackbar.LENGTH_INDEFINITE) - .setBehavior(object : BaseTransientBottomBar.Behavior() { - override fun canSwipeDismissView(child: View) = false - }) - .setAction("Reload") { _ -> it.action }.show() - } + is Command.ShowErrorWithAction -> showErrorSnackbar(it) + } + } + + private fun showErrorSnackbar(command: Command.ShowErrorWithAction) { + if (!errorSnackbar.view.isAttachedToWindow) { + errorSnackbar.setAction("Reload") { command.action() }.show() } } @@ -912,7 +915,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { webView?.onPause() } else { webView?.onResume() - viewModel.onViewVisible() + viewModel.onViewVisible(true) } } @@ -1133,11 +1136,12 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { } fun renderGlobalViewState(viewState: GlobalLayoutViewState) { - renderIfChanged(viewState, lastSeenGlobalViewState) { - if (lastSeenGlobalViewState is GlobalLayoutViewState.Invalidated) { - throw IllegalStateException("Can't recover from Invalidated State") - } + if (lastSeenGlobalViewState is GlobalLayoutViewState.Invalidated && + viewState is GlobalLayoutViewState.Browser) { + throw IllegalStateException("Invalid state transition") + } + renderIfChanged(viewState, lastSeenGlobalViewState) { lastSeenGlobalViewState = viewState when (viewState) { diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt index 2fbad9fbe1c4..5a6372b1ef0a 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt @@ -38,6 +38,8 @@ import com.duckduckgo.app.bookmarks.db.BookmarkEntity import com.duckduckgo.app.bookmarks.db.BookmarksDao import com.duckduckgo.app.bookmarks.ui.EditBookmarkDialogFragment.EditBookmarkListener import com.duckduckgo.app.browser.BrowserTabViewModel.Command.* +import com.duckduckgo.app.browser.BrowserTabViewModel.GlobalLayoutViewState.Browser +import com.duckduckgo.app.browser.BrowserTabViewModel.GlobalLayoutViewState.Invalidated import com.duckduckgo.app.browser.LongPressHandler.RequiredAction import com.duckduckgo.app.browser.SpecialUrlDetector.UrlType.IntentType import com.duckduckgo.app.browser.WebNavigationStateChange.* @@ -104,9 +106,11 @@ class BrowserTabViewModel( private var buildingSiteFactoryJob: Job? = null - sealed class GlobalLayoutViewState { - data class Browser(val isNewTabState: Boolean = true) : GlobalLayoutViewState() - object Invalidated : GlobalLayoutViewState() + sealed class GlobalLayoutViewState(open val isVisible: Boolean) { + data class Browser(override val isVisible: Boolean = true, + val isNewTabState: Boolean = true) : GlobalLayoutViewState(isVisible) + + data class Invalidated(override val isVisible: Boolean = true) : GlobalLayoutViewState(isVisible) } data class BrowserViewState( @@ -183,7 +187,7 @@ class BrowserTabViewModel( class SaveCredentials(val request: BasicAuthenticationRequest, val credentials: BasicAuthenticationCredentials) : Command() object GenerateWebViewPreviewImage : Command() object LaunchTabSwitcher : Command() - class ShowErrorWithAction(val action: (BrowserTabViewModel) -> Unit) : Command() + class ShowErrorWithAction(val action: () -> Unit) : Command() } val autoCompleteViewState: MutableLiveData = MutableLiveData() @@ -293,12 +297,18 @@ class BrowserTabViewModel( browserChromeClient.webViewClientListener = this } - fun onViewVisible() { + fun onViewVisible(isVisible: Boolean) { command.value = if (!currentBrowserViewState().browserShowing) ShowKeyboard else HideKeyboard ctaViewModel.refreshCta() + + globalLayoutState.value = currentGlobalLayoutState().copy(isVisible) + currentGlobalLayoutState() + .takeIf { it.isVisible && it is Invalidated } + ?.let { showErrorWithAction() } } fun onViewHidden() { + globalLayoutState.value = currentGlobalLayoutState().copy(isVisible = false) skipHome = false } @@ -341,7 +351,7 @@ class BrowserTabViewModel( command.value = Navigate(queryUrlConverter.convertQueryToUrl(trimmedInput)) } - globalLayoutState.value = GlobalLayoutViewState.Browser(isNewTabState = false) + globalLayoutState.value = Browser(isNewTabState = false) findInPageViewState.value = FindInPageViewState(visible = false, canFindInPage = true) omnibarViewState.value = currentOmnibarViewState().copy(omnibarText = trimmedInput, shouldMoveCaretToEnd = false) browserViewState.value = currentBrowserViewState().copy(browserShowing = true, showClearButton = false) @@ -731,7 +741,7 @@ class BrowserTabViewModel( } fun onWebSessionRestored() { - globalLayoutState.value = GlobalLayoutViewState.Browser(isNewTabState = false) + globalLayoutState.value = Browser(isNewTabState = false) } fun onDesktopSiteModeToggled(desktopSiteRequested: Boolean) { @@ -750,7 +760,7 @@ class BrowserTabViewModel( } private fun initializeViewStates() { - globalLayoutState.value = GlobalLayoutViewState.Browser() + globalLayoutState.value = Browser() browserViewState.value = BrowserViewState().copy(addToHomeVisible = addToHomeCapabilityDetector.isAddToHomeSupported()) loadingViewState.value = LoadingViewState() autoCompleteViewState.value = AutoCompleteViewState() @@ -861,7 +871,7 @@ class BrowserTabViewModel( override fun recoverFromRenderProcessGone() { invalidateUserNavigation() - command.value = ShowErrorWithAction { it.onUserSubmittedQuery(url.orEmpty()) } + showErrorWithAction() } override fun requiresAuthentication(request: BasicAuthenticationRequest) { @@ -882,7 +892,7 @@ class BrowserTabViewModel( } private fun invalidateUserNavigation() { - globalLayoutState.value = GlobalLayoutViewState.Invalidated + globalLayoutState.value = Invalidated() browserViewState.value = currentBrowserViewState().copy( canGoBack = false, canGoForward = false @@ -890,4 +900,15 @@ class BrowserTabViewModel( loadingViewState.value = LoadingViewState() findInPageViewState.value = FindInPageViewState() } + + private fun showErrorWithAction() { + command.value = ShowErrorWithAction { this.onUserSubmittedQuery(url.orEmpty()) } + } + + private fun GlobalLayoutViewState.copy(isVisible: Boolean): GlobalLayoutViewState { + return when (this) { + is Invalidated -> this.copy(isVisible = isVisible) + is Browser -> this.copy(isVisible = isVisible) + } + } } \ No newline at end of file diff --git a/app/src/main/java/com/duckduckgo/app/global/view/NonDismissibleBehavior.kt b/app/src/main/java/com/duckduckgo/app/global/view/NonDismissibleBehavior.kt new file mode 100644 index 000000000000..069909f9e3a0 --- /dev/null +++ b/app/src/main/java/com/duckduckgo/app/global/view/NonDismissibleBehavior.kt @@ -0,0 +1,24 @@ +/* + * Copyright (c) 2019 DuckDuckGo + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.duckduckgo.app.global.view + +import android.view.View +import com.google.android.material.snackbar.BaseTransientBottomBar + +class NonDismissibleBehavior : BaseTransientBottomBar.Behavior() { + override fun canSwipeDismissView(child: View) = false +} \ No newline at end of file From d65e3f63eed1ee1ea3bc94f4860ce0310ac82cad Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Wed, 4 Dec 2019 21:39:12 +0100 Subject: [PATCH 03/22] When Tab is invalidated and user submits another query: close current tab and open query in a new tab. --- .../java/com/duckduckgo/app/browser/BrowserTabViewModel.kt | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt index 5a6372b1ef0a..f8c2d6678cf5 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt @@ -334,6 +334,12 @@ class BrowserTabViewModel( return } + if (currentGlobalLayoutState() is Invalidated) { + viewModelScope.launch { closeCurrentTab() } + command.value = OpenInNewTab(input) + return + } + command.value = HideKeyboard val trimmedInput = input.trim() From 3864cfa5745c425b3695befabd214f31fc6f31e5 Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Wed, 4 Dec 2019 21:39:50 +0100 Subject: [PATCH 04/22] Handling WebView crashes in SurveyActivity --- .../main/java/com/duckduckgo/app/survey/ui/SurveyActivity.kt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/src/main/java/com/duckduckgo/app/survey/ui/SurveyActivity.kt b/app/src/main/java/com/duckduckgo/app/survey/ui/SurveyActivity.kt index cec9087d6b7e..36e5df03a584 100644 --- a/app/src/main/java/com/duckduckgo/app/survey/ui/SurveyActivity.kt +++ b/app/src/main/java/com/duckduckgo/app/survey/ui/SurveyActivity.kt @@ -165,5 +165,10 @@ class SurveyActivity : DuckDuckGoActivity() { viewModel.onSurveyFailedToLoad() } } + + override fun onRenderProcessGone(view: WebView?, detail: RenderProcessGoneDetail?): Boolean { + viewModel.onSurveyFailedToLoad() + return true + } } } \ No newline at end of file From c91f8daa2c80af8daa76843d7f537bc3bc355c99 Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Wed, 4 Dec 2019 21:48:16 +0100 Subject: [PATCH 05/22] Disable report a site when WebView crashes. --- .../java/com/duckduckgo/app/browser/BrowserTabFragment.kt | 1 + .../java/com/duckduckgo/app/browser/BrowserTabViewModel.kt | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt index 057af25c4b91..a7351028638f 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt @@ -1196,6 +1196,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { newTabPopupMenuItem.isEnabled = browserShowing addBookmarksPopupMenuItem?.isEnabled = viewState.canAddBookmarks sharePageMenuItem?.isEnabled = viewState.canSharePage + brokenSitePopupMenuItem?.isEnabled = viewState.canReportSite addToHome?.let { it.visibility = if (viewState.addToHomeVisible) VISIBLE else GONE diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt index f8c2d6678cf5..3c7086c6058c 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt @@ -126,6 +126,7 @@ class BrowserTabViewModel( val canAddBookmarks: Boolean = false, val canGoBack: Boolean = false, val canGoForward: Boolean = false, + val canReportSite: Boolean = true, val addToHomeEnabled: Boolean = false, val addToHomeVisible: Boolean = false ) @@ -901,7 +902,8 @@ class BrowserTabViewModel( globalLayoutState.value = Invalidated() browserViewState.value = currentBrowserViewState().copy( canGoBack = false, - canGoForward = false + canGoForward = false, + canReportSite = false ) loadingViewState.value = LoadingViewState() findInPageViewState.value = FindInPageViewState() From ef50eda9c291fc3619e35f36cc4a1b67f13084f2 Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Wed, 4 Dec 2019 22:26:36 +0100 Subject: [PATCH 06/22] Pipe refresh events into ViewModel to recover if Browser is in invalidated state, otherwise refresh URL. --- .../duckduckgo/app/browser/BrowserActivity.kt | 2 +- .../app/browser/BrowserTabFragment.kt | 6 ++++- .../app/browser/BrowserTabViewModel.kt | 22 ++++++++++++++----- 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt index 3b121d575370..a500144805c9 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt @@ -244,7 +244,7 @@ class BrowserActivity : DuckDuckGoActivity(), CoroutineScope { Timber.i("Processing command: $command") when (command) { is Query -> currentTab?.submitQuery(command.query) - is Refresh -> currentTab?.refresh() + is Refresh -> currentTab?.onRefreshRequested() is Command.DisplayMessage -> applicationContext?.longToast(command.messageId) is Command.LaunchPlayStore -> launchPlayStore() is Command.ShowAppEnjoymentPrompt -> showAppEnjoymentPrompt(AppEnjoymentDialogFragment.create(command.promptCount, viewModel)) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt index a7351028638f..0e460ecf08cb 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt @@ -314,7 +314,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { popupMenu.apply { onMenuItemClicked(view.forwardPopupMenuItem) { viewModel.onUserPressedForward() } onMenuItemClicked(view.backPopupMenuItem) { activity?.onBackPressed() } - onMenuItemClicked(view.refreshPopupMenuItem) { refresh() } + onMenuItemClicked(view.refreshPopupMenuItem) { viewModel.onRefreshRequested() } onMenuItemClicked(view.newTabPopupMenuItem) { viewModel.userRequestedOpeningNewTab() } onMenuItemClicked(view.bookmarksPopupMenuItem) { browserActivity?.launchBookmarks() } onMenuItemClicked(view.addBookmarksPopupMenuItem) { launch { viewModel.onBookmarkAddRequested() } } @@ -403,6 +403,10 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { webView?.loadUrl(url) } + fun onRefreshRequested() { + viewModel.onRefreshRequested() + } + fun refresh() { webView?.reload() } diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt index 3c7086c6058c..234e2bde9a6b 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt @@ -330,19 +330,18 @@ class BrowserTabViewModel( pixel.fire(pixelName, params) } - fun onUserSubmittedQuery(input: String) { - if (input.isBlank()) { + fun onUserSubmittedQuery(query: String) { + if (query.isBlank()) { return } if (currentGlobalLayoutState() is Invalidated) { - viewModelScope.launch { closeCurrentTab() } - command.value = OpenInNewTab(input) + recoverTabWithQuery(query) return } command.value = HideKeyboard - val trimmedInput = input.trim() + val trimmedInput = query.trim() viewModelScope.launch(dispatchers.io()) { searchCountDao.incrementSearchCount() @@ -386,6 +385,14 @@ class BrowserTabViewModel( } } + fun onRefreshRequested() { + if(currentGlobalLayoutState() is Invalidated){ + recoverTabWithQuery(url.orEmpty()) + } else { + command.value = Refresh + } + } + /** * Handles back navigation. Returns false if navigation could not be * handled at this level, giving system an opportunity to handle it @@ -913,6 +920,11 @@ class BrowserTabViewModel( command.value = ShowErrorWithAction { this.onUserSubmittedQuery(url.orEmpty()) } } + private fun recoverTabWithQuery(query: String) { + viewModelScope.launch { closeCurrentTab() } + command.value = OpenInNewTab(query) + } + private fun GlobalLayoutViewState.copy(isVisible: Boolean): GlobalLayoutViewState { return when (this) { is Invalidated -> this.copy(isVisible = isVisible) From 334d55d21059f95ecd4107bd29f825e43723fafa Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Sat, 7 Dec 2019 20:15:48 +0100 Subject: [PATCH 07/22] Using translations from resource (no translated) --- .../java/com/duckduckgo/app/browser/BrowserTabFragment.kt | 7 ++++--- app/src/main/res/values/strings.xml | 2 ++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt index 0e460ecf08cb..d76feab764e2 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt @@ -204,7 +204,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { private var webView: WebView? = null private val errorSnackbar: Snackbar by lazy { - Snackbar.make(rootView, "The webpage could not be displayed.", Snackbar.LENGTH_INDEFINITE) + Snackbar.make(rootView, R.string.crashedWebViewErrorMessage, Snackbar.LENGTH_INDEFINITE) .setBehavior(NonDismissibleBehavior()) } @@ -495,8 +495,9 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { } private fun showErrorSnackbar(command: Command.ShowErrorWithAction) { - if (!errorSnackbar.view.isAttachedToWindow) { - errorSnackbar.setAction("Reload") { command.action() }.show() + //Snackbar is global and it should appear only the foreground fragment + if (!errorSnackbar.view.isAttachedToWindow && isVisible) { + errorSnackbar.setAction(R.string.crashedWebViewErrorAction) { command.action() }.show() } } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index f62e40dbad6e..61f8501aec63 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -39,6 +39,8 @@ Refresh Back Forward + "The webpage could not be displayed." + "Reload" Search or type URL Clear search input From b32a59605d6ecae73b425205fded318f7ffcc52a Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Sat, 7 Dec 2019 20:18:49 +0100 Subject: [PATCH 08/22] Removing visibility from GlobalLayoutViewState + unit test --- .../app/browser/BrowserTabViewModelTest.kt | 209 ++++++++++++++---- .../app/browser/BrowserTabFragment.kt | 4 +- .../app/browser/BrowserTabViewModel.kt | 26 +-- 3 files changed, 177 insertions(+), 62 deletions(-) diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt index 00554dcdd603..09b906191a9a 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt @@ -75,6 +75,7 @@ import io.reactivex.Single import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.test.TestCoroutineScope import kotlinx.coroutines.test.runBlockingTest import kotlinx.coroutines.withContext import org.junit.After @@ -180,6 +181,8 @@ class BrowserTabViewModelTest { private lateinit var testee: BrowserTabViewModel + private val selectedTabLiveData = MutableLiveData() + @Before fun before() { MockitoAnnotations.initMocks(this) @@ -201,11 +204,13 @@ class BrowserTabViewModelTest { ) val siteFactory = SiteFactory(mockPrivacyPractices, mockTrackerNetworks, prevalenceStore = mockPrevalenceStore) - runBlockingTest { - whenever(mockTabsRepository.retrieveSiteData(any())).thenReturn(MutableLiveData()) - whenever(mockPrivacyPractices.privacyPracticesFor(any())).thenReturn(PrivacyPractices.UNKNOWN) - whenever(mockAppInstallStore.installTimestamp).thenReturn(System.currentTimeMillis() - TimeUnit.DAYS.toMillis(1)) - } + + whenever(mockOmnibarConverter.convertQueryToUrl(any())).thenReturn("duckduckgo.com") + whenever(mockVariantManager.getVariant()).thenReturn(DEFAULT_VARIANT) + whenever(mockTabsRepository.liveSelectedTab).thenReturn(selectedTabLiveData) + whenever(mockTabsRepository.retrieveSiteData(any())).thenReturn(MutableLiveData()) + whenever(mockPrivacyPractices.privacyPracticesFor(any())).thenReturn(PrivacyPractices.UNKNOWN) + whenever(mockAppInstallStore.installTimestamp).thenReturn(System.currentTimeMillis() - TimeUnit.DAYS.toMillis(1)) testee = BrowserTabViewModel( statisticsUpdater = mockStatisticsUpdater, @@ -232,9 +237,6 @@ class BrowserTabViewModelTest { testee.loadData("abc", null, false) testee.command.observeForever(mockCommandObserver) - - whenever(mockOmnibarConverter.convertQueryToUrl(any())).thenReturn("duckduckgo.com") - whenever(mockVariantManager.getVariant()).thenReturn(DEFAULT_VARIANT) } @ExperimentalCoroutinesApi @@ -269,7 +271,7 @@ class BrowserTabViewModelTest { } @Test - fun whenOpenInNewBackgroundRequestedThenTabRepositoryUpdatedAndCommandIssued() = runBlockingTest { + fun whenOpenInNewBackgroundRequestedThenTabRepositoryUpdatedAndCommandIssued() = ruleRunBlockingTest { val url = "http://www.example.com" testee.openInNewBackgroundTab(url) @@ -295,6 +297,13 @@ class BrowserTabViewModelTest { assertTrue(commandCaptor.allValues.contains(Command.ShowKeyboard)) } + @Test + fun whenInvalidatedGlobalLayoutRestoredAndVisibleThenErrorIsShown() { + givenInvalidatedGlobalLayout() + testee.onViewVisible() + assertCommandIssued() + } + @Test fun whenSubmittedQueryHasWhitespaceItIsTrimmed() { testee.onUserSubmittedQuery(" nytimes.com ") @@ -320,13 +329,13 @@ class BrowserTabViewModelTest { } @Test - fun whenBookmarkEditedThenDaoIsUpdated() = runBlockingTest { + fun whenBookmarkEditedThenDaoIsUpdated() = ruleRunBlockingTest { testee.editBookmark(0, "A title", "www.example.com") verify(mockBookmarksDao).update(BookmarkEntity(title = "A title", url = "www.example.com")) } @Test - fun whenBookmarkAddedThenDaoIsUpdatedAndUserNotified() = runBlockingTest { + fun whenBookmarkAddedThenDaoIsUpdatedAndUserNotified() = ruleRunBlockingTest { loadUrl("www.example.com", "A title") testee.onBookmarkAddRequested() @@ -343,7 +352,7 @@ class BrowserTabViewModelTest { } @Test - fun whenEmptyInputQueryThenQueryNavigateCommandNotSubmittedToActivityActivity() { + fun whenEmptyInputQueryThenQueryNavigateCommandNotSubmittedToActivity() { testee.onUserSubmittedQuery("") verify(mockCommandObserver, never()).onChanged(commandCaptor.capture()) } @@ -362,7 +371,27 @@ class BrowserTabViewModelTest { } @Test - fun whenBrowsingAndUrlLoadedThenSiteVisitedEntryAddedToLeaderboardDao() = runBlockingTest { + fun whenInvalidatedGlobalLayoutAndNonEmptyInputThenOpenInNewTab() { + givenOneActiveTabSelected() + givenInvalidatedGlobalLayout() + testee.onUserSubmittedQuery("foo") + assertCommandIssued() + } + + @Test + fun whenInvalidatedGlobalLayoutAndNonEmptyInputThenCloseCurrentTab() { + givenOneActiveTabSelected() + givenInvalidatedGlobalLayout() + + testee.onUserSubmittedQuery("foo") + + ruleRunBlockingTest { + verify(mockTabsRepository).delete(selectedTabLiveData.value!!) + } + } + + @Test + fun whenBrowsingAndUrlLoadedThenSiteVisitedEntryAddedToLeaderboardDao() = ruleRunBlockingTest { loadUrl("http://example.com/abc", isBrowserShowing = true) verify(mockNetworkLeaderboardDao).incrementSitesVisited() } @@ -431,7 +460,7 @@ class BrowserTabViewModelTest { } @Test - fun whenViewModelNotifiedThatUrlGotFocusThenViewStateIsUpdated() = runBlockingTest { + fun whenViewModelNotifiedThatUrlGotFocusThenViewStateIsUpdated() = ruleRunBlockingTest { withContext(Dispatchers.Main) { testee.onOmnibarInputStateChanged("", true, hasQueryChanged = false) assertTrue(omnibarViewState().isEditing) @@ -499,7 +528,7 @@ class BrowserTabViewModelTest { } @Test - fun whenUrlClearedThenPrivacyGradeIsCleared() = runBlockingTest { + fun whenUrlClearedThenPrivacyGradeIsCleared() = ruleRunBlockingTest { withContext(Dispatchers.Main) { loadUrl("https://duckduckgo.com") assertNotNull(testee.privacyGrade.value) @@ -509,7 +538,7 @@ class BrowserTabViewModelTest { } @Test - fun whenUrlLoadedThenPrivacyGradeIsReset() = runBlockingTest { + fun whenUrlLoadedThenPrivacyGradeIsReset() = ruleRunBlockingTest { withContext(Dispatchers.Main) { loadUrl("https://duckduckgo.com") assertNotNull(testee.privacyGrade.value) @@ -859,6 +888,34 @@ class BrowserTabViewModelTest { assertTrue(captureCommands().lastValue == Command.Refresh) } + @Test + fun whenRefreshRequestedWithInvalidatedGlobalLayoutThenOpenCurrentUrlInNewTab() { + givenOneActiveTabSelected() + givenInvalidatedGlobalLayout() + + testee.onRefreshRequested() + + assertCommandIssued() + } + + @Test + fun whenRefreshRequestedWithInvalidatedGlobalLayoutThenCloseCurrentTab() { + givenOneActiveTabSelected() + givenInvalidatedGlobalLayout() + + testee.onRefreshRequested() + + ruleRunBlockingTest { + verify(mockTabsRepository).delete(selectedTabLiveData.value!!) + } + } + + @Test + fun whenRefreshRequestedWithBrowserGlobalLayoutThenRefresh() { + testee.onRefreshRequested() + assertCommandIssued() + } + @Test fun whenUserBrowsingPressesBackAndBrowserCanGoBackThenNavigatesToPreviousPageAndHandledTrue() { setupNavigation(isBrowsing = true, canGoBack = true, stepsToPreviousPage = 2) @@ -936,7 +993,7 @@ class BrowserTabViewModelTest { } @Test - fun whenSiteLoadedAndUserSelectsToAddBookmarkThenAddBookmarkCommandSentWithUrlAndTitle() = runBlockingTest { + fun whenSiteLoadedAndUserSelectsToAddBookmarkThenAddBookmarkCommandSentWithUrlAndTitle() = ruleRunBlockingTest { loadUrl("foo.com") testee.titleReceived("Foo Title") testee.onBookmarkAddRequested() @@ -946,7 +1003,7 @@ class BrowserTabViewModelTest { } @Test - fun whenNoSiteAndUserSelectsToAddBookmarkThenBookmarkAddedWithBlankTitleAndUrl() = runBlockingTest { + fun whenNoSiteAndUserSelectsToAddBookmarkThenBookmarkAddedWithBlankTitleAndUrl() = ruleRunBlockingTest { whenever(mockBookmarksDao.insert(any())).thenReturn(1) testee.onBookmarkAddRequested() verify(mockBookmarksDao).insert(BookmarkEntity(title = "", url = "")) @@ -965,7 +1022,7 @@ class BrowserTabViewModelTest { } @Test - fun whenOnSiteAndBrokenSiteSelectedThenBrokenSiteFeedbackCommandSentWithUrl() = runBlockingTest { + fun whenOnSiteAndBrokenSiteSelectedThenBrokenSiteFeedbackCommandSentWithUrl() = ruleRunBlockingTest { loadUrl("foo.com", isBrowserShowing = true) testee.onBrokenSiteSelected() val command = captureCommands().value as Command.BrokenSiteFeedback @@ -989,7 +1046,7 @@ class BrowserTabViewModelTest { @Test fun whenWebSessionRestoredThenGlobalLayoutSwitchedToShowingBrowser() { testee.onWebSessionRestored() - assertFalse(globalLayoutViewState().isNewTabState) + assertFalse(browserGlobalLayoutViewState().isNewTabState) } @Test @@ -1020,11 +1077,11 @@ class BrowserTabViewModelTest { fun whenWebViewSessionRestorableThenSessionRestored() { whenever(webViewSessionStorage.restoreSession(anyOrNull(), anyString())).thenReturn(true) testee.restoreWebViewState(null, "") - assertFalse(globalLayoutViewState().isNewTabState) + assertFalse(browserGlobalLayoutViewState().isNewTabState) } @Test - fun whenUrlNullThenSetBrowserNotShowing() = runBlockingTest { + fun whenUrlNullThenSetBrowserNotShowing() = ruleRunBlockingTest { withContext(Dispatchers.Main) { testee.loadData("id", null, false) testee.determineShowBrowser() @@ -1033,7 +1090,7 @@ class BrowserTabViewModelTest { } @Test - fun whenUrlBlankThenSetBrowserNotShowing() = runBlockingTest { + fun whenUrlBlankThenSetBrowserNotShowing() = ruleRunBlockingTest { withContext(Dispatchers.Main) { testee.loadData("id", " ", false) testee.determineShowBrowser() @@ -1042,7 +1099,7 @@ class BrowserTabViewModelTest { } @Test - fun whenUrlPresentThenSetBrowserShowing() = runBlockingTest { + fun whenUrlPresentThenSetBrowserShowing() = ruleRunBlockingTest { withContext(Dispatchers.Main) { testee.loadData("id", "https://example.com", false) testee.determineShowBrowser() @@ -1050,6 +1107,72 @@ class BrowserTabViewModelTest { } } + @Test + fun whenRecoveringFromProcessGoneThenShowErrorWithAction() { + testee.recoverFromRenderProcessGone() + assertCommandIssued() + } + + @Test + fun whenUserClicksOnErrorActionThenOpenCurrentUrlInNewTab() { + givenOneActiveTabSelected() + testee.recoverFromRenderProcessGone() + verify(mockCommandObserver, atLeastOnce()).onChanged(commandCaptor.capture()) + val showErrorWithAction = commandCaptor.value as Command.ShowErrorWithAction + + showErrorWithAction.action() + + assertCommandIssued() + } + + @Test + fun whenUserClicksOnErrorActionThenOpenCurrentTabIsClosed() { + givenOneActiveTabSelected() + testee.recoverFromRenderProcessGone() + verify(mockCommandObserver, atLeastOnce()).onChanged(commandCaptor.capture()) + val showErrorWithAction = commandCaptor.value as Command.ShowErrorWithAction + + showErrorWithAction.action() + + ruleRunBlockingTest { + verify(mockTabsRepository).delete(selectedTabLiveData.value!!) + } + } + + @Test + fun whenRecoveringFromProcessGoneThenGlobalLayoutIsInvalidated() { + testee.recoverFromRenderProcessGone() + + assertTrue(globalLayoutViewState() is BrowserTabViewModel.GlobalLayoutViewState.Invalidated) + } + + @Test + fun whenRecoveringFromProcessGoneThenLoadingIsReset() { + testee.recoverFromRenderProcessGone() + + assertEquals(loadingViewState(), BrowserTabViewModel.LoadingViewState()) + } + + @Test + fun whenRecoveringFromProcessGoneThenFindInPageIsReset() { + testee.recoverFromRenderProcessGone() + + assertEquals(findInPageViewState(), BrowserTabViewModel.FindInPageViewState()) + } + + @Test + fun whenRecoveringFromProcessGoneThenExpectedBrowserOptionsAreDisabled() { + setupNavigation(skipHome = true, isBrowsing = true, canGoForward = true, canGoBack = true, stepsToPreviousPage = 1) + + testee.recoverFromRenderProcessGone() + + assertFalse(browserViewState().canGoBack) + assertFalse(browserViewState().canGoForward) + assertFalse(browserViewState().canReportSite) + assertFalse(browserViewState().canChangeBrowsingMode) + assertFalse(findInPageViewState().canFindInPage) + } + @Test fun whenAuthenticationIsRequiredThenRequiresAuthenticationCommandSent() { val mockHandler = mock() @@ -1105,7 +1228,7 @@ class BrowserTabViewModelTest { } @Test - fun whenUserSelectToEditQueryThenMoveCaretToTheEnd() = runBlockingTest { + fun whenUserSelectToEditQueryThenMoveCaretToTheEnd() = ruleRunBlockingTest { withContext(Dispatchers.Main) { testee.onUserSelectedToEditQuery("foo") assertTrue(omnibarViewState().shouldMoveCaretToEnd) @@ -1136,37 +1259,29 @@ class BrowserTabViewModelTest { @Test fun whenCloseCurrentTabSelectedThenTabDeletedFromRepository() = runBlocking { - val liveData = MutableLiveData() - liveData.value = TabEntity("TAB_ID", "", "", false, true, 0) - whenever(mockTabsRepository.liveSelectedTab).thenReturn(liveData) + givenOneActiveTabSelected() testee.closeCurrentTab() - verify(mockTabsRepository).delete(liveData.value!!) + verify(mockTabsRepository).delete(selectedTabLiveData.value!!) } @Test fun whenUserPressesBackAndSkippingHomeThenWebViewPreviewGenerated() { setupNavigation(isBrowsing = true, canGoBack = false, skipHome = true) testee.onUserPressedBack() - verifyGenerateWebViewPreviewCommandIssued() + assertCommandIssued() } @Test fun whenUserPressesBackAndNotSkippingHomeThenWebViewPreviewNotGenerated() { setupNavigation(isBrowsing = true, canGoBack = false, skipHome = false) testee.onUserPressedBack() - verifyGenerateWebViewPreviewCommandNotIssued() + verify(mockCommandObserver, never()).onChanged(commandCaptor.capture()) } - private fun verifyGenerateWebViewPreviewCommandIssued() { + private inline fun assertCommandIssued() { verify(mockCommandObserver, atLeastOnce()).onChanged(commandCaptor.capture()) - val generatedPreviewCommand = commandCaptor.allValues.find { it is Command.GenerateWebViewPreviewImage } - assertNotNull(generatedPreviewCommand) - } - - private fun verifyGenerateWebViewPreviewCommandNotIssued() { - verify(mockCommandObserver, atLeast(0)).onChanged(commandCaptor.capture()) - val generatedPreviewCommand = commandCaptor.allValues.find { it is Command.GenerateWebViewPreviewImage } - assertNull(generatedPreviewCommand) + val issuedCommand = commandCaptor.allValues.find { it is T } + assertNotNull(issuedCommand) } private fun pixelParams(showedBookmarks: Boolean, bookmarkCapable: Boolean) = mapOf( @@ -1174,6 +1289,15 @@ class BrowserTabViewModelTest { Pixel.PixelParameter.BOOKMARK_CAPABLE to bookmarkCapable.toString() ) + private fun givenInvalidatedGlobalLayout() { + testee.globalLayoutState.value = BrowserTabViewModel.GlobalLayoutViewState.Invalidated + } + + private fun givenOneActiveTabSelected() { + selectedTabLiveData.value = TabEntity("TAB_ID", "https://example.com", "", skipHome = false, viewed = true, position = 0) + testee.loadData("TAB_ID", "https://example.com", false) + } + private fun setBrowserShowing(isBrowsing: Boolean) { testee.browserViewState.value = browserViewState().copy(browserShowing = isBrowsing) } @@ -1183,6 +1307,7 @@ class BrowserTabViewModelTest { testee.navigationStateChanged(buildWebNavigation(originalUrl = url, currentUrl = url, title = title)) } + @Suppress("SameParameterValue") private fun updateUrl(originalUrl: String?, currentUrl: String?, isBrowserShowing: Boolean) { setBrowserShowing(isBrowserShowing) testee.navigationStateChanged(buildWebNavigation(originalUrl = originalUrl, currentUrl = currentUrl)) @@ -1230,4 +1355,8 @@ class BrowserTabViewModelTest { private fun autoCompleteViewState() = testee.autoCompleteViewState.value!! private fun findInPageViewState() = testee.findInPageViewState.value!! private fun globalLayoutViewState() = testee.globalLayoutState.value!! + private fun browserGlobalLayoutViewState() = testee.globalLayoutState.value!! as BrowserTabViewModel.GlobalLayoutViewState.Browser + + private fun ruleRunBlockingTest(block: suspend TestCoroutineScope.() -> Unit) = + coroutineRule.testDispatcher.runBlockingTest(block) } \ No newline at end of file diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt index d76feab764e2..c985fae8bccd 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt @@ -299,7 +299,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { super.onResume() addTextChangedListeners() appBarLayout.setExpanded(true) - viewModel.onViewVisible(isVisible) + viewModel.onViewVisible() logoHidingListener.onResume() } @@ -920,7 +920,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { webView?.onPause() } else { webView?.onResume() - viewModel.onViewVisible(true) + viewModel.onViewVisible() } } diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt index 234e2bde9a6b..14a51767ec7a 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt @@ -106,11 +106,9 @@ class BrowserTabViewModel( private var buildingSiteFactoryJob: Job? = null - sealed class GlobalLayoutViewState(open val isVisible: Boolean) { - data class Browser(override val isVisible: Boolean = true, - val isNewTabState: Boolean = true) : GlobalLayoutViewState(isVisible) - - data class Invalidated(override val isVisible: Boolean = true) : GlobalLayoutViewState(isVisible) + sealed class GlobalLayoutViewState { + data class Browser(val isNewTabState: Boolean = true) : GlobalLayoutViewState() + object Invalidated : GlobalLayoutViewState() } data class BrowserViewState( @@ -298,18 +296,13 @@ class BrowserTabViewModel( browserChromeClient.webViewClientListener = this } - fun onViewVisible(isVisible: Boolean) { + fun onViewVisible() { command.value = if (!currentBrowserViewState().browserShowing) ShowKeyboard else HideKeyboard ctaViewModel.refreshCta() - - globalLayoutState.value = currentGlobalLayoutState().copy(isVisible) - currentGlobalLayoutState() - .takeIf { it.isVisible && it is Invalidated } - ?.let { showErrorWithAction() } + if (currentGlobalLayoutState() is Invalidated) showErrorWithAction() } fun onViewHidden() { - globalLayoutState.value = currentGlobalLayoutState().copy(isVisible = false) skipHome = false } @@ -386,7 +379,7 @@ class BrowserTabViewModel( } fun onRefreshRequested() { - if(currentGlobalLayoutState() is Invalidated){ + if (currentGlobalLayoutState() is Invalidated) { recoverTabWithQuery(url.orEmpty()) } else { command.value = Refresh @@ -924,11 +917,4 @@ class BrowserTabViewModel( viewModelScope.launch { closeCurrentTab() } command.value = OpenInNewTab(query) } - - private fun GlobalLayoutViewState.copy(isVisible: Boolean): GlobalLayoutViewState { - return when (this) { - is Invalidated -> this.copy(isVisible = isVisible) - is Browser -> this.copy(isVisible = isVisible) - } - } } \ No newline at end of file From 71445cb12b3f9ae612d909b36615960a0b6f8ff9 Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Sat, 7 Dec 2019 20:19:34 +0100 Subject: [PATCH 09/22] cover BrowserWebViewClient logic added to pipe error into viewmodel --- .../duckduckgo/app/browser/BrowserWebViewClientTest.kt | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserWebViewClientTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserWebViewClientTest.kt index a76c3ce3974a..147973db102c 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserWebViewClientTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserWebViewClientTest.kt @@ -117,6 +117,15 @@ class BrowserWebViewClientTest { verify(offlinePixelCountDataStore, times(1)).webRendererGoneKilledCount = 1 } + @Test + @SdkSuppress(minSdkVersion = Build.VERSION_CODES.O) + fun whenRenderProcessGoneThenEmitEventIntoListener() { + val detail: RenderProcessGoneDetail = mock() + whenever(detail.didCrash()).thenReturn(true) + testee.onRenderProcessGone(webView, detail) + verify(listener, times(1)).recoverFromRenderProcessGone() + } + private class TestWebView(context: Context) : WebView(context) companion object { From 1c46cb44208d7c41b98c50546d29b015904b793e Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Sat, 7 Dec 2019 20:20:34 +0100 Subject: [PATCH 10/22] Disable changing browsing mode when WebView is gone. --- .../java/com/duckduckgo/app/browser/BrowserTabFragment.kt | 1 + .../java/com/duckduckgo/app/browser/BrowserTabViewModel.kt | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt index c985fae8bccd..5beda1b3c8f3 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt @@ -1202,6 +1202,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { addBookmarksPopupMenuItem?.isEnabled = viewState.canAddBookmarks sharePageMenuItem?.isEnabled = viewState.canSharePage brokenSitePopupMenuItem?.isEnabled = viewState.canReportSite + requestDesktopSiteCheckMenuItem?.isEnabled = viewState.canChangeBrowsingMode addToHome?.let { it.visibility = if (viewState.addToHomeVisible) VISIBLE else GONE diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt index 14a51767ec7a..f5c086153dee 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt @@ -115,6 +115,7 @@ class BrowserTabViewModel( val browserShowing: Boolean = false, val isFullScreen: Boolean = false, val isDesktopBrowsingMode: Boolean = false, + val canChangeBrowsingMode: Boolean = true, val showPrivacyGrade: Boolean = false, val showClearButton: Boolean = false, val showTabsButton: Boolean = true, @@ -903,7 +904,8 @@ class BrowserTabViewModel( browserViewState.value = currentBrowserViewState().copy( canGoBack = false, canGoForward = false, - canReportSite = false + canReportSite = false, + canChangeBrowsingMode = false ) loadingViewState.value = LoadingViewState() findInPageViewState.value = FindInPageViewState() From 0435a69213cfc612da19634995c4c55d9298db49 Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Sat, 7 Dec 2019 20:21:04 +0100 Subject: [PATCH 11/22] Fix: onBackPressed was not working properly. * Disabling navigation should affect also webNavigationState * Included a new StateChanged and new WebNavigationState to clear user navigations without affecting Site properties * Clear navigation using navigationStateChanged * Cover new logic and types with tests --- .../app/browser/EmptyNavigationStateTest.kt | 56 +++++++++++++++++++ .../app/browser/TestNavigationState.kt | 27 +++++++++ .../app/browser/WebNavigationStateTest.kt | 31 +++++----- .../app/browser/BrowserTabViewModel.kt | 17 ++++-- .../app/browser/WebNavigationState.kt | 28 ++++++++++ 5 files changed, 137 insertions(+), 22 deletions(-) create mode 100644 app/src/androidTest/java/com/duckduckgo/app/browser/EmptyNavigationStateTest.kt create mode 100644 app/src/androidTest/java/com/duckduckgo/app/browser/TestNavigationState.kt diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/EmptyNavigationStateTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/EmptyNavigationStateTest.kt new file mode 100644 index 000000000000..1b8a96a00c40 --- /dev/null +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/EmptyNavigationStateTest.kt @@ -0,0 +1,56 @@ +/* + * Copyright (c) 2019 DuckDuckGo + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.duckduckgo.app.browser + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Test + +class EmptyNavigationStateTest { + + @Test + fun whenEmptyNavigationStateFromNavigationStateThenBrowserPropertiesAreTheSame() { + val previousState = buildState("originalUrl", "currentUrl", "titlle") + val emptyNavigationState = EmptyNavigationState(previousState) + + assertEquals(emptyNavigationState.currentUrl, previousState.currentUrl) + assertEquals(emptyNavigationState.originalUrl, previousState.originalUrl) + assertEquals(emptyNavigationState.title, previousState.title) + } + + @Test + fun whenEmptyNavigationStateFromNavigationStateThenNavigationPropertiesAreCleared() { + val emptyNavigationState = EmptyNavigationState(buildState("originalUrl", "currentUrl", "titlle")) + + assertEquals(emptyNavigationState.stepsToPreviousPage, 0) + assertFalse(emptyNavigationState.canGoBack) + assertFalse(emptyNavigationState.canGoForward) + assertFalse(emptyNavigationState.hasNavigationHistory) + } + + private fun buildState(originalUrl: String?, currentUrl: String?, title: String? = null): WebNavigationState { + return TestNavigationState( + originalUrl = originalUrl, + currentUrl = currentUrl, + title = title, + stepsToPreviousPage = 1, + canGoBack = true, + canGoForward = true, + hasNavigationHistory = true + ) + } +} \ No newline at end of file diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/TestNavigationState.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/TestNavigationState.kt new file mode 100644 index 000000000000..7be977acba77 --- /dev/null +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/TestNavigationState.kt @@ -0,0 +1,27 @@ +/* + * Copyright (c) 2019 DuckDuckGo + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.duckduckgo.app.browser + +data class TestNavigationState( + override val originalUrl: String?, + override val currentUrl: String?, + override val title: String?, + override val stepsToPreviousPage: Int, + override val canGoBack: Boolean, + override val canGoForward: Boolean, + override val hasNavigationHistory: Boolean +) : WebNavigationState diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/WebNavigationStateTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/WebNavigationStateTest.kt index dbfa686f67d9..7f052c268871 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/WebNavigationStateTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/WebNavigationStateTest.kt @@ -91,13 +91,6 @@ class WebNavigationStateComparisonTest { assertEquals(UrlUpdated("http://same.com/latest"), latestState.compare(previousState)) } - @Test - fun whenPreviousContainsAnOriginalUrlAndCurrentUrlAndLatestContainsSameOriginalUrlAndNoCurrentUrlThenCompareReturnsOther() { - val previousState = buildState("http://same.com", "http://subdomain.previous.com") - val latestState = buildState("http://same.com", null) - assertEquals(Other, latestState.compare(previousState)) - } - @Test fun whenPreviousContainsAnOriginalUrlAndCurrentUrlAndLatestStateContainsNoOriginalUrlAndNoCurrentUrlThenCompareReturnsPageCleared() { val previousState = buildState("http://previous.com", "http://subdomain.previous.com") @@ -112,6 +105,20 @@ class WebNavigationStateComparisonTest { assertEquals(PageCleared, latestState.compare(previousState)) } + @Test + fun whenLatestStateIsEmptyNavigationCompareReturnsPageNavigationCleared() { + val previousState = buildState("http://previous.com", "http://subdomain.previous.com") + val latestState = EmptyNavigationState(previousState) + assertEquals(PageNavigationCleared, latestState.compare(previousState)) + } + + @Test + fun whenPreviousContainsAnOriginalUrlAndCurrentUrlAndLatestContainsSameOriginalUrlAndNoCurrentUrlThenCompareReturnsOther() { + val previousState = buildState("http://same.com", "http://subdomain.previous.com") + val latestState = buildState("http://same.com", null) + assertEquals(Other, latestState.compare(previousState)) + } + @Test fun whenPreviousContainsAnOriginalUrlAndCurrentUrlAndLatestStateContainsDifferentOriginalUrlAndNoCurrentUrlThenCompareReturnsOther() { val previousState = buildState("http://previous.com", "http://subdomain.previous.com") @@ -131,13 +138,3 @@ class WebNavigationStateComparisonTest { ) } } - -data class TestNavigationState( - override val originalUrl: String?, - override val currentUrl: String?, - override val title: String?, - override val stepsToPreviousPage: Int, - override val canGoBack: Boolean, - override val canGoForward: Boolean, - override val hasNavigationHistory: Boolean -) : WebNavigationState diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt index f5c086153dee..ff506b50cf71 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt @@ -457,6 +457,7 @@ class BrowserTabViewModel( is NewPage -> pageChanged(stateChange.url, stateChange.title) is PageCleared -> pageCleared() is UrlUpdated -> urlUpdated(stateChange.url) + is PageNavigationCleared -> disableUserNavigation() } } @@ -878,7 +879,10 @@ class BrowserTabViewModel( } override fun recoverFromRenderProcessGone() { - invalidateUserNavigation() + webNavigationState?.let { + navigationStateChanged(EmptyNavigationState(it)) + } + invalidateBrowsingActions() showErrorWithAction() } @@ -899,16 +903,19 @@ class BrowserTabViewModel( command.value = LaunchTabSwitcher } - private fun invalidateUserNavigation() { - globalLayoutState.value = Invalidated() + private fun invalidateBrowsingActions() { + globalLayoutState.value = Invalidated + loadingViewState.value = LoadingViewState() + findInPageViewState.value = FindInPageViewState() + } + + private fun disableUserNavigation() { browserViewState.value = currentBrowserViewState().copy( canGoBack = false, canGoForward = false, canReportSite = false, canChangeBrowsingMode = false ) - loadingViewState.value = LoadingViewState() - findInPageViewState.value = FindInPageViewState() } private fun showErrorWithAction() { diff --git a/app/src/main/java/com/duckduckgo/app/browser/WebNavigationState.kt b/app/src/main/java/com/duckduckgo/app/browser/WebNavigationState.kt index 9838b259892e..82d552fe58ad 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/WebNavigationState.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/WebNavigationState.kt @@ -37,11 +37,15 @@ sealed class WebNavigationStateChange { data class UrlUpdated(val url: String) : WebNavigationStateChange() object PageCleared : WebNavigationStateChange() object Unchanged : WebNavigationStateChange() + object PageNavigationCleared : WebNavigationStateChange() object Other : WebNavigationStateChange() } fun WebNavigationState.compare(previous: WebNavigationState?): WebNavigationStateChange { + if (this is EmptyNavigationState) + return PageNavigationCleared + if (this == previous) { return Unchanged } @@ -122,6 +126,30 @@ data class WebViewNavigationState(private val stack: WebBackForwardList) : WebNa } } +@Suppress("DataClassPrivateConstructor") +data class EmptyNavigationState private constructor(override val originalUrl: String?, + override val currentUrl: String?, + override val title: String?) : WebNavigationState { + companion object { + operator fun invoke(webNavigationState: WebNavigationState): EmptyNavigationState { + return EmptyNavigationState( + webNavigationState.originalUrl, + webNavigationState.currentUrl, + webNavigationState.title + ) + } + } + + override val stepsToPreviousPage: Int + get() = 0 + override val canGoBack: Boolean + get() = false + override val canGoForward: Boolean + get() = false + override val hasNavigationHistory: Boolean + get() = false +} + private val WebBackForwardList.originalUrl: String? get() = currentItem?.originalUrl From 9ba5c83dbcbb943e44492eb311ec0711a445d754 Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Sun, 8 Dec 2019 17:27:28 +0100 Subject: [PATCH 12/22] Tidy up code. --- .../app/browser/BrowserTabViewModelTest.kt | 2 +- .../app/browser/WebNavigationState.kt | 19 +++++++------------ 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt index 09b906191a9a..f56505a9f2cc 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt @@ -298,7 +298,7 @@ class BrowserTabViewModelTest { } @Test - fun whenInvalidatedGlobalLayoutRestoredAndVisibleThenErrorIsShown() { + fun whenInvalidatedGlobalLayoutRestoredThenErrorIsShown() { givenInvalidatedGlobalLayout() testee.onViewVisible() assertCommandIssued() diff --git a/app/src/main/java/com/duckduckgo/app/browser/WebNavigationState.kt b/app/src/main/java/com/duckduckgo/app/browser/WebNavigationState.kt index 82d552fe58ad..f5acd70059df 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/WebNavigationState.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/WebNavigationState.kt @@ -42,14 +42,13 @@ sealed class WebNavigationStateChange { } fun WebNavigationState.compare(previous: WebNavigationState?): WebNavigationStateChange { - - if (this is EmptyNavigationState) - return PageNavigationCleared - if (this == previous) { return Unchanged } + if (this is EmptyNavigationState) + return PageNavigationCleared + if (originalUrl == null && previous?.originalUrl != null) { return PageCleared } @@ -140,14 +139,10 @@ data class EmptyNavigationState private constructor(override val originalUrl: St } } - override val stepsToPreviousPage: Int - get() = 0 - override val canGoBack: Boolean - get() = false - override val canGoForward: Boolean - get() = false - override val hasNavigationHistory: Boolean - get() = false + override val stepsToPreviousPage: Int = 0 + override val canGoBack: Boolean = false + override val canGoForward: Boolean = false + override val hasNavigationHistory: Boolean = false } private val WebBackForwardList.originalUrl: String? From 31f0f6ec0b14a32755aca93b9da580f5026452c0 Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Mon, 9 Dec 2019 17:44:46 +0100 Subject: [PATCH 13/22] Moving untranslated string resources to a separated file. --- .../main/res/values/string-untranslated.xml | 21 +++++++++++++++++++ app/src/main/res/values/strings.xml | 2 -- 2 files changed, 21 insertions(+), 2 deletions(-) create mode 100644 app/src/main/res/values/string-untranslated.xml diff --git a/app/src/main/res/values/string-untranslated.xml b/app/src/main/res/values/string-untranslated.xml new file mode 100644 index 000000000000..7957c80f3008 --- /dev/null +++ b/app/src/main/res/values/string-untranslated.xml @@ -0,0 +1,21 @@ + + + + + "The webpage could not be displayed." + "Reload" + \ No newline at end of file diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 61f8501aec63..f62e40dbad6e 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -39,8 +39,6 @@ Refresh Back Forward - "The webpage could not be displayed." - "Reload" Search or type URL Clear search input From ad312aa9797574148b1261dc7c31e89d65b2a8f6 Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Tue, 10 Dec 2019 11:24:12 +0100 Subject: [PATCH 14/22] Ignoring untranslated string resources instead of using translatable property. --- app/src/main/res/values/string-untranslated.xml | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/app/src/main/res/values/string-untranslated.xml b/app/src/main/res/values/string-untranslated.xml index 7957c80f3008..97b4f4238726 100644 --- a/app/src/main/res/values/string-untranslated.xml +++ b/app/src/main/res/values/string-untranslated.xml @@ -1,5 +1,4 @@ - - - - "The webpage could not be displayed." - "Reload" + + "The webpage could not be displayed." + "Reload" \ No newline at end of file From 9a9d78ab5e17716bbaf3a3d63534d2a450510dea Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Tue, 10 Dec 2019 12:37:28 +0100 Subject: [PATCH 15/22] Add assertion to validate expected URL when opening new tab. --- .../com/duckduckgo/app/browser/BrowserTabViewModelTest.kt | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt index f56505a9f2cc..6c4c0e98e161 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt @@ -1122,7 +1122,9 @@ class BrowserTabViewModelTest { showErrorWithAction.action() - assertCommandIssued() + assertCommandIssued { + assertEquals("https://example.com", query) + } } @Test @@ -1278,10 +1280,11 @@ class BrowserTabViewModelTest { verify(mockCommandObserver, never()).onChanged(commandCaptor.capture()) } - private inline fun assertCommandIssued() { + private inline fun assertCommandIssued(instanceAssertions: T.() -> Unit = {}) { verify(mockCommandObserver, atLeastOnce()).onChanged(commandCaptor.capture()) val issuedCommand = commandCaptor.allValues.find { it is T } assertNotNull(issuedCommand) + (issuedCommand as T).apply { instanceAssertions() } } private fun pixelParams(showedBookmarks: Boolean, bookmarkCapable: Boolean) = mapOf( From fccad7b6f7ac8f130deeade564651f838b008884 Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Tue, 10 Dec 2019 16:06:12 +0100 Subject: [PATCH 16/22] Full clean up when WebView crashes. --- .../java/com/duckduckgo/app/survey/ui/SurveyActivity.kt | 9 ++++++++- app/src/main/res/layout/activity_user_survey.xml | 1 + 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/com/duckduckgo/app/survey/ui/SurveyActivity.kt b/app/src/main/java/com/duckduckgo/app/survey/ui/SurveyActivity.kt index 36e5df03a584..c608d9086566 100644 --- a/app/src/main/java/com/duckduckgo/app/survey/ui/SurveyActivity.kt +++ b/app/src/main/java/com/duckduckgo/app/survey/ui/SurveyActivity.kt @@ -101,8 +101,8 @@ class SurveyActivity : DuckDuckGoActivity() { private fun showError() { progress.gone() - webView.gone() errorView.show() + destroyWebView() } override fun onSaveInstanceState(outState: Bundle) { @@ -128,6 +128,13 @@ class SurveyActivity : DuckDuckGoActivity() { viewModel.onSurveyDismissed() } + private fun destroyWebView() { + webView.gone() + surveyActivityContainerViewGroup.removeView(webView) + webView.destroy() + webView.webViewClient = null + } + companion object { fun intent(context: Context, survey: Survey): Intent { diff --git a/app/src/main/res/layout/activity_user_survey.xml b/app/src/main/res/layout/activity_user_survey.xml index 56333bf7cd3e..480980388bdc 100644 --- a/app/src/main/res/layout/activity_user_survey.xml +++ b/app/src/main/res/layout/activity_user_survey.xml @@ -24,6 +24,7 @@ tools:context="com.duckduckgo.app.feedback.ui.common.FeedbackActivity"> From 0f292968aec5f8327e9548f76da458129ae81cb7 Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Tue, 10 Dec 2019 17:20:23 +0100 Subject: [PATCH 17/22] Use a more correct layout parent to show snackbar. (no behvior change) --- .../main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt index 5beda1b3c8f3..fd3e2b21222f 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt @@ -204,7 +204,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { private var webView: WebView? = null private val errorSnackbar: Snackbar by lazy { - Snackbar.make(rootView, R.string.crashedWebViewErrorMessage, Snackbar.LENGTH_INDEFINITE) + Snackbar.make(browserLayout, R.string.crashedWebViewErrorMessage, Snackbar.LENGTH_INDEFINITE) .setBehavior(NonDismissibleBehavior()) } From 254ba382289460879dbddf3d50ff13eb51630fdd Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Tue, 10 Dec 2019 17:22:32 +0100 Subject: [PATCH 18/22] Object comparison using type and not instance. --- .../com/duckduckgo/app/browser/BrowserTabFragment.kt | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt index fd3e2b21222f..a311c5dcb30f 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt @@ -413,7 +413,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { private fun processCommand(it: Command?) { when (it) { - Command.Refresh -> refresh() + is Command.Refresh -> refresh() is Command.OpenInNewTab -> { browserActivity?.openInNewTab(it.query) } @@ -428,10 +428,10 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { is Command.NavigateBack -> { webView?.goBackOrForward(-it.steps) } - Command.NavigateForward -> { + is Command.NavigateForward -> { webView?.goForward() } - Command.ResetHistory -> { + is Command.ResetHistory -> { resetWebView() } is Command.DialNumber -> { @@ -448,10 +448,10 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { val intent = Intent(Intent.ACTION_SENDTO, Uri.parse("smsto:${it.telephoneNumber}")) openExternalDialog(intent) } - Command.ShowKeyboard -> { + is Command.ShowKeyboard -> { showKeyboard() } - Command.HideKeyboard -> { + is Command.HideKeyboard -> { hideKeyboard() } is Command.BrokenSiteFeedback -> { @@ -467,7 +467,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { } is Command.DownloadImage -> requestImageDownload(it.url) is Command.FindInPageCommand -> webView?.findAllAsync(it.searchTerm) - Command.DismissFindInPage -> webView?.findAllAsync(null) + is Command.DismissFindInPage -> webView?.findAllAsync(null) is Command.ShareLink -> launchSharePageChooser(it.url) is Command.CopyLink -> { clipboardManager.primaryClip = ClipData.newPlainText(null, it.url) From ffad15d763874804341344f096cdb80b7746df34 Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Tue, 10 Dec 2019 18:47:38 +0100 Subject: [PATCH 19/22] Fix: Avoid showing home when webview crashes (only when home was behind webview). --- .../main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt index a311c5dcb30f..e1a97e8f5e53 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt @@ -379,6 +379,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { } private fun showHome() { + newTabLayout.show() showKeyboardImmediately() appBarLayout.setExpanded(true) webView?.onPause() @@ -388,6 +389,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { } private fun showBrowser() { + newTabLayout.gone() webView?.show() webView?.onResume() omnibarScrolling.enableOmnibarScrolling(toolbarContainer) From b87e4f504cd5b1bb0e82f39ca79d838801cc141c Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Tue, 10 Dec 2019 18:48:35 +0100 Subject: [PATCH 20/22] Avoid showing ErrorSnackbar when user goes to Home screen. --- .../java/com/duckduckgo/app/browser/BrowserTabFragment.kt | 1 + .../java/com/duckduckgo/app/browser/BrowserTabViewModel.kt | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt index e1a97e8f5e53..fa6d819d5401 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt @@ -379,6 +379,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { } private fun showHome() { + errorSnackbar.dismiss() newTabLayout.show() showKeyboardImmediately() appBarLayout.setExpanded(true) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt index ff506b50cf71..712797937234 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt @@ -300,7 +300,9 @@ class BrowserTabViewModel( fun onViewVisible() { command.value = if (!currentBrowserViewState().browserShowing) ShowKeyboard else HideKeyboard ctaViewModel.refreshCta() - if (currentGlobalLayoutState() is Invalidated) showErrorWithAction() + if (currentGlobalLayoutState() is Invalidated && currentBrowserViewState().browserShowing) { + showErrorWithAction() + } } fun onViewHidden() { From 75183a85f15cbf47dffba3cc8ed16ad0e10d5f20 Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Tue, 10 Dec 2019 18:51:52 +0100 Subject: [PATCH 21/22] When user navigates home, show navigate forward only if state is not invalid. --- .../main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt index 712797937234..73a8bb68a34f 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt @@ -422,7 +422,7 @@ class BrowserTabViewModel( browserViewState.value = currentBrowserViewState().copy( browserShowing = false, canGoBack = false, - canGoForward = true + canGoForward = currentGlobalLayoutState() !is Invalidated ) omnibarViewState.value = currentOmnibarViewState().copy(omnibarText = "", shouldMoveCaretToEnd = false) loadingViewState.value = currentLoadingViewState().copy(isLoading = false) From 41807ea3cba54ebff05c2f775c0ea5798ac69f0a Mon Sep 17 00:00:00 2001 From: Cristian Monforte Date: Tue, 10 Dec 2019 19:00:48 +0100 Subject: [PATCH 22/22] Unit tested cases: * show error when using is browing, not when is in home page. * when goes to home page after a crash, disable forward navigation. --- .../duckduckgo/app/browser/BrowserTabViewModelTest.kt | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt index 6c4c0e98e161..2beafad20e09 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt @@ -300,6 +300,7 @@ class BrowserTabViewModelTest { @Test fun whenInvalidatedGlobalLayoutRestoredThenErrorIsShown() { givenInvalidatedGlobalLayout() + setBrowserShowing(true) testee.onViewVisible() assertCommandIssued() } @@ -834,6 +835,15 @@ class BrowserTabViewModelTest { assertTrue(browserViewState().canGoForward) } + @Test + fun whenHomeShowingByPressingBackOnInvalidatedBrowserThenForwardButtonInactive() { + setupNavigation(isBrowsing = true) + givenInvalidatedGlobalLayout() + testee.onUserPressedBack() + assertFalse(browserViewState().browserShowing) + assertFalse(browserViewState().canGoForward) + } + @Test fun whenBrowserShowingAndCanGoForwardThenForwardButtonActive() { setupNavigation(isBrowsing = true, canGoForward = true)