Closed Bug 723470 Opened 12 years ago Closed 12 years ago

Add a method to cancel add-on installs in addons.js shared module

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: vladmaniac, Assigned: vladmaniac)

References

Details

(Whiteboard: [lib])

Attachments

(1 file, 2 obsolete files)

We need a method to cancel an add-on installation under addons.js shared module.
Assignee: nobody → vlad.mozbugs
Status: NEW → ASSIGNED
Whiteboard: [mozmill-shared-modules]
Blocks: 710143
Attached patch patch v1.0 - initial patch (obsolete) — Splinter Review
initial patch 

- included usage of the api function in testInstallTheme to prove helper function works 
- increased TIMEOUT constant to 7s instead of just 5s (in addonsManager.open()) 

Note: I was not able to trigger another modal dialog after cancel installation to say that the installation was cancelled. It just cancels it and the add-on is not being installed.
Attachment #593797 - Flags: review?(hskupin)
Comment on attachment 593797 [details] [diff] [review]
patch v1.0 - initial patch

Just some preliminary feedback. I think you should deal with the test in it's own patch in it's own bug after the shared module patch has landed.

Also, is the [mozmill-shared-module] whiteboard tag not redundant given that this bug is filed in the Mozmill Shared Module component?
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #3)
> Comment on attachment 593797 [details] [diff] [review]
> patch v1.0 - initial patch
> 
> Just some preliminary feedback. I think you should deal with the test in
> it's own patch in it's own bug after the shared module patch has landed.
I see your point and it makes sense. But also, we need to prove that the shared module helper 
works, so I am using it in a mozmill test (kind of to replace a unit test for the api change). Let's also get Henrik's input on this IMHO
> 
> Also, is the [mozmill-shared-module] whiteboard tag not redundant given that
> this bug is filed in the Mozmill Shared Module component?

Agreed - removed whiteboard tag
Whiteboard: [mozmill-shared-modules]
Comment on attachment 593797 [details] [diff] [review]
patch v1.0 - initial patch

>+  AddonManager.getAllInstalls(function (aInstalls) {
>+    if (aInstalls.length) {
>+      for (var i = 0; i < aInstalls.length; i++) {
>+        aInstalls[i].cancel();
>+      }

getAllInstalls always returns an array as given by the documentation on MDN. So there is no need to have a check for .length, which would even cause a strict js warning in this case. Please remove the if condition.

>+exports.cancelExtensionInstalls = cancelExtensionInstalls;

Not sure why this method name includes 'extension'. It applies to all types of add-ons. Please fix that.

>+++ b/tests/functional/restartTests/testAddons_installTheme/test1.js

Given the nature that this is toolkit code we do not have to test it. Just remove the test code from this patch and create a full patch for tests on the other bug.
Attachment #593797 - Flags: review?(hskupin) → review-
Attached patch patch v1.1 (obsolete) — Splinter Review
Fixed
Attachment #593797 - Attachment is obsolete: true
Attachment #594968 - Flags: review?(hskupin)
Comment on attachment 594968 [details] [diff] [review]
patch v1.1

>+function cancelAddonInstalls() {

Sorry missed that one. Can we please give a better name for our own API? I would propose a name like 'cancelAddonInstallations'. With it fixed, r=me.
Attachment #594968 - Flags: review?(hskupin) → review+
Attached patch fix patch v1.2Splinter Review
Nit fixed
Attachment #594968 - Attachment is obsolete: true
Attachment #594991 - Flags: review?(hskupin)
Comment on attachment 594991 [details] [diff] [review]
fix patch v1.2

Looks good. Based on the broken tests I'm going to land this for all rapid release branches and mozilla-esr10.
Attachment #594991 - Flags: review?(hskupin) → review+
Component: Mozmill Shared Modules → Mozmill Tests
Whiteboard: [lib]
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: