960584 - notify user of download progress indication when fetching APK

Status: RESOLVED WONTFIX

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

Description

[Aaron Train [:aaronmt]]

When a user initiates a web app install, the user should be notified of progress. In an example below, I sat staring at the screen wondering what was happening for about ~15 seconds on residential WiFi. If possible, we perhaps want to show an Android notification.

01-16 10:57:47.780 E/GeckoConsole(11777): * * WebappManager.jsm: downloadApk for http://hare9231.testmanifest.com/manifest.webapp


01-16 10:58:02.393 I/ActivityManager(  565): START u0 {act=android.intent.action.VIEW dat=file:///storage/emulated/0/Download/httphare9231testmanifestcommanifestwebapp.apk typ=application/vnd.android.package-archive cmp=com.android.packageinstaller/.PackageInstallerActivity} from pid 11777

Comment 1

[Myk Melez [:myk] [@mykmelez]]

The latest patch in bug 934760, which implements the update flow, displays a notification while updates are being downloaded, and we should be able to do the same thing here.

Comment 2

[Martyn Haigh (:mhaigh)]

I do believe this already happens

Comment 3

[Myk Melez [:myk] [@mykmelez]]

(In reply to Martyn Haigh (:mhaigh) from comment #2)
> I do believe this already happens

When Marketplace triggers an installation, it displays a throbber, but this bug is about the runtime itself displaying a download progress indication, i.e. a notification with a progressbar (ideally determinate, but it would be ok to make it indeterminate for now, as we haven't implemented determinacy for the update process either).

Comment 4

[Martyn Haigh (:mhaigh)]

Attachment: Add notification when downloading APK

Comment 5

[Martyn Haigh (:mhaigh)]

This patch adds an Android notification whilst the APK is downloading.  I had a bit of a mind blank whilst trying to work out the notification title & message - any suggestions?

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8434176 [details] [diff] [review]
Add notification when downloading APK

Review of attachment 8434176 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/locales/en-US/chrome/webapp.properties
@@ +8,5 @@
>  noUpdatesTitle=No updates available
>  noUpdatesMessage=There are no updates to your apps
>  
> +downloadingTitle=Downloading webapp
> +downloadingMessage=Firefox is download the webapp

Erm, "is download" is not quite right. ;-)

But also, references to Firefox need to replace a string specifier (%s) with the name of the given build, which is provided by brandShortName or brandFullName from brand.properties.  See the way the xpinstallPromptWarning2 string is formatted for an example.

But also!  We just rotated around the overuse of the word "download" in update notifications over in bug 1011093, so I'm cautious about introducing more uses.

The "retrieving update" notification for an individual update now reads:

  Retrieving 1 update…
  Retrieving update for NAME_OF_APP

While the code the downloads documents that Fennec can't display reads:

  Download started
  NAME_OF_DOCUMENT

So I suggest we make this read:

  Retrieving app…
  NAME_OF_APP

But let's also get feedback from ibarlow.

::: mobile/android/modules/WebappManager.jsm
@@ +150,5 @@
> +    let downloadingNotification = this._notify({
> +      title: Strings.GetStringFromName("downloadingTitle"),
> +      message: Strings.GetStringFromName("downloadingMessage"),
> +      icon: "drawable://alert_download_animation",
> +    });

This should also sport an indeterminate progressbar, like the "checking for updates" and "retrieving updates" notifications:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/WebappManager.jsm?rev=35a50dd7629d#443

Comment 7

[Ian Barlow (:ibarlow)]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #6)

> So I suggest we make this read:
> 
>   Retrieving app…
>   NAME_OF_APP
> 
> But let's also get feedback from ibarlow.

Sorry guys, I totally missed this! Yes, I agree with what Myk said, we should be consistent with the wording we use elsewhere.

Comment 8

[Martyn Haigh (:mhaigh)]

Currently we can't get the app name as the flow of the install process is incorrect, bug 1029691 will fix this.

Comment 9

[Martyn Haigh (:mhaigh)]

Attachment: Add notification when downloading APK

Corrected notification text and added an indeterminate progressbar

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8447417 [details] [diff] [review]
Add notification when downloading APK

Review of attachment 8447417 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/locales/en-US/chrome/webapp.properties
@@ +8,4 @@
>  noUpdatesTitle=No updates available
>  noUpdatesMessage=There are no updates to your apps
>  
> +downloadingTitle=Retrieving app...

Use an ellipsis character (…) instead of three periods.

@@ +8,5 @@
>  noUpdatesTitle=No updates available
>  noUpdatesMessage=There are no updates to your apps
>  
> +downloadingTitle=Retrieving app...
> +downloadingMessage=#1

Nit: call these retrievingTitle and retrievingMessage for consistency with the other messages.

::: mobile/android/modules/WebappManager.jsm
@@ +155,5 @@
>      }
>  
> +    let downloadingNotification = this._notify({
> +      title: Strings.GetStringFromName("downloadingTitle"),
> +      message: Strings.GetStringFromName("downloadingMessage").replace("#1", aApp.updateManifest.name),

Instead of using #1 and String.replace, use %1$S and Strings.formatStringFromName("downloadingMessage", [aApp.updateManifest.name], 1);

Also, if the app is hosted, then the manifest from which to get the name should be aApp.manifest.

@@ +158,5 @@
> +      title: Strings.GetStringFromName("downloadingTitle"),
> +      message: Strings.GetStringFromName("downloadingMessage").replace("#1", aApp.updateManifest.name),
> +      icon: "drawable://alert_download_animation",
> +      // TODO: make this a determinate progress indicator once we can determine
> +      // the sizes of the APKs and observe their progress.

Nit: we should probably reference bug 970210 here and then morph that bug into one that applies to both updates and initial downloads.

Comment 11

[Martyn Haigh (:mhaigh)]

Attachment: Add notification when downloading APK

I think this patch is ready although it's still blocked by, and relies upon the code from bug 1029691, so we shouldn't do anything with this patch until 1029691 is ready and then I'll have to check this patch hasn't rotted before check in.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8450912 [details] [diff] [review]
Add notification when downloading APK

Review of attachment 8450912 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, this looks good, but I just realized that it'll show these notifications during the update process, which it shouldn't do, since the update process already displays a set of notifications (and has logic for hiding them under certain circumstances).

So the notification should move up to _installApk, where we can display it only during the install process.

Comment 13

[Martyn Haigh (:mhaigh)]

Attachment: Add notification when downloading APK

Moved code in to _installApk function

Comment 15

[Myk Melez [:myk] [@mykmelez]]

As Martyn is off working on another project now, I'll take over landing this patch.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Pushed to fx-team with this comment added to the localization file:

# LOCALIZATION NOTE (retrievingMessage): %1$S is the name of the app.

https://hg.mozilla.org/integration/fx-team/rev/5aa9b76448b5

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5aa9b76448b5

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Arr, shiver me timbers, this depends on bug 1029691, which hasn't yet landed, so I've backed this out:

https://hg.mozilla.org/integration/fx-team/rev/0d1299b49f17

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/0d1299b49f17

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Per bug 1235869, we're going to disable the Android web runtime, so we won't fix this bug in it.

(This is part of a bulk resolution of bugs in the Firefox for Android::Web Apps component, from which I attempted to exclude bugs that are not specific to the runtime, but it's possible that I included one accidentally.  If so, I'm sorry, and please reopen the bug!)