Closed Bug 609043 Opened 14 years ago Closed 13 years ago

Add support for Open Web Apps

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 9

People

(Reporter: fabrice, Assigned: fabrice)

References

Details

Attachments

(1 file, 5 obsolete files)

Provide support for Open Web Apps (https://wiki.mozilla.org/Labs/Apps), to allow seamless installation from within web pages.
Attached patch m-c patch (obsolete) — Splinter Review
Adds a mozApp property to window.navigator. The real implementation is delegated to an XPCOM component.
Assignee: nobody → fabrice
Attachment #487622 - Flags: feedback?
Attachment #487622 - Flags: feedback? → feedback?(doug.turner)
Attached patch m-b patch (obsolete) — Splinter Review
Here we hook up Open Webapp support with the existing platform-dependent components.

We use the following parts of the manifest :
- name
- launch uri
- icon
- capabilities
and support l10n for the name.
Comment on attachment 487622 [details] [diff] [review]
m-c patch



>+++ b/dom/base/nsDOMClassInfo.cpp

You will also want to add classinfo for the class that implements xxx.  Do a grep in this file for GeoGeolocation to see what I mean.  Also add these to the nsDOMClassInfoClasses.h file.




>+
>+NS_IMETHODIMP nsNavigator::GetMozApp(nsIDOMOpenWebapp **aRetVal)
>+{
>+  NS_ENSURE_ARG_POINTER(aRetVal);
>+  *aRetVal = nsnull;
>+  
>+  if (mMozApp) {
>+    NS_ADDREF(*aRetVal = mMozApp);
>+    return NS_OK;
>+  }
>+  
>+  nsresult rv;
>+  mMozApp = do_CreateInstance("@mozilla.org/openwebapp;1", &rv);
>+  if (NS_FAILED(rv)) {
>+    NS_WARNING("No implementation of @mozilla.org/openwebapp;1 found");
>+    return NS_ERROR_NOT_IMPLEMENTED;
>+  }

Drop the rv.  You really aren't using it.  Instead just test for a null mMozApp.


>+#
>+# Contributor(s):
>+#  Fabrice Desré <fabrice@mozilla.com>
>+#

Is it just my dumb editor, or does your last name look wrong?


>+
>+[scriptable, function, uuid(9032f9d8-0fc2-4b6d-ad5b-a7c371d197b9)]
>+interface nsIOpenWebappCallback : nsISupports {
>+    void completed(in jsval manifest);    
>+};
>+
>+[scriptable, uuid(691d89a7-89b2-459b-ae7e-6bb9668a4b30)]
>+interface nsIDOMOpenWebapp : nsISupports {
>+    void install(in jsval manifest, in nsIOpenWebappCallback onSuccess, in nsIOpenWebappCallback onError);
>+    boolean isInstalled(in jsval manifest);
>+};
>+
>+/**
>+ * Property that extends the navigator object.
>+ */
>+[scriptable, uuid(8959e699-8c6c-451c-a842-581a12c8b35e)]
>+interface nsIDOMNavigatorOpenWebapp : nsISupports
>+{
>+  readonly attribute nsIDOMOpenWebapp mozApp;
>+};


Some people will complain about these 3 interfaces being in the same file.  Not really sure why - maybe some latent java style fetish.

Given that there is no spec, i'd comment wtf these API are suppose to do.

Using nsIOpenWebappCallback is probably wrong.  A better way is to use nsIDOMEventListener.  How much wiggle room do you have with this API.  Maybe I should provide feedback else where?

Otherwise looks fine.
Attachment #487622 - Flags: feedback?(doug.turner) → feedback-
Depends on: 655878
Fabrice - do you have an updated JS-only patch for this?
Also - it looks like the spec has changed from when Fabrice started to implement this.
(In reply to comment #5)
> Also - it looks like the spec has changed from when Fabrice started to
> implement this.

Yes, I'm currently implementing https://developer.mozilla.org/en/OpenWebApps/The_JavaScript_API in JS only, using a sandbox to inject the |apps| object into window.navigator
Attached patch JS only WIP (obsolete) — Splinter Review
This patch implements the installation API from https://developer.mozilla.org/en/OpenWebApps/The_JavaScript_API

Each application manifest is saved under $PROFILE/webapps/$APP_ID/manifest.js
The full list of applications is saved to $PROFILE/webapps/webapps.json

It also creates a manifest for webapps installed from the site menu.

Todo:
- implement part of the management API
- fix the "Add shortcut" android feature which is now broken.
Attachment #487622 - Attachment is obsolete: true
Attachment #487624 - Attachment is obsolete: true
A good test page is : https://apps.mozillalabs.com/appdir/
Comment on attachment 549516 [details] [diff] [review]
JS only WIP


>diff --git a/mobile/chrome/content/common-ui.js b/mobile/chrome/content/common-ui.js
> var WebappsUI = {
>   _dialog: null,
>-  _manifest: null,
>   _perms: [],
>-  
>+  _openwebapps: false,
>+  _application: null,
>+
>+  init: function() {
>+    Cu.import("resource:///modules/openwebapps.jsm");
>+    messageManager.addMessageListener("OpenWebapps:Install", this);
>+    messageManager.addMessageListener("OpenWebapps:GetInstalledBy", this);
>+    messageManager.addMessageListener("OpenWebapps:AmInstalled", this);
>+  },
>+
>+  // converts a manifest to an application as expected by openwebapps.install()
>+  convertManifest: function(aData) {
>+    let aManifest = JSON.parse(aData.manifest);
>+    let app = {};
>+
>+    app.manifest = aManifest;
>+    app.installData = aData.installData;
>+    app.storeURI = aData.storeURI;
>+    app.manifestURI = aData.manifestURI;

How about removing aManifest and doing:

      app.manifest = JSON.parse(aData.manifest);

if you really want to keep the local varibale, change "aManifest" to "manifest"


Also, we typically do this now:

      let app = {
        manifest: aManifest,
        installData: aData.installData,
        storeURI: aData.storeURI,
        manifestURI: aData.manifestURI
      };

You use aData.* in the code below, but you could use app.*

>+    let localeRoot;
>+
>+    if (aManifest.locales)

Remove the blank line

>+    if (icon)
>+      app.iconURI = icon;
>+    let root = baseURI.resolve("/").toString();

Add a blank line before "let root..."

>+    app.appURI = aManifest.launch_path ? baseURI.resolve(aManifest.launch_path) : root.substring(0, root.length - 1);

lanuch_path? launchPath seems more JavaScript-ish

>+    app.capabilities = [];
>+    app.successCallback = aData.successCallback;
>+    app.errorCallback = aData.errorCallback;

Couldn't you have added these above?

>+  receiveMessage: function(aMessage) {
>+    let browser = getBrowser();

      let browser = aMessage.target;

Just in case the selected browser did not send the request

>+    switch(aMessage.name) {
>+      case "OpenWebapps:Install":
>+        WebappsUI._openwebapps = true;
>+        WebappsUI.show(WebappsUI.convertManifest(aMessage.json));
>+        break;
>+      case "OpenWebapps:GetInstalledBy":
>+        let apps = OpenWebapps.getInstalledBy(aMessage.json.storeURI);
>+        browser.messageManager.sendAsyncMessage("OpenWebapps:GetInstalledBy:Return",
>+            { apps: apps, successCallback: aMessage.json.successCallback, errorCallback: aMessage.json.errorCallback });

How can you pass the callbacks across JSON? The JSON stringify will destroy and functions. I think you need to hold the functions on the content side.

UPDATE: I see that you do hold them in content and these are IDs. I would consider bundling all callbacks together into a single JS object, not stored individually. Here, you would have something like:

              { apps: apps, callbackID: aMessage.json.callbackID });

The callbackID would be used in content to retrieve a JS object with both success and error callbacks. See below for the content changes.

>     BrowserUI.popPopup(this);
>+    if (this._openwebapps) {
>+      let browser = getBrowser();
>+      browser.messageManager.sendAsyncMessage("OpenWebapps:InstallAborted", { successCallback: this._application.successCallback, errorCallback: this._application.errorCallback });

* Blank line after BrowserUI.popPopup
* Shouldn't you reset _openwebapps = false here too?

>+    this.install();
>+    let browser = getBrowser();
>+    if (willSendMessage)
>+      browser.messageManager.sendAsyncMessage("OpenWebapps:InstallDone", { successCallback: this._application.successCallback, errorCallback: this._application.errorCallback });

* Blank line after this.install()
* I don't like using getBrowser here. Maybe _openwebapps should point to the original aMessage.target and then be used here too.

>diff --git a/mobile/chrome/content/content.js b/mobile/chrome/content/content.js

>+let OpenWebapps = {

>+  getCallbackId: function(aCallback) {
>+    let id = "id" + Math.random();
>+    this._callbacks[id] = aCallback;
>+    return id;
>+  },
>+  
>+  getCallback: function(aId) {
>+    return this._callbacks[aId];
>+  },
>+
>+  removeCallback: function(aId) {
>+    delete this._callbacks[aId];
>+  },
>+
>+  install: function(aStoreURI, aManifestURI, aInstallData, aSuccessCallback, aErrorCallback) {
>+    let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);
>+    xhr.open("GET", aManifestURI, true);
>+    xhr.onload = function() {
>+      if (xhr.status == 200) {
>+        try {
>+          let manifest = JSON.parse(xhr.responseText);
>+          if (!OpenWebapps.checkManifest(manifest)) {
>+            if (aErrorCallback)
>+              aErrorCallback({ code: "invalidManifest", message: "Invalid manifest" });
>+          }
>+          else {

} else {

>+            sendAsyncMessage("OpenWebapps:Install", { storeURI: aStoreURI.href, manifestURI: aManifestURI, manifest: xhr.responseText,
>+                             installData: aInstallData, successCallback:  OpenWebapps.getCallbackId(aSuccessCallback), errorCallback: OpenWebapps.getCallbackId(aErrorCallback) });

Since each meaagse knows how many callbacks exist, let's simplify this and do:

            sendAsyncMessage("OpenWebapps:Install", { storeURI: aStoreURI.href, manifestURI: aManifestURI, manifest: xhr.responseText,
                             installData: aInstallData, callbackID: OpenWebapps.getCallbackId({ success: aSuccessCallback, error: aErrorCallback }) });

>+        }
>+        catch(e) {

} catch {

>+      }
>+      else if (aErrorCallback) {

} else if (...) {

>+      }
>+      
>+    }
>+    xhr.onerror = function() {
>+      if (aErrorCallback)
>+        aErrorCallback({ code: "networkError", message: "Unable to retrieve manifest" });
>+    }
>+    xhr.send(null);

* Remove blank line between } }
* Add blank lines between xhr.*

>+  amInstalled: function(aAppURI, aSuccessCallback, aErrorCallback) {
>+    sendAsyncMessage("OpenWebapps:AmInstalled", { appURI: aAppURI, successCallback:  OpenWebapps.getCallbackId(aSuccessCallback), errorCallback: OpenWebapps.getCallbackId(aErrorCallback) });

    sendAsyncMessage("OpenWebapps:AmInstalled", { appURI: aAppURI, callbackID: OpenWebapps.getCallbackId({ success: aSuccessCallback, error: aErrorCallback }) });

>+  getInstalledBy: function(aStoreURI, aSuccessCallback, aErrorCallback) {
>+    sendAsyncMessage("OpenWebapps:GetInstalledBy", { storeURI: aStoreURI.href, successCallback:  OpenWebapps.getCallbackId(aSuccessCallback), errorCallback: OpenWebapps.getCallbackId(aErrorCallback) });

    sendAsyncMessage("OpenWebapps:GetInstalledBy", { storeURI: aStoreURI.href, callbackID: OpenWebapps.getCallbackId({ success: aSuccessCallback, error: aErrorCallback }) });

>+  receiveMessage: function(aMessage) {

Update for new callback style

>+  init: function() {

>+    let observer = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
>+    observer.addObserver(this, "content-document-global-created", false);

Service.obs.addObserver

>+  getInjected: function() {
>+    let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);
>+    xhr.open("GET", "chrome://browser/content/injected.js", false);
>+    xhr.overrideMimeType("text/plain");
>+    xhr.send(null);
>+    return xhr.responseText;
>+  },
>+
>+  observe: function(subject, topic, data) {
>+    let sandbox = new Components.utils.Sandbox(Cc["@mozilla.org/systemprincipal;1"].createInstance(Ci.nsIPrincipal));
>+    sandbox.window = subject.wrappedJSObject;
>+
>+    sandbox.importFunction(function(aStoreURI, aManifestURI, aInstallData, aSuccessCallback, aErrorCallback) {
>+        OpenWebapps.install(aStoreURI, aManifestURI, aInstallData, aSuccessCallback, aErrorCallback);
>+        }, "OpenWebapps_install");
>+    
>+    sandbox.importFunction(function(aAppURI, aSuccessCallback, aErrorCallback) {
>+        OpenWebapps.amInstalled(aAppURI, aSuccessCallback, aErrorCallback);
>+        }, "OpenWebapps_amInstalled");
>+    
>+    sandbox.importFunction(function(aStoreURI, aSuccessCallback, aErrorCallback) {
>+        OpenWebapps.getInstalledBy(aStoreURI, aSuccessCallback, aErrorCallback);
>+        }, "OpenWebapps_getInstalledBy");
>+
>+    let toInject = OpenWebapps.getInjected();
>+    Cu.evalInSandbox(toInject, sandbox, "1.8", "chrome://browser/content/injected.js", 1);
>+  }
>+};
>+
>+OpenWebapps.init();

>diff --git a/mobile/chrome/content/injected.js b/mobile/chrome/content/injected.js
>+}
>\ No newline at end of file

Add new line

>diff --git a/mobile/modules/Makefile.in b/mobile/modules/Makefile.in

>   LocaleRepository.jsm \
>   linuxTypes.jsm \
>   video.jsm \
>+  openwebapps.jsm \

openWebapps.jsm ?

>+
>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+Cu.import("resource://gre/modules/Services.jsm");
>+
>+XPCOMUtils.defineLazyGetter(this, "NetUtil", function() {
>+  Cu.import("resource://gre/modules/NetUtil.jsm");
>+  return NetUtil;
>+});
>+
>+let OpenWebapps = {

>+  init: function() {

>+        // Read webapps json file into a string
>+        let state = Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
>+        state.data = NetUtil.readInputStreamToString(aStream, aStream.available()) || "";
>+        aStream.close();
>+
>+        let data = null;
>+        try {
>+          dump(data + "\n");
>+          data = JSON.parse(state.data);
>+          self.webapps = data;
>+        } catch (ex) {
>+          Cu.reportError("SessionStore: Could not parse JSON: " + ex);

* s/SessionStore/OpenWebappsSupport
* Rework this section. We don't need the nsISupportsString:

          let data = null;
          try {
            data = JSON.parse(NetUtil.readInputStreamToString(aStream, aStream.available()) || "");
            aStream.close();
            self.webapps = data;
          } catch (ex) {
            Cu.reportError("OpenWebappsSupport: Could not parse JSON: " + ex);

>+  _writeFile: function ss_writeFile(aFile, aData) {
>+    let stateString = Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
>+    stateString.data = aData;
>+    Services.obs.notifyObservers(stateString, "sessionstore-state-write", "");
>+
>+    // Don't touch the file if an observer has deleted all state data
>+    if (!stateString.data)
>+      return;

Remove this whole block
Attachment #549516 - Flags: feedback+
(In reply to Fabrice Desré [:fabrice] from comment #7)

> Each application manifest is saved under $PROFILE/webapps/$APP_ID/manifest.js
> The full list of applications is saved to $PROFILE/webapps/webapps.json

You could save the icon to $PROFILE/webapps/$APP_ID/icon.png
The fielURI to the icon.png could be used in $PROFILE/webapps/webapps.json

> It also creates a manifest for webapps installed from the site menu.

We may not need this, but it's ok for now.

> Todo:
> - implement part of the management API

The docs say this about the Management API:

'The Management API is part of the application repository's API which is privileged, intended to grant access to trusted pages, or "Dashboards".'

I don't know if we have the "privileged" and "trusted pages" parts of this worked yet do we? How does a user give the permission to the page? Do we need this API in the first phase?
Depends on: 641552
Attached patch addressing comments (obsolete) — Splinter Review
> >+    app.appURI = aManifest.launch_path ? baseURI.resolve(aManifest.launch_path) : root.substring(0, root.length - 1);
> 
> lanuch_path? launchPath seems more JavaScript-ish

Unfortunately this is part of the manifest defined here : https://developer.mozilla.org/en/OpenWebApps/The_Manifest
 
> UPDATE: I see that you do hold them in content and these are IDs. I would
> consider bundling all callbacks together into a single JS object, not stored
> individually. Here, you would have something like:
> 
>               { apps: apps, callbackID: aMessage.json.callbackID });
> 
> The callbackID would be used in content to retrieve a JS object with both
> success and error callbacks. See below for the content changes.

Nice idea!
 
> >     BrowserUI.popPopup(this);
> >+    if (this._openwebapps) {
> >+      let browser = getBrowser();
> >+      browser.messageManager.sendAsyncMessage("OpenWebapps:InstallAborted", { successCallback: this._application.successCallback, errorCallback: this._application.errorCallback });
> 
> * Shouldn't you reset _openwebapps = false here too?

Useful to help gc, but not needed for logic - we are "protected" by the use of a modal dialog here.


> >+    this.install();
> >+    let browser = getBrowser();
> >+    if (willSendMessage)
> >+      browser.messageManager.sendAsyncMessage("OpenWebapps:InstallDone", { successCallback: this._application.successCallback, errorCallback: this._application.errorCallback });
> * I don't like using getBrowser here. Maybe _openwebapps should point to the
> original aMessage.target and then be used here too.

I moved _openwebapps to be the browser (aMessage.target). I also removed the willSendMessage variable since we can now directly check if browser is null.
 

I also added implementation of 3 management methods :
list, launch and uninstall, since they are useful to implement a web dashboard.

To fully test, use the store at https://apps.mozillalabs.com/appdir/ and the test dashboard at http://people.mozilla.org/~fdesre/openwebapps/dashboard.html

I will change the name of property to navigator.mozApps instead of navigator.apps before pushing.
Attachment #549516 - Attachment is obsolete: true
Attachment #552939 - Flags: review?(mark.finkle)
Comment on attachment 552939 [details] [diff] [review]
addressing comments


>diff --git a/mobile/chrome/content/common-ui.js b/mobile/chrome/content/common-ui.js

> var WebappsUI = {

>+  show: function show(aApplication) {
>+    if (!aApplication) {
>+      this._openwebapps = null;
>       // Try every way to get an icon

nit: add a blank line before the comment

>+  install: function(aIconURI) {
>       } catch(e) {
>         Cu.reportError(e);
>       }
>     }
>     image.onerror = function() {
>       // can't load the icon (bad URI) : fallback to the default one from chrome

add a blank line above "image.onerror"

>+      self.install("chrome://browser/skin/images/favicon-default-30.png");
>     }
>-    image.src = aIcon;
>+    image.src = aIconURI || this._application.iconURI;

add a blank line above "image.src=..."

>diff --git a/mobile/chrome/content/content.js b/mobile/chrome/content/content.js
>+  install: function(aStoreURI, aManifestURI, aInstallData, aSuccessCallback, aErrorCallback) {

>+      if (xhr.status == 200) {
>+        try {
>+          let manifest = JSON.parse(xhr.responseText);
>+          if (!OpenWebapps.checkManifest(manifest)) {
>+            if (aErrorCallback)
>+              aErrorCallback({ code: "invalidManifest", message: "Invalid manifest" });
>+          }
>+          else {
>+            sendAsyncMessage("OpenWebapps:Install", { storeURI: aStoreURI.href, manifestURI: aManifestURI, manifest: xhr.responseText,
>+                             installData: aInstallData, callbackID:  OpenWebapps.getCallbackId({ success: aSuccessCallback, error: aErrorCallback }) });
>+          }
>+        }
>+        catch(e) {
>+          if (aErrorCallback)
>+            aErrorCallback({ code: "manifestParseError", message: "Unable to parse the manifest" });
>+        }
>+      }
>+      else if (aErrorCallback) {
>+        aErrorCallback({ code: "networkError", message: "Unable to retrieve manifest" });
>+      }      
>+    }

nit: use "} else {" and "} catch(e) {"

are the error codes ("networkError" for example) spec'ed? or are we supposed to just keep them understandable?


>diff --git a/mobile/chrome/content/injected.js b/mobile/chrome/content/injected.js

I know this file is temporary, but it is using a 4 space indent. Make it use 2 spaces

>diff --git a/mobile/components/Makefile.in b/mobile/components/Makefile.in

>@@ -76,7 +76,6 @@ EXTRA_COMPONENTS = \
>         LoginManager.js \
>         LoginManagerPrompter.js \
>         BlocklistPrompt.js \
>-        WebappsSupport.js \
>         $(NULL)

There is an IDL file (WebappSupport.idl) in here now too. You need to remove it too (MXR is out of date)

>diff --git a/mobile/modules/openWebapps.jsm b/mobile/modules/openWebapps.jsm

>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+Cu.import("resource://gre/modules/Services.jsm");
>+Cu.import("resource://gre/modules/Services.jsm");

Services.jsm is listed twice

>+let OpenWebapps = {

>+  install: function(aApplication) {
>+    let uuidGenerator = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
>+    let id = uuidGenerator.generateUUID().toString();
>+    let dir = this.appsDir.clone();
>+    dir.append(id);
>+    dir.create(Ci.nsIFile.DIRECTORY_TYPE, 0700);
>+    
>+    let manFile = dir.clone();
>+    manFile.append("manifest.json");
>+    this._writeFile(manFile, JSON.stringify(aApplication.manifest));
>+    
>+    this.webapps[id] = {
>+      title: aApplication.title,
>+      storeURI: aApplication.storeURI,
>+      appURI: aApplication.appURI,
>+      iconData: aApplication.iconData,
>+      installData: aApplication.installData,
>+      installTime: (new Date()).getTime(),
>+      manifest: aApplication.manifest
>+    };

Do we need to check for an existing webapp here? the UUID used for the folder will be fine (no conflicts), but the appURI that is used to compare applications in "amInstalled" is not checked here and could cause more than one app to be installed with the same appURI.

Should we check amInstalled first?

r+ but fix the nits, and think about the amInstalled question
Attachment #552939 - Flags: review?(mark.finkle) → review+
Nits fixed, and added a check to not install twice an application.

http://hg.mozilla.org/mozilla-central/rev/f085bbca2ee9
This appears to have triggered perma-orange on Android browser-chrome tests.  Backing out...
Backed out:  http://hg.mozilla.org/mozilla-central/rev/37dedb70a68a
(I also backed out 679194, because it layers on top of this, as noted in that bug)

Details: Android b-c has was mostly (entirely?) green on m-c until this landed.  Since this landed, it's been perma-orange, with each cycle experiencing a timeout in all or some of these four tests:
chrome://mochitests/content/browser/mobile/chrome/tests/browser_addons.js
chrome://mochitests/content/browser/mobile/chrome/tests/browser_scrollbar.js
chrome://mochitests/content/browser/mobile/chrome/tests/browser_tabs.js
chrome://mochitests/content/browser/mobile/chrome/tests/browser_thumbnails.js

Sample log:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1313444106.1313444625.12412.gz

Similarly, Android b-c was green on m-i up until the push that merged this over to m-i.  From that point on, m-i has had the same issue.

So, the above strongly points to this cset causing the timouts that turned Android b-c permaorange.
Blocks: 583750
Depends on: 683222
Attached patch updated patch (obsolete) — Splinter Review
Updated patch with the following changes:
- use an xpcom component to implement the navigator property
- move the xpcom and js module to /toolkit
- implemented a watchUpdates() and clearWatch() API calls (documented at https://developer.mozilla.org/en/OpenWebApps/The_JavaScript_API)
- cleaned up code we don't need anymore
Attachment #552939 - Attachment is obsolete: true
Attachment #561872 - Flags: review?(mark.finkle)
Fixed a bug in getInstalledBy where the JS apps array was not converted to an nsIOpenWebappsApplication array
Attachment #561872 - Attachment is obsolete: true
Attachment #562050 - Flags: review?(mark.finkle)
Attachment #561872 - Flags: review?(mark.finkle)
Comment on attachment 562050 [details] [diff] [review]
new updated patch

Looks good. The changes from the last full review are mostly limited to the new IDL & impl, plus moving to toolkit.

This is currently only used by mobile, but we plan to go through a simialr review/update when desktop starts to use the code. Putting the code in toolkit from the beginning just seemed like a good idea.
Attachment #562050 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/282cc7688390
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Depends on: 713468
You need to log in before you can comment on or make changes to this bug.