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)
Core Graveyard
DOM: Apps
Tracking
(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.
Updated•10 years ago
|
blocking-b2g: --- → 2.0+
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → tclancy
Target Milestone: --- → 1.4 S6 (25apr)
Assignee | ||
Updated•10 years ago
|
Whiteboard: [systemsfe][p=8] → [systemsfe][p=3]
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8411260 -
Flags: review?(jonas)
Assignee | ||
Comment 2•10 years ago
|
||
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)
Comment 3•10 years ago
|
||
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+
Updated•10 years ago
|
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)
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
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-
Updated•10 years ago
|
feature-b2g: --- → 2.0
Updated•10 years ago
|
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
Assignee | ||
Comment 7•10 years ago
|
||
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
Comment 8•10 years ago
|
||
I think bug 1007402 is almost ready, it will remove the observer notifications.
Assignee | ||
Comment 9•10 years ago
|
||
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".
Comment 10•10 years ago
|
||
(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...
Updated•10 years ago
|
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8411260 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8432562 -
Flags: review?(jonas)
Assignee | ||
Updated•10 years ago
|
Attachment #8432563 -
Flags: review?(jonas)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8432562 -
Attachment is obsolete: true
Attachment #8432562 -
Flags: review?(jonas)
Attachment #8433395 -
Flags: review?(jonas)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8432563 -
Attachment is obsolete: true
Attachment #8432563 -
Flags: review?(jonas)
Attachment #8433396 -
Flags: review?(jonas)
Assignee | ||
Updated•10 years ago
|
Attachment #8433395 -
Flags: review?(jonas) → review?(myk)
Assignee | ||
Updated•10 years ago
|
Attachment #8433396 -
Flags: review?(jonas) → review?(myk)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8433395 -
Attachment is obsolete: true
Attachment #8433395 -
Flags: review?(myk)
Attachment #8434364 -
Flags: review?(myk)
Comment 16•10 years ago
|
||
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 17•10 years ago
|
||
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-
Updated•10 years ago
|
feature-b2g: 2.0 → 2.1
Updated•10 years ago
|
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
Comment 18•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8433396 -
Attachment is obsolete: true
Attachment #8433396 -
Flags: review?(jonas)
Assignee | ||
Updated•10 years ago
|
Attachment #8434364 -
Attachment is obsolete: true
Attachment #8434364 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 19•10 years ago
|
||
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)
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8439325 -
Flags: review?(myk)
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8439326 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8439329 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8439330 -
Flags: review?(jonas)
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8439331 -
Flags: review?(myk)
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8439333 -
Flags: review?(jonas)
Assignee | ||
Comment 26•10 years ago
|
||
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.
Assignee | ||
Comment 27•10 years ago
|
||
Attachment #8439333 -
Attachment is obsolete: true
Attachment #8439333 -
Flags: review?(jonas)
Attachment #8439478 -
Flags: review?(jonas)
Comment 28•10 years ago
|
||
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 29•10 years ago
|
||
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 30•10 years ago
|
||
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 31•10 years ago
|
||
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 32•10 years ago
|
||
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+
Updated•10 years ago
|
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
Assignee | ||
Comment 33•10 years ago
|
||
Attachment #8439325 -
Attachment is obsolete: true
Attachment #8445429 -
Flags: review?(myk)
Assignee | ||
Comment 34•10 years ago
|
||
Attachment #8439326 -
Attachment is obsolete: true
Attachment #8445430 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 35•10 years ago
|
||
Attachment #8439329 -
Attachment is obsolete: true
Attachment #8445431 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 36•10 years ago
|
||
Attachment #8439331 -
Attachment is obsolete: true
Attachment #8445433 -
Flags: review?(myk)
Assignee | ||
Comment 37•10 years ago
|
||
Attachment #8439478 -
Attachment is obsolete: true
Attachment #8439478 -
Flags: review?(jonas)
Attachment #8445434 -
Flags: review?(mounir)
Assignee | ||
Comment 38•10 years ago
|
||
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.
Assignee | ||
Comment 39•10 years ago
|
||
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.
Assignee | ||
Comment 40•10 years ago
|
||
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.
Assignee | ||
Comment 41•10 years ago
|
||
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.
Assignee | ||
Comment 42•10 years ago
|
||
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.
Assignee | ||
Comment 43•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8445431 -
Flags: review?(mark.finkle) → review+
Attachment #8439330 -
Flags: review?(jonas) → review+
Comment 44•10 years ago
|
||
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 45•10 years ago
|
||
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 46•10 years ago
|
||
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+
Updated•10 years ago
|
Target Milestone: 2.0 S5 (4july) → 2.0 S6 (18july)
Attachment #8445434 -
Flags: review?(mounir) → review+
Assignee | ||
Comment 47•10 years ago
|
||
Attachment #8445429 -
Attachment is obsolete: true
Attachment #8452640 -
Flags: review?(myk)
Comment 48•10 years ago
|
||
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+
Assignee | ||
Comment 49•10 years ago
|
||
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 50•10 years ago
|
||
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+
Assignee | ||
Comment 51•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 52•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Attachment #8456138 -
Attachment is obsolete: true
Assignee | ||
Comment 53•10 years ago
|
||
Rebased against master.
Assignee | ||
Comment 54•10 years ago
|
||
Rebased against master.
Assignee | ||
Comment 55•10 years ago
|
||
Rebased against master.
Assignee | ||
Comment 56•10 years ago
|
||
Rebased against master.
Assignee | ||
Comment 57•10 years ago
|
||
Rebased against master.
Assignee | ||
Comment 58•10 years ago
|
||
Rebased against master.
Comment 59•10 years ago
|
||
Can we wait for the next branching to land that? This week is not the best one for large refactorings.
Assignee | ||
Comment 60•10 years ago
|
||
Attachment #8457929 -
Flags: review?(kgrandon)
Comment 61•10 years ago
|
||
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)
Updated•10 years ago
|
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
Assignee | ||
Updated•10 years ago
|
Updated•10 years ago
|
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
Assignee | ||
Updated•10 years ago
|
Attachment #8411264 -
Attachment is obsolete: true
Assignee | ||
Comment 62•10 years ago
|
||
Rebased against master.
Attachment #8456168 -
Attachment is obsolete: true
Assignee | ||
Comment 63•10 years ago
|
||
Rebased against master.
Attachment #8456172 -
Attachment is obsolete: true
Assignee | ||
Comment 64•10 years ago
|
||
Rebased against master.
Attachment #8456174 -
Attachment is obsolete: true
Assignee | ||
Comment 65•10 years ago
|
||
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
Assignee | ||
Comment 69•10 years ago
|
||
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
Comment 70•10 years ago
|
||
(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 =/
Assignee | ||
Comment 71•10 years ago
|
||
Ugh, sorry. No, I meant Bug 1042797. That's the corresponding Gaia side for this.
Assignee | ||
Comment 72•10 years ago
|
||
Okay, I'm removing the checkin-needed flag until Bug 1042797 is ready to be checked in.
Keywords: checkin-needed
Updated•10 years ago
|
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
Updated•10 years ago
|
feature-b2g: 2.1 → 2.2
Target Milestone: 2.1 S3 (29aug) → ---
Assignee | ||
Comment 76•10 years ago
|
||
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 77•10 years ago
|
||
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
Assignee | ||
Comment 78•10 years ago
|
||
|windowId| is used in browser/modules/WebappManager.jsm and webapprt/WebappManager.jsm.
Comment 79•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/8e2086458237 https://hg.mozilla.org/integration/b2g-inbound/rev/1a78ae9f8b9e https://hg.mozilla.org/integration/b2g-inbound/rev/85d73ceac34b https://hg.mozilla.org/integration/b2g-inbound/rev/28284ab9811b https://hg.mozilla.org/integration/b2g-inbound/rev/cb0704cad102 https://hg.mozilla.org/integration/b2g-inbound/rev/dd7d21806888 Please fix your config to generate hg patches with the correct metadata. I had to manually fix up every patch of yours to apply properly and will just clear checkin-needed next time. https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Flags: in-testsuite+
Keywords: checkin-needed
Comment 80•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8e2086458237 https://hg.mozilla.org/mozilla-central/rev/1a78ae9f8b9e https://hg.mozilla.org/mozilla-central/rev/85d73ceac34b https://hg.mozilla.org/mozilla-central/rev/28284ab9811b https://hg.mozilla.org/mozilla-central/rev/cb0704cad102 https://hg.mozilla.org/mozilla-central/rev/dd7d21806888
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Comment 81•10 years ago
|
||
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
Comment 83•10 years ago
|
||
Looks like this made the browser_alarm.js webapprt chrome test start to fail.
Comment 84•10 years ago
|
||
Actually, every test you run after another one times out :(
Assignee | ||
Comment 85•10 years ago
|
||
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)
Comment 86•10 years ago
|
||
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)
Updated•9 years ago
|
feature-b2g: 2.2? → ---
Updated•9 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
Updated•9 years ago
|
Blocks: TV_FxOS2.5
Updated•9 years ago
|
No longer blocks: TV_FxOS2.5
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•