957067 - Pass back error when incorrect data passed through in install APK

Status: RESOLVED FIXED

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

Description

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

When GeckoAppShell.installApk fails to retrieve the manifest URL from the data it is given, and thus it is unable to install the APK, it should propagate the error back to the web page that called mozApps.install().

Comment 1

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

We should be using optString instead of getString in this case, as we know the caller will always pass us a valid manifest URL, so we don't actually need to verify it.

Comment 2

[Martyn Haigh (:mhaigh)]

Attachment: Stop verifying manifestUrl in installApk

Comment 3

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

Comment on attachment 8365071 [details] [diff] [review]
Stop verifying manifestUrl in installApk

This seems reasonable to me.  It'll just need rebasing against the latest source, as the method in question has moved to EventListener.java.

Let's see what wesj thinks.

Comment 4

[Martyn Haigh (:mhaigh)]

Attachment: Stop verifying manifestUrl in installApk

Rebased against new codebase

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 8371456 [details] [diff] [review]
Stop verifying manifestUrl in installApk

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

::: mobile/android/base/webapp/EventListener.java
@@ +173,4 @@
>          String manifestUrl = null;
>          try {
>              argsObj = new JSONObject(data);
> +            manifestUrl = argsObj.optJSONObject("app").optString("manifestURL");

optJSONObject will return null if it fails. If its not optional, just getJSONObject. If it is, you'll have to check the return. Also, if this is all optional, you can move it out of the try-catch (or not... I don't have a strong preference there).

Comment 6

[Martyn Haigh (:mhaigh)]

closed in error

Comment 7

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

To provide a bit more context: mhaigh and I discussed this and agreed that actually the original issue for which we filed this bug is still valid, and we should indeed return an error when incorrect data is passed to installApk, so mhaigh updated the summary, and we'll use this bug to implement that fix (which will include sending the error back to the code that initiated the install via the message manager associated with the installation request).

Comment 8

[Martyn Haigh (:mhaigh)]

Attachment: Propigate APK install errors back to webpage

Comment 9

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

Comment on attachment 8392507 [details] [diff] [review]
Propigate APK install errors back to webpage

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

::: mobile/android/base/webapp/EventListener.java
@@ +196,5 @@
> +        try {
> +          filePath = message.getString("filePath");
> +          data = message.getString("data");
> +        } catch (Exception e) {
> +            Log.e(LOGTAG, "Exception handling message \"installApk\":", e);

Nit: I would make this message more specific, something like "error getting file path and data".

@@ +206,5 @@
>          // We get the manifest url out of javascript here so we can use it as a checksum
>          // in a minute, when a package has been installed.
>          String manifestUrl = null;
>          try {
> +            dataObj = new JSONObject(data);

This extra parsing step is required because WebappManager._installApk double-stringifies the "data" it sends to EventListener.installApk, since it stringifies it in the process of passing it to sendMessageToJava, which then stringifies it again.

We should stop doing that, which'll make this code simpler and easier to read, as we won't need to turn "data" into dataObj, because message.data will already be a JSONObject.

@@ +217,5 @@
> +            } catch (Exception ex) {
> +              Log.e(LOGTAG, "Exception handling message \"installApk\":", ex);
> +            }
> +            GeckoAppShell.notifyGeckoOfEvent(GeckoEvent.createBroadcastEvent(
> +                             "Webapps:InstallError", dataObj.toString()));

Nit: Fennec code usually doesn't wrap such statements.

Also, GeckoAppShell.notifyGeckoOfEvent will be useful for install errors that happen after EventListener.installApk has returned (f.e. when the user cancels the install), but this one happens synchronously, so can't we simply return the error the same way EventListener.getApkVersions returns a value, which sendMessageToJava then passes to its aCallback argument?

I suppose the value of employing this pattern here is that it means there'll be only one way for WebappManager to receive a failure.

::: mobile/android/chrome/content/browser.js
@@ +1388,4 @@
>    observe: function(aSubject, aTopic, aData) {
>      let browser = this.selectedBrowser;
>  
> +

Nit: remove this extraneous space.

@@ +1580,5 @@
>          WebappManager.install(JSON.parse(aData), aSubject);
>          break;
>  
> +      case "Webapps:InstallError":
> +        WebappManager.installError(JSON.parse(aData));

Nit: the name of the WebappManager.installError method should start with a verb, like WebappManager.reportInstallError; and its corresponding message should have the same name.

::: mobile/android/modules/WebappManager.jsm
@@ +51,5 @@
>      }
>    },
>  
> +  _messageMap : null,
> +  _messageKeyMap : null,

