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

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: vladmaniac, Assigned: vladmaniac)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lib])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
We need a method to cancel an add-on installation under addons.js shared module.
(Assignee)

Updated

6 years ago
Assignee: nobody → vlad.mozbugs
Status: NEW → ASSIGNED
Whiteboard: [mozmill-shared-modules]
(Assignee)

Updated

6 years ago
Blocks: 710143
(Assignee)

Comment 1

6 years ago
See https://bugzilla.mozilla.org/show_bug.cgi?id=710143#c7 for further details
(Assignee)

Comment 2

6 years ago
Created attachment 593797 [details] [diff] [review]
patch v1.0 - initial patch

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?
(Assignee)

Comment 4

6 years ago
(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-
(Assignee)

Comment 6

6 years ago
Created attachment 594968 [details] [diff] [review]
patch v1.1

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+
(Assignee)

Comment 8

6 years ago
Created attachment 594991 [details] [diff] [review]
fix patch v1.2

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+
Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/ce360cfa699c (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/3a7cc3324fd1 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/3f179098a5ca (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/149d8e373cc8 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/2984713a45df (esr10)
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
OS: Linux → All
Hardware: x86 → All
Resolution: --- → FIXED
Component: Mozmill Shared Modules → Mozmill Tests
Whiteboard: [lib]
You need to log in before you can comment on or make changes to this bug.