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)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g18+ fixed, b2g18-v1.0.1 fixed)

VERIFIED FIXED
Tracking Status
b2g18 + fixed
b2g18-v1.0.1 --- fixed

People

(Reporter: julienw, Assigned: julienw)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Blocks: app-install
No longer depends on: app-install
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
Attached image screenshot
Blocks: app-install
No longer depends on: app-install
Assignee: nobody → felash
Attached patch patch v1 (obsolete) — Splinter Review
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)
Flags: in-testsuite+
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
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+
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.
Attached patch patch v2Splinter Review
Etienne, is it better like that ?
Attachment #712254 - Attachment is obsolete: true
Attachment #712447 - Flags: review?(etienne)
Attachment #712447 - Flags: review?(etienne) → review+
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?
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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+
QA Contact: jsmith
Keywords: qawanted
Only need verifyme in this case if it's already landed.
v1-train: 86c89f5eee72f3c3c7a19c5d22f75e82ef0b8b4f
John, I believe the v1.0.0 flag was incorrectly set to "fixed" ?
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
Status: RESOLVED → VERIFIED
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.