There are a lot of different cases and symptoms but at least the following STR makes this visible: * install an hosted+appcache app (for example, the app from http://everlong.org/mozilla/selfupdatinghosted/) * stop the install * reboot Expected: * the icon is in "pending mode" Actual: * the icon is in "installed mode" (this happens if you have the network) This specific problem is because we reset app.installState to "installed" in "updateHostedApp". However we do that for other properties too, most notably "downloading", "downloadSize". Asking for tef+ on that.

If the app is in a non-working state, the user will uninstall/reinstall. Not a blocker.

If there's some user impact we aren't understanding, please clarify.

I'm not exactly sure. I think the appcache will download when the user launches the app. The impact is that he thinks the app is installed whereas it's not and we'll make him download stuff when he thinks he would not. This can be problematic if the user pays his data. Is it sufficient to block ? Also, Bug 839068 will kick in in the uninstall/reinstall case.

I hit this issue while looking into Bug 838909, so I tried to fix it.

Comment on attachment 712989 [details] [diff] [review] v1 Review of attachment 712989 [details] [diff] [review]: ----------------------------------------------------------------- I don't disagree with this patch, but does this really solve this bug?

Comment on attachment 712989 [details] [diff] [review] v1 Review of attachment 712989 [details] [diff] [review]: ----------------------------------------------------------------- Right, I was talking about the part in : http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1362 until line 1366. This patch would fix the specific STR, but I think we must also fix lines 1362-1366 because otherwise we might have less visible bugs with installed apps.

Comment on attachment 713429 [details] [diff] [review] v2 Review of attachment 713429 [details] [diff] [review]: ----------------------------------------------------------------- This seems correct with the following change. I didn't had the time to try it yet (not before tomorrow) so I'm keeping the feedback? flag for now. ::: dom/apps/src/Webapps.jsm @@ +1342,5 @@ > } > > + // The installState would be changed to "updating" once we start > + // downloading the offline cache in startOfflineCacheDownload. Until > + // then, we keep its previous state. there is the same in the "onlyCheckAppCache" case

The patch should really do the same changes in the "onlyCheckAppCache" case...

Yes, I have another patch for this.

Comment on attachment 714451 [details] [diff] [review] v3 Review of attachment 714451 [details] [diff] [review]: ----------------------------------------------------------------- still didn't have time to test it on the device but this feels good (except the app.downloadAvailable change). ::: dom/apps/src/Webapps.jsm @@ -1169,5 @@ > aApp.downloading = true; > - let cacheUpdate = aProfileDir > - ? updateSvc.scheduleCustomProfileUpdate(appcacheURI, docURI, aProfileDir) > - : updateSvc.scheduleAppUpdate(appcacheURI, docURI, aApp.localId, false); > - be careful as there is "app.progress = 0" in the current code here, we don't want to remove this. Maybe it should be moved just after aApp.downloading = true. @@ +1346,5 @@ > + }, true); > + > + app.name = manifest.name; > + app.csp = manifest.csp || ""; > + app.downloadAvailable = !!manifest.appcache_path; I'd say you should remove this line. downloadAvailable will be set later in the observer. @@ +2822,5 @@ > mustSave = (app.installState != aStatus); > app.installState = aStatus; > app.progress = aProgress; > if (aStatus == "installed") { > + app.updateTime = Date.now(); this doesn't apply directly either in current code but this is easy to merge

Comment on attachment 715098 [details] [diff] [review] v4 [Approval Request Comment] User impact if declined: Comment 3 Testing completed: Local testing. Risk to taking this patch (and alternatives if risky): Low risk. String or UUID changes made by this patch: None

Comment on attachment 715098 [details] [diff] [review] v4 Approving for v1-train uplift, low risk and helps with network data usage.