Closed Bug 1000315 Opened 10 years ago Closed 10 years ago

The uninstall() method in mozApps should prompt the user for confirmation

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set
normal

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
mozilla34
tracking-b2g backlog

People

(Reporter: tedders1, Assigned: tedders1)

References

Details

(Whiteboard: [systemsfe][p=3])

Attachments

(6 files, 32 obsolete files)

1.31 KB, patch
Details | Diff | Splinter Review
3.80 KB, patch
Details | Diff | Splinter Review
4.81 KB, patch
Details | Diff | Splinter Review
22.03 KB, patch
Details | Diff | Splinter Review
33.20 KB, patch
Details | Diff | Splinter Review
18.75 KB, patch
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #899994 +++

From Bug 899994, Comment 17:

When uninstall() is called, the platform should put up a prompt. We should then remove the prompt that (if I understand correctly) the current homescreen app implements itself.
blocking-b2g: --- → 2.0+
Assignee: nobody → tclancy
Target Milestone: --- → 1.4 S6 (25apr)
Whiteboard: [systemsfe][p=8] → [systemsfe][p=3]
Attached patch bug-1000315-fix-part-1.patch (obsolete) — Splinter Review
Attachment #8411260 - Flags: review?(jonas)
Attached file Bug 1000315 fix - Part 2 (obsolete) —
In addition to adding the prompt to the System app, this patch also removes the similar prompts from the Homescreen and Settings apps.
Attachment #8411264 - Flags: review?(21)
Depends on: 910473
Comment on attachment 8411264 [details] [review]
Bug 1000315 fix - Part 2

Very close but I would like to see a new version before giving r+.

It makes me happy to factorize the various prompts here :)
Attachment #8411264 - Flags: review?(21) → feedback+
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
Comment on attachment 8411260 [details] [diff] [review]
bug-1000315-fix-part-1.patch

Fabrice should review this.
Attachment #8411260 - Flags: review?(jonas) → review?(fabrice)
This shouldn't block - this is a feature, which is something we won't block on unless we're past FL & we're planning to still land the feature.
blocking-b2g: 2.0+ → backlog
Comment on attachment 8411260 [details] [diff] [review]
bug-1000315-fix-part-1.patch

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

