Closed
Bug 839068
Opened 11 years ago
Closed 11 years ago
app install manager doesn't handle the uninstall of an app
Categories
(Firefox OS Graveyard :: Gaia::System, defect)
Tracking
(b2g18+ fixed, b2g18-v1.0.1 fixed)
VERIFIED
FIXED
People
(Reporter: julienw, Assigned: julienw)
References
Details
Attachments
(2 files, 1 obsolete file)
86.85 KB,
image/png
|
Details | |
12.82 KB,
patch
|
etienne
:
review+
lsblakk
:
approval-gaia-v1+
|
Details | Diff | Splinter Review |
STR: * install an app * stop the download * uninstall the app => the app install manager still has handles on the app This means that if we install the same app again, completely this time, we might get messages both from the app install manager and the update manager for the same app. This is not blocking but I nom for tracking as this is easy to do.
Updated•11 years ago
|
Blocks: app-install
No longer depends on: app-install
Assignee | ||
Comment 1•11 years ago
|
||
another strange STR because of this : * install an app * uninstall it * reinstall it * cancel the download before the end of install => the download icons are not removed. I've seen also a case where there were 2 progress bar (maybe because we receive 2 progress events ?), and one of these stays after the cancel (see screenshot). Again, not a big deal, not something users will do often (even if some of our limitations force them to do that sometimes), and fixed by a reboot, but still this is easy to fix.
No longer blocks: app-install
Depends on: app-install
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Blocks: app-install
No longer depends on: app-install
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → felash
Assignee | ||
Comment 3•11 years ago
|
||
Apply with |git am| Correctly handle the uninstall event in the app install manager. Remove the notification and the download icon if they're displayed too. --- apps/system/js/app_install_manager.js | 9 +++ apps/system/test/unit/app_install_manager_test.js | 73 +++++++++++++++------ 2 files changed, 62 insertions(+), 20 deletions(-) The "normal" users won't see this problem much but we trigger this a lot when we're testing.
Attachment #712254 -
Flags: review?(etienne)
Assignee | ||
Updated•11 years ago
|
Flags: in-testsuite+
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Comment 4•11 years ago
|
||
Comment on attachment 712254 [details] [diff] [review] patch v1 Review of attachment 712254 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/system/test/unit/app_install_manager_test.js @@ +462,5 @@ > } > > + var dispatchInstallEvent = dispatchEvent.bind(null, 'applicationinstall'), > + dispatchUninstallEvent = dispatchEvent.bind(null, > + 'applicationuninstall'); Not blocking but |dispatchUninstallEvent()| should do something like https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/unit/update_manager_test.js#L311-314 to reflect the fact that we're not getting a full-fledged app object here.
Attachment #712254 -
Flags: review?(etienne) → review+
Assignee | ||
Comment 5•11 years ago
|
||
Comment on attachment 712254 [details] [diff] [review] patch v1 Review of attachment 712254 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/system/test/unit/app_install_manager_test.js @@ +462,5 @@ > } > > + var dispatchInstallEvent = dispatchEvent.bind(null, 'applicationinstall'), > + dispatchUninstallEvent = dispatchEvent.bind(null, > + 'applicationuninstall'); ah right, completely forgot this. Will change the test to better reflect this.
Assignee | ||
Comment 6•11 years ago
|
||
Etienne, is it better like that ?
Attachment #712254 -
Attachment is obsolete: true
Attachment #712447 -
Flags: review?(etienne)
Updated•11 years ago
|
Attachment #712447 -
Flags: review?(etienne) → review+
Assignee | ||
Comment 7•11 years ago
|
||
Comment on attachment 712447 [details] [diff] [review] patch v2 [Approval Request Comment] Bug caused by (feature/regressing bug #): n/a User impact if declined: little impact normally _but_ there are some use cases where we said "bah the user can uninstall and reinstall the app". Jason will know which ones. Also, this has an impact when we're testing because we're doing this a lot, and so we may see strange bugs because of that. Testing completed: yes Risk to taking this patch (and alternatives if risky): very low as we're not changing the logic at all on existing code paths, but merely filling a void. Also this is one of the most unit-tested part of Gaia.
Attachment #712447 -
Flags: approval-gaia-v1?
Assignee | ||
Comment 8•11 years ago
|
||
master: https://github.com/mozilla-b2g/gaia/commit/9157646edfa2065d098c3c0bd4b5cf270a84c9fa
Assignee | ||
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Comment 9•11 years ago
|
||
Comment on attachment 712447 [details] [diff] [review] patch v2 given that this is a well-tested section in the code and it helps with the uninstalling process, we'll take the uplift. let's get QA to verify that this is clearing up the issues found in known apps - will put Jason on the QA contact for this.
Attachment #712447 -
Flags: approval-gaia-v1? → approval-gaia-v1+
Updated•11 years ago
|
QA Contact: jsmith
Updated•11 years ago
|
Comment 10•11 years ago
|
||
Only need verifyme in this case if it's already landed.
Comment 11•11 years ago
|
||
v1-train: 86c89f5eee72f3c3c7a19c5d22f75e82ef0b8b4f
Assignee | ||
Comment 12•11 years ago
|
||
John, I believe the v1.0.0 flag was incorrectly set to "fixed" ?
Comment 14•11 years ago
|
||
Didn't really see any regressions with this patch, although there could potentially be an issue I'm seeing with the progress bar rendering incorrectly (size is known, but progress bar shows undetermined timeframe for download). Working on getting a STR on this.
Keywords: verifyme
Updated•11 years ago
|
Status: RESOLVED → VERIFIED
Comment 15•11 years ago
|
||
Filed bug 844219 about the issue I found in comment 14.
You need to log in
before you can comment on or make changes to this bug.
Description
•