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)
Toolkit
Add-ons Manager
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)
9.39 KB,
patch
|
Unfocused
:
review+
|
Details | Diff | Splinter Review |
15.10 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•14 years ago
|
||
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
Assignee | ||
Updated•14 years ago
|
Whiteboard: [rewrite]
Assignee | ||
Comment 2•14 years ago
|
||
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.
Updated•14 years ago
|
Whiteboard: [rewrite] → [AddonsRewrite]
Comment 3•14 years ago
|
||
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
Assignee | ||
Comment 4•14 years ago
|
||
Pull out code from gListView so that the empty category detection code can use it.
Assignee | ||
Comment 5•14 years ago
|
||
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)
Assignee | ||
Comment 6•14 years ago
|
||
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 7•14 years ago
|
||
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-
Assignee | ||
Comment 8•14 years ago
|
||
Made changes requested by Blair.
Attachment #452447 -
Attachment is obsolete: true
Attachment #452671 -
Flags: review?(bmcbride)
Comment 9•14 years ago
|
||
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+
Updated•14 years ago
|
Flags: in-testsuite?
Flags: in-litmus?
Comment 10•14 years ago
|
||
(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.
Assignee | ||
Comment 11•14 years ago
|
||
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)
Assignee | ||
Comment 12•14 years ago
|
||
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 13•14 years ago
|
||
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 14•14 years ago
|
||
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 15•14 years ago
|
||
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-
Assignee | ||
Comment 16•14 years ago
|
||
Made changes requested by Blair.
Attachment #453244 -
Attachment is obsolete: true
Attachment #453304 -
Flags: review?(bmcbride)
Assignee | ||
Comment 17•14 years ago
|
||
Resubmit as patch.
Attachment #453304 -
Attachment is obsolete: true
Attachment #453305 -
Flags: review?(bmcbride)
Attachment #453304 -
Flags: review?(bmcbride)
Updated•14 years ago
|
Attachment #453305 -
Flags: review?(bmcbride) → review+
Assignee | ||
Comment 18•14 years ago
|
||
> > 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.
Assignee | ||
Comment 19•14 years ago
|
||
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)
Assignee | ||
Comment 20•14 years ago
|
||
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)
Updated•14 years ago
|
Attachment #453584 -
Flags: review?(bmcbride) → review+
Comment 21•14 years ago
|
||
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 22•14 years ago
|
||
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+
Assignee | ||
Comment 23•14 years ago
|
||
Made changes requested by Dave. Will do from now on Blair.
Attachment #453587 -
Attachment is obsolete: true
Attachment #453662 -
Flags: review?(dtownsend)
Comment 24•14 years ago
|
||
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+
Comment 25•14 years ago
|
||
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
Comment 26•14 years ago
|
||
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
Assignee | ||
Comment 27•14 years ago
|
||
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.
Description
•