::: dom/apps/src/Webapps.jsm
@@ +3433,3 @@
>    },
>  
> +  uninstall: function(aData, aMm) {

that's breaking the webapps actor: see https://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webapps.js#613

@@ +3456,5 @@
> +      Services.obs.notifyObservers(aMm, "webapps-ask-uninstall",
> +                                   JSON.stringify(aData));
> +#else
> +      confirmUninstall(aData);
> +#endif

That needs to work on all platform, and as we discussed on irc, to not use observer notifications anymore.
Attachment #8411260 - Flags: review?(fabrice) → review-
feature-b2g: --- → 2.0
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
After discussion with Fabrice and Gregor on IRC, it was decided that we'll have to use observer notifications for now, so I'm removing the dependence on 910473.
No longer depends on: 910473
I think bug 1007402 is almost ready, it will remove the observer notifications.
Fabrice,

Regarding the lines:

> +#ifdef MOZ_WIDGET_GONK
> +      Services.obs.notifyObservers(aMm, "webapps-ask-uninstall",
> +                                   JSON.stringify(aData));
> +#else
> +      confirmUninstall(aData);
> +#endif

You said that this needs to work on all platforms. But since we don't have a proper API available yet, and since B2G is the only platform where we need to prompt the user to confirm uninstalls, is it okay if I leave this #ifdef in for now? It should be removed later as part of 1007402.

The alternative is that I add "webapps-ask-uninstall" notifications to every platform, even though the other platforms won't do anything except respond immediately with "webapps-uninstall-granted".
(In reply to Marco Castelluccio [:marco] from comment #8)
> I think bug 1007402 is almost ready, it will remove the observer
> notifications.

Famous last words...

(In reply to Ted Clancy [:tedders1] from comment #9)
> Regarding the lines:
> 
> > +#ifdef MOZ_WIDGET_GONK
> > +      Services.obs.notifyObservers(aMm, "webapps-ask-uninstall",
> > +                                   JSON.stringify(aData));
> > +#else
> > +      confirmUninstall(aData);
> > +#endif
> 
> You said that this needs to work on all platforms. But since we don't have a
> proper API available yet, and since B2G is the only platform where we need
> to prompt the user to confirm uninstalls, is it okay if I leave this #ifdef
> in for now? It should be removed later as part of 1007402.

If we change the permission level to "privileged", that will also work in other runtimes. We don't want any privileged app eg. on Android to be able to wipe out all apps silently.

> The alternative is that I add "webapps-ask-uninstall" notifications to every
> platform, even though the other platforms won't do anything except respond
> immediately with "webapps-uninstall-granted".

They actually should prompt I think...
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
Attached patch uninstall-confirmation.patch (obsolete) — Splinter Review
Attachment #8411260 - Attachment is obsolete: true
Attachment #8432562 - Flags: review?(jonas)
Attachment #8432563 - Flags: review?(jonas)
Attached patch uninstall-confirmation.patch (obsolete) — Splinter Review
Attachment #8432562 - Attachment is obsolete: true
Attachment #8432562 - Flags: review?(jonas)
Attachment #8433395 - Flags: review?(jonas)
Attachment #8432563 - Attachment is obsolete: true
Attachment #8432563 - Flags: review?(jonas)
Attachment #8433396 - Flags: review?(jonas)
Attachment #8433395 - Flags: review?(jonas) → review?(myk)
Attachment #8433396 - Flags: review?(jonas) → review?(myk)
Attached patch uninstall-confirmation.patch (obsolete) — Splinter Review
Attachment #8433395 - Attachment is obsolete: true
Attachment #8433395 - Flags: review?(myk)
Attachment #8434364 - Flags: review?(myk)
Comment on attachment 8433396 [details] [diff] [review]
auto-uninstall-confirmation.patch

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

Nit: there's an extra whitespace character on one line you changed:

06-05 13:16 > git apply ~/auto-uninstall-confirmation.patch
/Users/myk/auto-uninstall-confirmation.patch:433: trailing whitespace.
      SpecialPowers.autoConfirmAppInstall(() => 
warning: 1 line adds whitespace errors.

Otherwise, there's just the timeout issue below.  The rest looks great.  And these are obvious, straightforward changes.

But I can only grant review for the dom/apps/ and dom/tests/mochitest/webapps/ changes.  The rest need at least a rubberstamp from someone like sicking.

::: dom/apps/tests/test_packaged_app_install.html
@@ +91,4 @@
>      SpecialPowers.autoConfirmAppInstall(PackagedTestHelper.next);
>    },
>    function() {
> +    ok(true, "autoConfirmAppUninstall");

I'm not sure what value this message has, but it's consistent with the one above it, so I guess it's ok.

::: dom/tests/mochitest/webapps/test_cross_origin.xul
@@ +103,2 @@
>      };
> +  }, 5000);

Why does this code and the code in test_getNotInstalled.xul need to wait a few seconds?  Finite timeouts that resolve problems on developer machines are notorious for causing test failures on the unexpectedly slow hardware we use in automation.

I do see timing issues when running these tests on my local machine without the timeout.  Sometimes the uninstall dialogs don't get confirmed automatically, as they should.

But I see those issues with the install dialog for Basic App 2 as well, yet that one isn't causing failures in automation.  So if you did this because you see such issues on your local machine, then perhaps that isn't a problem (or at least it's a separable problem).

Have you tried running the tests without the timeout on tryserver?
Attachment #8433396 - Flags: review?(myk)
Attachment #8433396 - Flags: review?(jonas)
Attachment #8433396 - Flags: review+
Comment on attachment 8434364 [details] [diff] [review]
uninstall-confirmation.patch

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

Note: I reviewed the entire patch, but I only have authority to grant review for the changes to the following files:

  browser/locales/en-US/chrome/browser/browser.properties
  browser/modules/WebappManager.jsm
  dom/apps/src/Webapps.js
  dom/apps/src/Webapps.jsm
  webapprt/WebappManager.jsm
  webapprt/locales/en-US/webapprt/webapp.properties

So these five will need other reviewers:

  b2g/chrome/content/shell.js
  mobile/android/chrome/content/browser.js
  mobile/android/locales/en-US/chrome/browser.properties
  mobile/android/modules/WebappManager.jsm
  toolkit/devtools/server/actors/webapps.js

I think ochameau can do the first and the last of those, so requesting review from him.

mfinkle can do the middle three, so requesting review from him on those.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +438,5 @@
> +webapps.uninstall = Uninstall
> +webapps.uninstall.accesskey = U
> +webapps.doNotUninstall = Do Not Uninstall
> +webapps.doNotUninstall.accesskey = D
> +webapps.requestUninstall = Do you want to uninstall "%1$S"?

Provide a localization note like this one for webapps.requestInstall:

> #LOCALIZATION NOTE (webapps.requestInstall) %1$S is the web app name, %2$S is the site from which the web app is installed

::: browser/modules/WebappManager.jsm
@@ +93,5 @@
> +        let win = this._getWindowForId(data.windowId);
> +        if (win && win.location.href == data.from) {
> +          this.doUninstall(data, win);
> +        }
> +        break;

the "webapps-ask-install" case no longer declares the *win* variable using a *let* statement within the *switch* block, but this "webapps-ask-uninstall" case does.  Either neither of them should, or they should both do so within case-specific blocks, i.e.:

  case "webapps-ask-uninstall": {
    let win = this._getWindowForId(data.windowId);
    if (win && win.location.href == data.from) {
      this.doUninstall(data, win);
    }
    break;
  }

@@ +217,5 @@
> +
> +    let bundle = chromeWin.gNavigatorBundle;
> +    let jsonManifest = aData.app.manifest;
> +
> +    let notification;

This early declaration before the variable's later definition isn't necessary, since *notification* isn't referenced until after it's defined (even though the callbacks that reference it are defined beforehand).

::: dom/apps/src/Webapps.jsm
@@ +2258,5 @@
>    queuedDownload: {},
>    queuedPackageDownload: {},
>  
> +  onInstallSuccessAck: function onInstallSuccessAck(aManifestURL,
> +                                                  aDontNeedNetwork) {

The DevTools team recommends no longer naming function expressions assigned to object properties, as its tools can derive their names from the properties to which they're assigned.  So we should no longer give them names (and should remove them whenever changing such an expression's property name or call signature).

@@ +3483,4 @@
>    },
>  
>    doUninstall: function(aData, aMm) {
> +    this.uninstall(aData, aMm);

Nice refactoring!

@@ +3487,4 @@
>    },
>  
> +  uninstall: function(aData, aMm) {
> +    var manifestURL = aData.manifestURL;

Use *let* instead of *var*.

@@ +3512,5 @@
> +          Services.prefs.getBoolPref(prefName)) {
> +        this.confirmUninstall(aData);
> +      } else {
> +        Services.obs.notifyObservers(aMm, "webapps-ask-uninstall",
> +                                     JSON.stringify(aData));

Note the comment below:

    // We have to clone the app object as nsIDOMApplication objects are
    // stringified as an empty object. (see bug 830376)

But DOMApplicationRegistry.getAppByManifestURL, which calls out to AppsUtils.getAppByManifestURL, now returns a mozIApplication, whose implementation doesn't appear to have this problem, so I think aData.app will stringify just fine and retain its properties when parsed in confirmInstall.

However, you should remove that comment (and the *appClone* variable we created to work around that problem) from confirmUninstall, since *app* in that function is no longer a nsIDOMApplication nor even a mozIApplication but rather a generic Object that was parsed from a JSON string.

@@ +3517,5 @@
> +      }
> +    });
> +  },
> +
> +  confirmUninstall: function(aData, aUninstallSuccessCallback) {

Instead of adding a callback function, make confirmUninstall an asynchronous function with Task.async, so it returns a promise that callers can chain to a promise resolution handler.

The only additional change you'll need to make is to wrap the function in a Task.async call and then call *yield this._saveApps();* instead of *this._saveApps.then(…)* at the end of the function.

See the implementation of confirmInstall for an example.

@@ +3567,5 @@
>      });
>    },
>  
> +  denyUninstall: function(aData, aReason) {
> +    debug("Webapps.jsm - denyUninstall");

Leave out the name of the script from this debug message, as the *debug* function includes it automagically.

@@ +3574,5 @@
> +    //   - the app to be uninstalled is not removable.
> +    //   - the user declined the confirmation
> +    if (aReason) {
> +      debug("Failed to uninstall app: " + aReason);
> +    }

This seems redundant with the previous debug message.  I would set a default aReason and then output the reason unconditionally, something like:

  denyUninstall: function(aData, aReason = "ERROR_UNKNOWN_FAILURE") {
    debug("Failed to uninstall app: " + aReason);

@@ +3575,5 @@
> +    //   - the user declined the confirmation
> +    if (aReason) {
> +      debug("Failed to uninstall app: " + aReason);
> +    }
> +    aData.mm.sendAsyncMessage("Webapps:Uninstall:Return:KO", aData);

mozApps.install reports the reason for an installation failure in its error event; shouldn't mozApps.uninstall do the same?  It should be as simple as setting aData.error to aReason (or some generic failure value if aReason is not defined).

::: mobile/android/chrome/content/browser.js
@@ +7176,5 @@
> +    let jsonManifest = aData.isPackage ? aData.app.updateManifest : aData.app.manifest;
> +    let manifest = new ManifestHelper(jsonManifest, aData.app.origin);
> +
> +    if (Services.prompt.confirm(null, Strings.browser.GetStringFromName("webapps.uninstallTitle"), manifest.name + "\n" + aData.app.origin)) {
> +      // Get a profile for the app to be installed in. We'll download everything before creating the icons.

Nit: remove this comment, which doesn't apply here (it looks like it was inadvertently copied from doInstall).

::: mobile/android/modules/WebappManager.jsm
@@ +186,5 @@
> +  askUninstall: function(aData) {
> +    DOMApplicationRegistry.registryReady.then(() => {
> +      DOMApplicationRegistry.confirmUninstall(aData);
> +    });
> +  },

This won't work, because the app's native package also needs to be uninstalled, and of course we need to prompt the user to confirm the uninstallation.

But this new web runtime implementation on Android doesn't yet support programmatically uninstalling apps, not even via the existing mozApps.mgmt.uninstall method.

So it would be sufficient to call DOMApplicationRegistry.denyUninstall here, giving something like NOT_SUPPORTED for the reason, and then implement this in a followup.

(Sidenote: I'm going to implement uninstallation in bug 1019054, so we might be able to reuse some code from that change, although that implementation won't need to do user confirmation, since it's chrome code responding to a user request).

::: toolkit/devtools/server/actors/webapps.js
@@ +24,4 @@
>    */
>  }
>  
> +function supportSystemMessages() {

Nit: the name of this function suggests that calling it will cause us to support system messages, whereas this just returns a boolean indicating whether or not we do, so I would call it supportsSystemMessages instead.

@@ +697,4 @@
>      let manifestURL = aRequest.manifestURL;
>      if (!manifestURL) {
> +      deferred.resolve({ error: "missingParameter",
> +               message: "missing parameter manifestURL" });

Nit: I would align *message* with the *error* property.

Also, errors should reject the promise, but I notice that they don't do so elsewhere in this file either, so it's better to be consistent.

@@ +713,5 @@
> +
> +    return deferred.promise;
> +  },
> +
> +  _uninstallInternal: function(aManifest, aApp) {

It seems like this duplicates a lot of code in DOMApplicationRegistry.confirmUninstall.  I wonder if we could factor that out.

@@ +737,5 @@
> +    reg._clearPrivateData(app.localId, false);
> +
> +    // Then notify observers.
> +    // We have to clone the app object as nsIDOMApplication objects are
> +    // stringified as an empty object. (see bug 830376)

As I mentioned earlier, I don't think this is the case anymore.

::: webapprt/WebappManager.jsm
@@ +81,4 @@
>        try {
>          localDir = nativeApp.createProfile();
>        } catch (ex) {
> +        DOMApplicationRegistry.denyInstall(data);

Nice catch!
Attachment #8434364 - Flags: review?(poirot.alex)
Attachment #8434364 - Flags: review?(myk)
Attachment #8434364 - Flags: review?(mark.finkle)
Attachment #8434364 - Flags: review-
feature-b2g: 2.0 → 2.1
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
Comment on attachment 8434364 [details] [diff] [review]
uninstall-confirmation.patch

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

That would be great to avoid code duplication between Webapps.jsm and toolkit/devtools/server/actors/webapps.js.
Otherwise b2g/ and toolkit/ modification looks good and works fine on device with the gaia patch.

::: toolkit/devtools/server/actors/webapps.js
@@ +697,4 @@
>      let manifestURL = aRequest.manifestURL;
>      if (!manifestURL) {
> +      deferred.resolve({ error: "missingParameter",
> +               message: "missing parameter manifestURL" });

For what looks like historical reason, we never reject actor response promises for explicit errors. If we reject the promise, the error object being passed to reject() will be wrapped in an unexpected error and change the protocol behavior.
By using resolve() we ensure that the client receive this exact same object.

@@ +713,5 @@
> +
> +    return deferred.promise;
> +  },
> +
> +  _uninstallInternal: function(aManifest, aApp) {

+1! Every time we are going to modify DOMApplicationRegistry.confirmUninstall, we will most likely forget to update this method.
Attachment #8434364 - Flags: review?(poirot.alex)
Attachment #8433396 - Attachment is obsolete: true
Attachment #8433396 - Flags: review?(jonas)
Attachment #8434364 - Attachment is obsolete: true
Attachment #8434364 - Flags: review?(mark.finkle)
Comment on attachment 8411264 [details] [review]
Bug 1000315 fix - Part 2

I made the suggested changes. I just need your r+, Vivien.
Attachment #8411264 - Flags: review?(21)
Attached patch bug-1000315-fix-part-3.patch (obsolete) — Splinter Review
Attachment #8439325 - Flags: review?(myk)
Attached patch bug-1000315-fix-part-4.patch (obsolete) — Splinter Review
Attachment #8439326 - Flags: review?(poirot.alex)
Attached patch bug-1000315-fix-part-5.patch (obsolete) — Splinter Review
Attachment #8439329 - Flags: review?(mark.finkle)
Attached patch bug-1000315-fix-part-6.patch (obsolete) — Splinter Review
Attachment #8439330 - Flags: review?(jonas)
Attached patch bug-1000315-fix-part-7.patch (obsolete) — Splinter Review
Attachment #8439331 - Flags: review?(myk)
Attached patch bug-1000315-fix-part-8.patch (obsolete) — Splinter Review
Attachment #8439333 - Flags: review?(jonas)
Thanks for your reviews, everyone. (Especially Myk. That was a big section.)

I've attached a new round of patches. These patches are split into sections so they can be reviewed and approved separately.

Myk and Vivien, I've implemented your suggestions.

OChameau, there were no changes, but I'll need you to re-approve your section.

MFinkle, I removed the MOZ_ANDROID_SYNTHAPKS stuff (as we discussed yesterday). Your section is now just a stub.

Jonas, I know you're busy for the next couple weeks, but hopefully you'll get a chance to review this. Your sections aren't complicated.

Thanks again, everyone.
Attached patch bug-1000315-fix-part-8.patch (obsolete) — Splinter Review
Attachment #8439333 - Attachment is obsolete: true
Attachment #8439333 - Flags: review?(jonas)
Attachment #8439478 - Flags: review?(jonas)
Comment on attachment 8439329 [details] [diff] [review]
bug-1000315-fix-part-5.patch

Looks fine to me
Attachment #8439329 - Flags: review?(mark.finkle) → review+
Comment on attachment 8439326 [details] [diff] [review]
bug-1000315-fix-part-4.patch

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

::: toolkit/devtools/server/actors/webapps.js
@@ +755,5 @@
> +      reg.broadcastMessage("Webapps:RemoveApp", { id: id });
> +      deferred.resolve({});
> +    }).catch((e) => {
> +      deferred.resolve({ error: e.message });
> +    });

Can you factorize this code with Webapps.jsm's confirmUninstall method?

It should be about moving:

  aData.mm.sendAsyncMessage("Webapps:Uninstall:Return:OK", aData);

from confirmUninstall() to install(). And may be also switching from aData to app as first argument.

Then from here you would only do:
  DOMApplicationRegistry.confirmUninstall(app);
Attachment #8439326 - Flags: review?(poirot.alex) → review-
Comment on attachment 8439325 [details] [diff] [review]
bug-1000315-fix-part-3.patch

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

Close!  Just a few issues and some nits.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +436,5 @@
>  webapps.install.success = Application Installed
>  webapps.install.inprogress = Installation in progress
> +webapps.uninstall = Uninstall
> +webapps.uninstall.accesskey = U
> +webapps.doNotUninstall = Do Not Uninstall

Make this "Don't uninstall" for consistency with usage elsewhere.

::: browser/modules/WebappManager.jsm
@@ +201,4 @@
>                                                       "webapps-install",
>                                                       message,
>                                                       "webapps-notification-icon",
>                                                       mainAction);

Erm, it looks like you consolidated the declaration and definition sites for *notification* here in doInstall while leaving them separate in doUninstall.

My previous review comment was actually about doUninstall.  Since doInstall isn't otherwise changing, I'd rather we leave it alone.  But if you do want to change it, then you should re-indent the arguments to match the way you've indented them in doUninstall.

Otherwise, since doInstall declares and defines *notification* separately, which I didn't realize in my first review, feel free to do the same thing in doUninstall; the consistency is probably worth it.

::: dom/apps/src/Webapps.jsm
@@ +2268,5 @@
>    // on the app before we start firing progress events.
>    queuedDownload: {},
>    queuedPackageDownload: {},
>  
> +  onInstallSuccessAck: function (aManifestURL, aDontNeedNetwork) {

Nit: remove this extraneous whitespace change.

@@ +3520,5 @@
> +    let id = app.id;
> +
> +    this._readManifests([{ id: id }]).then((aResult) => {
> +      app.manifest = aResult[0].manifest;
> +      aData.app = app;

Nit: since you're essentially rewriting uninstall, make it an asynchronous function in the process, which will enable you to un-nest the code inside this callback, i.e.:

  uninstall: Task.async(function*(aData, aMm) {

    …

    let result = yield this._readManifests([{ id: id }]);
    app.manifest = result[0].manifest;
    aData.app = app;

    …
  }),

@@ +3525,5 @@
> +
> +      let prefName = "dom.mozApps.auto_confirm_uninstall";
> +      if (Services.prefs.prefHasUserValue(prefName) &&
> +          Services.prefs.getBoolPref(prefName)) {
> +        this.confirmUninstall(aData);

aData doesn't have an *mm* property at this point, so when confirmUninstall calls `aData.mm.sendAsyncMessage("Webapps:Uninstall:Return:OK", aData)`, it'll trigger an exception.

I'm not a fan of the way the WebappManager implementations set the *mm* property on their *data* objects.  I'd rather they pass the message manager to confirmUninstall as a separate parameter.

But since they work this way currently, with confirmInstall/denyInstall both expecting *mm* to be a property of aData, we should reuse that pattern for confirmUninstall/denyUninstall, as you've done.  We just need to make sure to set aData.mm here as well.

(This is a problem for the confirmInstall calls in doInstall and doInstallPackage as well, for which I've filed bug 1026067.)

@@ +3533,5 @@
> +      }
> +    });
> +  },
> +
> +  confirmUninstall: Task.async(function*(aData, aUninstallSuccessCallback) {

Remove the aUninstallSuccessCallback parameter and make callers use the returned promise to handle uninstall success, i.e.:

  confirmUninstall: Task.async(function*(aData) {

    …

    return app;
  }),

And then in webapprt/WebappManager.jsm:

  DOMApplicationRegistry.confirmUninstall(aData).then((aApp) => {
    WebappOSUtils.uninstall(aApp);
  });

@@ +3572,5 @@
> +      aData.mm.sendAsyncMessage("Webapps:Uninstall:Return:OK", aData);
> +    } catch(ex) {
> +      Cu.reportError("DOMApplicationRegistry: Exception on app uninstall: " +
> +                     ex + "\n" + ex.stack);
> +    }

This try/catch shouldn't be necessary anymore, since it surrounds transparent internal code rather than an opaque callback function; and thus we should remove it.

(But we need to make sure to set aData.mm when calling confirmUninstall directly in *uninstall*, as I mentioned above; otherwise this statement will throw in that case!).
Attachment #8439325 - Flags: review?(myk) → review-
Comment on attachment 8439331 [details] [diff] [review]
bug-1000315-fix-part-7.patch

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

Overall looks good.  Just one remaining issue in the code itself.  And then there are these desktop runtime test failures (mach webapprt-test-chrome), most of which are due to the aData.mm issue I previously mentioned, but there's also one instance of "ReferenceError: BrowserTabList is not defined":

 2:01.04 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_alarm.js | Cleanup function threw an exception at resource://gre/modules/Webapps.jsm:3436 - TypeError: aData.mm is undefined
 2:01.05 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_debugger.js | uncaught exception - ReferenceError: BrowserTabList is not defined at chrome://webapprt/content/dbg-webapp-actors.js:50
 2:01.05 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_debugger.js | Test timed out
 2:01.05 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_debugger.js | Cleanup function threw an exception at resource://gre/modules/Webapps.jsm:3436 - TypeError: aData.mm is undefined
 2:01.05 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_download.js | Cleanup function threw an exception at resource://gre/modules/Webapps.jsm:3436 - TypeError: aData.mm is undefined
 2:01.05 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_geolocation-prompt-noperm.js | Cleanup function threw an exception at resource://gre/modules/Webapps.jsm:3436 - TypeError: aData.mm is undefined
 2:01.05 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_geolocation-prompt-perm.js | Cleanup function threw an exception at resource://gre/modules/Webapps.jsm:3436 - TypeError: aData.mm is undefined
 2:01.05 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_getUserMedia.js | Cleanup function threw an exception at resource://gre/modules/Webapps.jsm:3436 - TypeError: aData.mm is undefined
 2:01.05 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_mozpay.js | Cleanup function threw an exception at resource://gre/modules/Webapps.jsm:3436 - TypeError: aData.mm is undefined
 2:01.05 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_noperm.js | Cleanup function threw an exception at resource://gre/modules/Webapps.jsm:3436 - TypeError: aData.mm is undefined
 2:01.05 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_sample.js | Cleanup function threw an exception at resource://gre/modules/Webapps.jsm:3436 - TypeError: aData.mm is undefined
 2:01.05 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_webperm.js | Cleanup function threw an exception at resource://gre/modules/Webapps.jsm:3436 - TypeError: aData.mm is undefined
 2:01.05 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_window-open-blank.js | Cleanup function threw an exception at resource://gre/modules/Webapps.jsm:3436 - TypeError: aData.mm is undefined
 2:01.05 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_window-open-self.js | Cleanup function threw an exception at resource://gre/modules/Webapps.jsm:3436 - TypeError: aData.mm is undefined
 2:01.05 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_window-open.js | Cleanup function threw an exception at resource://gre/modules/Webapps.jsm:3436 - TypeError: aData.mm is undefined
 2:01.05 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_window-title.js | Cleanup function threw an exception at resource://gre/modules/Webapps.jsm:3436 - TypeError: aData.mm is undefined
06-16 13:16 >

::: dom/tests/mochitest/webapps/test_getNotInstalled.xul
@@ +134,5 @@
> +    window.navigator.mozApps.mgmt.uninstall(app).onsuccess = function onUninstall() {
> +      app = null;
> +      next();
> +    }
> +  }, 5000);

This is still an issue.  Here's what I wrote in the previous review:

> ::: dom/tests/mochitest/webapps/test_cross_origin.xul
> @@ +103,2 @@
> >      };
> > +  }, 5000);
> 
> Why does this code and the code in test_getNotInstalled.xul need to wait a
> few seconds?  Finite timeouts that resolve problems on developer machines
> are notorious for causing test failures on the unexpectedly slow hardware we
> use in automation.
> 
> I do see timing issues when running these tests on my local machine without
> the timeout.  Sometimes the uninstall dialogs don't get confirmed
> automatically, as they should.
> 
> But I see those issues with the install dialog for Basic App 2 as well, yet
> that one isn't causing failures in automation.  So if you did this because
> you see such issues on your local machine, then perhaps that isn't a problem
> (or at least it's a separable problem).
> 
> Have you tried running the tests without the timeout on tryserver?
Attachment #8439331 - Flags: review?(myk) → review-
Comment on attachment 8411264 [details] [review]
Bug 1000315 fix - Part 2

r+ if you fix the behavior in the settings app. My understanding of the current code is that this.back() should be called only if the app has been uninstalled.
Attachment #8411264 - Flags: review?(21) → review+
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
Attached patch bug-1000315-fix-part-3.patch (obsolete) — Splinter Review
Attachment #8439325 - Attachment is obsolete: true
Attachment #8445429 - Flags: review?(myk)
Attached patch bug-1000315-fix-part-4.patch (obsolete) — Splinter Review
Attachment #8439326 - Attachment is obsolete: true
Attachment #8445430 - Flags: review?(poirot.alex)
Attached patch bug-1000315-fix-part-5.patch (obsolete) — Splinter Review
Attachment #8439329 - Attachment is obsolete: true
Attachment #8445431 - Flags: review?(mark.finkle)
Attached patch bug-1000315-fix-part-7.patch (obsolete) — Splinter Review
Attachment #8439331 - Attachment is obsolete: true
Attachment #8445433 - Flags: review?(myk)
Attached patch bug-1000315-fix-part-8.patch (obsolete) — Splinter Review
Attachment #8439478 - Attachment is obsolete: true
Attachment #8439478 - Flags: review?(jonas)
Attachment #8445434 - Flags: review?(mounir)
Comment on attachment 8445429 [details] [diff] [review]
bug-1000315-fix-part-3.patch

Hi, Myk. I refactored how uninstall works in Webapps.jsm.

It no longer needs to rely on the *mm* property being set in *aData*. (So hopefully we can remove that soon.) It also uses async functions, and returns promises.

I think it's overall cleaner and more robust now, though still slightly convoluted.
Comment on attachment 8445430 [details] [diff] [review]
bug-1000315-fix-part-4.patch

Hi, Alexandre. I refactored how uninstall() works in Webapps.jsm, so I don't need to duplicate the code in toolkit anymore.
Comment on attachment 8445431 [details] [diff] [review]
bug-1000315-fix-part-5.patch

Hi, Mark. DOMApplicationRegistry.uninstall() now returns a Promise, so there's a very slight change to the patch for Android.
Comment on attachment 8439330 [details] [diff] [review]
bug-1000315-fix-part-6.patch

Hi Jonas. You should be able to review this patch, as it's very similar to similar code you reviewed for Bobby Holley on 4 Jan 2014. Let me know if that's not the case.
Comment on attachment 8445433 [details] [diff] [review]
bug-1000315-fix-part-7.patch

Hi again, Myk. I was able to remove the 5000ms delays. I found the source of the timing problem. The fix is in this patch in dom/tests/mochitest/webapps/head.js.
Comment on attachment 8445434 [details] [diff] [review]
bug-1000315-fix-part-8.patch

Hi Mounir. I'm assigning this patch to you because I see you've reviewed similar code before. Uninstalling apps now prompts the user for confirmation, so this patch skips the confirmation for our automated tests.
Attachment #8445431 - Flags: review?(mark.finkle) → review+
Comment on attachment 8445430 [details] [diff] [review]
bug-1000315-fix-part-4.patch

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

Looks good, thanks!
Attachment #8445430 - Flags: review?(poirot.alex) → review+
Comment on attachment 8445429 [details] [diff] [review]
bug-1000315-fix-part-3.patch

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

(In reply to Ted Clancy [:tedders1] from comment #38)
> Comment on attachment 8445429 [details] [diff] [review]
> bug-1000315-fix-part-3.patch
> 
> Hi, Myk. I refactored how uninstall works in Webapps.jsm.
> 
> It no longer needs to rely on the *mm* property being set in *aData*. (So
> hopefully we can remove that soon.) It also uses async functions, and
> returns promises.

I like it!  It hides the API complexity of sending a message and receiving a function call, so the code that prompts the user can issue a single asynchronous function call and await the response.  Async functions FTW!

It does, however, expose DOMApplicationRegistry to the risk of leaking the message manager.  Read on for the details and a suggestion for how to eliminate that risk.

::: browser/modules/WebappManager.jsm
@@ +78,5 @@
>    },
>  
>    observe: function(aSubject, aTopic, aData) {
>      let data = JSON.parse(aData);
>      data.mm = aSubject;

Nit: here and in the other WebappManager, since uninstallation no longer uses the message manager, I would set data.mm only for the cases that still use it to avoid confusing future hackers (even though it does no harm to set it unconditionally).

::: dom/apps/src/Webapps.jsm
@@ +157,5 @@
>    children: [ ],
>    allAppsLaunchable: false,
>    _updateHandlers: [ ],
> +  _pendingUninstalls: {},
> +  _nextUninstallId: 0,

The request already has a unique ID in the aData.requestID property, so we should be able to reuse that ID to store a reference to the pending uninstall.

@@ +3499,5 @@
>  
>      throw aError;
>    },
>  
> +  _getAppWithManifest: Task.async(function*(aManifestURL) {

Nit: I would put this below getAppByManifestURL.

@@ +3506,5 @@
> +      throw "NO_SUCH_APP";
> +    }
> +
> +    let result = yield this._readManifests([{ id: app.id }]);
> +    app.manifest = result[0].manifest;

Also note DOMApplicationRegistry.getManifestFor, which would do this for you (although it behaves a bit differently).

@@ +3511,5 @@
> +
> +    return app;
> +  }),
> +
> +  uninstall: function(aManifestURL) {

Nit: I'd keep "uninstall" below doUninstall (and right above _uninstallApp).

@@ +3513,5 @@
> +  }),
> +
> +  uninstall: function(aManifestURL) {
> +    return this._getAppWithManifest(aManifestURL)
> +      .then((app) => this._uninstallApp(app));

Nit: you could avoid the intermediate anonymous function by binding _uninstallApp to its "this" object:

    return this._getAppWithManifest(aManifestURL)
      .then(this._uninstallApp.bind(this));

@@ +3520,5 @@
> +  doUninstall: Task.async(function*(aData, aMm) {
> +    try {
> +      let app = yield this._getAppWithManifest(aData.manifestURL);
> +      aData.app = app;
> +  

Nit: trailing whitespace.

@@ +3526,5 @@
> +      if (Services.prefs.prefHasUserValue(prefName) &&
> +          Services.prefs.getBoolPref(prefName)) {
> +        yield this._uninstallApp(app);
> +      } else {
> +        yield this.promptForUninstall(aData, aMm);

A problem with this approach is that it exposes DOMApplicationRegistry to the risk of leaking the message manager, and therefore the entire page that requested the uninstall, if promptForUninstall never resolves, since the manager holds a reference to the page.

It's possible that could happen if the user doesn't respond to the prompt (which is non-modal on desktop, although it's implementation might prevent this from happening) and then closes the tab; or it could happen because of a bug in the runtime.

(The install prompt could have the same problem, but in that case it's entirely the runtime's problem, since DOMApplicationRegistry doesn't retain a reference to the message manager while it's waiting for the runtime to confirm the install.)

One way to solve this problem would be for doInstall to be a regular (synchronous) function that passes a weak reference to the message manager into another, asynchronous function that then awaits the prompt, something like:

  doInstall: function(aData, aMm) {
    …
    this._askInstall(aData, Cu.getWeakReference(aMm));
    // This now returns, so its scope is GCed.
  },

  _askInstall: Task.async(function*(aData, aMmWeakRef) {
    …
    yield this.promptForUninstall(aData);
    // The yield could get stuck forever, but the scope holds only
    // a weak reference to the message manager at this point.
    // If it does return, then we can retrieve the manager and use it
    // if it's still defined.
    let aMm = aMmWeakRef.get();
    if (aMm) {
      …
    }
  }),

(At this point I would also consider whether _askInstall and promptForInstall could be combined into a single function, which seems like it should be doable but would require more thought.)

@@ +3543,2 @@
>        debug("Error: cannot uninstall a non-removable app.");
> +      throw "NON_REMOVABLE_APP";

Nit: here and elsewhere, throw new Error("NON_REMOVABLE_APP") to propagate more information about the error.

@@ +3579,5 @@
> +
> +    return aApp;
> +  }),
> +
> +  promptForUninstall: Task.async(function*(aData, aMm) {

This doesn't actually need to be an asynchronous function, since it never yields to another asynchronous function call.  Thus it can simply be a regular function and "return deferred.promise":

  promptForUninstall: function(aData, aMm) {
    let deferred = Promise.defer();
    …
    return deferred.promise;
  },

Also, shouldn't this be private (i.e. have a leading underscore in its name)?

@@ +3592,5 @@
> +      }),
> +      deny: Task.async(function*(aReason) {
> +        yield deferred.reject(aReason)
> +      })
> +    };

Nit: pendingUninstall.deny doesn't need to be an asynchronous function either, since "deferred.reject(aReason)" isn't an asynchronous function call.

But instead of doing this confirm/deny work here in promptForUninstall, I would do it all in the confirmInstall and denyInstall functions by storing only the deferred:

  promptForUninstall: function(aData) {
    let deferred = Promise.defer();
    this._pendingUninstalls[aData.requestID] = deferred;
    Services.obs.notifyObservers(null, "webapps-ask-uninstall",
                                 JSON.stringify(aData));
    return deferred.promise;
  },

Then confirmUninstall can call _uninstallApp itself before retrieving and resolving the deferred, while denyUninstall can simply retrieve and reject the deferred.

@@ +3598,5 @@
> +    let uninstallId = this._nextUninstallId++;
> +    this._pendingUninstalls[uninstallId] = pendingUninstall;
> +
> +    aData.uninstallId = uninstallId;
> +    Services.obs.notifyObservers(aMm, "webapps-ask-uninstall",

Since uninstall observers no longer need the message manager, we should simply pass a null object as the subject.

@@ +3613,5 @@
> +    }
> +    return Promise.reject("PENDING_UNINSTALL_NOT_FOUND");
> +  },
> +
> +  denyUninstall: function({uninstallId: uninstalLId}, aReason = "ERROR_UNKNOWN_FAILURE") {

uninstalLId -> uninstallId

Also, you can shorten the uninstallId object destructuring, since the name of the variable to assign is the same as the name of the property whose value is being assigned to it:

  denyUninstall: function({ uninstallId }, aReason = "ERROR_UNKNOWN_FAILURE") {
Attachment #8445429 - Flags: review?(myk) → review-
Comment on attachment 8445433 [details] [diff] [review]
bug-1000315-fix-part-7.patch

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

::: dom/tests/mochitest/webapps/head.js
@@ +29,5 @@
>    }
>    next();
>  }
>  
>  function confirmNextInstall() {

Nit: maybe we should rename this to confirmNextPopup, as it's a bit strange to see confirmNextInstall calls followed by uninstall calls.

@@ +44,5 @@
>  
>    function onPopupShown() {
>      popupPanel.removeEventListener("popupshown", onPopupShown, false);
>      SpecialPowers.wrap(this).childNodes[0].button.doCommand();
> +    popupNotifications._dismiss();

Nice catch!  Although I don't quite understand why this is needed, since PopupNotifications._onButtonCommand seems to call either _dismiss or _remove, but not both, and the button callbacks both call notification.remove already.

But _onButtonCommand also calls _update, which can call _dismiss, and we don't call that here.  Perhaps this should call _update instead of (or in addition to) _dismiss?
Attachment #8445433 - Flags: review?(myk) → review+
Target Milestone: 2.0 S5 (4july) → 2.0 S6 (18july)
Attached patch bug-1000315-fix-part-3.patch (obsolete) — Splinter Review
Attachment #8445429 - Attachment is obsolete: true
Attachment #8452640 - Flags: review?(myk)
Comment on attachment 8452640 [details] [diff] [review]
bug-1000315-fix-part-3.patch

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

Note that a test failure occurs with this patch when running "mach mochitest dom/apps/tests":

 1:39.57 43 INFO TEST-UNEXPECTED-FAIL | /tests/dom/apps/tests/test_app_update.html | uncaught exception - TypeError: app.manifest is undefined at http://mochi.test:8888/tests/dom/apps/tests/test_app_update.html:271

But perhaps this is fixed by the "part 7" patch? That patch doesn't apply cleanly at this point, so I can't confirm it.

Otherwise, this looks great, but note the comment below about combining doInstall with _askInstall to eliminate the possibility of a leak.  r=myk with that change.

::: dom/apps/src/Webapps.jsm
@@ +3556,5 @@
> +
> +      let prefName = "dom.mozApps.auto_confirm_uninstall";
> +      if (Services.prefs.prefHasUserValue(prefName) &&
> +          Services.prefs.getBoolPref(prefName)) {
> +        yield this._uninstallApp(app);

Nit: if this reference to "app" were instead a reference to "aData.app", you could avoid creating "app" entirely and simply assign the yielded value of _getAppWithManifest to aData.app directly:

      aData.app = yield this._getAppWithManifest(aData.manifestURL);
      …
        yield this._uninstallApp(aData.app);

@@ +3570,3 @@
>  
> +    aMm.sendAsyncMessage("Webapps:Uninstall:Return:OK", aData);
> +  }),

Hmm, any asynchronous operation makes it possible to leak the message manager, including the two yielded function calls here in doUninstall.  And we should avoid that possibility, however slim.

So I would do this a bit differently.  Instead of passing a weak reference to _askInstall, combine doUninstall and _askInstall, replacing the strong reference with a weak one while doing the asynchronous work, i.e.:

  doUninstall: Task.async(function*(aData, aMm) {
    // The yields here could get stuck forever, so we only hold
    // a weak reference to the message manager while yielding, to avoid
    // leaking the whole page associated with the message manager.
    aMm = Cu.getWeakReference(aMm);

    try {
      let app = yield this._getAppWithManifest(aData.manifestURL);
      aData.app = app;

      let prefName = "dom.mozApps.auto_confirm_uninstall";
      if (Services.prefs.prefHasUserValue(prefName) &&
          Services.prefs.getBoolPref(prefName)) {
        yield this._uninstallApp(app);
      } else {
        yield this._promptForUninstall(aData);
      }

      if (aMm = aMm.get()) {
        aMm.sendAsyncMessage("Webapps:Uninstall:Return:OK", aData);
      }
    } catch (error) {
      if (aMm = aMm.get()) {
        aData.error = error;
        aMm.sendAsyncMessage("Webapps:Uninstall:Return:KO", aData);
      }
    }
  }),

This has the added benefit that it yields to all the asynchronous function calls in doInstall, which further reduces the risk of a race condition (a problem that has been plaguing this code lately).

@@ +3573,5 @@
>  
> +  _askUninstall: Task.async(function*(aData, aMmWeakRef) {
> +    // This yield could get stuck forever, so we only hold
> +    // a weak reference to the message manager, to avoid
> +    // leaking the whole message manager.

Nit: whole message manager -> whole page associated with the message manager

@@ +3994,5 @@
> +      throw new Error("NO_SUCH_APP");
> +    }
> +
> +    let result = yield this._readManifests([{ id: app.id }]);
> +    app.manifest = result[0].manifest;

Nit: this could be a one-liner (albeit a complex one) and avoid a temporary variable:

    app.manifest = (yield this._readManifests([{ id: app.id }]))[0].manifest;

::: webapprt/WebappManager.jsm
@@ +114,5 @@
> +      null,
> +      null,
> +      {});
> +
> +    // Perform the install if the user allows it

Nit: install -> uninstall.
Attachment #8452640 - Flags: review?(myk) → review+
Attached patch bug-1000315-fix-part-9.patch (obsolete) — Splinter Review
Hi, Myk. You're right, this is better. 

I slightly rearranged your fix so that the weak-reference is only dereferenced in one place. So, in the unlikely event that sendAsyncMessage throws an exception, we don't attempt to do |aMm = aMm.get()| twice.

This patch has to be applied on top of bug-1000315-fix-part-3.patch.
Attachment #8454664 - Flags: review?(myk)
Comment on attachment 8454664 [details] [diff] [review]
bug-1000315-fix-part-9.patch

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

Looks great, r=myk!  Let's ship it!
Attachment #8454664 - Flags: review?(myk) → review+
This patch combines the patches that were previously reviewed separately. It's been rebased against master, so it should merge cleanly.
Attachment #8439330 - Attachment is obsolete: true
Attachment #8445430 - Attachment is obsolete: true
Attachment #8445431 - Attachment is obsolete: true
Attachment #8445433 - Attachment is obsolete: true
Attachment #8445434 - Attachment is obsolete: true
Attachment #8452640 - Attachment is obsolete: true
Attachment #8454664 - Attachment is obsolete: true
Keywords: checkin-needed
1.) You need to include a Try link when requesting checkin per the dev-platform post from 2 months ago.
2.) From the Try link you gave me over IRC, it's not clear whether the Mnw failure is expected or not (https://tbpl.mozilla.org/php/getParsedLog.php?id=43825450&tree=Try). Multi-tree backouts aren't fun, so it would be good to know exactly what failures to expect from the Gecko patch w/o the Gaia patch.
3.) The attached patch is missing commit information.
Keywords: checkin-needed
Attachment #8456138 - Attachment is obsolete: true
Rebased against master.
Attached patch bug-1000315-fix-part-4.patch (obsolete) — Splinter Review
Rebased against master.
Attached patch bug-1000315-fix-part-5.patch (obsolete) — Splinter Review
Rebased against master.
Rebased against master.
Attached patch bug-1000315-fix-part-7.patch (obsolete) — Splinter Review
Rebased against master.
Attached patch bug-1000315-fix-part-8.patch (obsolete) — Splinter Review
Rebased against master.
Can we wait for the next branching to land that? This week is not the best one for large refactorings.
Attachment #8457929 - Flags: review?(kgrandon)
Comment on attachment 8457929 [details] [review]
Bug 1000315 - Part 10: Fixing gaia integration tests. r=kgrandon

