Feature/phishing url fix (#390)

* Make use of onProgressChanged instead on onPageCommitVisible

We erroneously relied on onPageCommitVisible() but it was only ever called on API 23 or up, breaking functionality for Lollipop users.

We put in a patch, which made use of onPageCommitVisible() for newer users only. Now, we're stopping relying on onPageCommitVisible altogether and making use of onProgressChanged instead for the functionality.

* Alternative approach to handling when url has changed

* Remove redundant variable

Author: CDRussell

app/src/androidTest/java/com/duckduckgo/app/browser/BrowserChromeClientTest.kt

@@ -16,9 +16,13 @@
 
 package com.duckduckgo.app.browser
 
+import android.content.Context
 import android.support.test.InstrumentationRegistry
+import android.support.test.annotation.UiThreadTest
 import android.view.View
 import android.webkit.WebChromeClient
+import android.webkit.WebView
+import com.nhaarman.mockito_kotlin.any
 import com.nhaarman.mockito_kotlin.mock
 import com.nhaarman.mockito_kotlin.times
 import com.nhaarman.mockito_kotlin.verify
@@ -28,14 +32,17 @@ import org.junit.Test
 class BrowserChromeClientTest {
 
     private lateinit var testee: BrowserChromeClient
+    private lateinit var webView: TestWebView
     private lateinit var mockWebViewClientListener: WebViewClientListener
     private val fakeView = View(InstrumentationRegistry.getTargetContext())
 
+    @UiThreadTest
     @Before
     fun setup() {
         testee = BrowserChromeClient()
         mockWebViewClientListener = mock()
         testee.webViewClientListener = mockWebViewClientListener
+        webView = TestWebView(InstrumentationRegistry.getTargetContext())
     }
 
     @Test
@@ -66,4 +73,59 @@ class BrowserChromeClientTest {
         testee.onHideCustomView()
         verify(mockWebViewClientListener).exitFullScreen()
     }
+
+    @UiThreadTest
+    @Test
+    fun whenOnProgressChangedCalledThenListenerInstructedToUpdateProgress() {
+        testee.onProgressChanged(webView, 10)
+        verify(mockWebViewClientListener).progressChanged(10)
+    }
+
+    @UiThreadTest
+    @Test
+    fun whenOnProgressChangedCalledButNoUrlChangeThenListenerInstructedToUpdateProgressASecondTime() {
+        webView.stubUrl = "foo.com"
+        testee.onProgressChanged(webView, 10)
+        testee.onProgressChanged(webView, 20)
+        verify(mockWebViewClientListener, times(2)).progressChanged(any())
+    }
+
+    @UiThreadTest
+    @Test
+    fun whenOnProgressChangedCalledAfterUrlChangeThenListenerInstructedToUpdateProgressAgain() {
+        webView.stubUrl = "foo.com"
+        testee.onProgressChanged(webView, 10)
+        testee.onProgressChanged(webView, 20)
+        webView.stubUrl = "bar.com"
+        testee.onProgressChanged(webView, 30)
+        verify(mockWebViewClientListener, times(3)).progressChanged(any())
+    }
+
+    @UiThreadTest
+    @Test
+    fun whenOnProgressChangedCalledThenListenerInstructedToUpdateUrl() {
+        val url = "https://example.com"
+        webView.stubUrl = url
+        testee.onProgressChanged(webView, 10)
+        verify(mockWebViewClientListener).urlChanged(url)
+    }
+
+    @UiThreadTest
+    @Test
+    fun whenOnProgressChangedCalledAfterUrlChangeThenListenerInstructedToUpdateUrlEveryTime() {
+        webView.stubUrl = "foo.com"
+        testee.onProgressChanged(webView, 10)
+        testee.onProgressChanged(webView, 20)
+        webView.stubUrl = "bar.com"
+        testee.onProgressChanged(webView, 30)
+        verify(mockWebViewClientListener, times(3)).urlChanged(any())
+    }
+
+    private class TestWebView(context: Context) : WebView(context) {
+        var stubUrl: String = ""
+
+        override fun getUrl(): String {
+            return stubUrl
+        }
+    }
 }

app/src/androidTest/java/com/duckduckgo/app/browser/BrowserWebViewClientTest.kt

@@ -17,10 +17,8 @@
 package com.duckduckgo.app.browser
 
 import android.content.Context
-import android.os.Build
 import android.support.test.InstrumentationRegistry
 import android.support.test.annotation.UiThreadTest
-import android.support.test.filters.SdkSuppress
 import android.webkit.WebView
 import com.duckduckgo.app.httpsupgrade.HttpsUpgrader
 import com.duckduckgo.app.statistics.pixels.Pixel
@@ -63,35 +61,11 @@ class BrowserWebViewClientTest {
 
     @UiThreadTest
     @Test
-    @SdkSuppress(minSdkVersion = Build.VERSION_CODES.M)
-    fun whenOnPageStartedCalledOnNewerDevicesThenListenerNotInstructedToUpdateUrl() {
+    fun whenOnPageStartedCalledThenListenerNeverInstructedToUpdateUrl() {
         testee.onPageStarted(webView, EXAMPLE_URL, null)
         verify(listener, never()).urlChanged(any())
     }
 
-    @UiThreadTest
-    @Test
-    @SdkSuppress(maxSdkVersion = Build.VERSION_CODES.LOLLIPOP_MR1)
-    fun whenOnPageStartedCalledOnOlderDevicesThenListenerInstructedToUpdateUrl() {
-        testee.onPageStarted(webView, EXAMPLE_URL, null)
-        verify(listener).urlChanged(any())
-    }
-
-    @SdkSuppress(minSdkVersion = Build.VERSION_CODES.M)
-    @UiThreadTest
-    @Test
-    fun whenOnPageCommitVisibleCalledThenListenerInstructedToUpdateUrl() {
-        testee.onPageCommitVisible(webView, EXAMPLE_URL)
-        verify(listener).urlChanged(EXAMPLE_URL)
-    }
-
-    @UiThreadTest
-    @Test
-    fun whenOnPageCommitVisibleCalledThenListenerInstructedToUpdateNavigationOptions() {
-        testee.onPageCommitVisible(webView, EXAMPLE_URL)
-        verify(listener).navigationOptionsChanged(any())
-    }
-
     @UiThreadTest
     @Test
     fun whenOnPageFinishedCalledThenListenerNotified() {
@@ -106,6 +80,13 @@ class BrowserWebViewClientTest {
         verify(listener).navigationOptionsChanged(any())
     }
 
+    @UiThreadTest
+    @Test
+    fun whenOnPageFinishedCalledThenListenerInstructedToUpdateUrl() {
+        testee.onPageFinished(webView, EXAMPLE_URL)
+        verify(listener).urlChanged(EXAMPLE_URL)
+    }
+
     private class TestWebView(context: Context) : WebView(context)
 
     companion object {

app/src/main/java/com/duckduckgo/app/browser/BrowserChromeClient.kt

@@ -32,7 +32,7 @@ class BrowserChromeClient @Inject constructor() : WebChromeClient() {
     private var customView: View? = null
 
     override fun onShowCustomView(view: View, callback: CustomViewCallback?) {
-        Timber.i("on show custom view")
+        Timber.d("on show custom view")
 
         if (customView != null) {
             callback?.onCustomViewHidden()
@@ -44,14 +44,22 @@ class BrowserChromeClient @Inject constructor() : WebChromeClient() {
     }
 
     override fun onHideCustomView() {
-        Timber.i("hide custom view")
+        Timber.d("on hide custom view")
 
         webViewClientListener?.exitFullScreen()
         customView = null
     }
 
     override fun onProgressChanged(webView: WebView, newProgress: Int) {
+        Timber.d("onProgressChanged - $newProgress - ${webView.url}")
+
         webViewClientListener?.progressChanged(newProgress)
+
+        val currentUrl = webViewClientListener?.currentUrl()
+        if (currentUrl != webView.url) {
+            Timber.i("Url has changed from $currentUrl to ${webView.url}")
+            webViewClientListener?.urlChanged(webView.url)
+        }
     }
 
     override fun onReceivedTitle(view: WebView, title: String) {

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

@@ -85,7 +85,6 @@ class BrowserTabViewModel(
     private val addToHomeCapabilityDetector: AddToHomeCapabilityDetector,
     appConfigurationDao: AppConfigurationDao
 ) : WebViewClientListener, SaveBookmarkListener, ViewModel() {
-
     data class GlobalLayoutViewState(
         val isNewTabState: Boolean = true
     )
@@ -343,7 +342,11 @@ class BrowserTabViewModel(
             findInPageViewState.value = FindInPageViewState(visible = false, canFindInPage = false)
 
             val currentBrowserViewState = currentBrowserViewState()
-            browserViewState.value = currentBrowserViewState.copy(canAddBookmarks = false, addToHomeEnabled = false, addToHomeVisible = addToHomeCapabilityDetector.isAddToHomeSupported())
+            browserViewState.value = currentBrowserViewState.copy(
+                canAddBookmarks = false,
+                addToHomeEnabled = false,
+                addToHomeVisible = addToHomeCapabilityDetector.isAddToHomeSupported()
+            )
 
             return
         }
@@ -367,8 +370,10 @@ class BrowserTabViewModel(
             statisticsUpdater.refreshRetentionAtb()
         }
 
-        site = siteFactory.build(url)
-        onSiteChanged()
+        if (currentUrl() != url) {
+            site = siteFactory.build(url)
+            onSiteChanged()
+        }
     }
 
     private fun omnibarTextForUrl(url: String): String {
@@ -557,6 +562,10 @@ class BrowserTabViewModel(
         }
     }
 
+    override fun currentUrl(): String? {
+        return site?.url
+    }
+
     fun resetView() {
         site = null
         url.value = null

app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt

@@ -52,8 +52,6 @@ class BrowserWebViewClient @Inject constructor(
 
     private var currentUrl: String? = null
 
-    private val willGetNotifiedOfPageCommits = Build.VERSION.SDK_INT >= Build.VERSION_CODES.M
-
     /**
      * This is the new method of url overriding available from API 24 onwards
      */
@@ -109,33 +107,27 @@ class BrowserWebViewClient @Inject constructor(
     }
 
     override fun onPageStarted(webView: WebView, url: String?, favicon: Bitmap?) {
-        Timber.d("onPageStarted $url")
-
-        webViewClientListener?.loadingStarted()
+        Timber.d("\nonPageStarted {\nurl: $url\nwebView.url: ${webView.url}\n}\n")
 
-        if (!willGetNotifiedOfPageCommits) onUrlChanged(url, webView)
+        currentUrl = url
+        webViewClientListener?.let {
+            it.loadingStarted()
+            it.navigationOptionsChanged(determineNavigationOptions(webView))
+        }
 
-        val uri = if (url != null) Uri.parse(url) else null
+        val uri = if (currentUrl != null) Uri.parse(currentUrl) else null
         if (uri != null) {
             reportHttpsIfInUpgradeList(uri)
         }
     }
 
-    /**
-     * Note, this method is only called on APIs >= 23
-     * While this is the ideal time to indicate the URL has changed, on lower APIs we need to handle that instead in onPageStarted()
-     */
-    override fun onPageCommitVisible(webView: WebView, url: String?) {
-        Timber.d("onPageCommitVisible $url")
-
-        onUrlChanged(url, webView)
-    }
-
     override fun onPageFinished(webView: WebView, url: String?) {
         Timber.d("onPageFinished $url")
 
+        currentUrl = url
         webViewClientListener?.let {
             it.loadingFinished(url)
+            it.urlChanged(url)
             it.navigationOptionsChanged(determineNavigationOptions(webView))
         }
     }
@@ -222,14 +214,6 @@ class BrowserWebViewClient @Inject constructor(
         return BrowserNavigationOptions(canGoBack, canGoForward)
     }
 
-    private fun onUrlChanged(url: String?, webView: WebView) {
-        currentUrl = url
-        webViewClientListener?.let {
-            it.urlChanged(url)
-            it.navigationOptionsChanged(determineNavigationOptions(webView))
-        }
-    }
-
     data class BrowserNavigationOptions(val canGoBack: Boolean, val canGoForward: Boolean)
 
 }

app/src/main/java/com/duckduckgo/app/browser/WebViewClientListener.kt

@@ -24,6 +24,8 @@ import com.duckduckgo.app.browser.BrowserWebViewClient.BrowserNavigationOptions
 import com.duckduckgo.app.trackerdetection.model.TrackingEvent
 
 interface WebViewClientListener {
+    fun currentUrl(): String?
+
     fun loadingStarted()
     fun loadingFinished(url: String? = null)
     fun progressChanged(newProgress: Int)