749618 - Cannot Use Add to Home Screen for a Web App Without an Icon

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect)

Description

[Jason Smith [:jsmith]]

Steps:

1. Install an application with no icon
2. Go to about:apps
3. Hard press the app installed
4. Select to Add to Home Screen

Expected:

The application icon should be added to the home screen.

Actual:

The icon was not added to the home screen.

Comment 1

[Jason Smith [:jsmith]]

Build: Fennec Native Nightly
Device: Samsung Galaxy Nexus

Additional Notes:

Tested with the below manifest, in which the icon reference used in the manifest does not point to a real file on the server:

{
  "version": "1.0",
  "name": "Mozilla QA WebRT Tester",
  "description": "An app for running web runtime test cases with native applications",
  "icons": {
    "128": "/data/webapps/mozqa.com/qalogo.png"
  },
  "developer": {
    "url": "http://mozqa.com",
    "name": "Mozilla QA"
  },
  "installs_allowed_from": ["*"],
  "launch_path": "/data"
}

Comment 2

[Jason Smith [:jsmith]]

Nominating for k9o, as there are definitely some marketplace apps that do not specify an icon in their app manifest. Adding a shortcut to the home screen is a key feature to the android web runtime experience as well, including when no icon is specified.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Jason, do we have a test app I can use to test a patch?

Comment 4

[Jason Smith [:jsmith]]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> Jason, do we have a test app I can use to test a patch?

http://ram3385.testmanifest.com/

By the way, you can use testmanifest.com to generate manifests to test and install.

Link: http://testmanifest.com/

Comment 5

[Jason Smith [:jsmith]]

Definitely a blocker for the 1st release - this is a typical install app scenario.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch

I stole some code from desktop here:

http://mxr.mozilla.org/mozilla-central/source/browser/modules/WebappsIconHelpers.js#18

But maybe we'd be better moving this into toolkit and reusing more desktop code:

http://mxr.mozilla.org/mozilla-central/source/browser/modules/WebappsInstaller.jsm#75

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 635104 [details] [diff] [review]
Patch

This may need a better icon from UX too. I stole the one used for about:addons

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 635104 [details] [diff] [review]
Patch

>diff --git a/mobile/android/chrome/content/aboutApps.js b/mobile/android/chrome/content/aboutApps.js

>+function getBiggestIcon(aIcons) {
>+  if (!aIcons) {
>+    return "chrome://browser/skin/images/default-app-icon.png";
>+  }
>+
>+  let iconSizes = Object.keys(aIcons);
>+  if (iconSizes.length == 0) {
>+    return "chrome://browser/skin/images/default-app-icon.png";
>+  }
>+  iconSizes.sort(function(a, b) a - b);
>+  return aIcons[iconSizes.pop()];
>+}

Don't put this here. Just use the one you put in browser.js

> function onLoad(aEvent) {

>   AppsUI.shortcut = contextmenus.add(gStrings.GetStringFromName("appsContext.shortcut"), contextmenus.SelectorContext("div[mozApp]"),
>     function(aTarget) {
>       let manifest = aTarget.manifest;
>-      gChromeWin.WebappsUI.createShortcut(manifest.name, manifest.fullLaunchPath(), manifest.iconURLForSize("64"), "webapp");
>+      gChromeWin.WebappsUI.createShortcut(manifest.name, manifest.fullLaunchPath(), getBiggestIcon(manifest.icons), "webapp");

You missed a | this. | here but now that we are moving this to browser.js, use: gChromeWin.WebappsUI.getBiggestIcon(...)

>-  img.src = manifest.iconURLForSize("64");
>+  img.src = getBiggestIcon(manifest.icons);

Same

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+  getBiggestIcon: function getBiggestIcon(aIcons) {
>+    if (!aIcons) {
>+      return "chrome://browser/skin/images/default-app-icon.png";
>+    }

No {}

>+    if (iconSizes.length == 0) {
>+      return "chrome://browser/skin/images/default-app-icon.png";
>+    }

No {}

>diff --git a/mobile/android/themes/core/images/default-app-icon.png b/mobile/android/themes/core/images/default-app-icon.png

Just copy this one instead (for now):
http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/webapps-64.png

Comment 9

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21a070f6afcd

Comment 10

[Brian Nicholson (:bnicholson)]

Backed out: http://hg.mozilla.org/integration/mozilla-inbound/rev/a4dc234066d2

This is missing a "," after getBiggestIcon.

Comment 11

[Wesley Johnston (:wesj)]

Tested and landed again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/756beeab80a6

Comment 12

[Wesley Johnston (:wesj)]

I have no idea what's wrong with me this week, but I forgot to qref. THIS adds the missing comma:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f1fa78d43a1

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/756beeab80a6
https://hg.mozilla.org/mozilla-central/rev/8f1fa78d43a1

Comment 14

[Aaron Train [:aaronmt]]

Unable to verify this due to bug 769712.