Nice job! I don't think landing to master with two confirm dialogs is a good approach though, so let's get a patch for that first. Or I can give an R+ on this one as part of a bigger group of patches. Thanks!
Attachment #8457929 - Flags: review?(kgrandon)
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
Blocks: 899994
No longer depends on: 899994
Blocks: 1042797
No longer blocks: 1042797
Depends on: 1042797
Blocks: 1042807
No longer blocks: 1042807
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
Attachment #8411264 - Attachment is obsolete: true
Attached patch bug-1000315-fix-part-3.patch (obsolete) — Splinter Review
Rebased against master.
Attachment #8456168 - Attachment is obsolete: true
Rebased against master.
Attachment #8456172 - Attachment is obsolete: true
Attached patch bug-1000315-fix-part-7.patch (obsolete) — Splinter Review
Rebased against master.
Attachment #8456174 - Attachment is obsolete: true
Comment on attachment 8457929 [details] [review]
Bug 1000315 - Part 10: Fixing gaia integration tests. r=kgrandon

This patch was moved to Bug 1042797.
Attachment #8457929 - Attachment is obsolete: true
Updated.
Attachment #8456170 - Attachment is obsolete: true
Attached patch bug-1000315-fix-part-7.patch (obsolete) — Splinter Review
Updated.
Attachment #8474740 - Attachment is obsolete: true
Attached patch bug-1000315-fix-part-3.patch (obsolete) — Splinter Review
Updated.
Attachment #8474737 - Attachment is obsolete: true
Here's a green TBPL run: https://tbpl.mozilla.org/?tree=Try&rev=dff01292e871

