Merge #6945

6945: Fixes #5725 - Comment on Gecko manifest processing r=rocketsroger a=NotWoods
Co-authored-by: Tiger Oakes <contact@tigeroakes.com>

components/concept/engine/src/main/java/mozilla/components/concept/engine/manifest/WebAppManifestParser.kt

@@ -44,6 +44,16 @@ class WebAppManifestParser {
     /**
      * Parses the provided JSON and returns a [WebAppManifest] (wrapped in [Result.Success] if parsing was successful.
      * Otherwise [Result.Failure].
+     *
+     * Gecko performs some initial parsing on the Web App Manifest, so the [JSONObject] we work with
+     * does not match what was originally provided by the website. Gecko:
+     * - Changes relative URLs to be absolute
+     * - Changes some space-separated strings into arrays (purpose, sizes)
+     * - Changes colors to follow Android format (#AARRGGBB)
+     * - Removes invalid enum values (ie display: halfscreen)
+     * - Ensures display, dir, start_url, and scope always have a value
+     * - Trims most strings (name, short_name, ...)
+     * See https://searchfox.org/mozilla-central/source/dom/manifest/ManifestProcessor.jsm
      */
     fun parse(json: JSONObject): Result {
         return try {

components/concept/engine/src/test/resources/manifests/minimal.json

 {
   "name": "Minimal",
   "start_url": "/"
-}
\ No newline at end of file
+}

components/feature/pwa/src/test/java/mozilla/components/feature/pwa/ManifestStorageTest.kt

@@ -30,6 +30,13 @@ import org.mockito.Mockito.verify
 @ExperimentalCoroutinesApi
 @RunWith(AndroidJUnit4::class)
 class ManifestStorageTest {
+
+    private val firefoxManifest = WebAppManifest(
+        name = "Firefox",
+        startUrl = "https://firefox.com",
+        scope = "/"
+    )
+
     @Test
     fun `load returns null if entry does not exist`() = runBlocking {
         val storage = spy(ManifestStorage(testContext))
@@ -53,9 +60,8 @@ class ManifestStorageTest {
     fun `save saves the manifest as JSON`() = runBlocking {
         val storage = spy(ManifestStorage(testContext))
         val dao = mockDatabase(storage)
-        val manifest = WebAppManifest(name = "Firefox", startUrl = "https://firefox.com")
 
-        storage.saveManifest(manifest)
+        storage.saveManifest(firefoxManifest)
         verify(dao).insertManifest(any())
         Unit
     }
@@ -64,16 +70,15 @@ class ManifestStorageTest {
     fun `update replaces the manifest as JSON`() = runBlocking {
         val storage = spy(ManifestStorage(testContext))
         val dao = mockDatabase(storage)
-        val manifest = WebAppManifest(name = "Firefox", startUrl = "https://firefox.com")
         val existing = ManifestEntity(
-            manifest = manifest,
+            manifest = firefoxManifest,
             createdAt = 0,
             updatedAt = 0
         )
 
         `when`(dao.getManifest("https://firefox.com")).thenReturn(existing)
 
-        storage.updateManifest(manifest)
+        storage.updateManifest(firefoxManifest)
         verify(dao).updateManifest(any())
         Unit
     }
@@ -82,11 +87,10 @@ class ManifestStorageTest {
     fun `update does not replace non-existed manifest`() = runBlocking {
         val storage = spy(ManifestStorage(testContext))
         val dao = mockDatabase(storage)
-        val manifest = WebAppManifest(name = "Firefox", startUrl = "https://firefox.com")
 
         `when`(dao.getManifest("https://firefox.com")).thenReturn(null)
 
-        storage.updateManifest(manifest)
+        storage.updateManifest(firefoxManifest)
         verify(dao, never()).updateManifest(any())
         Unit
     }

components/feature/pwa/src/test/java/mozilla/components/feature/pwa/WebAppLauncherActivityTest.kt

@@ -22,16 +22,17 @@ import org.mockito.Mockito.verify
 @RunWith(AndroidJUnit4::class)
 class WebAppLauncherActivityTest {
 
+    private val baseManifest = WebAppManifest(
+        name = "Test",
+        startUrl = "https://www.mozilla.org"
+    )
+
     @Test
     fun `DisplayMode-Browser launches browser`() {
         val activity = spy(WebAppLauncherActivity())
         doNothing().`when`(activity).launchBrowser(any())
 
-        val manifest = WebAppManifest(
-            name = "Test",
-            startUrl = "https://www.mozilla.org",
-            display = WebAppManifest.DisplayMode.BROWSER
-        )
+        val manifest = baseManifest.copy(display = WebAppManifest.DisplayMode.BROWSER)
 
         activity.routeManifest(manifest.startUrl.toUri(), manifest)
 
@@ -43,11 +44,7 @@ class WebAppLauncherActivityTest {
         val activity = spy(WebAppLauncherActivity())
         doNothing().`when`(activity).launchWebAppShell("https://www.mozilla.org".toUri())
 
-        val manifest = WebAppManifest(
-            name = "Test",
-            startUrl = "https://www.mozilla.org",
-            display = WebAppManifest.DisplayMode.MINIMAL_UI
-        )
+        val manifest = baseManifest.copy(display = WebAppManifest.DisplayMode.MINIMAL_UI)
 
         activity.routeManifest(manifest.startUrl.toUri(), manifest)
 
@@ -59,11 +56,7 @@ class WebAppLauncherActivityTest {
         val activity = spy(WebAppLauncherActivity())
         doNothing().`when`(activity).launchWebAppShell("https://www.mozilla.org".toUri())
 
-        val manifest = WebAppManifest(
-            name = "Test",
-            startUrl = "https://www.mozilla.org",
-            display = WebAppManifest.DisplayMode.FULLSCREEN
-        )
+        val manifest = baseManifest.copy(display = WebAppManifest.DisplayMode.FULLSCREEN)
 
         activity.routeManifest(manifest.startUrl.toUri(), manifest)
 
@@ -75,11 +68,7 @@ class WebAppLauncherActivityTest {
         val activity = spy(WebAppLauncherActivity())
         doNothing().`when`(activity).launchWebAppShell("https://www.mozilla.org".toUri())
 
-        val manifest = WebAppManifest(
-            name = "Test",
-            startUrl = "https://www.mozilla.org",
-            display = WebAppManifest.DisplayMode.STANDALONE
-        )
+        val manifest = baseManifest.copy(display = WebAppManifest.DisplayMode.STANDALONE)
 
         activity.routeManifest(manifest.startUrl.toUri(), manifest)
 
@@ -92,11 +81,7 @@ class WebAppLauncherActivityTest {
         doReturn("test").`when`(activity).packageName
         doNothing().`when`(activity).startActivity(any())
 
-        val manifest = WebAppManifest(
-            name = "Test",
-            startUrl = "https://www.mozilla.org",
-            display = WebAppManifest.DisplayMode.BROWSER
-        )
+        val manifest = baseManifest.copy(display = WebAppManifest.DisplayMode.BROWSER)
 
         activity.launchBrowser(manifest.startUrl.toUri())
 

components/feature/pwa/src/test/java/mozilla/components/feature/pwa/WebAppShortcutManagerTest.kt

@@ -53,6 +53,10 @@ class WebAppShortcutManagerTest {
     @Mock private lateinit var storage: ManifestStorage
     @Mock private lateinit var icons: BrowserIcons
     private lateinit var manager: WebAppShortcutManager
+    private val baseManifest = WebAppManifest(
+        name = "Demo",
+        startUrl = "https://example.com"
+    )
 
     @Before
     fun setup() {
@@ -73,9 +77,7 @@ class WebAppShortcutManagerTest {
 
     @Test
     fun `requestPinShortcut no-op if pinning unsupported`() = runBlockingTest {
-        val manifest = WebAppManifest(
-            name = "Demo",
-            startUrl = "https://example.com",
+        val manifest = baseManifest.copy(
             display = WebAppManifest.DisplayMode.STANDALONE,
             icons = listOf(WebAppManifest.Icon(
                 src = "https://example.com/icon.png",
@@ -99,9 +101,7 @@ class WebAppShortcutManagerTest {
     @Test
     fun `requestPinShortcut won't make a PWA icon if the session is not installable`() = runBlockingTest {
         setSdkInt(Build.VERSION_CODES.O)
-        val manifest = WebAppManifest(
-            name = "Demo",
-            startUrl = "https://example.com",
+        val manifest = baseManifest.copy(
             display = WebAppManifest.DisplayMode.STANDALONE,
             icons = emptyList() // no icons
         )
@@ -118,9 +118,7 @@ class WebAppShortcutManagerTest {
     @Test
     fun `requestPinShortcut pins PWA shortcut`() = runBlockingTest {
         setSdkInt(Build.VERSION_CODES.O)
-        val manifest = WebAppManifest(
-            name = "Demo",
-            startUrl = "https://example.com",
+        val manifest = baseManifest.copy(
             display = WebAppManifest.DisplayMode.STANDALONE,
             icons = listOf(WebAppManifest.Icon(
                 src = "https://example.com/icon.png",
@@ -202,7 +200,7 @@ class WebAppShortcutManagerTest {
 
     @Test
     fun `updateShortcuts no-op`() = runBlockingTest {
-        val manifests = listOf(WebAppManifest(name = "Demo", startUrl = "https://example.com"))
+        val manifests = listOf(baseManifest)
         doReturn(null).`when`(manager).buildWebAppShortcut(context, manifests[0])
 
         manager.updateShortcuts(context, manifests)
@@ -217,7 +215,7 @@ class WebAppShortcutManagerTest {
     @Test
     fun `updateShortcuts updates list of existing shortcuts`() = runBlockingTest {
         setSdkInt(Build.VERSION_CODES.N_MR1)
-        val manifests = listOf(WebAppManifest(name = "Demo", startUrl = "https://example.com"))
+        val manifests = listOf(baseManifest)
         val shortcutCompat: ShortcutInfoCompat = mock()
         val shortcut: ShortcutInfo = mock()
         doReturn(shortcutCompat).`when`(manager).buildWebAppShortcut(context, manifests[0])
@@ -229,13 +227,12 @@ class WebAppShortcutManagerTest {
 
     @Test
     fun `buildWebAppShortcut builds shortcut and saves manifest`() = runBlockingTest {
-        val manifest = WebAppManifest(name = "Demo", startUrl = "https://example.com")
-        doReturn(mock<IconCompat>()).`when`(manager).buildIconFromManifest(manifest)
+        doReturn(mock<IconCompat>()).`when`(manager).buildIconFromManifest(baseManifest)
 
-        val shortcut = manager.buildWebAppShortcut(context, manifest)!!
+        val shortcut = manager.buildWebAppShortcut(context, baseManifest)!!
         val intent = shortcut.intent
 
-        verify(storage).saveManifest(manifest)
+        verify(storage).saveManifest(baseManifest)
 
         assertEquals("https://example.com", shortcut.id)
         assertEquals("Demo", shortcut.longLabel)

components/feature/pwa/src/test/java/mozilla/components/feature/pwa/ext/ActivityKtTest.kt

@@ -13,17 +13,16 @@ import org.mockito.Mockito.anyInt
 import org.mockito.Mockito.verify
 
 class ActivityKtTest {
+    private val baseManifest = WebAppManifest(
+        name = "Test Manifest",
+        startUrl = "/"
+    )
+
     @Test
     fun `applyOrientation calls setRequestedOrientation for every value`() {
         WebAppManifest.Orientation.values().forEach { orientation ->
             val activity: Activity = mock()
-            activity.applyOrientation(
-                WebAppManifest(
-                    name = "Test Manifest",
-                    startUrl = "/",
-                    orientation = orientation
-                )
-            )
+            activity.applyOrientation(baseManifest.copy(orientation = orientation))
             verify(activity).requestedOrientation = anyInt()
         }
     }
@@ -32,28 +31,19 @@ class ActivityKtTest {
     fun `applyOrientation applies common orientations`() {
         run {
             val activity: Activity = mock()
-            activity.applyOrientation(WebAppManifest(
-                name = "Test Manifest",
-                startUrl = "/",
-                orientation = WebAppManifest.Orientation.ANY))
+            activity.applyOrientation(baseManifest.copy(orientation = WebAppManifest.Orientation.ANY))
             verify(activity).requestedOrientation = ActivityInfo.SCREEN_ORIENTATION_USER
         }
 
         run {
             val activity: Activity = mock()
-            activity.applyOrientation(WebAppManifest(
-                name = "Test Manifest",
-                startUrl = "/",
-                orientation = WebAppManifest.Orientation.PORTRAIT))
+            activity.applyOrientation(baseManifest.copy(orientation = WebAppManifest.Orientation.PORTRAIT))
             verify(activity).requestedOrientation = ActivityInfo.SCREEN_ORIENTATION_USER_PORTRAIT
         }
 
         run {
             val activity: Activity = mock()
-            activity.applyOrientation(WebAppManifest(
-                name = "Test Manifest",
-                startUrl = "/",
-                orientation = WebAppManifest.Orientation.LANDSCAPE))
+            activity.applyOrientation(baseManifest.copy(orientation = WebAppManifest.Orientation.LANDSCAPE))
             verify(activity).requestedOrientation = ActivityInfo.SCREEN_ORIENTATION_USER_LANDSCAPE
         }
 

components/feature/pwa/src/test/java/mozilla/components/feature/pwa/feature/ManifestUpdateFeatureTest.kt

@@ -34,6 +34,11 @@ class ManifestUpdateFeatureTest {
     @Mock private lateinit var shortcutManager: WebAppShortcutManager
     @Mock private lateinit var storage: ManifestStorage
     private val sessionId = "external-app-session-id"
+    private val baseManifest = WebAppManifest(
+        name = "Mozilla",
+        startUrl = "https://mozilla.org",
+        scope = "https://mozilla.org"
+    )
 
     @Before
     fun setup() {
@@ -43,8 +48,7 @@ class ManifestUpdateFeatureTest {
 
     @Test
     fun `start and stop controls session observer`() {
-        val manifest = WebAppManifest(name = "Mozilla", startUrl = "https://mozilla.org")
-        val feature = ManifestUpdateFeature(testContext, sessionManager, shortcutManager, storage, sessionId, manifest)
+        val feature = ManifestUpdateFeature(testContext, sessionManager, shortcutManager, storage, sessionId, baseManifest)
 
         feature.start()
         verify(session).register(feature)
@@ -55,9 +59,8 @@ class ManifestUpdateFeatureTest {
 
     @Test
     fun `start and stop handle null session`() {
-        val manifest = WebAppManifest(name = "Mozilla", startUrl = "https://mozilla.org")
         `when`(sessionManager.findSessionById(sessionId)).thenReturn(null)
-        val feature = ManifestUpdateFeature(testContext, sessionManager, shortcutManager, storage, sessionId, manifest)
+        val feature = ManifestUpdateFeature(testContext, sessionManager, shortcutManager, storage, sessionId, baseManifest)
 
         feature.start()
         verify(session, never()).register(feature)
@@ -68,12 +71,11 @@ class ManifestUpdateFeatureTest {
 
     @Test
     fun `updateStoredManifest is called when the manifest changes`() = runBlockingTest {
-        val initialManifest = WebAppManifest(name = "Mozilla", startUrl = "https://mozilla.org")
-        val feature = spy(ManifestUpdateFeature(testContext, sessionManager, shortcutManager, storage, sessionId, initialManifest))
+        val feature = spy(ManifestUpdateFeature(testContext, sessionManager, shortcutManager, storage, sessionId, baseManifest))
         doReturn(Unit).`when`(feature).updateStoredManifest(any())
         feature.start()
 
-        val manifest = WebAppManifest(name = "Mozilla", startUrl = "https://mozilla.org", shortName = "Moz")
+        val manifest = baseManifest.copy(shortName = "Moz")
         feature.onWebAppManifestChanged(session, manifest)
 
         verify(feature).updateStoredManifest(manifest)
@@ -81,19 +83,17 @@ class ManifestUpdateFeatureTest {
 
     @Test
     fun `updateStoredManifest is not called when the manifest is the same`() = runBlockingTest {
-        val initialManifest = WebAppManifest(name = "Mozilla", startUrl = "https://mozilla.org")
-        val feature = spy(ManifestUpdateFeature(testContext, sessionManager, shortcutManager, storage, sessionId, initialManifest))
+        val feature = spy(ManifestUpdateFeature(testContext, sessionManager, shortcutManager, storage, sessionId, baseManifest))
         feature.start()
 
-        feature.onWebAppManifestChanged(session, initialManifest)
+        feature.onWebAppManifestChanged(session, baseManifest)
 
-        verify(feature, never()).updateStoredManifest(initialManifest)
+        verify(feature, never()).updateStoredManifest(baseManifest)
     }
 
     @Test
     fun `updateStoredManifest is not called when the manifest is null`() = runBlockingTest {
-        val initialManifest = WebAppManifest(name = "Mozilla", startUrl = "https://mozilla.org")
-        val feature = spy(ManifestUpdateFeature(testContext, sessionManager, shortcutManager, storage, sessionId, initialManifest))
+        val feature = spy(ManifestUpdateFeature(testContext, sessionManager, shortcutManager, storage, sessionId, baseManifest))
         feature.start()
 
         feature.onWebAppManifestChanged(session, null)
@@ -103,8 +103,7 @@ class ManifestUpdateFeatureTest {
 
     @Test
     fun `updateStoredManifest is not called when the manifest has a different start URL`() = runBlockingTest {
-        val initialManifest = WebAppManifest(name = "Mozilla", startUrl = "https://mozilla.org")
-        val feature = spy(ManifestUpdateFeature(testContext, sessionManager, shortcutManager, storage, sessionId, initialManifest))
+        val feature = spy(ManifestUpdateFeature(testContext, sessionManager, shortcutManager, storage, sessionId, baseManifest))
         feature.start()
 
         val manifest = WebAppManifest(name = "Mozilla", startUrl = "https://netscape.com")
@@ -115,10 +114,9 @@ class ManifestUpdateFeatureTest {
 
     @Test
     fun `updateStoredManifest updates storage and shortcut`() = runBlockingTest {
-        val initialManifest = WebAppManifest(name = "Mozilla", startUrl = "https://mozilla.org")
-        val feature = ManifestUpdateFeature(testContext, sessionManager, shortcutManager, storage, sessionId, initialManifest)
+        val feature = ManifestUpdateFeature(testContext, sessionManager, shortcutManager, storage, sessionId, baseManifest)
 
-        val manifest = WebAppManifest(name = "Mozilla", startUrl = "https://mozilla.org", shortName = "Moz")
+        val manifest = baseManifest.copy(shortName = "Moz")
         feature.updateStoredManifest(manifest)
 
         verify(storage).updateManifest(manifest)
@@ -127,11 +125,10 @@ class ManifestUpdateFeatureTest {
 
     @Test
     fun `start updates last web app usage`() = runBlockingTest {
-        val initialManifest = WebAppManifest(name = "Mozilla", startUrl = "https://mozilla.org", scope = "https://mozilla.org")
-        val feature = ManifestUpdateFeature(testContext, sessionManager, shortcutManager, storage, sessionId, initialManifest)
+        val feature = ManifestUpdateFeature(testContext, sessionManager, shortcutManager, storage, sessionId, baseManifest)
 
         feature.start()
 
-        verify(storage).updateManifestUsedAt(initialManifest)
+        verify(storage).updateManifestUsedAt(baseManifest)
     }
 }