Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Commit

Permalink
Merge #6902 #6906
Browse files Browse the repository at this point in the history 
6902: Closes #6881: Register/unregister download notification actions receiver r=csadilek a=Amejia481



6906: Issue #2985: Support loading reader view ext. page in new session r=jonalmeida a=csadilek

Basically, a leftover from the refactoring. We used to turn on reader mode when the port connected and reader mode was active for this session. Now we have a separate page which could be loaded in a new session. When the reader extension page connect, we can simply "show" reader mode (and mark it as active for the session, but that part already worked.)

Co-authored-by: Arturo Mejia <[email protected]>
Co-authored-by: Christian Sadilek <[email protected]>
  • Loading branch information
@MozLando @Amejia481 @csadilek
3 people committed May 7, 2020
3 parents 9adccaa + cc550a1 + 3b8908f commit 246fc69
Show file tree
Hide file tree
Showing 5 changed files with 89 additions and 39 deletions.
18 changes: 15 additions & 3 deletions ...nloads/src/main/java/mozilla/components/feature/downloads/AbstractFetchDownloadService.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,9 +211,13 @@ abstract class AbstractFetchDownloadService : Service() {

override fun onBind(intent: Intent?): IBinder? = null

override fun onCreate() {
super.onCreate()
registerNotificationActionsReceiver()
}

override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int {
val download = intent?.getDownloadExtra() ?: return START_REDELIVER_INTENT
registerForUpdates()

// If the job already exists, then don't create a new ID. This can happen when calling tryAgain
val foregroundServiceId = downloadJobs[download.id]?.foregroundServiceId ?: Random.nextInt()
Expand Down Expand Up @@ -319,7 +323,8 @@ abstract class AbstractFetchDownloadService : Service() {
clearAllDownloadsNotificationsAndJobs()
}

// Cancels all running jobs and remove all notifications
// Cancels all running jobs and remove all notifications.
// Also cleans any resources that we were holding like broadcastReceivers
internal fun clearAllDownloadsNotificationsAndJobs() {
val notificationManager = NotificationManagerCompat.from(context)

Expand All @@ -331,6 +336,7 @@ abstract class AbstractFetchDownloadService : Service() {
notificationManager.cancel(state.foregroundServiceId)
state.job?.cancel()
}
unregisterNotificationActionsReceiver()
}

internal fun startDownloadJob(downloadId: Long) {
Expand All @@ -348,7 +354,8 @@ abstract class AbstractFetchDownloadService : Service() {
downloadedFile.delete()
}

private fun registerForUpdates() {
@VisibleForTesting
internal fun registerNotificationActionsReceiver() {
val filter = IntentFilter().apply {
addAction(ACTION_PAUSE)
addAction(ACTION_RESUME)
Expand All @@ -360,6 +367,11 @@ abstract class AbstractFetchDownloadService : Service() {
context.registerReceiver(broadcastReceiver, filter)
}

@VisibleForTesting
internal fun unregisterNotificationActionsReceiver() {
context.unregisterReceiver(broadcastReceiver)
}

@Suppress("ComplexCondition")
internal fun performDownload(download: DownloadState) {
val currentDownloadJobState = downloadJobs[download.id] ?: return
Expand Down
28 changes: 28 additions & 0 deletions ...ds/src/test/java/mozilla/components/feature/downloads/AbstractFetchDownloadServiceTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -692,6 +692,7 @@ class AbstractFetchDownloadServiceTest {
putDownloadExtra(download)
}

service.registerNotificationActionsReceiver()
service.onStartCommand(downloadIntent, 0, 0)
service.downloadJobs.values.forEach { assertTrue(it.job!!.isActive) }

Expand Down Expand Up @@ -730,6 +731,7 @@ class AbstractFetchDownloadServiceTest {
putDownloadExtra(download)
}

service.registerNotificationActionsReceiver()
service.onStartCommand(downloadIntent, 0, 0)
service.downloadJobs.values.forEach { it.job?.join() }

Expand Down Expand Up @@ -759,6 +761,7 @@ class AbstractFetchDownloadServiceTest {
job = CoroutineScope(IO).launch { while (true) { } }
)

service.registerNotificationActionsReceiver()
service.downloadJobs[download.id] = downloadState

val notification = DownloadNotification.createOngoingDownloadNotification(testContext, downloadState)
Expand Down Expand Up @@ -788,15 +791,40 @@ class AbstractFetchDownloadServiceTest {

doReturn(testContext).`when`(service).context

service.registerNotificationActionsReceiver()
service.onTaskRemoved(null)

service.registerNotificationActionsReceiver()
verify(service).clearAllDownloadsNotificationsAndJobs()

service.onDestroy()

verify(service, times(2)).clearAllDownloadsNotificationsAndJobs()
}

@Test
fun `register and unregister notification actions receiver`() = runBlocking {
val service = spy(object : AbstractFetchDownloadService() {
override val httpClient = client
})

doReturn(testContext).`when`(service).context

service.onCreate()

verify(service).registerNotificationActionsReceiver()

service.onTaskRemoved(null)

verify(service).unregisterNotificationActionsReceiver()

service.registerNotificationActionsReceiver()

service.onDestroy()

verify(service, times(2)).unregisterNotificationActionsReceiver()
}

@Test
fun `cancelled download does not prevent other notifications`() = runBlocking {
val cancelledDownload = DownloadState("https://example.com/file.txt", "file.txt")
Expand Down
19 changes: 8 additions & 11 deletions ...ature/readerview/src/main/java/mozilla/components/feature/readerview/ReaderViewFeature.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import kotlinx.coroutines.cancel
import kotlinx.coroutines.flow.collect
import kotlinx.coroutines.flow.mapNotNull
import mozilla.components.browser.state.action.ReaderAction
import mozilla.components.browser.state.selector.findTab
import mozilla.components.browser.state.selector.selectedTab
import mozilla.components.browser.state.state.TabSessionState
import mozilla.components.browser.state.store.BrowserStore
Expand Down Expand Up @@ -120,7 +119,7 @@ class ReaderViewFeature(
connectReaderViewContentScript(tab)
}
if (tab.readerState.checkRequired) {
checkReaderable(tab)
checkReaderState(tab)
}

if (tab.id == store.state.selectedTabId) {
Expand Down Expand Up @@ -196,10 +195,10 @@ class ReaderViewFeature(
}

@VisibleForTesting
internal fun checkReaderable(session: TabSessionState? = store.state.selectedTab) {
internal fun checkReaderState(session: TabSessionState? = store.state.selectedTab) {
session?.engineState?.engineSession?.let { engineSession ->
if (extensionController.portConnected(engineSession)) {
extensionController.sendContentMessage(createCheckReaderableMessage(), engineSession)
extensionController.sendContentMessage(createCheckReaderStateMessage(), engineSession)
}
store.dispatch(ReaderAction.UpdateReaderableCheckRequiredAction(session.id, false))
}
Expand Down Expand Up @@ -250,12 +249,7 @@ class ReaderViewFeature(
private val config: WeakReference<Config>
) : MessageHandler {
override fun onPortConnected(port: Port) {
val config = config.get() ?: return

port.postMessage(createCheckReaderableMessage())
if (store.state.findTab(sessionId)?.readerState?.active == true) {
port.postMessage(createShowReaderMessage(config))
}
port.postMessage(createCheckReaderStateMessage())
}

override fun onPortMessage(message: Any, port: Port) {
Expand All @@ -267,6 +261,9 @@ class ReaderViewFeature(
store.dispatch(ReaderAction.UpdateReaderableAction(sessionId, readerable))

if (message.has(ACTIVE_URL_RESPONSE_MESSAGE_KEY)) {
config.get()?.let { config ->
port.postMessage(createShowReaderMessage(config))
}
val activeUrl = message.getString(ACTIVE_URL_RESPONSE_MESSAGE_KEY)
store.dispatch(ReaderAction.UpdateReaderActiveUrlAction(sessionId, activeUrl))
}
Expand Down Expand Up @@ -304,7 +301,7 @@ class ReaderViewFeature(
internal const val FONT_SIZE_KEY = "mozac-readerview-fontsize"
internal const val FONT_SIZE_DEFAULT = 3

private fun createCheckReaderableMessage(): JSONObject {
private fun createCheckReaderStateMessage(): JSONObject {
return JSONObject().put(ACTION_MESSAGE_KEY, ACTION_CHECK_READER_STATE)
}

Expand Down
60 changes: 35 additions & 25 deletions ...e/readerview/src/test/java/mozilla/components/feature/readerview/ReaderViewFeatureTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ class ReaderViewFeatureTest {

testDispatcher.advanceUntilIdle()
val tabCaptor = argumentCaptor<TabSessionState>()
verify(readerViewFeature).checkReaderable(tabCaptor.capture())
verify(readerViewFeature).checkReaderState(tabCaptor.capture())
assertEquals(tab.id, tabCaptor.value.id)
}

Expand Down Expand Up @@ -337,12 +337,12 @@ class ReaderViewFeatureTest {
}

@Test
fun `readerable check sends message to web extension`() {
fun `reader state check sends message to web extension`() {
val port = mock<Port>()
val message = argumentCaptor<JSONObject>()
val readerViewFeature = prepareFeatureForTest(port)

readerViewFeature.checkReaderable()
readerViewFeature.checkReaderState()
verify(port, times(1)).postMessage(message.capture())
assertEquals(ReaderViewFeature.ACTION_CHECK_READER_STATE, message.value[ReaderViewFeature.ACTION_MESSAGE_KEY])
}
Expand Down Expand Up @@ -437,37 +437,49 @@ class ReaderViewFeatureTest {
}

@Test
fun `reader mode is activated when port connects`() {
fun `state is updated when reader state arrives`() {
val engine: Engine = mock()
val view: ReaderViewControlsView = mock()
val engineSession: EngineSession = mock()
val ext: WebExtension = mock()
val controller: WebExtensionController = mock()

val tab = createTab("https://www.mozilla.org", id = "test-tab")
val store = BrowserStore(BrowserState(tabs = listOf(tab)))
val store = spy(BrowserStore(initialState = BrowserState(tabs = listOf(tab))))
store.dispatch(EngineAction.LinkEngineSessionAction(tab.id, engineSession)).joinBlocking()
store.dispatch(TabListAction.SelectTabAction(tab.id)).joinBlocking()
store.dispatch(ReaderAction.UpdateReaderActiveAction(tab.id, true)).joinBlocking()

val ext: WebExtension = mock()
val messageHandler = argumentCaptor<MessageHandler>()
val message = argumentCaptor<JSONObject>()

WebExtensionController.installedExtensions[ReaderViewFeature.READER_VIEW_EXTENSION_ID] = ext

val readerViewFeature = spy(ReaderViewFeature(testContext, engine, store, mock()))
readerViewFeature.start()
verify(ext).registerContentMessageHandler(eq(engineSession), eq(ReaderViewFeature.READER_VIEW_EXTENSION_ID), messageHandler.capture())

val port: Port = mock()
whenever(port.engineSession).thenReturn(engineSession)
whenever(ext.getConnectedPort(any(), any())).thenReturn(port)

whenever(controller.portConnected(any(), any())).thenReturn(true)
val readerViewFeature = spy(ReaderViewFeature(testContext, engine, store, view))
readerViewFeature.extensionController = controller

val messageHandler = argumentCaptor<MessageHandler>()
val message = argumentCaptor<JSONObject>()
readerViewFeature.start()
verify(controller).registerContentMessageHandler(eq(engineSession), messageHandler.capture(), any())

messageHandler.value.onPortConnected(port)
verify(port, times(2)).postMessage(message.capture())
assertEquals(ReaderViewFeature.ACTION_CHECK_READER_STATE, message.allValues[0][ReaderViewFeature.ACTION_MESSAGE_KEY])
assertEquals(ReaderViewFeature.ACTION_SHOW, message.allValues[1][ReaderViewFeature.ACTION_MESSAGE_KEY])
verify(port).postMessage(message.capture())
assertEquals(ReaderViewFeature.ACTION_CHECK_READER_STATE, message.value[ReaderViewFeature.ACTION_MESSAGE_KEY])

val readerStateMessage = JSONObject()
.put("readerable", true)
.put("baseUrl", "moz-extension://")
.put("activeUrl", "http://mozilla.org/article")
messageHandler.value.onPortMessage(readerStateMessage, port)
verify(store).dispatch(ReaderAction.UpdateReaderableAction(tab.id, true))
verify(store).dispatch(ReaderAction.UpdateReaderBaseUrlAction(tab.id, "moz-extension://"))
verify(store).dispatch(ReaderAction.UpdateReaderActiveUrlAction(tab.id, "http://mozilla.org/article"))
}

@Test
fun `state is updated when reader state arrives`() {
fun `reader is shown when state arrives from reader page`() {
val engine: Engine = mock()
val view: ReaderViewControlsView = mock()
val engineSession: EngineSession = mock()
Expand Down Expand Up @@ -495,17 +507,15 @@ class ReaderViewFeatureTest {
verify(controller).registerContentMessageHandler(eq(engineSession), messageHandler.capture(), any())

messageHandler.value.onPortConnected(port)
verify(port).postMessage(message.capture())
assertEquals(ReaderViewFeature.ACTION_CHECK_READER_STATE, message.value[ReaderViewFeature.ACTION_MESSAGE_KEY])

val readerableMessage = JSONObject()
val readerStateMessage = JSONObject()
.put("readerable", true)
.put("baseUrl", "moz-extension://")
.put("activeUrl", "http://mozilla.org/article")
messageHandler.value.onPortMessage(readerableMessage, port)
verify(store).dispatch(ReaderAction.UpdateReaderableAction(tab.id, true))
verify(store).dispatch(ReaderAction.UpdateReaderBaseUrlAction(tab.id, "moz-extension://"))
verify(store).dispatch(ReaderAction.UpdateReaderActiveUrlAction(tab.id, "http://mozilla.org/article"))
messageHandler.value.onPortMessage(readerStateMessage, port)
verify(port, times(2)).postMessage(message.capture())
assertEquals(ReaderViewFeature.ACTION_CHECK_READER_STATE, message.allValues[0][ReaderViewFeature.ACTION_MESSAGE_KEY])
assertEquals(ReaderViewFeature.ACTION_SHOW, message.allValues[1][ReaderViewFeature.ACTION_MESSAGE_KEY])
}

private fun prepareFeatureForTest(
Expand Down
3 changes: 3 additions & 0 deletions docs/changelog.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ permalink: /changelog/
* [Gecko](https://github.com/mozilla-mobile/android-components/blob/master/buildSrc/src/main/java/Gecko.kt)
* [Configuration](https://github.com/mozilla-mobile/android-components/blob/master/buildSrc/src/main/java/Config.kt)

* **feature-downloads**
* Fixed issue [#6881](https://github.com/mozilla-mobile/android-components/issues/6881).

* **feature-addons**
* Added optional `addonAllowPrivateBrowsingLabelDrawableRes` DrawableRes parameter to `AddonPermissionsAdapter.Style` constructor to allow the clients to add their own drawable. This is used to clearly label the WebExtensions that run in private browsing.

Expand Down

0 comments on commit 246fc69

Please sign in to comment.