Nit: no space before ":".

Also, these are singletons and should be initialized at script evaluation time.

Finally, these properties could use some documentation, as it isn't immediately obvious why they exist (especially the second one).

@@ +64,4 @@
>      this._installApk(aMessage, aMessageManager);
>    },
>  
> +  installError: function(aMessage) {

Nit: put this after _autoUpdate to maintain the (admittedly tenuous) relationship between the order of the notification observers in browser.js and the order of the public methods of WebappManager.

@@ +65,5 @@
>    },
>  
> +  installError: function(aMessage) {
> +    let key = this._messageKeyMap[aMessage.requestID];
> +    if(!!this._messageMap && this._messageMap.has(key)) {

Nit: space between "if" and opening parenthesis.

Also, WeakMap.has() will throw if "key" is not a non-null object, so this needs to test "key" before passing it to that method.

@@ +68,5 @@
> +    let key = this._messageKeyMap[aMessage.requestID];
> +    if(!!this._messageMap && this._messageMap.has(key)) {
> +      let messageManager = this._messageMap.get(key);
> +      messageManager.sendAsyncMessage("Webapps:Install:Return:KO", aMessage);
> +      debug("error installing APK: " + JSON.stringify(aMessage));

We should be cautious about dumping a whole message, both because it creates a large log entry and because we don't know what all it contains, so we could leak personal information.  Thus here we should just report the error message and possible the manifest URL or another string that identifies the app.

@@ +72,5 @@
> +      debug("error installing APK: " + JSON.stringify(aMessage));
> +      this._messageMap.delete(key);
> +      delete this._messageKeyMap[aMessage.requestID];
> +    } else {
> +      debug ("Cannot find message with key of " + key);

Nit: no space between "debug" and opening parenthesis.

Also, we should probably also report a failure to find the key for aMessage.requestID.

@@ +210,5 @@
>    },
>  
>    autoInstall: function(aData) {
> +    let key = this._messageKeyMap[aData.requestID];
> +    if(!!this._messageMap && this._messageMap.has(key)) {

Nit: space between "if" and opening parenthesis.

Also, WeakMap.has() will throw if "key" is not a non-null object, so this needs to test "key" before passing it to that method.

Comment 10

[Martyn Haigh (:mhaigh)]

Attachment: Propigate APK install errors back to webpage

As per the previous feedback, I've modified the method used to return the error back to the calling JavaScript.  It now makes use of the existing Java -> JS bridge to return the error message apart from in the case of a RESULT_CANCELED result from the apk installer, in which case it fires asynchronously.

Comment 11

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

Comment on attachment 8399459 [details] [diff] [review]
Propigate APK install errors back to webpage

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

This is feedback+ for overall approach but still has a few issues.

::: mobile/android/base/webapp/EventListener.java
@@ +86,5 @@
>              if (AppConstants.MOZ_ANDROID_SYNTHAPKS && event.equals("Webapps:InstallApk")) {
> +
> +                JSONObject obj = new JSONObject();
> +                obj.put("data", installApk(GeckoAppShell.getGeckoInterface().getActivity(), message));
> +                EventDispatcher.sendResponse(message, obj);

The return value of installApk is already a JSONObject, so this doesn't need to wrap it in another JSONObject; instead, it can simply send the return value.

However, EventDispatcher also has a sendError method that calls the error callback that is the third parameter to sendMessageToJava.  We should use that here, for which the best approach is to move this EventDispatcher.sendResponse call into installApk, which can then call EventDispatcher.sendError instead when an error occurs.

@@ +205,5 @@
> +    private static JSONObject createInstallError(JSONObject obj, String message, JSONException e) {
> +        JSONObject errorReturn = obj;
> +        if(errorReturn == null) {
> +            errorReturn = new JSONObject();
> +        }

Nit: conditionally creating this object seems like overcomplexity, given that the callers can simply create one inline if needed:

    createInstallError(new JSONObject(), …);

Also, in general I tend to avoid factoring out error reporting like this, as it's easier to understand the flow of code when errors are reported inline, even if that means writing a few more lines of code at each error site.

@@ +230,5 @@
> +        } catch (JSONException e) {
> +            return createInstallError(null, "Error getting file path and data", e);
> +        }
> +
> +        final JSONObject dataObj = data;

Why create a second reference to this value?

@@ +235,5 @@
>  
>          // We get the manifest url out of javascript here so we can use it as a checksum
>          // in a minute, when a package has been installed.
>          String manifestUrl = null;
> +        JSONObject appObject = null;

Nit: it isn't necessary to append "Object" to the name of this variable, as there isn't another "app" symbol in this scope that we need to differentiate from this one.

@@ +240,5 @@
> +        try {
> +            appObject = data.getJSONObject("app");
> +        } catch (JSONException e) {
> +            return createInstallError(null, "Can't get app object from JSON data", e);
> +        }

Couldn't we use the same try/catch block for all three of the filePath, data, and app properties?  It isn't clear what benefits accrue from catching two possible exceptions in one block and the third in another.

@@ +247,2 @@
>          } catch (JSONException e) {
> +            return createInstallError(appObject, "Can't get manifest URL from JSON data", e);

The same question applies here: why not get manifestUrl in the same try/catch block as for the other properties?  I know here we return appObject, whereas the other blocks return a new JSONObject; but the JS recipient of this response doesn't seem to do anything with the object besides access its "error" property, so that doesn't seem necessary.

And while it's nice to have more specific error messages, it seems like overkill to trap each exception individually, especially given that these are errors that should never happen (hence the Log.wtf calls).

::: mobile/android/chrome/content/browser.js
@@ +1609,5 @@
>          WebappManager.install(JSON.parse(aData), aSubject);
>          break;
>  
> +      case "Webapps:InstallError":
> +        WebappManager.installError(JSON.parse(aData));

Nit: this should have a verb-initial name or be called onInstallError to make it clear that it handles InstallError events.

::: mobile/android/modules/WebappManager.jsm
@@ +43,5 @@
>  this.WebappManager = {
>    __proto__: DOMRequestIpcHelper.prototype,
>  
> +  //used to store keys and references to message managers incase of error in the
> +  //java which needs to be routed back to the calling web page

Nit: used -> Used, incase -> in case, java -> Java, period at end.

@@ +79,5 @@
>    _installApk: function(aMessage, aMessageManager) { return Task.spawn((function*() {
>      let filePath;
>  
> +    this._messageKeyMap[aMessage.requestID] = new String(aMessage.requestID);
> +    this._messageMap.set(this._messageKeyMap[aMessage.requestID], aMessageManager);

Nit: this doesn't need to happen until after EventListener.installApk succeeds.

@@ +85,5 @@
> +    let throwError = function(errorMessage) {
> +      aMessage.error = errorMessage;
> +      aMessageManager.sendAsyncMessage("Webapps:Install:Return:KO", aMessage);
> +      debug("error downloading APK: " + errorMessage);
> +    }

If _installApk populates _messageKeyMap and _messageMap early, as it currently does, then this function needs to delete those references.  But if it waits until EventListener.installApk returns, then it doesn't.

Also, here again I would avoid factoring out the handling of errors, especially given that there are only two callers.  It'll make the code easier to read if the error handling happens inline than if one has to jump back and forth between the code doing the work and this code handling the errors to understand the code flow.

@@ +99,5 @@
>        type: "Webapps:InstallApk",
>        filePath: filePath,
> +      data: aMessage,
> +    }, returnData => {
> +      let data = JSON.parse(returnData).data;

As of a few days ago, you no longer need to (and shouldn't) parse the response passed into the callback!

@@ +101,5 @@
> +      data: aMessage,
> +    }, returnData => {
> +      let data = JSON.parse(returnData).data;
> +
> +      if(!!data && !!data.error) {

Nit: space between "if" and opening parenthesis.

@@ +207,5 @@
>    },
>  
>    autoInstall: function(aData) {
> +    let key = this._messageKeyMap[aData.requestID];
> +    if(!!this._messageMap && key && this._messageMap.has(key)) {

Nit: space between "if" and opening parenthesis.

@@ +287,5 @@
>  
> +  installError: function(aMessage) {
> +    let key = this._messageKeyMap[aMessage.requestID];
> +    if (!!this._messageMap && !!key && this._messageMap.has(key)) {
> +      let messageManager = this._messageMap.get(key);

This also needs to check that messageManager is still a valid reference (i.e. not undefined), since it might have been garbage-collected.

Comment 12

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

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

> > +        final JSONObject dataObj = data;
> 
> Why create a second reference to this value?

Update: Martyn and I chatted about this, and he helpfully educated me that Java requires the reference to be `final` in order for the inner ActivityResultHandler class to access it.

And, after further investigation, I figured out why I didn't get a compile-time error when I tried removing the second reference: the inner class's onActivityResult method has a same-named parameter, so it was accessing the inner `data`!

That led me to wonder if we can make the original reference `final`.  It only gets initialized once, and if initialization fails, then the method exits early.  So it seems like we should be able to make it final and access it from both the inner and outer method.

Ok, we'd need to give it a different name, or give the inner method's `data` parameter (which isn't used) a different name; but that's easy enough.  So I recommend we rename the outer variable to `messageData`, or rename the inner method's parameter to `intentData`, and then make the outer variable `final`.

(We also need to stop initializing the variable to `null` at the point of declaration, which seems fine, since the variable isn't used until after it's successfully initialized to the value in the message).

Comment 13

[Martyn Haigh (:mhaigh)]

Attachment: Propigate APK install errors back to webpage

I appear to have got this working without the need for a second variable and without any hacks, although I'm a little confused as to why this wasn't working the other day. I have a feeling that something has changed in the build tools as I'm seeing quite different build output now.

Comment 14

[Martyn Haigh (:mhaigh)]

Attachment: Propigate APK install errors back to webpage

Had accidentally included work for bug 978143.  This has been stripped out now.

Comment 15

[Martyn Haigh (:mhaigh)]

Attachment: Propigate APK install errors back to webpage

Fixing a couple of nits

Comment 16

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

Comment on attachment 8403357 [details] [diff] [review]
Propigate APK install errors back to webpage

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

(In reply to Martyn Haigh (:mhaigh) from comment #13)

> I appear to have got this working without the need for a second variable and
> without any hacks, although I'm a little confused as to why this wasn't
> working the other day. I have a feeling that something has changed in the
> build tools as I'm seeing quite different build output now.

In the version of the patch that accompanied this message, you made messageData *final*, which enables it to be accessed by the inner class.  So presumably that's why you didn't need to declare a final second variable.

Overall, this is coming along!  There are just a few issues, see below!

::: mobile/android/base/webapp/EventListener.java
@@ +200,5 @@
>  
> +    public static void installApk(final Activity context, JSONObject message) {
> +        String filePath = null;
> +        JSONObject messageData;
> +        JSONObject returnObject = new JSONObject();

Nit: it's simpler and less redundant to avoid appending variable names with their types.  I would call this simply "response", which is what EventDispatcher.sendError calls it.  But see below for why we don't need this variable.

@@ +209,5 @@
> +        } catch (JSONException e) {
> +            Log.wtf(LOGTAG, "Error getting file path and data", e);
> +
> +            try {
> +                returnObject.put("error", "Error getting file path and data");

Nit: I would put the exception text into the error message, i.e. `"Error getting file path and data: " + e.toString()`.

@@ +211,5 @@
> +
> +            try {
> +                returnObject.put("error", "Error getting file path and data");
> +            } catch (Exception ex) {
> +                Log.e(LOGTAG, "Exception handling message \"installApk\":", ex);

Nit: since putting a string literal into a JSONObject should never fail, I would make this `Log.wtf`.  But see below for why we don't need this try/catch block.

@@ +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);

Code moves quickly in Fennecland!  Yesterday mcomella landed bug 993195, which deprecates the two-argument form of EventDispatcher.sendResponse/sendError in favor of one-argument forms.

We can still use the deprecated form here, but we don't actually need to send the message, since the recipient still has access to it.  So we might as well use the simpler, non-deprecated one-argument form.

Also, after another recent change, sendError (both forms) takes an arbitrary Object, so we don't need to create a JSONObject just to send a simple string; we can simply send the string!  Which means we also don't need to catch an exception when putting the string into the JSONObject.  I.e. this can be as simple as:

  EventDispatcher.sendError("Error getting file path and data: " + e.toString());

@@ +237,2 @@
>              return;
>          }

Nit: it still isn't clear why we have two different try/catch blocks for the four statements that retrieve values from the JSON message.  It would be simpler to do all four retrievals in a single block.

@@ +282,4 @@
>                  }
>              }
>          });
> +        EventDispatcher.sendResponse(message, returnObject);

There's no need to return any actual values here.  Better to send the simplest valid Object (null?):

  EventDispatcher.sendResponse(null);

Or simply don't send a response at all!  Then the callback can assume an error.  As far as I know, it isn't necessary to send a response; it's entirely optional; and in this case there isn't any benefit to doing so.

::: mobile/android/modules/WebappManager.jsm
@@ +86,4 @@
>      sendMessageToJava({
>        type: "Webapps:InstallApk",
>        filePath: filePath,
> +      data: aMessage,

There's a second instance of sendMessageToJava being used to send "Webapps:InstallApk", in _updateApks.  We need to make this change there as well.

@@ -337,4 @@
>  
>      sendMessageToJava({
>        type: "Webapps:GetApkVersions",
> -      packageNames: packageNames 

I know it sucks, but we should leave trailing whitespace like this one so that "blame" shows an accurate history of substantive changes.

Comment 17

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

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

> @@ +282,4 @@
> >                  }
> >              }
> >          });
> > +        EventDispatcher.sendResponse(message, returnObject);
> 
> There's no need to return any actual values here.  Better to send the
> simplest valid Object (null?):
> 
>   EventDispatcher.sendResponse(null);
> 
> Or simply don't send a response at all!  Then the callback can assume an
> error.  As far as I know, it isn't necessary to send a response; it's
> entirely optional; and in this case there isn't any benefit to doing so.

Erm, actually the new "one argument" form is called EventDispatcher.sendSuccess <http://hg.mozilla.org/mozilla-central/annotate/5a5ed08df529/mobile/android/base/EventDispatcher.java#l245>, so this would be:

  EventDispatcher.sendSuccess(null);

But we still don't need to send it at all.

Comment 18

[Martyn Haigh (:mhaigh)]

Attachment: Propigate APK install errors back to webpage

EventListener has been converted to use NativeEventListener and containing code converted to use NativeJSObject.

Comment 19

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

Comment on attachment 8418716 [details] [diff] [review]
Propigate APK install errors back to webpage

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

::: mobile/android/base/webapp/EventListener.java
@@ +83,3 @@
>          try {
>              if (AppConstants.MOZ_ANDROID_SYNTHAPKS && event.equals("Webapps:InstallApk")) {
> +                installApk(GeckoAppShell.getGeckoInterface().getActivity(), message, callback);

I like the way this passes the message to the installApk handler without extracting individual values from it first.  It makes handleMessage a generic dispatcher, separating its general logic for receiving and forwarding messages from the message-specific logic in each message handler.  They should all be this way!  (But that's a separate bug!)

@@ +211,5 @@
> +            app = new JSONObject(message.toString());
> +            filePath = app.getString("filePath");
> +            messageData = new JSONObject(app.getString("data"));
> +            app = messageData.getJSONObject("app");
> +            manifestUrl = app.getString("manifestURL");

Is it necessary to convert the entire message to a JSONObject and then retrieve all values from that object?  It looks like filePath and manifestURL could be retrieved from the original message object using NativeJSObject.getString, while "data" could be retrieved from it via either NativeJSObject.getString (if it is being stringified by the sender) or NativeJSObject.getObject (if it isn't), i.e.:

  filePath = message.getString("filePath");
  manifestUrl = message.getString("manifestURL");

And then:

  messageData = new JSONObject(message.getString("data"));
    - or -
  messageData = new JSONObject(message.getObject("data").toString());

Then you would never need to create the "app" object just to retrieve those values from the "message" object.

::: mobile/android/modules/WebappManager.jsm
@@ +86,4 @@
>      sendMessageToJava({
>        type: "Webapps:InstallApk",
>        filePath: filePath,
> +      data: aMessage,

This Webapps:InstallApk message sender stops stringifying the value of its "data" property, but the other message sender is still stringifying it.  The two message senders need to agree with each other (and the message recipient) about whether or not the "data" property is stringified.

Comment 20

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

Comment on attachment 8418716 [details] [diff] [review]
Propigate APK install errors back to webpage

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

::: mobile/android/modules/WebappManager.jsm
@@ +89,5 @@
> +      data: aMessage,
> +    }, (returnData, returnError) => {
> +
> +      if (!!returnError) {
> +        aMessage.error = returnError.error;

One more thing!  This and the other sendMessageToJava callback below both retrieve the error message from returnError.error, but the patch on bug 978143 retrieves it from returnError itself.  My reading of the Messaging.jsm API this code is using suggests you'll need to retrieve it from returnError itself here.

Also, nit: "data" and "error" would be more conventional names for these callback parameters.

Comment 21

[Martyn Haigh (:mhaigh)]

Attachment: Propigate APK install errors back to webpage

- Changed argument names to data & error in the sendMessageToJava callback.
- Removed superfluous addition of callback funtion to sendMessageToJava as this is actually a change more closely linked to bug 978143, so will add to that. 
- Made how we pass the value of the data attribute in sendMessageToJava consistent.
- cleaned up how we deal with the JSON in EventListener.java installApk

Comment 22

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

Comment on attachment 8420963 [details] [diff] [review]
Propigate APK install errors back to webpage

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

Looks good, just one minor nit!

::: mobile/android/base/webapp/EventListener.java
@@ +117,3 @@
>                  obj.put("versions", getApkVersions(GeckoAppShell.getGeckoInterface().getActivity(),
> +                                                   new JSONArray(data.getString("packageNames"))));
> +                callback.sendSuccess(obj);

Nit: I missed this the last time around, but it shouldn't be necessary to convert the entire message to a JSONObject first here either, as you should be able to retrieve the packageNames string directly from the NativeJSObject:

                obj.put("versions", getApkVersions(GeckoAppShell.getGeckoInterface().getActivity(),
                                                   new JSONArray(message.getString("packageNames"))));

Comment 23

[Martyn Haigh (:mhaigh)]

Attachment: Propigate APK install errors back to webpage

Factored out variable from GetApkVersions code

Comment 24

[Wesley Johnston (:wesj)]

Comment on attachment 8421771 [details] [diff] [review]
Propigate APK install errors back to webpage

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

I think we should switch this over to the new array support if it did land.

::: mobile/android/base/webapp/EventListener.java
@@ +114,5 @@
>              } else if (event.equals("Webapps:GetApkVersions")) {
>                  JSONObject obj = new JSONObject();
>                  obj.put("versions", getApkVersions(GeckoAppShell.getGeckoInterface().getActivity(),
> +                                                   new JSONArray(message.getObject("packageNames")
> +                                                                        .toString())));

I think jchen landed real array support last week. It might be worth looking into.

Comment 25

[Martyn Haigh (:mhaigh)]

Attachment: Stop verifying manifestUrl in installApk

Implemented getStringArray method of NativeJSObject

Comment 26

[Wesley Johnston (:wesj)]

Comment on attachment 8433424 [details] [diff] [review]
Stop verifying manifestUrl in installApk

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

::: mobile/android/base/webapp/EventListener.java
@@ +101,4 @@
>              } else if (event.equals("Webapps:GetApkVersions")) {
>                  JSONObject obj = new JSONObject();
>                  obj.put("versions", getApkVersions(GeckoAppShell.getGeckoInterface().getActivity(),
> +                                                   message.getStringArray("packageNames")));

I didn't know we had getStringArray :) Makes me giddy :)

Comment 27

[Martyn Haigh (:mhaigh)]

https://tbpl.mozilla.org/?tree=Try&rev=0ac3f4297ff9

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d4625a62c1bb