Closed Bug 572561 Opened 14 years ago Closed 14 years ago

Addons Manager hides Language category if there are no locale packs installed but some are pending install

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla2.0b1
Tracking Status
blocking2.0 --- final+

People

(Reporter: bparr, Assigned: bparr)

References

Details

(Whiteboard: [AddonsRewrite])

Attachments

(2 files, 9 obsolete files)

Steps to repoduce:
1) Have Addons Manager open with no language packs installed
2) Install a language pack (http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central-l10n/). The language category becomes visible and shows the installing language pack.
3) Refresh the Addons Manager. The langague category is now hidden.

The language pack is still installed when Firefox is restarted.

This is just a problem with the detection of empty categories; it only checks for Addons and not AddonInstalls.
We're going to be disabling the refresh button when the add-ons manager is open anyway so I'm not sure this is a big deal
Depends on: 553771
Whiteboard: [rewrite]
Refreshing is just a clear way to see the problem. The bug will occur any time the Addons Manager initialization code is run. So, for example, another way to reproduce this is to start with the Addons Manager closed and no language packs installed. Then do step 2. Then open the Addons Manager. When you do this, the language category will be hidden.
Whiteboard: [rewrite] → [AddonsRewrite]
Ok, makes sense
blocking2.0: --- → final+
Summary: Refreshing Addons Manager hides Language category even if a language pack is about to be installed → Addons Manager hides Language category if there are no locale packs installed but some are pending install
Attached patch Patch (obsolete) — Splinter Review
Pull out code from gListView so that the empty category detection code can use it.
Assignee: nobody → bparr
Status: NEW → ASSIGNED
Attachment #451884 - Flags: review?(bmcbride)
Attached patch Patch v2 (obsolete) — Splinter Review
Do not return non-active installs (state == STATE_AVAILABLE), as per recent discussion.
Attachment #451884 - Attachment is obsolete: true
Attachment #452197 - Flags: review?(bmcbride)
Attachment #451884 - Flags: review?(bmcbride)
Attached patch Patch v3 (obsolete) — Splinter Review
Updated patch to resolve conflicts with the recently landed patch for Bug 562899.
Attachment #452197 - Attachment is obsolete: true
Attachment #452447 - Flags: review?(bmcbride)
Attachment #452197 - Flags: review?(bmcbride)
Comment on attachment 452447 [details] [diff] [review]
Patch v3

