Closed Bug 749618 Opened 9 years ago Closed 9 years ago

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

Categories

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

ARM
Android
defect
Not set
normal

Tracking

(blocking-kilimanjaro:+, firefox16 verified, firefox17 verified)

VERIFIED FIXED
Firefox 16
blocking-kilimanjaro +
Tracking Status
firefox16 --- verified
firefox17 --- verified

People

(Reporter: jsmith, Assigned: wesj)

References

Details

Attachments

(1 file)

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.
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"
}
tracking-fennec: --- → ?
Whiteboard: [Phase1]
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.
blocking-kilimanjaro: --- → ?
Blocks: 738546
blocking-kilimanjaro: ? → +
Jason, do we have a test app I can use to test a patch?
(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/
Blocks: 766260
No longer blocks: 738546
Definitely a blocker for the 1st release - this is a typical install app scenario.
Whiteboard: [Phase1]
Attached patch PatchSplinter Review
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
Assignee: nobody → wjohnston
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
Attachment #635104 - Flags: review?(mark.finkle)
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
Attachment #635104 - Flags: review?(mark.finkle) → review+
Whiteboard: [qa+]
Backed out: http://hg.mozilla.org/integration/mozilla-inbound/rev/a4dc234066d2

This is missing a "," after getBiggestIcon.
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
https://hg.mozilla.org/mozilla-central/rev/756beeab80a6
https://hg.mozilla.org/mozilla-central/rev/8f1fa78d43a1
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
No longer blocks: Blocking-FFA-WebRT1+
QA Contact: aaron.train
Unable to verify this due to bug 769712.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+]
tracking-fennec: ? → ---
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.