From 1e8332fbea9ce9f258cde24778843ddc7ab9c49b Mon Sep 17 00:00:00 2001 From: Christian Sadilek Date: Thu, 25 Apr 2019 18:37:13 -0400 Subject: [PATCH] Closes #1385: Determine and expose reader-ability state --- .../components/browser/session/Session.kt | 8 ++ .../components/browser/session/SessionTest.kt | 25 ++++++ .../extensions/readerview/readerview.js | 34 ++++++-- .../feature/readerview/ReaderViewFeature.kt | 78 ++++++++++++++----- .../readerview/ReaderViewFeatureTest.kt | 70 ++++++++++++++++- 5 files changed, 190 insertions(+), 25 deletions(-) diff --git a/components/browser/session/src/main/java/mozilla/components/browser/session/Session.kt b/components/browser/session/src/main/java/mozilla/components/browser/session/Session.kt index 6299d721b62..9f8012c346b 100644 --- a/components/browser/session/src/main/java/mozilla/components/browser/session/Session.kt +++ b/components/browser/session/src/main/java/mozilla/components/browser/session/Session.kt @@ -72,6 +72,7 @@ class Session( fun onMediaAdded(session: Session, media: List, added: Media) = Unit fun onCrashStateChanged(session: Session, crashed: Boolean) = Unit fun onIconChanged(session: Session, icon: Bitmap?) = Unit + fun onReaderableStateUpdated(session: Session, readerable: Boolean) = Unit } /** @@ -371,6 +372,13 @@ class Session( notifyObservers(old, new) { onCrashStateChanged(this@Session, new) } } + /** + * Readerable state, whether or not the current page can be shown in a reader view. + */ + var readerable: Boolean by Delegates.observable(false) { _, _, new -> + notifyObservers { onReaderableStateUpdated(this@Session, new) } + } + /** * Returns whether or not this session is used for a Custom Tab. */ diff --git a/components/browser/session/src/test/java/mozilla/components/browser/session/SessionTest.kt b/components/browser/session/src/test/java/mozilla/components/browser/session/SessionTest.kt index af89bf9885e..bb4b29bb0c7 100644 --- a/components/browser/session/src/test/java/mozilla/components/browser/session/SessionTest.kt +++ b/components/browser/session/src/test/java/mozilla/components/browser/session/SessionTest.kt @@ -609,6 +609,7 @@ class SessionTest { defaultObserver.onMediaAdded(session, emptyList(), mock()) defaultObserver.onMediaRemoved(session, emptyList(), mock()) defaultObserver.onIconChanged(session, mock()) + defaultObserver.onReaderableStateUpdated(session, true) } @Test @@ -932,4 +933,28 @@ class SessionTest { assertEquals(bitmapMock, notifiedIcon) } + + @Test + fun `observer is notified when readerable state updated`() { + val observer = mock(Session.Observer::class.java) + + val session = Session("https://www.mozilla.org") + session.register(observer) + + session.readerable = true + + verify(observer).onReaderableStateUpdated( + eq(session), + eq(true)) + + // We want to notify observers every time readerability is determined, + // not only when the state changed. + session.readerable = true + + verify(observer, times(2)).onReaderableStateUpdated( + eq(session), + eq(true)) + + assertTrue(session.readerable) + } } diff --git a/components/feature/readerview/src/main/assets/extensions/readerview/readerview.js b/components/feature/readerview/src/main/assets/extensions/readerview/readerview.js index 99514ed829b..f645588e84b 100644 --- a/components/feature/readerview/src/main/assets/extensions/readerview/readerview.js +++ b/components/feature/readerview/src/main/assets/extensions/readerview/readerview.js @@ -5,9 +5,24 @@ /* Avoid adding ID selector rules in this style sheet, since they could * inadvertently match elements in the article content. */ +const supportedProtocols = ["http:", "https:"]; +const blockedHosts = ["amazon.com", "github.com", "mail.google.com", "pinterest.com", "reddit.com", "twitter.com", "youtube.com"]; + class ReaderView { static isReaderable() { + if (!supportedProtocols.includes(location.protocol)) { + return false; + } + + if (blockedHosts.some(blockedHost => location.hostname.endsWith(blockedHost))) { + return false; + } + + if (location.pathname == "/") { + return false; + } + return isProbablyReaderable(document); } @@ -261,15 +276,24 @@ class ReaderView { } } -// TODO remove (for testing purposes only) let port = browser.runtime.connectNative("mozacReaderview"); -port.postMessage(`Hello from ReaderView on page ${location.hostname}`); -port.onMessage.addListener((response) => { - console.log(`Received: ${JSON.stringify(response)} on page ${location.hostname}`); +port.onMessage.addListener((message) => { + switch (message.action) { + case 'show': + readerView.show({fontSize: 3, fontType: "serif", colorScheme: "light"}); + break; + case 'hide': + readerView.hide(); + break; + case 'checkReaderable': + port.postMessage({readerable: ReaderView.isReaderable()}); + break; + default: + console.error(`Received invalid action ${message.action}`); + } }); - // TODO remove hostname check (for testing purposes only) // e.g. https://blog.mozilla.org/firefox/reader-view if (ReaderView.isReaderable() && location.hostname.endsWith("blog.mozilla.org")) { diff --git a/components/feature/readerview/src/main/java/mozilla/components/feature/readerview/ReaderViewFeature.kt b/components/feature/readerview/src/main/java/mozilla/components/feature/readerview/ReaderViewFeature.kt index 78ec85c6ae2..140a8101277 100644 --- a/components/feature/readerview/src/main/java/mozilla/components/feature/readerview/ReaderViewFeature.kt +++ b/components/feature/readerview/src/main/java/mozilla/components/feature/readerview/ReaderViewFeature.kt @@ -38,8 +38,11 @@ typealias OnReaderViewAvailableChange = (available: Boolean) -> Unit * @property sessionManager a reference to the application's [SessionManager]. * @property onReaderViewAvailableChange a callback invoked to indicate whether * or not reader view is available for the page loaded by the currently selected - * (active) session. + * session. The callback will be invoked when a page is loaded or refreshed, + * on any navigation (back or forward), and when the selected session + * changes. */ +@Suppress("TooManyFunctions") class ReaderViewFeature( private val context: Context, private val engine: Engine, @@ -93,6 +96,8 @@ class ReaderViewFeature( ReaderViewFeature.install(engine) } + checkReaderable() + controlsInteractor.start() } @@ -101,26 +106,24 @@ class ReaderViewFeature( return } - // TODO https://github.com/mozilla-mobile/android-components/issues/2624 val messageHandler = object : MessageHandler { override fun onPortConnected(port: Port) { - ReaderViewFeature.ports[port.engineSession] = port + ports[port.engineSession] = port + checkReaderable() } override fun onPortDisconnected(port: Port) { - ReaderViewFeature.ports.remove(port.engineSession) + ports.remove(port.engineSession) } override fun onPortMessage(message: Any, port: Port) { - Logger.info("Received message: $message") - val response = JSONObject().apply { - put("response", "Message received! Hello from Android!") + if (message is JSONObject) { + activeSession?.readerable = message.optBoolean(READERABLE_RESPONSE_MESSAGE_KEY, false) } - ReaderViewFeature.sendMessage(response, port.engineSession!!) } } - ReaderViewFeature.registerMessageHandler(sessionManager.getOrCreateEngineSession(session), messageHandler) + registerMessageHandler(sessionManager.getOrCreateEngineSession(session), messageHandler) } override fun stop() { @@ -136,19 +139,32 @@ class ReaderViewFeature( override fun onSessionSelected(session: Session) { // TODO restore selected state of whether the controls are open or not registerContentMessageHandler(activeSession) + checkReaderable() super.onSessionSelected(session) } override fun onSessionRemoved(session: Session) { - ReaderViewFeature.ports.remove(sessionManager.getEngineSession(session)) + ports.remove(sessionManager.getEngineSession(session)) + } + + override fun onUrlChanged(session: Session, url: String) { + checkReaderable() + } + + override fun onReaderableStateUpdated(session: Session, readerable: Boolean) { + onReaderViewAvailableChange(readerable) } fun showReaderView() { - // TODO send message to show reader view (-> see ReaderView.show()) + activeSession?.let { + sendMessage(JSONObject().put(ACTION_MESSAGE_KEY, ACTION_SHOW), it) + } } fun hideReaderView() { - // TODO send message to hide reader view (-> see ReaderView.hide()) + activeSession?.let { + sendMessage(JSONObject().put(ACTION_MESSAGE_KEY, ACTION_HIDE), it) + } } /** @@ -165,12 +181,43 @@ class ReaderViewFeature( controlsPresenter.hide() } + internal fun checkReaderable() { + activeSession?.let { + if (ports.containsKey(sessionManager.getEngineSession(it))) { + sendMessage(JSONObject().put(ACTION_MESSAGE_KEY, ACTION_CHECK_READERABLE), it) + } + } + } + + private fun sendMessage(msg: Any, session: Session) { + val port = ports[sessionManager.getEngineSession(session)] + port?.postMessage(msg) ?: throw IllegalStateException("No port connected for the provided session") + } + companion object { - @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + @VisibleForTesting internal const val READER_VIEW_EXTENSION_ID = "mozacReaderview" + + @VisibleForTesting internal const val READER_VIEW_EXTENSION_URL = "resource://android/assets/extensions/readerview/" + @VisibleForTesting + internal const val ACTION_MESSAGE_KEY = "action" + + @VisibleForTesting + internal const val ACTION_SHOW = "show" + + @VisibleForTesting + internal const val ACTION_HIDE = "hide" + + @VisibleForTesting + internal const val ACTION_CHECK_READERABLE = "checkReaderable" + + @VisibleForTesting + internal const val READERABLE_RESPONSE_MESSAGE_KEY = "readerable" + @Volatile + @VisibleForTesting internal var installedWebExt: WebExtension? = null @Volatile @@ -205,10 +252,5 @@ class ReaderViewFeature( installedWebExt?.let { registerContentMessageHandler(it) } } - - fun sendMessage(msg: Any, engineSession: EngineSession) { - val port = ports[engineSession] - port?.postMessage(msg) ?: throw IllegalStateException("No port connected for the provided session") - } } } diff --git a/components/feature/readerview/src/test/java/mozilla/components/feature/readerview/ReaderViewFeatureTest.kt b/components/feature/readerview/src/test/java/mozilla/components/feature/readerview/ReaderViewFeatureTest.kt index 95a47616ae3..516e8004558 100644 --- a/components/feature/readerview/src/test/java/mozilla/components/feature/readerview/ReaderViewFeatureTest.kt +++ b/components/feature/readerview/src/test/java/mozilla/components/feature/readerview/ReaderViewFeatureTest.kt @@ -18,6 +18,8 @@ import mozilla.components.support.test.any import mozilla.components.support.test.argumentCaptor import mozilla.components.support.test.eq import mozilla.components.support.test.mock +import org.json.JSONObject +import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull @@ -28,6 +30,7 @@ import org.junit.runner.RunWith import org.mockito.ArgumentMatchers.anyInt import org.mockito.Mockito.`when` import org.mockito.Mockito.mock +import org.mockito.Mockito.never import org.mockito.Mockito.spy import org.mockito.Mockito.times import org.mockito.Mockito.verify @@ -100,7 +103,6 @@ class ReaderViewFeatureTest { val engineSession: EngineSession = mock() val ext: WebExtension = mock() val messageHandler = argumentCaptor() - val message: Any = mock() ReaderViewFeature.installedWebExt = ext @@ -117,8 +119,9 @@ class ReaderViewFeatureTest { messageHandler.value.onPortConnected(port) assertTrue(ReaderViewFeature.ports.containsValue(port)) + val message = JSONObject().put("readerable", true) messageHandler.value.onPortMessage(message, port) - verify(port).postMessage(any()) + verify(session).readerable = true messageHandler.value.onPortDisconnected(port) assertFalse(ReaderViewFeature.ports.containsValue(port)) @@ -158,4 +161,67 @@ class ReaderViewFeatureTest { verify(view).hideControls() } + + @Test + fun `check readerable on start`() { + val engine = mock(Engine::class.java) + val sessionManager: SessionManager = mock() + val view: ReaderViewControlsView = mock() + + val readerViewFeature = spy(ReaderViewFeature(context, engine, sessionManager, view)) + readerViewFeature.start() + + verify(readerViewFeature).checkReaderable() + } + + @Test + fun `check readerable when session is selected`() { + val engine = mock(Engine::class.java) + val sessionManager: SessionManager = mock() + val view: ReaderViewControlsView = mock() + + val readerViewFeature = spy(ReaderViewFeature(context, engine, sessionManager, view)) + readerViewFeature.onSessionSelected(mock()) + + verify(readerViewFeature).checkReaderable() + } + + @Test + fun `check readerable when url changed`() { + val engine = mock(Engine::class.java) + val sessionManager: SessionManager = mock() + val view: ReaderViewControlsView = mock() + + val readerViewFeature = spy(ReaderViewFeature(context, engine, sessionManager, view)) + readerViewFeature.onUrlChanged(mock(), "") + + verify(readerViewFeature).checkReaderable() + } + + @Test + fun `readerable check sends message to web extension`() { + val engine = mock(Engine::class.java) + val sessionManager: SessionManager = mock() + val view: ReaderViewControlsView = mock() + val ext: WebExtension = mock() + val session: Session = mock() + val engineSession: EngineSession = mock() + val port: Port = mock() + val message = argumentCaptor() + + `when`(sessionManager.selectedSession).thenReturn(session) + `when`(sessionManager.getEngineSession(session)).thenReturn(engineSession) + `when`(sessionManager.getOrCreateEngineSession(session)).thenReturn(engineSession) + val readerViewFeature = spy(ReaderViewFeature(context, engine, sessionManager, view)) + ReaderViewFeature.installedWebExt = ext + ReaderViewFeature.ports[engineSession] = port + + readerViewFeature.checkReaderable() + verify(port, never()).postMessage(eq(message)) + + readerViewFeature.observeSelected() + readerViewFeature.checkReaderable() + verify(port, times(1)).postMessage(message.capture()) + assertEquals(ReaderViewFeature.ACTION_CHECK_READERABLE, message.value[ReaderViewFeature.ACTION_MESSAGE_KEY]) + } }