Closed Bug 957062 Opened 6 years ago Closed 6 years ago

notify user and installing web page on synthetic APK download failure

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: myk, Assigned: mhaigh)

References

Details

Attachments

(1 file, 1 obsolete file)

When a synthetic APK download fails, WebappManager should notify the user via a toast notification and also dispatch an error event back to the web page that initiated the app install (if any).
Priority: -- → P1
Assignee: nobody → mhaigh
Propagate error back to web page.  The web page already includes a notification of the error, albeit an HTML notification, not an Android toast but as this functionality is already written I think this is the correct way to go.
Attachment #8364345 - Flags: feedback?(myk)
Comment on attachment 8364345 [details] [diff] [review]
Pass APK download error back to webpage

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

Looks great!  Just two nits, so feedback+, and over to wesj for review.

::: mobile/android/modules/WebappManager.jsm
@@ +28,4 @@
>  this.WebappManager = {
>    __proto__: DOMRequestIpcHelper.prototype,
>  
> +  downloadApk: function(aSubject, aMsg) {

Nit: aSubject is a reasonable name for the aSubject parameter to the nsIObserver.observe method that calls this function, but this function's first parameter isn't a generic "subject", it's a very specific "message manager", so we should give it a specific name, like aMessageManager.

@@ +63,4 @@
>          sendMessageToJava({
>            type: "WebApps:InstallApk",
>            filePath: file.path,
> +          data: JSON.stringify(aMsg)

Nit: I would prefer to keep trailing commas in object literals, since it means we don't have to change an existing line (obscuring its history in the changelog) when adding a new name/value pair.
Attachment #8364345 - Flags: review?(wjohnston)
Attachment #8364345 - Flags: feedback?(myk)
Attachment #8364345 - Flags: feedback+
Note that there's a bit of overlap between the patch here and the one in bug 962858.  Whichever one lands first, the other one will need to be rebased.  But the conflict is obvious and trivial to reconcile.
Fixing nits
Attachment #8364345 - Attachment is obsolete: true
Attachment #8364345 - Flags: review?(wjohnston)
Attachment #8365106 - Flags: feedback?(wjohnston)
Comment on attachment 8365106 [details] [diff] [review]
Pass APK download error back to requesting webpage

This one is ready for review.  Let's get a review rather than just feedback.
Attachment #8365106 - Flags: feedback?(wjohnston) → review?(wjohnston)
Comment on attachment 8365106 [details] [diff] [review]
Pass APK download error back to requesting webpage

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

Nice.

::: mobile/android/chrome/content/browser.js
@@ +1584,4 @@
>  
>  #ifdef MOZ_ANDROID_SYNTHAPKS
>        case "webapps-download-apk":
> +        WebappManager.downloadApk(aSubject, JSON.parse(aData));

I'm kinda surprised you don't need to QI here, but I guess no one cares what this object is...?
Attachment #8365106 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #6)
> > +        WebappManager.downloadApk(aSubject, JSON.parse(aData));
> 
> I'm kinda surprised you don't need to QI here, but I guess no one cares what
> this object is...?

We care, since WebappManager has to call its sendAsyncMessage method if the download fails.  But presumably it has already been QIed to the necessary interface (nsIMessageSender?), and passing it to the nsISupports aSubject parameter of nsIObserverService.notifyObservers doesn't strip that interface from it.
https://hg.mozilla.org/mozilla-central/rev/14de085ae57f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.