This might be the expected behaviour (matches desktop) but it does not match fxos. When installing an app through marketplace using `mozApps.install` cancelling the Android install dialog does not trigger the registered `onerror` callback. Hitting cancel on fxos does trigger the callback however.

I think mhaigh is working on a variant of this bug, if not a duplicate.

This patch needs the patch from 957967 applied first

Comment on attachment 8392541 [details] [diff] [review] Propigate APK install cancellation back to webpage Review of attachment 8392541 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/webapp/EventListener.java @@ +189,4 @@ > }); > } > > + public static void installApk(final Activity context, final JSONObject message) { Nice optimization! @@ +253,5 @@ > receiver.cleanup(); > + JSONObject dataObj = null; > + try { > + dataObj = new JSONObject(message.getString("data")); > + dataObj.put("error", "user cancelled apk install"); Nits: cancelled => canceled, apk => APK @@ +255,5 @@ > + try { > + dataObj = new JSONObject(message.getString("data")); > + dataObj.put("error", "user cancelled apk install"); > + } catch (Exception ex) { > + Log.e(LOGTAG, "Exception handling message \"installApk\":", ex); Nit: make this a more specific message, like "error setting error string". @@ +259,5 @@ > + Log.e(LOGTAG, "Exception handling message \"installApk\":", ex); > + return; > + } > + GeckoAppShell.notifyGeckoOfEvent(GeckoEvent.createBroadcastEvent( > + "Webapps:InstallError", dataObj.toString())); Nit: put this statement all on one line.

Revised patch relies on patch from bug 957067 to be applied first

Comment on attachment 8403362 [details] [diff] [review] Propigate APK install cancellation back to webpage Review of attachment 8403362 [details] [diff] [review]: ----------------------------------------------------------------- Overall, this looks great, modulo a few issues noted below. But jhugman pointed me at Components.utils.getWeakReference <https://developer.mozilla.org/en-US/docs/Components.utils.getWeakReference>, and it looks like it'd actually make this implementation much easier. Instead of having two maps, we can have a single map using a simple object literal: messageMap: {}, Store weak references to the message managers via string literal keys: this.messageMap[aMessage.requestID] = Cu.getWeakReference(aMessageManager); And retrieve them via the same keys: let messageManager = this.messageMap[aMessage.requestID].get(); So I think we should employ this method, which jhugman is planning to use over in bug 986085. ::: mobile/android/base/webapp/EventListener.java @@ +279,4 @@ > // unregistered the receiver). > Log.e(LOGTAG, "error unregistering install receiver: ", e); > } > + GeckoAppShell.notifyGeckoOfEvent(GeckoEvent.createBroadcastEvent("Webapps:InstallError", messageData.toString())); Since you changed the name of the event in browser.js, you'd need to change it here as well; except that you should change it back in browser.js, per my comments below! ::: mobile/android/chrome/content/browser.js @@ +1582,5 @@ > WebappManager.install(JSON.parse(aData), aSubject); > break; > > + case "Webapps:onInstallError": > + WebappManager.installError(JSON.parse(aData)); Erm, I actually meant that the name of the WebappManager method should be "onInstallError", as that's a conventional name for event handlers when their names aren't verb-initial (like all the other method names in WebappManager), i.e.: case "Webapps:InstallError": WebappManager.onInstallError(JSON.parse(aData)); However, the more I think about it, the more I suspect that aligning the method name to the event name (which has been the convention to date for all the verb-initial event/method names in WebappManager) is better, so let's leave it the way you had it initially: case "Webapps:InstallError": WebappManager.installError(JSON.parse(aData)); And sorry about the churn! ::: mobile/android/modules/WebappManager.jsm @@ +43,5 @@ > this.WebappManager = { > __proto__: DOMRequestIpcHelper.prototype, > > + //Used to store keys and references to message managers in case of error in > + //the Java which needs to be routed back to the calling web page. Nit: add a space between the comment delimiters and their comments. @@ +203,5 @@ > }, > > autoInstall: function(aData) { > + let key = this._messageKeyMap[aData.requestID]; > + if(!!this._messageMap && key && this._messageMap.has(key)) { Nit: add a space between the "if" keyword and the opening parenthesis. @@ +282,5 @@ > }).bind(this)); }, > > + installError: function(aMessage) { > + let key = this._messageKeyMap[aMessage.requestID]; > + if (!!this._messageMap && !!key && this._messageMap.has(key)) { this._messageMap should always exist, since we define it unconditionally and never delete it, so we don't need to check it here. @@ +284,5 @@ > + installError: function(aMessage) { > + let key = this._messageKeyMap[aMessage.requestID]; > + if (!!this._messageMap && !!key && this._messageMap.has(key)) { > + let messageManager = this._messageMap.get(key); > + if(messageManager) { Nit: add a space between the "if" keyword and the opening parenthesis.

Implemented getWeakReference and updated according to feedback.

Comment on attachment 8410290 [details] [diff] [review] Propigate APK install cancellation back to webpage Review of attachment 8410290 [details] [diff] [review]: ----------------------------------------------------------------- Looking good, just minor issues. Note that the fix in bug 957067 is back in the patch, and a bunch of my comments just repeat stuff I said about that fix. If you'd like, we can work on both fixes together in this bug. That way you don't have to keep separating them out. ::: mobile/android/base/webapp/EventListener.java @@ +200,5 @@ > > + public static void installApk(final Activity context, JSONObject message) { > + String filePath = null; > + final JSONObject messageData; > + JSONObject returnObject = new JSONObject(); Nit: I would avoid using "object" in the names of variables and simply name this after its purpose; and since we only need to use it when sending an error, I would call it either "error" or "response" (which is the name of the EventDispatcher.sendError parameter to which we pass it). @@ +211,5 @@ > + > + try { > + returnObject.put("error", "Error getting file path and data"); > + } catch (Exception ex) { > + Log.e(LOGTAG, "Exception handling message \"installApk\":", ex); Nit: I would actually reverse the severity of these two log calls, i.e. make "Exception handling message \"installApk\" be Log.wtf and "Error getting file path and data" above be Log.e. After all, installApk may not expect "Error getting file path and data", but it knows that it can happen, since it doesn't control the input that its caller provides. Whereas it's in complete control of returnObject and the literal string error message that it puts into it. So that should never fail, and failure is completely unexpected. However, in this case we don't need returnObject at all, since EventDispatcher.sendError accepts any Object for its "response" parameter, so we should simply pass the literal string "Error getting file path and data". @@ +213,5 @@ > + returnObject.put("error", "Error getting file path and data"); > + } catch (Exception ex) { > + Log.e(LOGTAG, "Exception handling message \"installApk\":", ex); > + } > + EventDispatcher.sendError(message, returnObject); We don't need to send the message, since the recipient already has access to it. So we should use the simpler, one-parameter form EventDispatcher.sendError(Object response) here. @@ +225,3 @@ > try { > + app = messageData.getJSONObject("app"); > + manifestUrl = app.getString("manifestURL"); Nit: there's no need to use two separate try/catch blocks to retrieve the various bits of data from the message and handle any errors. We should combine them into one such block. ::: mobile/android/modules/WebappManager.jsm @@ +44,5 @@ > __proto__: DOMRequestIpcHelper.prototype, > > + // Used to store keys and references to message managers in case of error in > + // the Java which needs to be routed back to the calling web page. > + _messageMap: {}, Nit: this actually maps message managers, not messages, so I would call this _messageManagers (with a plural instead of the word "map"), which is a more conventional name for such maps. @@ +90,5 @@ > sendMessageToJava({ > type: "Webapps:InstallApk", > filePath: filePath, > + data: aMessage, > + }, (returnData, returnError) => { sendMessageToJava takes separate success and error callbacks, so this error handling should be in a separate method. @@ +203,5 @@ > autoInstall: function(aData) { > + let messageManager = this._messageMap[aData.requestID]; > + if (messageManager) { > + delete this._messageMap[aData.requestID]; > + } Nit: since this only uses messageManager once, it might as well simply check this._messageMap[aData.requestID] directly: if (this._messageMap[aData.requestID]) { delete this._messageMap[aData.requestID]; } @@ +365,4 @@ > > sendMessageToJava({ > type: "Webapps:GetApkVersions", > + packageNames: packageNames Nit: to preserve history, avoid fixing whitespace on lines that aren't otherwise being changed.

Newly constructed patch based on bug 957067 rewrite

Comment on attachment 8419422 [details] [diff] [review] Propigate APK install cancellation back to webpage Review of attachment 8419422 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! ::: mobile/android/modules/WebappManager.jsm @@ +474,4 @@ > data: JSON.stringify(msg), > }, (returnData, returnError) => { > if (!!returnError) { > + // There's no page to report back to so drop the error Nit: delimit this complete sentence with a period! Also, it'd be handy to add a note here that we should notify the user about the failure (although the functionality itself we should add in a separate bug). @@ +474,5 @@ > data: JSON.stringify(msg), > }, (returnData, returnError) => { > if (!!returnError) { > + // There's no page to report back to so drop the error > + debug("APK install failed : " + returnError) Nit: delimit this statement with a semi-colon!

- Moved some code from Bug 957067 dealing with the sendMessageToJava callback function. - added delimiters - added TODO for user notification of apk install fail on app update

Comment on attachment 8421000 [details] [diff] [review] Propigate APK install cancellation back to webpage Review of attachment 8421000 [details] [diff] [review]: ----------------------------------------------------------------- Looks great!

Comment on attachment 8421000 [details] [diff] [review] Propigate APK install cancellation back to webpage Erm, I meant to give feedback+. This still needs review from a module peer.

This is bitrotted. Please rebase.

Updated patch to cure bitrot

I'm still hitting tons of bitrot on this (though now I'm trying to apply it on top of bug 957067 which probably isn't helping).

Fixed bitrot again!

Yay persistence! :) https://hg.mozilla.org/integration/fx-team/rev/31db3f3b1107

Yay! Happy days.