Warning: Checking this in might temporarily break a few Gaia tests until Bug 1050091 lands.
Keywords: checkin-needed
(In reply to Ted Clancy [:tedders1] from comment #69)
> Warning: Checking this in might temporarily break a few Gaia tests until Bug
> 1050091 lands.

Is that the right bug? We would rather not have to break gaia tests if we can avoid it =/
Ugh, sorry. No, I meant Bug 1042797. That's the corresponding Gaia side for this.
Okay, I'm removing the checkin-needed flag until Bug 1042797 is ready to be checked in.
Keywords: checkin-needed
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
feature-b2g: 2.1 → 2.2
Target Milestone: 2.1 S3 (29aug) → ---
Rebase
Attachment #8476074 - Attachment is obsolete: true
Rebase
Attachment #8476073 - Attachment is obsolete: true
Rebase
Attachment #8456176 - Attachment is obsolete: true
This needs to land at the same time as as Bug 1042797.

Here's a successful TBPL run: https://tbpl.mozilla.org/?tree=Try&rev=8e5e25d7365c
Keywords: checkin-needed
Comment on attachment 8477900 [details] [diff] [review]
bug-1000315-fix-part-3.patch

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

::: dom/apps/src/Webapps.js
@@ +727,5 @@
> +      origin: aApp.origin,
> +      manifestURL: aApp.manifestURL,
> +      oid: this._id,
> +      from: this._window.location.href,
> +      windowId: this._windowId,

what is this for? I didn't find any use in Webapps.jsm
|windowId| is used in browser/modules/WebappManager.jsm and webapprt/WebappManager.jsm.
This is currently causing Gij to go perma-red. We will either need to fix it tonight, or backout in conjunction with bug 1055427 and bug 1042797.
Depends on: 1058362
Use feature-b2g:2.2? rather than just 2.2.
feature-b2g: 2.2 → 2.2?
Looks like this made the browser_alarm.js webapprt chrome test start to fail.
Actually, every test you run after another one times out :(
Depends on: 1072798
Hey Marco. 

> Actually, every test you run after another one times out :(

I'm not quite sure what you mean. Is there are particular test that causes all subsequent tests to fail?
Flags: needinfo?(mar.castelluccio)
I think this has regressed the way we were running webapprt chrome tests, so every time you run more than one test the first succeeds and the others fail.

To run webapprt chrome test, you can do: ./mach webapprt-test-chrome
Flags: needinfo?(mar.castelluccio)
Blocks: 1092726
feature-b2g: 2.2? → ---
blocking-b2g: backlog → ---
Blocks: TV_FxOS2.5
No longer blocks: TV_FxOS2.5
Depends on: 1194243
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.