>+function getAddonsAndInstalls(aType, aCallback, aThis) {

Not a fan of passing scope objects in JS (aThis) - the same can be achieved by using closures. |var self = this;| is used throughout this file.


>+    aCallback.call(aThis, results, true);

I'd rather the callback only ever be called once - otherwise using this function is more complex than it needs to be. That should also remove the need for the boolean param there. The array of addons and installs in the results object could potentially be passed as normal params too, but I'll leave that up to you.
Attachment #452447 - Flags: review?(bmcbride) → review-
Attached patch Patch v4 (obsolete) — Splinter Review
Made changes requested by Blair.
Attachment #452447 - Attachment is obsolete: true
Attachment #452671 - Flags: review?(bmcbride)
Comment on attachment 452671 [details] [diff] [review]
Patch v4

Great, thanks Ben.

Don't think we can automatically test this *yet*, since the mocking provider doesn't currently handle installs.
Attachment #452671 - Flags: review?(bmcbride) → review+
Flags: in-testsuite?
Flags: in-litmus?
(In reply to comment #9)
> (From update of attachment 452671 [details] [diff] [review])
> Great, thanks Ben.
> 
> Don't think we can automatically test this *yet*, since the mocking provider
> doesn't currently handle installs.

Actually the mock provider should send out onExternalInstall when a new add-on is added to it which should be enough here right? We should be able to test the following:

Open the add-ons manager, check that the languages category is hidden.
Add a language add-on to the mock provider, check that the language category is visible.
Close then re-open the manager and check that the category is still visible.
Attached patch Patch v5 (obsolete) — Splinter Review
I changed the listener that is added if the locale category is hidden. It no longer does anything for onNewInstall, and instead listens for onDownloadStarted. This is because we no longer show non-active installs. Without this change, the firing of onNewInstall would show the locale category, but if you clicked the locale category, no items would be shown.

I also added an "Initialized" event that is fired when all the initialization code is finished, including the asynchronous code. This is needed to make a test for this bug, and could also be helpful for extension developers.
Attachment #452671 - Attachment is obsolete: true
Attachment #453244 - Flags: review?(bmcbride)
Attached patch Mochitest (obsolete) — Splinter Review
Dave's suggestion unfortunately doesn't work because we need a way for installs to show up when doing getInstallsByTypes. Patch implements a MockInstall that has a bunch of "to be implemented when needed." The MockInstall also allows tests to add listeners that are run after the standard listeners are notified.
Attachment #453256 - Flags: review?(dtownsend)
Attachment #453256 - Flags: review?(bmcbride)
Comment on attachment 453244 [details] [diff] [review]
Patch v5

Looks good.

>+var pendingInits = 1;

Global variables should have a "g" prefix (see the style guide, https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Prefixes). And since the rest of the file doesn't shorten "initialize", may as well make it gPendingInitializations.

r=me with that small change.
Attachment #453244 - Flags: review?(bmcbride) → review+
Comment on attachment 453256 [details] [diff] [review]
Mochitest

>+Components.utils.import("resource://gre/modules/AddonManager.jsm");

Don't need to include this in individual tests - head.js does it for you.


>+function check_hidden(aExpectedHidden) {
>+  var category = gManagerWindow.document.getElementById("category-languages");
>+  is(category.hidden, aExpectedHidden, "Should have correct hidden state");
>+}

Instead of using getElementById directly, use gCategoryManager.get() - since its what the code you're testing will use.


>-    wait_for_view_load(aManagerWindow, aCallback);
>+    var pending = 2;
>+    aManagerWindow.document.addEventListener("Initialized", function() {
>+      aManagerWindow.document.removeEventListener("Initialized", arguments.callee, false);
>+      if (--pending == 0)
>+        aCallback(aManagerWindow);
>+    }, false);
>+
>+    aManagerWindow.document.addEventListener("ViewChanged", function() {
>+      aManagerWindow.document.removeEventListener("ViewChanged", arguments.callee, false);
>+      if (--pending == 0)
>+        aCallback(aManagerWindow);
>+    }, false);
>   }
> 
>   if ("switchToTabHavingURI" in window) {
>     switchToTabHavingURI(MANAGER_URI, true, function(aBrowser) {
>       setup_manager(aBrowser.contentWindow.wrappedJSObject);
>     });
>     return;

This is a bit of an edge-case for these tests, but if about:addons is already open in a tab, and its Initialized event has already fired, then setup_manager will never call the callback, and the test will timeout. Also, you've removed the check to see if the view has already loaded (which is in wait_for_view_load), which could also cause a test to timeout.


>   getInstallsByTypes: function MP_getInstallsByTypes(aTypes, aCallback) {
>     var installs = this.installs.filter(function(aInstall) {
>-      if (aTypes && aTypes.length > 0 && aTypes.indexOf(aInstall.type) == -1)
>+      if (aInstall.state == AddonManager.STATE_CANCELED ||
>+          (aTypes && aTypes.length > 0 && aTypes.indexOf(aInstall.type) == -1))

Why is this needed? And could you add a comment.


>+  addEndListener: function(aListener) {
>+    if (!this.endListeners.some(function(i) i == aListener))
>+      this.endListeners.push(aListener);
>+  },
>+
>+  removeEndListener: function(aListener) {
>+    this.endListeners = this.endListeners.filter(function(i) i != aListener);
>+  },

It's not clear to me why an additional listener type is needed here. What's an "end listener"?
Attachment #453256 - Flags: review?(bmcbride) → review-
Comment on attachment 453256 [details] [diff] [review]
Mochitest

This is in general awesome stuff, thanks for going that extra mile to improve
the test harness for this. Couple of things that could do with cleaning up
though:

>diff -r 818e3d03e19d toolkit/mozapps/extensions/test/browser/head.js

>-    wait_for_view_load(aManagerWindow, aCallback);
>+    var pending = 2;
>+    aManagerWindow.document.addEventListener("Initialized", function() {
>+      aManagerWindow.document.removeEventListener("Initialized", arguments.callee, false);
>+      if (--pending == 0)
>+        aCallback(aManagerWindow);
>+    }, false);
>+
>+    aManagerWindow.document.addEventListener("ViewChanged", function() {
>+      aManagerWindow.document.removeEventListener("ViewChanged", arguments.callee, false);
>+      if (--pending == 0)
>+        aCallback(aManagerWindow);
>+    }, false);

Rather than using both of these, why not just wait for Initialized and then
call wait_for_view_load? That has code to check whether it need to wait for the
ViewChanged event or not.

>+function restart_manager(aOldManager, aView, aCallback) {
>+  aOldManager.addEventListener("unload", function() {
>+    this.removeEventListener("unload", arguments.callee, false);
>+    open_manager(aView, aCallback);
>+  }, false);
>+
>+  aOldManager.close();
>+}

Maybe we should complete the set and have a close_manager function. Not
terribly bothered but it might save us from changing lots of tests if we want
to change that in the future.

>    * @param  aTypes
>    *         An array of types or null to get all types
>    * @param  aCallback
>    *         A callback to pass the array of AddonInstalls to
>    */
>   getInstallsByTypes: function MP_getInstallsByTypes(aTypes, aCallback) {
>     var installs = this.installs.filter(function(aInstall) {
>-      if (aTypes && aTypes.length > 0 && aTypes.indexOf(aInstall.type) == -1)
>+      if (aInstall.state == AddonManager.STATE_CANCELED ||
>+          (aTypes && aTypes.length > 0 && aTypes.indexOf(aInstall.type) == -1))

This is getting a bit long, can you split it into two if statements, one to
test the state and a second to test the rest. Alternatively make this like the
real thing and just remove the install from the list when it is done with.

>+  cancel: function() {
>+    switch (this.state) {
>+      case AddonManager.STATE_AVAILABLE:
>+        this.state = AddonManager.STATE_CANCELLED;
>+        break;
>+      case AddonManager.STATE_INSTALLED:
>+        this.state = AddonManager.STATE_CANCELLED;
>+        this.callListeners("onInstallCancelled");
>+        break;
>+      default:
>+        // Handling cancelling when downloading to be implemented when needed
>+        ok(false, "Cannnot cancel when state = " + this.state);

Too many n's!

>+  addEndListener: function(aListener) {
>+    if (!this.endListeners.some(function(i) i == aListener))
>+      this.endListeners.push(aListener);
>+  },
>+
>+  removeEndListener: function(aListener) {
>+    this.endListeners = this.endListeners.filter(function(i) i != aListener);
>+  },

I'm not sure why these are here. Why can't the test code just register with
addListener?
Attachment #453256 - Flags: review?(dtownsend) → review-
Attached file Patch v6 (obsolete) —
Made changes requested by Blair.
Attachment #453244 - Attachment is obsolete: true
Attachment #453304 - Flags: review?(bmcbride)
Attached patch Patch v6 (obsolete) — Splinter Review
Resubmit as patch.
Attachment #453304 - Attachment is obsolete: true
Attachment #453305 - Flags: review?(bmcbride)
Attachment #453304 - Flags: review?(bmcbride)
Attachment #453305 - Flags: review?(bmcbride) → review+
> >   getInstallsByTypes: function MP_getInstallsByTypes(aTypes, aCallback) {
> >     var installs = this.installs.filter(function(aInstall) {
> >-      if (aTypes && aTypes.length > 0 && aTypes.indexOf(aInstall.type) == -1)
> >+      if (aInstall.state == AddonManager.STATE_CANCELED ||
> >+          (aTypes && aTypes.length > 0 && aTypes.indexOf(aInstall.type) == -1))
> 
> Why is this needed? And could you add a comment.

When an addon is canceled, it should no longer appear in getInstallsByTypes. The real provider, as Dave said, does this by removing the install from its list of active installs. Doing this for the MockProvider is somewhat annoying. For example, it is possible to add a single MockInstall to an arbitrary amount of different MockProviders using MockProvider.addInstall(). Also, not deleting the canceled installs may prove to be helpful for tests later on...maybe. Finally the memory and performance drawbacks from not completely removing canceled installs is small because an instance of the MockProvider will only be used within a single test file.

> >+  addEndListener: function(aListener) {
> >+    if (!this.endListeners.some(function(i) i == aListener))
> >+      this.endListeners.push(aListener);
> >+  },
> >+
> >+  removeEndListener: function(aListener) {
> >+    this.endListeners = this.endListeners.filter(function(i) i != aListener);
> >+  },
> 
> It's not clear to me why an additional listener type is needed here. What's an
> "end listener"?

The naming is pretty cruddy, but it was the best I could think of. The additional listener type solves a race condition with using InstallListeners inside of tests. For example, part of the test for this bug checks to make sure the locale category was shown after the onInstallStarted was fired. In order to do this, we need to ensure that this check happened after the Add-on Manager's listeners ran. Otherwise, for example, the "check to see if locale category is showing" code could run (and does run) before the "show locale category" code. So, the idea is to have a separate list of InstallListeners that are forced to run after the standard InstallListenrs are run, therefore avoiding this race condition.
Attached patch Patch v7Splinter Review
Add global variable gIsInitializing, and add isLoading to gViewController so that it is easy to know if you need to listen for the Initialized event and ViewChanged event, respectively.
Attachment #453305 - Attachment is obsolete: true
Attachment #453584 - Flags: review?(bmcbride)
Attached patch Mochitest v2 (obsolete) — Splinter Review
Made changes requested by Blair and Dave. Kept the STATE_CANCELLED check in getInstallsByTypes, and added a comment to it. Also kept the endListeners in MockInstall, but renamed then to testListeners and added a few comments.
Attachment #453256 - Attachment is obsolete: true
Attachment #453587 - Flags: review?(dtownsend)
Attachment #453587 - Flags: review?(bmcbride)
Attachment #453584 - Flags: review?(bmcbride) → review+
Comment on attachment 453587 [details] [diff] [review]
Mochitest v2

Nice work!

>+  /**
>    * Creates a set of mock add-on objects and adds them to the list of add-ons
>-   * managed by this provider.
>+   * managed by this provider. Returns the new mock add-ons.
>    *
>    * @param  aAddonProperties
>    *         An array of objects containing properties describing the add-ons
>    */

Nit: Use @return to indicate what the function returns. Same below.

>+  addTestListener: function(aListener) {
>+    if (!this.testListeners.some(function(i) i == aListener))
>+      this.testListeners.push(aListener);
>+  },
>+
>+  removeTestListener: function(aListener) {
>+    this.testListeners = this.testListeners.filter(function(i) i != aListener);
>+  },

I think I'm willing to go with this. Using executeSoon may have got you there anyway but at least this is sure to be correct and it is only shipping in test code.
Attachment #453587 - Flags: review?(dtownsend) → review+
Comment on attachment 453587 [details] [diff] [review]
Mochitest v2

Looks good.

In the future, it'd be helpful (for me, at least) to have the tests in the same
diff as the code changes. I think its easier to keep track of things that way -
and the patches aren't huge enough to need to be separate (and they're all
touching basically the same code/feature).
Attachment #453587 - Flags: review?(bmcbride) → review+
Attached patch Mochitest v3Splinter Review
Made changes requested by Dave.

Will do from now on Blair.
Attachment #453587 - Attachment is obsolete: true
Attachment #453662 - Flags: review?(dtownsend)
Comment on attachment 453662 [details] [diff] [review]
Mochitest v3

Generally once a reviewer has marked r+ you only need to upload a new patch with the comments addressed. You only need to re-request review if you ended up making more changes that the reviewer may not have expected in the fixes.
Attachment #453662 - Flags: review?(dtownsend) → review+
Landed: http://hg.mozilla.org/mozilla-central/rev/207984eb2cf1
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus?
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a6
Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a6pre) Gecko/20100625 Minefield/3.7a6pre

Right now we have a slight delay in showing the language panel when opening the add-ons manager. Can we improve that to immediately show the language panel? It doesn't only apply for installations but also post starts of Firefox.
Status: RESOLVED → VERIFIED
Henrik, I filed a bug for making the delay less noticeable (Bug 577990).
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: