Closed Bug 898647 Opened 11 years ago Closed 10 years ago

Backend for app updates

Categories

(Firefox Graveyard :: Web Apps, defect, P1)

defect

Tracking

(firefox29 verified, firefox30 verified)

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: marco, Assigned: marco)

References

Details

(Whiteboard: DesktopWebRT2)

Attachments

(3 files, 10 obsolete files)

137.64 KB, patch
Details | Diff | Splinter Review
902 bytes, patch
myk
: review+
Details | Diff | Splinter Review
5.50 KB, patch
Details | Diff | Splinter Review
      No description provided.
Attached patch Patch (obsolete) — Splinter Review
I just need some feedback on the approach. You can also avoid looking at the patch.
I've split the installation in two functions, one that creates the runtime specific files (that are the same for every application and so don't depend on manifest data) and the other creates the webapp specific files.
This way the installation function calls both of them, the update function calls just the second one overwriting the files that contain manifest-specific data.
Attachment #782820 - Flags: feedback?(myk)
Comment on attachment 782820 [details] [diff] [review]
Patch

(In reply to Marco Castelluccio [:marco] from comment #1)
> I've split the installation in two functions, one that creates the runtime
> specific files (that are the same for every application and so don't depend
> on manifest data) and the other creates the webapp specific files.
> This way the installation function calls both of them, the update function
> calls just the second one overwriting the files that contain
> manifest-specific data.

That seems reasonable.

Keep in mind that we will want to start updating the runtime-specific files too at some point, so they stay in sync with the runtime itself.  Thus eventually we will start updating the runtime-specific files without updating the webapp-specific files.  But that's a different issue that we should tackle separately.

Also, it seems like this patch assumes update checks (and updates) are happening in the browser.  I don't think we should assume that, since some users will use apps without using the browser (or use them more often than the browser), and we should still keep their apps up-to-date.

Nevertheless, app-driven updates might need to wait until we have a solution for sharing profile data with multiple processes, since they'll require an app to write to the registry.
Attachment #782820 - Flags: feedback?(myk) → feedback+
(In reply to Myk Melez [:myk] [@mykmelez] from comment #2)
> Also, it seems like this patch assumes update checks (and updates) are
> happening in the browser.  I don't think we should assume that, since some
> users will use apps without using the browser (or use them more often than
> the browser), and we should still keep their apps up-to-date.

The update is executed when the checkForUpdate function is called (http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/apps/nsIDOMApplicationRegistry.idl#52).
This function could be called in the browser but also in the webapp runtime.

> Nevertheless, app-driven updates might need to wait until we have a solution
> for sharing profile data with multiple processes, since they'll require an
> app to write to the registry.

I think we already have this problem with the current situation. An application running in the webapp runtime could call checkForUpdate thus it'd modify the webapps registry while the browser is using it.
Priority: -- → P1
Whiteboard: DesktopWebRT2
Depends on: 749826
Depends on: 915480
Attachment #782820 - Attachment is obsolete: true
Depends on: 921103
Attached patch WIP (obsolete) — Splinter Review
For updates (and reinstalls) I'm creating a new installation in an "update" subdirectory under the installation directory.
The update is only applied when the application is executed again. Before applying the update, we back up the old files (and restore them if the update isn't applied correctly).

You can also avoid looking at the patch, I'd just like a quick feedback about this approach.
Attachment #813415 - Flags: feedback?(myk)
Comment on attachment 813415 [details] [diff] [review]
WIP

Ok, I avoided looking at the patch! ;-)

Based on your description, this seems like a reasonable approach.
Attachment #813415 - Flags: feedback?(myk) → feedback+
Attached patch WIP v2 (obsolete) — Splinter Review
Still need to test thoroughly.
Attachment #813415 - Attachment is obsolete: true
Attachment #825016 - Flags: feedback?(myk)
Comment on attachment 825016 [details] [diff] [review]
WIP v2

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

(In reply to Marco Castelluccio [:marco] from comment #6)
> Still need to test thoroughly.

Can the testing be automated?

::: toolkit/webapps/WebappsInstaller.jsm
@@ -32,5 @@
>  const HOME_DIR = OS.Constants.Path.homeDir;
>  const TMP_DIR = OS.Constants.Path.tmpDir;
>  
>  this.WebappsInstaller = {
> -  shell: null,

I like the removal of this WebappsInstaller.shell property, which seems like a footgun.

I also wonder why we make API consumers call WebappsInstaller.init() to construct a *NativeApp* object and then have WebappsInstaller.install() and WebappsInstaller.update() functions that take a *NativeApp* object as their first argument.  It seems like we could simply expose a *NativeApp* constructor (that delegates to a platform-specific constructor) with install() and update() functions.

@@ +429,5 @@
> +  update: function(aZipPath) {
> +    return Task.spawn(function() {
> +      this.installDir = WebappOSUtils.getInstallPath(this.app);
> +      if (!this.installDir) {
> +        throw("The application isn't installed");

This has me thinking about ways to ensure that the API presented by the various *NativeApp* implementations is consistent (to the extent appropriate).  At the very least, it would be handy to move messages like this, which we use in each implementation, to a shared constant.
Attachment #825016 - Flags: feedback?(myk) → feedback+
(In reply to Myk Melez [:myk] [@mykmelez] from comment #7)
> Can the testing be automated?

I think it'd be quite difficult at the moment, because we haven't any test that actually installs and executes an app.
The tests in dom/apps/tests don't install the app natively.
The tests in dom/tests/mochitest/webapps install the app natively, but never execute it.
The webapprt tests create a fake environment to execute the app, but the app is never actually installed.

Maybe there is some hacky way to have a comprehensive test (install, launch, update), I'm looking into a couple of ideas I've had.

> ::: toolkit/webapps/WebappsInstaller.jsm
> I also wonder why we make API consumers call WebappsInstaller.init() to
> construct a *NativeApp* object and then have WebappsInstaller.install() and
> WebappsInstaller.update() functions that take a *NativeApp* object as their
> first argument.  It seems like we could simply expose a *NativeApp*
> constructor (that delegates to a platform-specific constructor) with
> install() and update() functions.

I'm planning to change this part with the refactoring in bug 910473.
I've created a couple of unit tests that I think should be enough.
They cover the most "delicate" part of the update process, that is the WebappsInstaller,
but they don't test the runtime.
Depends on: 942445
Attached patch WIP v3 (obsolete) — Splinter Review
I've decided to do a small refactoring as I was there.

I've split the three platform specific NativeApp objects in three different files (just because for me it's easier to work with them this way).
I've removed WebappsInstaller, now the users of the module use NativeApp directly.

I've modified the signature of the NativeApp constructor and the NativeApp::init function (that is now called setData). Now the data that is actually needed in the constructor and the setData function is clearly defined.
In the future refactoring in bug 910473 I'd like to get rid of the setData function and actually construct the entire object in the constructor.

I can't test the patch on Mac (and probably we won't be able to enable the automated tests for Mac, see bug 924445), so there's likely still something wrong on Mac.
You should be able to run the tests and tell me the result ;)

Let me know if you like the way I refactored things or if you'd like me to do something different.
Attachment #825016 - Attachment is obsolete: true
Attachment #8340576 - Flags: review?(myk)
Comment on attachment 8340576 [details] [diff] [review]
WIP v3

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

The specialpowersAPI.js changes should get a review from a Test Harness peer like ctalbert, and the Webapps.jsm changes should get review from Fabrice.  Requesting review from them.


(In reply to Marco Castelluccio [:marco] from comment #10)
> I can't test the patch on Mac (and probably we won't be able to enable the
> automated tests for Mac, see bug 924445), so there's likely still something
> wrong on Mac.
> You should be able to run the tests and tell me the result ;)

Most tests pass, but two of them fail with the same error:

0:21.82 12 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/webapps/tests/test_hosted.xul | Error during test: Unix error 66 during operation removeEmptyDir (Directory not empty)
0:21.82 30 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/webapps/tests/test_packaged.xul | Error during test: Unix error 66 during operation removeEmptyDir (Directory not empty)


> Let me know if you like the way I refactored things or if you'd like me to
> do something different.

I mostly like it a lot!  In particular, I like putting the platform-specific implementations in different files and moving construction and some initialization into the NativeApp constructor.  I'm also a fan of eventually merging that constructor and setData.

But the inversion of the relationship between NativeApp and PlatformSpecificNativeApp is strange to me, since it makes a more general prototype inherit from a more specific one.  Also, there isn't much shared code between the platform-specific install, update, and applyUpdate functions, so it seems like over-refactoring to implement them in both the platform-specific and the common prototypes.  Finally, PlatformSpecificNativeApp is a mouthful!

So I would retain the existing relationship between the two prototypes, continuing to make the platform-specific one inherit from the common one.  But I would call the platform-specific one NativeApp, and that's the one I would export from the module.  So consumers directly instantiate a platform-specific object which then delegates to a common prototype only when it saves significant amounts of code duplication (as with the constructor, but not with install/update/applyUpdate).

One code snippet that I *would* factor out, however, is the one that checks the "dry run" preference, since that appears frequently and requires a clunky try/catch due to poor design of the prefs API.  It could be a unary getter (and memoizing, if we don't expect its value to change at runtime) that we use like this:

    if (this._dryRun) {
      return null; // or return Promise.resolve();
    }


This might also be a good time to rename WebappsInstaller.jsm (and the platform-specific files) to reflect that it does more than just install apps.  Over in the Android runtime, we're putting such code into a WebappManager.jsm module.

::: toolkit/webapps/WebappsInstaller.jsm
@@ +57,5 @@
>   *
> + * @param aApp the app object provided to the install function
> + * @param aManifest the manifest data provided by the web app
> + * @param aCategoriesArray array of app categories
> + * @param aRegistryDir (optional) path to the registry

Nit: Mozilla code generally avoids including the type of a variable in its name, and it doesn't seem to be necessary for the variable storing a list of categories, so I would call it simply aCategories.

I would also include variable types within curly brackets in these @param values, i.e.:

 * @param aData {Object} the data object provided to the install function
 * @param aApp {Object} the app object provided to the install function
 * @param aManifest {Object} the manifest data provided by the web app
 * @param aCategories {Array} app categories
 * @param aRegistryDir {String} (optional) path to the registry

::: toolkit/webapps/tests/head.js
@@ +13,5 @@
> +  return Task.spawn(function() {
> +    for (let file of files) {
> +      if (!(yield OS.File.exists(file))) {
> +        dump("TEST-INFO | File doesn't exist: " + file + "\n");
> +        throw new Task.Result(false);

Nit: use function* generators so you can simply return instead of throwing a Task.Result.  See the task I recently added to the downloadPackage function in Webapps.jsm for an example.

::: toolkit/webapps/tests/test_hosted.xul
@@ +241,5 @@
> +  let stat = yield OS.File.stat(installPath);
> +  let installTime = stat.lastModificationDate;
> +
> +  // Wait one second, otherwise the last modification date is the same.
> +  yield wait();

That seems like a long time to wait for the last modification date to change, but I guess this is a POSIX timestamp with resolution only to the nearest second?

::: webapprt/WebappRT.jsm
@@ +44,5 @@
> +                                  "webapp.json");
> +    this.config = yield AppsUtils.loadJSONAsync(webappFile);
> +  },
> +
> +  applyUpdate: function() {

Nit: it's weird to have a function called applyUpdate that doesn't necessarily apply an update.  It'd be more understandable (when reading code that calls this function) to have two functions, one of which checks to see if an update is pending and the other of which then applies it, so the consumer code looks something like:

  if (yield WebappRT.isUpdatePending()) {
    yield WebappRT.applyUpdate();
  }
Attachment #8340576 - Flags: review?(myk)
Attachment #8340576 - Flags: review?(fabrice)
Attachment #8340576 - Flags: review?(ctalbert)
Attachment #8340576 - Flags: review-
Comment on attachment 8340576 [details] [diff] [review]
WIP v3

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

::: dom/apps/src/Webapps.jsm
@@ +1447,5 @@
> +          appObject.manifest = aData;
> +          appObject.updateManifest = aUpdateManifest;
> +          let data = { app: appObject,
> +                       zipPath: appFile.path };
> +          Services.obs.notifyObservers(null, "webapps-update", JSON.stringify(data));

I'm not sure to understand why you are doing that in applyDownload();

::: webapprt/content/webapp.js
@@ -64,5 @@
>    gAppBrowser.addEventListener("click", onContentClick, false, true);
> -
> -  if (WebappRT.config.app.manifest.fullscreen) {
> -    enterFullScreen();
> -  }

Is this directly related to this bug?
Attachment #8340576 - Flags: review?(fabrice)
(In reply to Myk Melez [:myk] [@mykmelez] from comment #11)
> Most tests pass, but two of them fail with the same error:
> 
> 0:21.82 12 ERROR TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/chrome/toolkit/webapps/tests/test_hosted.xul |
> Error during test: Unix error 66 during operation removeEmptyDir (Directory
> not empty)
> 0:21.82 30 ERROR TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/chrome/toolkit/webapps/tests/test_packaged.xul |
> Error during test: Unix error 66 during operation removeEmptyDir (Directory
> not empty)

Could you send me the full log? It'd help my investigation.
Attached file log for failing test run on Mac (obsolete) —
(In reply to Marco Castelluccio [:marco] from comment #13)
> Could you send me the full log? It'd help my investigation.

Here it is, although I'm not sure how helpful it'll be.  I'll build a debug build and see if it produces a more useful log.
(In reply to Myk Melez [:myk] [@mykmelez] from comment #14)
> Here it is, although I'm not sure how helpful it'll be.  I'll build a debug
> build and see if it produces a more useful log.

I don't see anything useful in the debug test run log.
(In reply to Myk Melez [:myk] [@mykmelez] from comment #15)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #14)
> > Here it is, although I'm not sure how helpful it'll be.  I'll build a debug
> > build and see if it produces a more useful log.
> 
> I don't see anything useful in the debug test run log.

Ok, thank you anyway. I'll try to figure out the problem.

(I think there's a way to enable OS.File logging, it was enabled in try debug builds and it gave me some useful information)
Attached file debug log for failing test run on Mac (obsolete) —
Here's the debug log.  I don't see anything OS.File-related, but I'm happy to enable logging for OS.File and run the tests again if you tell me how to do so!
Comment on attachment 8340576 [details] [diff] [review]
WIP v3

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

::: testing/specialpowers/content/specialpowersAPI.js
@@ +954,5 @@
>    //
>    // The provided callback is invoked once the prompt is disabled.
>    autoConfirmAppInstall: function(cb) {
> +    this.pushPrefEnv({set: [['dom.mozApps.auto_confirm_install', true],
> +                            ['browser.mozApps.installer.dry_run', true]]}, cb);

I realize you're just adding the new preference here to this code, but I am not finding where you are using this API in any of these files. Can you explain at a high level why this is necessary here? Is this for as-yet unwritten tests? 

And, while I realize this may be an unfair question since it isn't something you changed, but why do we *not* need to reset the pref after the test runs? I would rather all tests start with the same default state w.r.t. preferences rather than having every test that runs after the one that makes this call starting with a slightly different state.
Attachment #8340576 - Flags: review?(ctalbert) → review-
Attached patch Patch (obsolete) — Splinter Review
Myk, on Mac I'm moving the zip package to Contents/Resources, I think it shouldn't be a problem because packaged apps support for desktop isn't yet advertised (and is not supported from Marketplace). It isn't really required, but I think it's better.
I haven't changed the name of the modules yet, do you like "WebappsManager.jsm"?

(In reply to Myk Melez [:myk] [@mykmelez] from comment #11)
> That seems like a long time to wait for the last modification date to
> change, but I guess this is a POSIX timestamp with resolution only to the
> nearest second?

Yes.

(In reply to Fabrice Desré [:fabrice] from comment #12)
> ::: dom/apps/src/Webapps.jsm
> @@ +1447,5 @@
> > +          appObject.manifest = aData;
> > +          appObject.updateManifest = aUpdateManifest;
> > +          let data = { app: appObject,
> > +                       zipPath: appFile.path };
> > +          Services.obs.notifyObservers(null, "webapps-update", JSON.stringify(data));
> 
> I'm not sure to understand why you are doing that in applyDownload();

applyDownload is where we're applying the download and actually updating the registry, so I thought it would be the best place to also install the update locally.

(In reply to Clint Talbert ( :ctalbert ) from comment #18)
> ::: testing/specialpowers/content/specialpowersAPI.js
> @@ +954,5 @@
> >    //
> >    // The provided callback is invoked once the prompt is disabled.
> >    autoConfirmAppInstall: function(cb) {
> > +    this.pushPrefEnv({set: [['dom.mozApps.auto_confirm_install', true],
> > +                            ['browser.mozApps.installer.dry_run', true]]}, cb);
> 
> I realize you're just adding the new preference here to this code, but I am
> not finding where you are using this API in any of these files. Can you
> explain at a high level why this is necessary here? Is this for as-yet
> unwritten tests? 

The pref isn't actually needed here. I did add it for consistency (because in DOM apps tests we're always skipping the native installation).
It isn't needed because only the WebappRuntime is registered for the "webapps-update" notification (so in mochitests, that are not run in the WebappRuntime, the "webapps-update" notification is just ignored and the native update isn't executed).

Anyway this pref is always set to true on try servers, I don't know why.

And we should change the name of these prefs, because they can be confusing (dom.mozApps.auto_confirm_install doesn't only auto confirm installs, but also implies browser.mozApps.installer.dry_run).

> And, while I realize this may be an unfair question since it isn't something
> you changed, but why do we *not* need to reset the pref after the test runs?
> I would rather all tests start with the same default state w.r.t.
> preferences rather than having every test that runs after the one that makes
> this call starting with a slightly different state.

We do reset the pref, don't we?
Attachment #8340576 - Attachment is obsolete: true
Attachment #8343820 - Attachment is obsolete: true
Attachment #8343920 - Attachment is obsolete: true
Attachment #8349717 - Flags: review?(myk)
Attachment #8349717 - Flags: review?(fabrice)
Comment on attachment 8349717 [details] [diff] [review]
Patch

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

Thanks for the great patch!  I have mostly nits, but there are a few substantial issues.


Nit: remove this trailing whitespace:

01-03 09:16 > git apply ~/check_for_updates_webapprt
/Users/myk/check_for_updates_webapprt:2055: trailing whitespace.
 * 
/Users/myk/check_for_updates_webapprt:2061: trailing whitespace.
 
/Users/myk/check_for_updates_webapprt:2473: trailing whitespace.
    let uninstallContent = 
warning: 3 lines add whitespace errors.


(In reply to Marco Castelluccio [:marco] from comment #19)

> I haven't changed the name of the modules yet, do you like
> "WebappsManager.jsm"?

Close!  I prefer WebappManager.jsm with the singular "webapp".

::: browser/modules/webappsUI.jsm
@@ +150,5 @@
>  
> +        DOMApplicationRegistry.confirmInstall(aData, localDir,
> +          (aManifest, aZipPath) => {
> +            Task.spawn(function() {
> +            try {

Nits:

* Indent the "try" line two more spaces.

* Reduce the nesting by turning the fat arrow function into a function expression, which eliminates a block (and returns the task's promise from the callback):

          (aManifest, aZipPath) => Task.spawn(function() {
            try {

* Use the clearer and more standard function* syntax to declare the generator you pass to Task.spawn:

          (aManifest, aZipPath) => Task.spawn(function*() {
            try {

* Surround the bound function with parentheses to make it more obvious that the function is bound when reading the code from top to bottom:

          (aManifest, aZipPath) => Task.spawn((function*() {
            try {
…
          }).bind(this));

@@ +188,5 @@
>  
>    }
>  }
>  
> +function installationSuccessNotification(aApp, aNativeApp, aBundle) {

Nit: since you're changing the signature of this function, I would take the opportunity to also change its name to start with a verb, which is more conventional; and also shorten it, which is easier to read; making it something like "notifyInstallSuccess".

::: dom/apps/src/AppsUtils.jsm
@@ +204,5 @@
>  #ifdef MOZ_WIDGET_GONK
>      isCoreApp = app.basePath == this.getCoreAppsBasePath();
>  #endif
>      debug(app.basePath + " isCoreApp: " + isCoreApp);
> +    return { "path":  WebappOSUtils.getPackagePath(app),

Nit: while you're here, remove the extra character of whitespace between the "path" name and value.

::: toolkit/webapps/LinuxInstaller.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// The ${InstallDir} and desktop entry filename are: sanitized app name +
> +// "-" + manifest url hash.

Nit: this comment lacks context (unlike similar comments in the other platform-specific files).  I'd move it to the place(s) where those values are defined.

@@ +6,5 @@
> +// "-" + manifest url hash.
> +
> +// Look at the CommonNativeApp constructor for a description of these
> +// parameters.
> +function NativeApp(aApp, aManifest, aCategories, aRegistryDir) {

Nit: the comments on this function are inconsistent between the platform-specific files.  This is the only one that mentions the CommonNativeApp constructor, and one of the others doesn't have a comment at all.

Also, even though the parameters are all the same, I'd still document them in each file, as it would make the code more readable.

@@ +50,5 @@
> +
> +      // If the application is already installed, this is a reinstallation.
> +      if (WebappOSUtils.getInstallPath(this.app)) {
> +        return this.update(aManifest, aZipPath);
> +      }

This resolves the install task's promise with the update task's promise rather than the update task promise's result, which means that installation will appear to complete before updating has even begun.

It should instead yield until the update task promise resolves and then return its result:

      if (WebappOSUtils.getInstallPath(this.app)) {
        return yield this.update(aManifest, aZipPath);
      }

@@ +58,5 @@
> +      this.installDir = OS.Path.join(HOME_DIR, "." + this.uniqueName);
> +
> +      let dir = getFile(TMP_DIR, this.uniqueName);
> +      dir.createUnique(Ci.nsIFile.DIRECTORY_TYPE, PERMS_DIRECTORY);
> +      this.tmpInstallDir = dir.path;

Neither installDir nor tmpInstallDir are used externally (by NativeApp instantiators), they're only ever referenced from inside the NativeApp implementation.  Is there a reason for them to be public?  If not, we should mark them as private to the implementation by prepending their names with an underscore.

I'm also concerned about the way those properties can have different values depending on whether or not the native app is installed.  And I'm concerned that there's no guarantee they will be defined before they get used, since they get set by the install, update, and applyUpdate methods but are used by other methods that could be called first.

Ok, most of those other methods are internal, and we can code this carefully to make sure we call install/update/applyUpdate before any function that references those properties.  But it'd be better to structure the code to ensure that's the case.  And at least one public method, processIcon, also references tmpInstallDir (although that method actually isn't used externally and should probably be private too).

Nevertheless, I'm uncertain how best to provide such a guarantee.  One option would be to express those properties as caching getters that provide state-specific values (and update those values when the app installation state changes).

@@ +95,5 @@
> +   * @param aManifest {Object} the manifest data provided by the web app
> +   * @param aZipPath {String} path to the zip file for packaged apps (undefined
> +   *                          for hosted apps)
> +   */
> +  update: function(aManifest, aZipPath) {

Nit: we should call this something like prepareUpdate to make it clearer that it doesn't actually update the app.

@@ +96,5 @@
> +   * @param aZipPath {String} path to the zip file for packaged apps (undefined
> +   *                          for hosted apps)
> +   */
> +  update: function(aManifest, aZipPath) {
> +    return Task.spawn(function() {

Nit: use function*() and surround the bound function with parentheses.

@@ +113,5 @@
> +      let oldUniqueName = baseName.substring(1, baseName.length);
> +      if (this.uniqueName != oldUniqueName) {
> +        // Bug 919799: If the app is still in the registry, migrate its data to
> +        // the new format.
> +        throw(ERR_UPDATES_UNSUPPORTED_OLD_NAMING_SCHEME);

Nit: there's no need to surround the thrown value with parentheses, and doing so makes it look like you're calling a function, which is not the case, so it's better to avoid using parentheses around this value.

::: toolkit/webapps/MacInstaller.js
@@ +282,5 @@
> +    function conversionDone(aSubject, aTopic) {
> +      if (aTopic == "process-finished") {
> +        deferred.resolve();
> +      } else {
> +        deferred.reject("Failure converting icon.");

Nit: when the process fails, aSubject.exitValue will be its exit code, and this rejection message should include it to provide a hint in the log about why the process failed.

@@ +287,5 @@
> +      }
> +    }
> +
> +    let process = Cc["@mozilla.org/process/util;1"].
> +                  createInstance(Ci.nsIProcess);

Nit: Cc[…].createInstance/getService(…) expressions should be formatted consistently in the new files created by this patch.  Currently, those files mix four different styles: this one:

  Cc[…].
  createInstance/getService(…)

And these three others:

  Cc[…].
    createInstance/getService(…)

  Cc[…]
  .createInstance/getService(…)

  Cc[…]
    .createInstance/getService(…)

(Of the styles, I prefer the first one, but I'm ok with any of them provided we use it consistently.)

@@ +296,5 @@
> +                "format", "icns",
> +                aIcon.path,
> +                "--out", OS.Path.join(this.tmpInstallDir, this.iconFile),
> +                "-z", "128", "128"],
> +                9, conversionDone);

Nit: the indentation of these lines seems unusual, as does the way arguments are broken across lines.  I would group arguments together logically while indenting to the opening parenthesis of the runAsync call, i.e.:

    process.runAsync(["-s", "format", "icns",
                      aIcon.path,
                      "--out", OS.Path.join(this.tmpInstallDir, this.iconFile),
                      "-z", "128", "128"],
                     9, conversionDone);

::: toolkit/webapps/WebappsInstaller.jsm
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +this.EXPORTED_SYMBOLS = ["NativeApp"];

I'm inclined to rename this file to NativeApp.jsm, since it now exposes a single NativeApp symbol, and then rename the platform-specific subscripts accordingly to LinuxNativeApp.js, MacNativeApp.js, and WinNativeApp.js.

@@ +18,5 @@
>  Cu.import("resource://gre/modules/Task.jsm");
>  Cu.import("resource://gre/modules/Promise.jsm");
>  
> +const ERR_NOT_INSTALLED = "The application isn't installed";
> +const ERR_UPDATES_UNSUPPORTED_OLD_NAMING_SCHEME = "Updates for apps installed with the old naming scheme unsupported";

Nit: wrap this line:

const ERR_UPDATES_UNSUPPORTED_OLD_NAMING_SCHEME =
  "Updates for apps installed with the old naming scheme unsupported";

@@ +65,5 @@
> +
> +  this.registryDir = aRegistryDir;
> +  if (!this.registryDir) {
> +    this.registryDir = OS.Constants.Path.profileDir;
> +  }

Nit: this could be simply:

  this.registryDir = aRegistryDir || OS.Constants.Path.profileDir;

@@ +230,5 @@
> +      return null;
> +    } catch (ex) {
> +      Cu.reportError("Error installing app: " + ex);
> +      return null;
> +    }

Installation used to fail if createDefaultProfileForApp threw an unknown exception, because createAppProfile wouldn't catch the exception, so it would propagate back to WebappsInstaller.init, which would return null to WebappsHandler.doInstall, which would call DOMApplicationRegistry.denyInstall.

But now createProfile catches the exception and returns null to WebappsHandler.doInstall, which unconditionally calls DOMApplicationRegistry.confirmInstall.  Shouldn't it only do so if createProfile returns a valid value?

::: toolkit/webapps/WinInstaller.js
@@ +197,5 @@
> +    }.bind(this));
> +  },
> +
> +  _applyTempInstallation: function() {
> +    yield moveDirectory(this.tmpInstallDir, this.installDir);

This works because _applyTempInstallation is always called from within a task, which automatically wraps the iterator returned by this generator in another task.

But it's easy to miss this implicit encapsulation (I missed it the first time I read through this patch, and I've missed it in other changes to these files).  And it feels like a footgun, since other functions that perform asynchronous operations return promises, including both those that use Task and those that use Promise directly; so that's what I expect all such "asynchronous functions" to return.

Thus I would prefer for all such async functions to return promises, either via Task or via Promise, such that they have the same behavior regardless of whether or not they are called from within a task.

(I also want sugar in Task.jsm for declaring "task methods", i.e. functions assigned to object properties whose entire implementation is encapsulated in a task; but that's a subject for another discussion.)

@@ +234,5 @@
> +      uninstallKey.open(uninstallKey.ROOT_KEY_CURRENT_USER,
> +                        "SOFTWARE\\Microsoft\\Windows\\" +
> +                        "CurrentVersion\\Uninstall",
> +                        uninstallKey.ACCESS_WRITE);
> +      if(uninstallKey.hasChild(this.uninstallSubkeyStr)) {

Nit: space between "if" and opening parenthesis.

@@ +240,5 @@
> +      }
> +    } catch (e) {
> +    } finally {
> +      if(uninstallKey)
> +        uninstallKey.close();

Nits: space between "if" and opening parenthesis; braces around conditional block.

@@ +353,5 @@
> +                uninstallContent);
> +  },
> +
> +  /**
> +   * Writes the keys to the system registry that are necessary for the app operation

Nit: wrap this line at 80 columns.

@@ +368,5 @@
> +      parentKey.open(parentKey.ROOT_KEY_CURRENT_USER,
> +                     "SOFTWARE\\Microsoft\\Windows\\CurrentVersion",
> +                     parentKey.ACCESS_WRITE);
> +      uninstallKey = parentKey.createChild("Uninstall", parentKey.ACCESS_WRITE)
> +      subKey = uninstallKey.createChild(this.uninstallSubkeyStr, uninstallKey.ACCESS_WRITE);

Nit: wrap this statement at 80 columns.

@@ +373,5 @@
> +
> +      subKey.writeStringValue("DisplayName", this.appName);
> +
> +      let uninstallerPath = OS.Path.join(this.installDir,
> +                                         this.uninstallerFile);

Nit: unwrap this statement, as it fits into 80 columns.

@@ +444,5 @@
> +   * Process the icon from the imageStream as retrieved from
> +   * the URL by getIconForApp(). This will save the icon to the
> +   * topwindow.ico file.
> +   *
> +   * @param aMimeType     ahe icon mimetype

Nit: ahe -> the

::: toolkit/webapps/tests/head.js
@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +const Cu = Components.utils;
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;

Nit: you can use destructuring assignment to simplify this!

const { classes: Cc, interfaces: Ci, utils: Cu } = Components;

@@ +12,5 @@
> +function checkFiles(files) {
> +  return Task.spawn(function*() {
> +    for (let file of files) {
> +      if (!(yield OS.File.exists(file))) {
> +        dump("TEST-INFO | File doesn't exist: " + file + "\n");

Can we use SimpleTest's *info* function to log this?

@@ +48,5 @@
> +    deferred.resolve();
> +  }, 1000);
> +
> +  return deferred.promise;
> +}

Nit: when I'm reading code that calls this function, it isn't always clear how long the code is going to wait; sometimes the call has an explanatory comment, but other times it doesn't; so I would do something to make the wait time clearer at the call site, either by renaming the function to something like waitOneSecond or by parameterizing the wait time so the callers pass the wait time to it.

::: toolkit/webapps/tests/test_hosted.xul
@@ +138,5 @@
> +      uninstallKey.open(uninstallKey.ROOT_KEY_CURRENT_USER,
> +                        "SOFTWARE\\Microsoft\\Windows\\" +
> +                        "CurrentVersion\\Uninstall",
> +                        uninstallKey.ACCESS_WRITE);
> +      if(uninstallKey.hasChild(WebappOSUtils.getUniqueName(app))) {

Nit: space between if and (.

@@ +142,5 @@
> +      if(uninstallKey.hasChild(WebappOSUtils.getUniqueName(app))) {
> +        uninstallKey.removeChild(WebappOSUtils.getUniqueName(app));
> +      }
> +    } catch (e) {
> +    } finally {

Nit: did you intend to catch the exception and ignore it?  If so, it'd be useful to have a comment here explaining why it doesn't matter.

@@ +144,5 @@
> +      }
> +    } catch (e) {
> +    } finally {
> +      if(uninstallKey)
> +        uninstallKey.close();

Nits: space between if and (, braces around block.

::: toolkit/webapps/tests/test_packaged.xul
@@ +157,5 @@
> +    } catch (e) {
> +    } finally {
> +      if(uninstallKey)
> +        uninstallKey.close();
> +    }

Same nits here as in the hosted app test: if-space-(, comment explaining why exception is caught and ignored.

::: webapprt/Startup.jsm
@@ +98,5 @@
>      }
>  
> +    let appUpdated = yield WebappRT.isUpdatePending();
> +    if (appUpdated) {
> +      yield WebappRT.applyUpdate();

Nit: the names here are confusing, because isUpdatePending and applyUpdate suggest that the update is in progress, while appUpdated suggests it has completed.

Since the update is actually in progress at this point, the first two names are the more accurate ones, so I would change "appUpdated" to something like "updatePending".

::: webapprt/WebappRT.jsm
@@ +59,5 @@
> +  },
> +
> +  applyUpdate: function() {
> +    return Task.spawn(function*() {
> +      this.config = yield AppsUtils.loadJSONAsync(OS.Path.join(updateDir, "webapp.json"));

I'm a bit uneasy about the way loadConfig loads this.config from one file while this function loads it from another, but I don't have a specific suggestion at this point for what to do differently.

@@ +73,5 @@
> +    let manifestURL = WebappRT.config.app.manifestURL;
> +    // We can't update apps that haven't a manifestURL property.
> +    if (!manifestURL) {
> +      return;
> +    }

Under what circumstances would we not have a manifestURL?  If we're expecting this, we should explain when it will happen.  Otherwise, if it's unexpected, then we should log a warning here at the very least.

@@ +91,5 @@
> +        }
> +
> +        if (!thisApp) {
> +          return;
> +        }

Here again it would be useful to see a comment saying that we expect this to happen under certain circumstances (or logging a warning that we don't understand why this is happening).
Attachment #8349717 - Flags: review?(myk) → review-
Comment on attachment 8349717 [details] [diff] [review]
Patch

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

clearing review until Myk's comment are addressed
Attachment #8349717 - Flags: review?(fabrice)
(In reply to Myk Melez [:myk] [@mykmelez] from comment #20)
> @@ +58,5 @@
> > +      this.installDir = OS.Path.join(HOME_DIR, "." + this.uniqueName);
> > +
> > +      let dir = getFile(TMP_DIR, this.uniqueName);
> > +      dir.createUnique(Ci.nsIFile.DIRECTORY_TYPE, PERMS_DIRECTORY);
> > +      this.tmpInstallDir = dir.path;
> 
> Neither installDir nor tmpInstallDir are used externally (by NativeApp
> instantiators), they're only ever referenced from inside the NativeApp
> implementation.  Is there a reason for them to be public?  If not, we should
> mark them as private to the implementation by prepending their names with an
> underscore.
> 
> I'm also concerned about the way those properties can have different values
> depending on whether or not the native app is installed.  And I'm concerned
> that there's no guarantee they will be defined before they get used, since
> they get set by the install, update, and applyUpdate methods but are used by
> other methods that could be called first.
> 
> Ok, most of those other methods are internal, and we can code this carefully
> to make sure we call install/update/applyUpdate before any function that
> references those properties.  But it'd be better to structure the code to
> ensure that's the case.  And at least one public method, processIcon, also
> references tmpInstallDir (although that method actually isn't used
> externally and should probably be private too).
> 
> Nevertheless, I'm uncertain how best to provide such a guarantee.  One
> option would be to express those properties as caching getters that provide
> state-specific values (and update those values when the app installation
> state changes).

We could make the internal functions (those called by install, update, etc.) not use |tmpInstallDir| and accept a |directory| parameter. Then in install, update, etc. we would just have a local tmpInstallDir variable.
We could make getIcon an internal function (as it should be).

About |installDir|, unlike |tmpInstallDir| to me it looks like a real property of the object, even if it is initialized in different functions. I think we should just mark it as internal. Otherwise, we could take the same approach proposed for tmpInstallDir.

What do you think?

> (I also want sugar in Task.jsm for declaring "task methods", i.e. functions
> assigned to object properties whose entire implementation is encapsulated in
> a task; but that's a subject for another discussion.)

I'd love that, I didn't wrap the functions in a task just because I don't like too much indentation :P

> ::: webapprt/WebappRT.jsm
> @@ +59,5 @@
> > +  },
> > +
> > +  applyUpdate: function() {
> > +    return Task.spawn(function*() {
> > +      this.config = yield AppsUtils.loadJSONAsync(OS.Path.join(updateDir, "webapp.json"));
> 
> I'm a bit uneasy about the way loadConfig loads this.config from one file
> while this function loads it from another, but I don't have a specific
> suggestion at this point for what to do differently.

I could read the file in a local variable in applyUpdate and in this.config in loadConfig. This would mean reading the file twice, but just on updates.
(In reply to Marco Castelluccio [:marco] from comment #22)
> We could make the internal functions (those called by install, update, etc.)
> not use |tmpInstallDir| and accept a |directory| parameter. Then in install,
> update, etc. we would just have a local tmpInstallDir variable.

This seems like a good idea.


> We could make getIcon an internal function (as it should be).

This too!


> About |installDir|, unlike |tmpInstallDir| to me it looks like a real
> property of the object, even if it is initialized in different functions. I
> think we should just mark it as internal. Otherwise, we could take the same
> approach proposed for tmpInstallDir.

Making it internal is a worthy change, but it still feels incomplete.

First, it may be a real property of the object, but it means different things in different functions, since *install* sets it to the directory into which it *intends* to install the app, while other functions set it to the directory into which the app is *already installed* (as determined by either WebappOSUtils.getInstallPath or WebappOSUtils.getLaunchTarget).

So I would also use a local variable in *install* until the installation completes successfully, after which the property has the same meaning.

Second, it's a property of the object whether or not a consumer calls any method on the object (i.e. an installed app has an installDir even if you haven't called *update*), and yet it isn't defined until one of three methods are called, which is inconsistent and a trap for a future developer who adds a method and assumes it can simply access that property.

So I would refactor the code that determines the value of installDir into a getter for the property.


> > (I also want sugar in Task.jsm for declaring "task methods", i.e. functions
> > assigned to object properties whose entire implementation is encapsulated in
> > a task; but that's a subject for another discussion.)
> 
> I'd love that, I didn't wrap the functions in a task just because I don't
> like too much indentation :P

I chatted with Yoric and paolo, who are both on board, so I'm going to file a bug and attach a patch!


> > I'm a bit uneasy about the way loadConfig loads this.config from one file
> > while this function loads it from another, but I don't have a specific
> > suggestion at this point for what to do differently.
> 
> I could read the file in a local variable in applyUpdate and in this.config
> in loadConfig. This would mean reading the file twice, but just on updates.

Hmm, I'm unsure why it requires reading the same file twice, but reading it into a local variable in applyUpdate seems like a good idea.
I've a new patch almost ready, there's a weird test failure on Windows (only test_packaged on Windows 7 Opt and Windows 8 Opt).

The problem is that the _dryRun property is true (even if the dry_run pref is set to false), indeed if I set the property manually to true the test passes.
Anyway, this is unexpected because I'm setting the pref to false before calling the NativeApp constructor (that sets _dryRun according to the pref).
I'll make a Windows release build and see if I can reproduce the failure locally.

The green try run (modified to set _dryRun "manually" to false) is here: https://tbpl.mozilla.org/?tree=Try&rev=e39df4a55542
Attached patch Patch (obsolete) — Splinter Review
Fixed the test problem (it was a timing problem, the test_hosted cleanup function was resetting the pref to true while test_packaged was being executed)
Attachment #8349717 - Attachment is obsolete: true
Attachment #8374139 - Flags: review?(myk)
Comment on attachment 8374139 [details] [diff] [review]
Patch

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

Overall looks great.  Thanks for sticking with this big and complex change!

Note: there are some trivial conflicts in Startup.jsm; otherwise, my only comments this time around are a bunch of nits, so r=myk!

This needs review from Fabrice for the dom/apps/ changes.

It also needs review from Gavin for the browser/ changes, even though browser/modules/WebappManager.jsm (née webappsUI.jsm) is informally part of the Desktop Runtime module, because the patch also touches browser/components/nsBrowserGlue.js, browser/installer/package-manifest.in, and browser/modules/moz.build.

::: dom/apps/src/Webapps.jsm
@@ +76,5 @@
>  const MIN_PROGRESS_EVENT_DELAY = 1500;
>  
>  const WEBAPP_RUNTIME = Services.appinfo.ID == "webapprt@mozilla.org";
>  
> +let chromeWindowType = WEBAPP_RUNTIME ? "webapprt:webapp" : "navigator:browser";

Nit: this never changes, so it should be const CHROME_WINDOW_TYPE.

@@ +1524,5 @@
> +          let appObject = AppsUtils.cloneAppObject(app);
> +          appObject.manifest = aData;
> +          appObject.updateManifest = aUpdateManifest;
> +          let data = { app: appObject,
> +                       zipPath: appFile.path };

Nit: I'd put this all on one line, given its small size, to make it easier to read.

::: dom/tests/browser/browser_webapps_permissions.js
@@ +62,5 @@
>      gWindow = null;
>      gBrowser.removeTab(tab);
>  
>      // The installation may have created a XUL alert window
> +    // (see WebappManager.installationSuccessNotification).

Nit: installationSuccessNotification -> notifyInstallSuccess; also, this message (like the old one) makes it sound like the function is a method of the WebappManager object, whereas it's actually just a function in the WebappManager.jsm file; so I'd reference it as "notifyInstallSuccess in WebappManager.jsm".

::: toolkit/webapps/NativeApp.jsm
@@ +366,5 @@
>   * @param destPath A path to the destination directory
>   * @param filesToIgnore An array of files. If one of those files can't be
>   *                      overwritten the function will not fail.
>   */
> +function moveDirectory(srcPath, destPath) {

Nit: you should remove the reference to @param filesToIgnore now that you've removed the parameter itself.

::: toolkit/webapps/tests/head.js
@@ +48,5 @@
> +
> +  return deferred.promise;
> +}
> +
> +SimpleTest.waitForExplicitFinish();

Is this necessary?  The two test files both call SimpleTest.waitForExplicitFinish() themselves, and it seems better for them to do it, so it's more obvious that it's been done when reading a test file (and so one can add a new test file that does a synchronous test).

::: toolkit/webapps/tests/test_hosted.xul
@@ +211,5 @@
> +
> +Services.prefs.setBoolPref("browser.mozApps.installer.dry_run", false);
> +
> +SimpleTest.registerCleanupFunction(function() {
> +  Services.prefs.setBoolPref("browser.mozApps.installer.dry_run", old_dry_run);

Nit: if the pref didn't exist, and getBoolPref threw, then it seems like it'd be better to clearUserPref than to setBoolPref to false; so I would let old_dry_run be undefined if the pref didn't exist and then clearBoolPref in that case here.

::: toolkit/webapps/tests/test_packaged.xul
@@ +154,5 @@
> +        uninstallKey = Cc["@mozilla.org/windows-registry-key;1"].
> +                       createInstance(Ci.nsIWindowsRegKey);
> +        uninstallKey.open(uninstallKey.ROOT_KEY_CURRENT_USER,
> +                          "SOFTWARE\\Microsoft\\Windows\\" +
> +                          "CurrentVersion\\Uninstall",

Nit: in theory, breaking up the string is a useful way to wrap it to 80 columns; but other strings in this file violate that convention more significantly than this line would; and if it's ok to exceed 80 columns for those lines, then it would certainly be ok to do so here.

(FWIW, I think it's reasonable to relax that requirement a bit for test files like these, on lines containing long strings, as it makes the test code easier to read.)

@@ +159,5 @@
> +                          uninstallKey.ACCESS_WRITE);
> +        if (uninstallKey.hasChild(WebappOSUtils.getUniqueName(app))) {
> +          uninstallKey.removeChild(WebappOSUtils.getUniqueName(app));
> +        }
> +      } catch (e) {

Nit: this catch block should include a comment that explains why we're catching and ignoring the exception.

@@ +178,5 @@
> +    });
> +  };
> +} else if (navigator.platform.startsWith("Mac")) {
> +  installPath = OS.Path.join(OS.Constants.Path.macLocalApplicationsDir, "Sample packaged app.app");
> +  let appProfileDir = OS.Path.join(OS.Constants.Path.macUserLibDir, "Application Support", WebappOSUtils.getUniqueName(app));

Nit: however, a line like this really pushes the boundaries of line length, as it's even longer than 120 columns!  I would write it as:

  let appProfileDir = OS.Path.join(OS.Constants.Path.macUserLibDir, "Application Support", 
                                   WebappOSUtils.getUniqueName(app));

::: webapprt/WebappRT.jsm
@@ +82,5 @@
> +  },
> +
> +  startUpdateService: function() {
> +    let manifestURL = WebappRT.config.app.manifestURL;
> +    // We used to intall apps without storing their manifest URL.

Nit: intall -> install

@@ +105,5 @@
> +
> +        // This shouldn't happen if the app is installed.
> +        if (!thisApp) {
> +          Cu.reportError("WebappRT: the app object is null during an update," +
> +                         "this shouldn't have happened");

Nit: "app object is null" would be more understandable as something like "couldn't find app in webapp registry".  Also, no need to prepend "WebappRT" to the message.  This'll only show up in places where it's obvious that it comes from the runtime rather than the app.
Attachment #8374139 - Flags: review?(myk)
Attachment #8374139 - Flags: review?(gavin.sharp)
Attachment #8374139 - Flags: review?(fabrice)
Attachment #8374139 - Flags: review+
Fabrice and I chatted a bit on IRC about the way this patch hooks into the registry.  In the interest of avoiding adding yet another observer notification, while also not blocking on the complete runtime/registry glue interface in bug 910473, let's add a DOMApplicationRegistry method for registering an update handler and make this patch use it to register its update handler.

(We can then expand this mechanism to other runtimes and other handlers incrementally, or decide to ditch it in favor of the XPCOM API in bug 910473.  But at least this gets us started on the path to a proper glue without requiring that glue to land all at once, which is a difficult lift given the number of runtimes and their rapid evolution.)
Attachment #8374139 - Flags: review?(fabrice)
Attached patch Patch (obsolete) — Splinter Review
Attachment #8374139 - Attachment is obsolete: true
Attachment #8374139 - Flags: review?(gavin.sharp)
Attachment #8383589 - Flags: review?(gavin.sharp)
Attachment #8383589 - Flags: review?(fabrice)
Comment on attachment 8383589 [details] [diff] [review]
Patch

The browser/ changes look like a trivial renaming (webappsUI.jsm->WebappManager.jsm), and it looks like there are no significant behavior changes in WebappManager.jsm or WebappManager.init() worth worrying about.

If I'm missing something that would impact non-webapprt Firefox, let me know.
Attachment #8383589 - Flags: review?(gavin.sharp) → feedback+
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #29)
> Comment on attachment 8383589 [details] [diff] [review]
> Patch
> 
> The browser/ changes look like a trivial renaming
> (webappsUI.jsm->WebappManager.jsm), and it looks like there are no
> significant behavior changes in WebappManager.jsm or WebappManager.init()
> worth worrying about.
> 
> If I'm missing something that would impact non-webapprt Firefox, let me know.

You're not missing anything!  Sorry, I should've made that more obvious.  The only reason I requested your review is because the name change from webappsUI.jsm to WebappManager.jsm triggered changes to build system files that aren't part of the runtime module and thus technically require review by a Firefox peer (unlike the changes to webappsUI.jsm/WebappManager.jsm itself, which we previously agreed can be reviewed by a runtime peer unless they're significant enough to warrant further review by a Firefox peer).
Comment on attachment 8383589 [details] [diff] [review]
Patch

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

::: dom/apps/src/Webapps.jsm
@@ +1200,5 @@
>  
> +  registerUpdateHandler: function(aHandler) {
> +    this._updateHandlers.push(aHandler);
> +  },
> +

bonus points for unregisterUpdateHandler()

@@ +1495,5 @@
>          }
>  
>          delete app.retryingDownload;
>  
> +        this._loadJSONAsync(staged.path).then((aUpdateManifest) => {

staged is moved at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1451 so how does that work?

@@ +1500,5 @@
> +          let appObject = AppsUtils.cloneAppObject(app);
> +          appObject.updateManifest = aUpdateManifest;
> +          this.notifyUpdateHandlers(appObject, aData, appFile.path);
> +        });
> +

Here you call notifyUpdateHandlers before updateAppHandlers, and do the opposite in the next chunk. I think we need to do it like in the second chunk here to.
Attachment #8383589 - Flags: review?(fabrice) → review-
Attached patch Patch (obsolete) — Splinter Review
(In reply to Fabrice Desré [:fabrice] from comment #31)
> @@ +1495,5 @@
> >          }
> >  
> >          delete app.retryingDownload;
> >  
> > +        this._loadJSONAsync(staged.path).then((aUpdateManifest) => {
> 
> staged is moved at
> https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1451
> so how does that work?

The path is updated when the file is moved (https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIFile#moveTo%28%29).
Attachment #8383589 - Attachment is obsolete: true
Attachment #8387902 - Flags: review?(fabrice)
Blocks: 981249
Attachment #8387902 - Flags: review?(fabrice) → review+
Attached patch PatchSplinter Review
Myk, what do you think? Should we land this now in 30 or the next week in 31?
Attachment #8387902 - Attachment is obsolete: true
Attachment #8391183 - Flags: checkin?(myk)
Comment on attachment 8391183 [details] [diff] [review]
Patch

It's a large patch, and the next merge is nigh.  But I just went back over the changes that are outside of the runtime code, and they're relatively small and well-contained, so the risk of regression to the rest of the codebase is low.

Plus the patch has gone through a number of rounds of review by multiple reviewers, so it feels about as seasoned as a patch of its size can be.

Thus I think we should land it, and I have gone ahead and pushed it to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ce831d82c1f
Attachment #8391183 - Flags: checkin?(myk)
As the saying goes, "famous last words!".

Actually, the fix is straightforward.
Attachment #8391697 - Flags: review?(myk)
Flags: needinfo?(mar.castelluccio)
Attachment #8391697 - Flags: review?(myk) → review+
https://hg.mozilla.org/mozilla-central/rev/e9bff15a8d19
https://hg.mozilla.org/mozilla-central/rev/c8c75db14c4e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Attached patch Patch to upliftSplinter Review
This is the really small part of the patch that I'd like to uplift to avoid the need for the transition between apps installed in 29 (that have the package in the installation directory) and apps installed in 30 (that have the package in the Resources directory, a subdirectory of the installation directory).
(In reply to Marco Castelluccio [:marco] from comment #41)
> This is the really small part of the patch that I'd like to uplift to avoid
> the need for the transition between apps installed in 29 (that have the
> package in the installation directory) and apps installed in 30 (that have
> the package in the Resources directory, a subdirectory of the installation
> directory).

Marco, can you request the uplift formally?  To do so, go to the details page for the attachment and select "?" from the approval-mozilla-beta and/or approval-mozilla-aurora dropdown menus, then fill out the form that automagically appears in the Comment field.  See bug 982559, comment 11 for an example of a verbose uplift request and bug 958709, comment 19 for a succinct one.
Flags: needinfo?(mar.castelluccio)
Comment on attachment 8398861 [details] [diff] [review]
Patch to uplift

[Approval Request Comment]
Bug caused by (feature/regressing bug #): When we landed the support for updating apps (in this bug), we've decided to install the application package on Mac in another directory.

User impact if declined: People installing packaged apps in Firefox 29, won't be able to update them.

Testing completed (on m-c, etc.): The patch has been tested manually, but is a very small subset of what we landed in Firefox 30, that has been tested manually and with new automated tests.

Risk to taking this patch (and alternatives if risky): Very low risk. The alternative is writing some code to handle the transition between 29 and 30 and uplifting to 30, but I think this is riskier because it would be new, untested, transitory code.

String or IDL/UUID changes made by this patch: None.
Attachment #8398861 - Flags: approval-mozilla-beta?
Flags: needinfo?(mar.castelluccio)
Attachment #8398861 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Could you please provide some guidance in order for the QA to verify this issue? Thanks!
Flags: needinfo?(mar.castelluccio)
(In reply to Manuela Muntean [:Manuela] [QA] from comment #45)
> Could you please provide some guidance in order for the QA to verify this
> issue? Thanks!

In Firefox 29, you should test installing and running packaged apps on all desktop platforms.
In Firefox 30, you should test updating the apps you've installed.

I guess Jason could provide sample apps that he uses to test things on b2g.
Flags: needinfo?(mar.castelluccio) → needinfo?(jsmith)
My test page for apps testing is here:

http://mozilla.github.io/qa-testcase-data/webapi/apps/
Flags: needinfo?(jsmith)
I've tested using apps from the link provided in comment 47 by installing/running them in Fx 29 and updating them in Fx 30 as mentioned in comment 46. 
Also tested with Boilerplate App.

Verification was made on Windows 7 64bit, Ubuntu 12.04 and Mac OS X 10.9 using:
#Fx 29 Beta 7, build ID: 20140410150427
#Fx 30 Aurora, build ID: 20140414004003.

The apps installed and updated properly.
Marking this verified.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Depends on: 998089
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.