It seems nsIMsgFilter.idl::searchTerms attribute is the largest remaining source of nsISupportsArray usage in comm-central. http://mxr.mozilla.org/comm-central/search?string=nsISupportsArray&find=%2Fmail&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central The proposal is probably to convert the term list to nsCOMArray<nsIMsgSearchTerm> internally. The question is whether to remove the searchTerms attribute from the interface or make it readonly (but then convert it to nsIArray or something). There are places where we want to just copy the term list (e.g. mailviews) but we can also substitute that with accessors (getTermAt). There is one spot where we manipulate the searchTerms array directly (bug 392848) so we need a plan for that (modifying accessor).

The most natural thing from an extension's perspective is just to convert from nsISupportsArray to nsIArray. I located several extensions that use searchTerms, mostly get but at least one set (I think, I would have to trace more carefully to confirm). What is the argument for removing the functionality to set the searchTerms?

(In reply to Kent James (:rkent) from comment #1) > What is the argument for removing the functionality to set the searchTerms? Starting at https://bugzilla.mozilla.org/show_bug.cgi?id=392848#c31 .

Funny - thanks aceman for adding me to the cc - I was just using searchTerm while adding a more universal search feature to the searchbox in mail filters and realized the code was looking very clunky: let stCollection = aFilter.searchTerms.QueryInterface(Ci.nsICollection); for (let t = 0; t < stCollection.Count(); t++) { let searchTerm = stCollection.QueryElementAt(t, Ci.nsIMsgSearchTerm); can we be more specific in the bug title and try to achieve something that can be used with fixIterator, as many of the other interfaces that have been moved away from nsISupportsArray? The searchTerms coding is quite involved and I want to keep this as simple as possible for Addons authors to minimize collateral damage.

(In reply to Axel Grude from comment #3) > let stCollection = aFilter.searchTerms.QueryInterface(Ci.nsICollection); > for (let t = 0; t < stCollection.Count(); t++) { > let searchTerm = stCollection.QueryElementAt(t, Ci.nsIMsgSearchTerm); My preference would be for: for (var t = 0; t < aFilter.termCount; t++) { var searchTerm = aFilter.getTermAt(t);

(In reply to neil@parkwaycc.co.uk from comment #4) > (In reply to Axel Grude from comment #3) > > let stCollection = aFilter.searchTerms.QueryInterface(Ci.nsICollection); > > for (let t = 0; t < stCollection.Count(); t++) { > > let searchTerm = stCollection.QueryElementAt(t, Ci.nsIMsgSearchTerm); > > My preference would be for: > > for (var t = 0; t < aFilter.termCount; t++) { > var searchTerm = aFilter.getTermAt(t); OK, I could go along with that, although it makes it harder to write that code compatible with SeaMonkey / Postbox as it either necessitates branching the code there or wrapping with an iterator. As somebody who is multi-application aware I am used to grief...

If .searchTerms would be kept as a read-only nsIArray then that can be iterated over with fixIterator.

(In reply to Axel Grude [:realRaven] from comment #5) > > for (var t = 0; t < aFilter.termCount; t++) { > > var searchTerm = aFilter.getTermAt(t); > > OK, I could go along with that, although it makes it harder to write that > code compatible with SeaMonkey Seamonkey will follow TB in this case as they need to be converted both at once (as mailnews core changes). I think the main question here is whether we need to export/import all the terms as an array (nsIArray) to JS code (whether internal or addons) and in many places and often. If not, we could hide the array as nsCOMArray<nsIMsgSearchTerm> internally and just expose individual terms with the accessors. We could even export a readonly array of the terms (constructing an nsIArray on demand). I am not sure what is faster, but I would assume using nsCOMArray<nsIMsgSearchTerm> instead of nsIArray in the core code would at least allow proper C++ type checks by the compiler. I think you can put anything into a nsIArray.

This takes the approach of just changing search terms to nsIMutableArray. It was possible to just use nsIArray in a few places that were only iterating over the array. I think this is best solution for now, pursing a new interface that hides a nsCOMArray under the hood would certainly be nice but should be left to a follow up. This seems to compile (comm-central is a bit busted right now), I have not tested it. :aceman can you redirect the reviews for me? Thanks!

Comment on attachment 8811453 [details] [diff] [review] Convert nsIMsgFilter::searchTerms from nsISupportsArray to nsIMutableArray Review of attachment 8811453 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for helping here! Nice work, this compiles and passes xpc-shell tests (locally, as try server is broken). Not sure about mozmill yet, it does not work for me locally. I tried running with this. I got these problems: 1. when editing a filter, I get: 23:47:43.577 TypeError: searchTerms.QueryElementAt is not a function 1 searchTermOverlay.js:177:26 initializeSearchRows chrome://messenger/content/searchTermOverlay.js:177:26 initializeDialog chrome://messenger/content/FilterEditor.js:399:3 filterEditorOnLoad chrome://messenger/content/FilterEditor.js:76:7 onload chrome://messenger/content/FilterEditor.xul:1:1 onEditFilter chrome://messenger/content/FilterListDialog.js:331:3 oncommand chrome://messenger/content/FilterListDialog.xul:1:1 2. The same when editing a Saved search folder. The UI is then broken after this error. Jorg, are your mozmill tests working locally? Can you run with this patch? That is after the fix to the above problem. I like that you convert to nsIArray where mutability is not needed. I can continue from there in a followup bug to see if we can tighten the interface even more or if we need direct access to the search terms list from external callers. They could probably put any object into the array which would make a mess. r? to rkent who is the owner of the filter engine and IanN for the /usite parts, it seems it has some specific code in the /suite/mailnews files. ::: mail/base/modules/searchSpec.js @@ +360,5 @@ > updateSession: function SearchSpec_applySearch() { > let session = this.session; > > // clear out our current terms and scope > + session.searchTerms.QueryInterface(Ci.nsIMutableArray).clear(); Is the QI needed? ::: mailnews/base/search/public/nsIMsgFilter.idl @@ +10,2 @@ > > interface nsIArray; I think nsIArray is not needed when there is already nsIMutableArray. ::: mailnews/base/search/src/nsMsgFilter.cpp @@ +174,5 @@ > m_expressionTree(nullptr) > { > + m_termList = nsArray::Create(); > + if (!m_termList) > + NS_ASSERTION(false, "Failed to allocate a nsIArray for nsMsgFilter"); m_termlist is a nsIMutableArray. Should the message say that instead of nsIArray? ::: mailnews/base/search/src/nsMsgLocalSearch.h @@ +23,5 @@ > > class nsMsgSearchOfflineMail : public nsMsgSearchAdapter, public nsIUrlListener > { > public: > + nsMsgSearchOfflineMail (nsIMsgSearchScopeTerm*, nsIArray *); Please remove the space before * while there :) @@ +45,5 @@ > nsMsgSearchBoolExpression ** aExpressionTree, > bool *pResult); > > static nsresult MatchTermsForSearch(nsIMsgDBHdr * msgTomatch, > + nsIArray * termList, Please remove the space after * while there :) @@ +76,5 @@ > bool ForFilters, > nsMsgSearchBoolExpression ** aExpressionTree, > bool *pResult); > > + static nsresult ConstructExpressionTree(nsIArray * termList, Please remove the space after * while there :) @@ +90,5 @@ > > class nsMsgSearchOfflineNews : public nsMsgSearchOfflineMail > { > public: > + nsMsgSearchOfflineNews (nsIMsgSearchScopeTerm*, nsIArray *); Please remove the space before * while there :) ::: mailnews/base/search/src/nsMsgSearchSession.cpp @@ +34,5 @@ > m_expressionTree = nullptr; > m_searchPaused = false; > + m_termList = nsArray::Create(); > + if (!m_termList) > + NS_ASSERTION(false, "Failed to allocate a nsIArray for nsMsgFilter"); m_termlist is a nsIMutableArray. ::: mailnews/base/src/virtualFolderWrapper.js @@ +34,1 @@ > * nsIMsgSearchTermbs. Please fix this typo to nsIMsgSearchTerms while there. ::: mailnews/extensions/mailviews/src/nsMsgMailViewList.cpp @@ +26,5 @@ > > nsMsgMailView::nsMsgMailView() > { > + mViewSearchTerms = nsArray::Create(); > + //mViewSearchTerms = do_CreateInstance(NS_SUPPORTSARRAY_CONTRACTID); Is the commented out line needed or a leftover? ::: suite/mailnews/commandglue.js @@ +977,5 @@ > if (!realFolder.isServer) > gSearchSession.addScopeTerm(!searchOnline ? nsMsgSearchScope.offlineMail : GetScopeForFolder(realFolder), realFolder); > } > > + var termsArray = searchTerms.QueryInterface(Components.interfaces.nsIMutableArray); Does this need a QI here? It seems to me the passed in searchTerms are created in CreateGroupedSearchTerms where a nsIMutableArray is already returned. @@ +984,5 @@ > gSearchSession.appendTerm(term); > } > } > > function CreateGroupedSearchTerms(searchTermsArray) Please document the argument type here using the syntax: /** * Function description... * * @param searchTermsArray ... (I think nsIArray) * * @return nsIMutableArray of search terms. */ ::: suite/mailnews/msgViewPickerOverlay.js @@ +260,5 @@ > PrepareForViewChange(); > > // create an i supports array to store our search terms > + var searchTermsArray = Components.classes["@mozilla.org/array;1"] > + .createInstance(Components.interfaces.nsIMuatbleArray); Typo in nsIMuatbleArray. @@ +288,5 @@ > if (virtualFolderSearchTerms) > { > var isupports = null; > var searchTerm; > + var termsArray = virtualFolderSearchTerms.QueryInterface(Components.interfaces.nsIMutableArray); I think virtualFolderSearchTerms is nsIArray already. Yes, I do not know why the QI was there before (to nsISupportsArray). @@ +293,4 @@ > for (var i = 0; i < termsArray.Count(); i++) > { > isupports = termsArray.GetElementAt(i); > searchTerm = isupports.QueryInterface(Components.interfaces.nsIMsgSearchTerm); Does this need the 'isupports' middle-man? Just a general question, do we need the QI to nsIMsgSearchTerm if we are reasonably sure the array only contains such objects? E.g. by enforcing the nsCOMArray<nsIMsgSearchTerm> internally. ::: suite/mailnews/searchBar.js @@ +288,1 @@ > function createSearchTermsWithList(aTermsArray) You could document what we expect aTermsArray to be. @@ +293,5 @@ > > gSearchSession.clearScopes(); > var searchTerms = gSearchSession.searchTerms; > + var searchTermsArray = searchTerms.QueryInterface(Components.interfaces.nsIMutableArray); > + searchTermsArray.clear(); What? Does this actually use the result of gSearchSession.searchTerms? Actually it seems these 2 variables are not even used in this function. @@ +321,5 @@ > gSearchSession.addScopeTerm(nsMsgSearchScope.offlineMail, selectedFolder); > } > // add each item in termsArray to the search session > > + var termsArray = aTermsArray.QueryInterface(Components.interfaces.nsIMutableArray); Is QI needed? @@ +377,5 @@ > if (defaultSearchTerms) > { > var isupports = null; > var searchTerm; > + var termsArray = defaultSearchTerms.QueryInterface(Components.interfaces.nsIMutableArray); Needed?

(In reply to :aceman from comment #9) > Thanks for helping here! Nice work, this compiles and passes xpc-shell tests > (locally, as try server is broken). Well, with the right patches, you can do try runs. I do daily try runs during the bustage period. > 1. when editing a filter, I get: > 23:47:43.577 TypeError: searchTerms.QueryElementAt is not a function 1 > searchTermOverlay.js:177:26 I get this, too. > Jorg, are your mozmill tests working locally? Can you run with this patch? > That is after the fix to the above problem. I can run the Mozmill tests on the try server. However, who will fix this problem? There's nothing apparent. The JS files haven't changed, so there must be a subtle difference in the C++.

(In reply to Jorg K (GMT+1) from comment #10) > > 1. when editing a filter, I get: > > 23:47:43.577 TypeError: searchTerms.QueryElementAt is not a function 1 > > searchTermOverlay.js:177:26 > I get this, too. > > > Jorg, are your mozmill tests working locally? Can you run with this patch? > > That is after the fix to the above problem. > I can run the Mozmill tests on the try server. However, who will fix this > problem? There's nothing apparent. The JS files haven't changed, so there > must be a subtle difference in the C++. I think it just needs to change to queryElementAt (lowercase q), similar to the Clear() -> clear() changes. The same may be needed at https://dxr.mozilla.org/comm-central/source/mailnews/base/search/content/FilterEditor.js#142

OK, I've changed Q to q in FilterEditor.js#142 and searchTermOverlay.js#17. I couldn't run it locally since I have to wait for a rebuild. I pushed it to try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=23471ffea8a0cc6973536f18d3bd8c9d8c3ba56e

(In reply to Jorg K (GMT+1) from comment #12) > OK, I've changed Q to q in FilterEditor.js#142 and searchTermOverlay.js#17. > > I couldn't run it locally since I have to wait for a rebuild. I pushed it to > try: > https://treeherder.mozilla.org/#/jobs?repo=try-comm- > central&revision=23471ffea8a0cc6973536f18d3bd8c9d8c3ba56e Yeah there will probably be a few others like that, the C++ side a bit easier to manage due to compilation errors. You can reference nsIMutableArray.idl [1] for the conversions for mutable stuff. For iterating there shouldn't need to be changes. Basically lower-case the names, if appending or inserting you need to specify a second weak = false param. [1] http://searchfox.org/mozilla-central/rev/8562d3859b89ac89f46690b9ed2c473e0728d6c0/xpcom/ds/nsIMutableArray.idl

:aceman, thanks for thorough feedback! I'm going to unassign myself from this, I think it's going to be easier for someone who can build and run the tests to finish it off. Please feel free to update the patches as you see fit and of course ni me for any issues you run into.

Aceman, can you take over, please. I've done the try run you wanted and it came out green (apart from the failure we have from bug 1317674): https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=23471ffea8a0cc6973536f18d3bd8c9d8c3ba56e I won't upload my patch with the two Q to q changes as detailed in comment #12 since you may want to address additional nits from comment #9 (typos, whitespace, etc.). I'll do a local build now.

Sorry, wrong bug number: I've done the try run you wanted and it came out green (apart from the failure we have from bug 1316256).

(In reply to Eric Rahm [:erahm] from comment #14) > :aceman, thanks for thorough feedback! I'm going to unassign myself from > this, I think it's going to be easier for someone who can build and run the > tests to finish it off. Please feel free to update the patches as you see > fit and of course ni me for any issues you run into. Yes, I can take this over, thanks for the initial work. But I am not such an expert on the QueryInterface stuff, can you answer my questions from the review?

(In reply to Jorg K (GMT+1) from comment #16) > Sorry, wrong bug number: > I've done the try run you wanted and it came out green (apart from the > failure we have from bug 1316256). Thanks, it found at least one new problem: 17:56:32 INFO - TEST-START | /builds/slave/test/build/tests/mozmill/folder-display/test-deletion-from-virtual-folders.js | test_create_virtual_folders 17:56:32 INFO - ** Error saving element 0: TypeError: searchTerms.SetElementAt is not a function I'll replace it with replaceElementAt().

Hmm, why didn't that show up here? https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=23471ffea8a0cc6973536f18d3bd8c9d8c3ba56e

It did, that is where I took it from.

I don't see it as failure in the parsed treeherder log. Anyway, you looked at the complete log, good on you (as they say in Australia) ;-) Since bug 1314955 landed, there will most likely not be any more try pushes until I do some more magic. Latest push here will most likely fail big time: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=947b30754f90994ada3ce0071e1a2f46f6001951 Follow bug 1318258.

(In reply to Jorg K (GMT+1) from comment #21) > I don't see it as failure in the parsed treeherder log. Anyway, you looked > at the complete log, good on you (as they say in Australia) ;-) Yes, that was the point :) It does not cause a test failure, but produces visible JS errors. > Since bug 1314955 landed, there will most likely not be any more try pushes > until I do some more magic. Latest push here will most likely fail big time: > https://treeherder.mozilla.org/#/jobs?repo=try-comm- > central&revision=947b30754f90994ada3ce0071e1a2f46f6001951 > Follow bug 1318258. Could you make a patch that disables building calendar on try ? :)

(In reply to :aceman from comment #22) > Could you make a patch that disables building calendar on try ? :) I'll see how we can get us building again.

Build was busted by something else, I fixed that, to let's see whether this one is better: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a854a856131554c2b0aaa273d57d1769cd547b6d BTW, the bustage bug 1317674 landed in M-inbound, so we should be unbusted soon.

(In reply to :aceman from comment #9) > Comment on attachment 8811453 [details] [diff] [review] > Convert nsIMsgFilter::searchTerms from nsISupportsArray to nsIMutableArray > > Review of attachment 8811453 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mail/base/modules/searchSpec.js > @@ +360,5 @@ > > updateSession: function SearchSpec_applySearch() { > > let session = this.session; > > > > // clear out our current terms and scope > > + session.searchTerms.QueryInterface(Ci.nsIMutableArray).clear(); > > Is the QI needed? I left this how I found it, but I don't think the QI is (or was) necessary. The nice thing on the JS side of xpcom is that usually you don't need to QI. > ::: mailnews/base/search/public/nsIMsgFilter.idl > @@ +10,2 @@ > > > > interface nsIArray; > > I think nsIArray is not needed when there is already nsIMutableArray. Yes that can be removed. > ::: mailnews/base/search/src/nsMsgFilter.cpp > @@ +174,5 @@ > > m_expressionTree(nullptr) > > { > > + m_termList = nsArray::Create(); > > + if (!m_termList) > > + NS_ASSERTION(false, "Failed to allocate a nsIArray for nsMsgFilter"); > > m_termlist is a nsIMutableArray. Should the message say that instead of > nsIArray? nsIMurableArray is an nsIArray, but yes it could be more specific. > ::: mailnews/base/search/src/nsMsgSearchSession.cpp > @@ +34,5 @@ > > m_expressionTree = nullptr; > > m_searchPaused = false; > > + m_termList = nsArray::Create(); > > + if (!m_termList) > > + NS_ASSERTION(false, "Failed to allocate a nsIArray for nsMsgFilter"); > > m_termlist is a nsIMutableArray. nsIMurableArray is an nsIArray, but yes it could be more specific. > ::: mailnews/extensions/mailviews/src/nsMsgMailViewList.cpp > @@ +26,5 @@ > > > > nsMsgMailView::nsMsgMailView() > > { > > + mViewSearchTerms = nsArray::Create(); > > + //mViewSearchTerms = do_CreateInstance(NS_SUPPORTSARRAY_CONTRACTID); > > Is the commented out line needed or a leftover? Just leftover. > ::: suite/mailnews/commandglue.js > @@ +977,5 @@ > > if (!realFolder.isServer) > > gSearchSession.addScopeTerm(!searchOnline ? nsMsgSearchScope.offlineMail : GetScopeForFolder(realFolder), realFolder); > > } > > > > + var termsArray = searchTerms.QueryInterface(Components.interfaces.nsIMutableArray); > > Does this need a QI here? It seems to me the passed in searchTerms are > created in CreateGroupedSearchTerms where a nsIMutableArray is already > returned. Yeah I think we can get rid of the QI. > @@ +984,5 @@ > > gSearchSession.appendTerm(term); > > } > > } > > > > function CreateGroupedSearchTerms(searchTermsArray) > > Please document the argument type here using the syntax: > /** > * Function description... > * > * @param searchTermsArray ... (I think nsIArray) > * > * @return nsIMutableArray of search terms. > */ > > ::: suite/mailnews/msgViewPickerOverlay.js > @@ +260,5 @@ > > PrepareForViewChange(); > > > > // create an i supports array to store our search terms > > + var searchTermsArray = Components.classes["@mozilla.org/array;1"] > > + .createInstance(Components.interfaces.nsIMuatbleArray); > > Typo in nsIMuatbleArray. Ugh, yeah. > @@ +288,5 @@ > > if (virtualFolderSearchTerms) > > { > > var isupports = null; > > var searchTerm; > > + var termsArray = virtualFolderSearchTerms.QueryInterface(Components.interfaces.nsIMutableArray); > > I think virtualFolderSearchTerms is nsIArray already. Yes, I do not know why > the QI was there before (to nsISupportsArray). Agreed. > @@ +293,4 @@ > > for (var i = 0; i < termsArray.Count(); i++) > > { > > isupports = termsArray.GetElementAt(i); > > searchTerm = isupports.QueryInterface(Components.interfaces.nsIMsgSearchTerm); > > Does this need the 'isupports' middle-man? No. > Just a general question, do we need the QI to nsIMsgSearchTerm if we are > reasonably sure the array only contains such objects? E.g. by enforcing the > nsCOMArray<nsIMsgSearchTerm> internally. Yes as long as you're sure they're nsIMsgSearchTerms you don't need the QI. > @@ +293,5 @@ > > > > gSearchSession.clearScopes(); > > var searchTerms = gSearchSession.searchTerms; > > + var searchTermsArray = searchTerms.QueryInterface(Components.interfaces.nsIMutableArray); > > + searchTermsArray.clear(); > > What? Does this actually use the result of gSearchSession.searchTerms? > Actually it seems these 2 variables are not even used in this function. It's clearing the |gSearchSession.searchTerms| array (searchTermsArray is a ref, not a copy), which TBH is horrible but that's how it is. This could probably be shortened to |gSearchSession.searchTerms.clear();| > @@ +321,5 @@ > > gSearchSession.addScopeTerm(nsMsgSearchScope.offlineMail, selectedFolder); > > } > > // add each item in termsArray to the search session > > > > + var termsArray = aTermsArray.QueryInterface(Components.interfaces.nsIMutableArray); > > Is QI needed? No. > @@ +377,5 @@ > > if (defaultSearchTerms) > > { > > var isupports = null; > > var searchTerm; > > + var termsArray = defaultSearchTerms.QueryInterface(Components.interfaces.nsIMutableArray); > > Needed? Probably not, this one is trickier. You'll have to look at how defaultSearchTerms gets set and make sure they're all nsIMutableArrays.

In this version I found some more method calls in JS that need to be converted, like RemoveElement(). With this version editing a filter seems to work, in all operations. Jorg, please push to try if it is possible now.

Here's your try, you don't see the M-C "magic" that was pushed previously. Bustage will be over tomorrow. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e414c93dee3c6f5820c1f2fa51b0c4dbeea62bb7 Note that Mozmill tests are currently *crashing* due to bug 1314790 comment #36. Does Mozmill restart if it crashed? All the tests are run separately, right?, so if one crashes, you still get the subsequent ones.

Comment on attachment 8811929 [details] [diff] [review] Convert nsIMsgFilter::searchTerms from nsISupportsArray to nsIMutableArray v1.1 I'm not sure which sort of feedback you're after. Yes, we want to remove nsISupportsArray, yes, you did a good job fixing JS errors. I looked at the log of the Mozmill test and couldn't see any errors caused by this. <off-topic> I did see however a disturbing amount of errors. Here the most prominent ones: INFO - JavaScript error: chrome://global/content/bindings/notification.xml, line 478: TypeError: this.close is not a function INFO - JavaScript error: chrome://messenger/content/chat/imStatusSelector.js, line 36: TypeError: document.getElementById(...) is null INFO - JavaScript error: chrome://messenger/content/cloudfile/addAccountDialog.js, line 265: NS_ERROR_UNEXPECTED: INFO - JavaScript error: chrome://messenger/content/folderDisplay.js, line 1166: TypeError: this.view.displayedFolder is null INFO - JavaScript error: chrome://messenger/content/folderDisplay.js, line 2514: TypeError: this.view.dbView is null INFO - JavaScript error: chrome://messenger/content/folderPane.js, line 2185: TypeError: null has no properties INFO - JavaScript error: chrome://messenger/content/messageWindow.js, line 254: TypeError: this.folderDisplay.view.dbView is null INFO - JavaScript error: chrome://messenger/content/plugins.js, line 681: TypeError: notificationBox.removeNotification is not a function INFO - JavaScript error: resource:///modules/activity/alertHook.js, line 48: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIMsgMailNewsUrl.server] INFO - JavaScript error: resource://gre/components/nsLoginManager.js, line 308: Error: This login already exists. And the winner is: INFO - JavaScript error: resource://gre/modules/FinderHighlighter.jsm, line 1165: TypeError: window is null I think I'll look into this one now myself. </off-topic>

For FinderHighlighter.jsm see bug 1279695 comment #36.

(In reply to Jorg K (GMT+1) from comment #28) > I'm not sure which sort of feedback you're after. Yes, we want to remove > nsISupportsArray, yes, you did a good job fixing JS errors. Sorry, I meant the f? to only notify you to push to try. Nothing more needed, thanks. > And the winner is: > INFO - JavaScript error: resource://gre/modules/FinderHighlighter.jsm, line > 1165: TypeError: window is null > I think I'll look into this one now myself. > </off-topic> I think I have seen this one also when running TB trunk.

Comment on attachment 8811929 [details] [diff] [review] Convert nsIMsgFilter::searchTerms from nsISupportsArray to nsIMutableArray v1.1 I'm also not really sure what I am being asked to look at here. Yes we want to remove nsISupportsArray, and quick glance at the code tells me this is generally being done competently here.

(In reply to Kent James (mostly AFK 2016-11-19 to 2016-11-26) (:rkent) from comment #31) > I'm also not really sure what I am being asked to look at here. Yes we want > to remove nsISupportsArray, and quick glance at the code tells me this is > generally being done competently here. Whether you are fine with landing the current patch (with mutable .searchTerms), or if we want to pursue the more thorough rewrite (as in comment 7).

(In reply to :aceman from comment #32) > (In reply to Kent James (mostly AFK 2016-11-19 to 2016-11-26) (:rkent) from > comment #31) > > I'm also not really sure what I am being asked to look at here. Yes we want > > to remove nsISupportsArray, and quick glance at the code tells me this is > > generally being done competently here. > > Whether you are fine with landing the current patch (with mutable > .searchTerms), or if we want to pursue the more thorough rewrite (as in > comment 7). I strongly suggest saving that for a follow up if at all.

My understanding is that this change is motivated by a desire to get rid of nsISupportsArray as that is going away in core. As such, I don't think this is the best time to consider a rewrite with the goal of trying to improve the isolation, or to improve performance, unless the changes are small with known significant gains. Making things as easy as possible for addon authors to adjust to is an important goal, and it seems to me that nsIMutableArray is the best way to do that.

Ok. So I think this is ready now. It seems the changes are fine and seems to pass the tests. I give general f+ for the patch. Because I have done further changes in the /mail/*.js so I forward to Magnus to look over those.

Comment on attachment 8812498 [details] [diff] [review] Convert nsIMsgFilter::searchTerms from nsISupportsArray to nsIMutableArray v1.2 Review of attachment 8812498 [details] [diff] [review]: ----------------------------------------------------------------- This is fine with one issue/question addressed. General comments: 1) Kent only works on TB for two hours each day. We need him for other jobs. Please do not ask him for review unless *absolutely* necessary. 2) IanN is also busy and we don't need to involve him in mechanical refactoring. This has been looked at by three people so far: Eric, Aceman, Jörg. If we get Magnus to look it over we're truly well covered. ::: mailnews/base/search/content/searchTermOverlay.js @@ +490,5 @@ > { > var matchAll = gSearchBooleanRadiogroup.value == 'matchAll'; > var i; > for (i = 0; i < gSearchRemovedTerms.length; i++) > + searchTerms.removeElementAt(searchTerms.indexOf(0, gSearchRemovedTerms[i])); I don't understand the .indexOf(0, gSearchRemovedTerms[i])). I think you need to lose |0, |.

(In reply to Jorg K (GMT+1) from comment #36) > General comments: > 1) Kent only works on TB for two hours each day. We need him for other jobs. > Please do not ask him for review unless *absolutely* necessary. I provided the reason for choosing him, but yes, he already gave feedback so we can consider it enough. > 2) IanN is also busy and we don't need to involve him in mechanical > refactoring. I provided the reason for choosing him. Also, none of the tree people have tried the patch under running Seamonkey. Some of the changes needed were found by me by running TB and trying filtering operations. > This has been looked at by three people so far: Eric, Aceman, Jörg. If we > get Magnus to look it over we're truly well covered. You can't count the original author as a reviewer (doubly so he can't run TB:)). I have looked over Eric's changes and tested the TB parts. I have made some additions. Who checks those? So either you did, or we need e.g. Magnus. But we still need Ian for SM as nobody checked that. Or we can try Ratty. > ::: mailnews/base/search/content/searchTermOverlay.js > @@ +490,5 @@ > > { > > var matchAll = gSearchBooleanRadiogroup.value == 'matchAll'; > > var i; > > for (i = 0; i < gSearchRemovedTerms.length; i++) > > + searchTerms.removeElementAt(searchTerms.indexOf(0, gSearchRemovedTerms[i])); > > I don't understand the .indexOf(0, gSearchRemovedTerms[i])). I think you > need to lose |0, |. On an nsIArray, .indexOf() strangely has 2 arguments (https://dxr.mozilla.org/comm-central/rev/f09e137ead39230eaa94f47988ccce2cfcda4195/mozilla/xpcom/ds/nsIArray.idl#80). This is one of my additions on top of Eric's patch and I have tested it.

(In reply to :aceman from comment #37) > You can't count the original author as a reviewer (doubly so he can't run > TB:)). I was counting eyes, not reviewers. We will have three reviewers, yourself, me and Magnus for various parts. > I have looked over Eric's changes and tested the TB parts. I have made some > additions. Who checks those? So either you did, or we need e.g. Magnus. I did and Magnus will. > But we still need Ian for > SM as nobody checked that. Or we can try Ratty. Yes, get one of the SM people to try it: Ratty, FRG, rsx11m or Ian, if he has time.

Comment on attachment 8812498 [details] [diff] [review] Convert nsIMsgFilter::searchTerms from nsISupportsArray to nsIMutableArray v1.2 In fact, Ratty is my preferred reviewer here. This guy is so sharp and finds issues that others have overlooked. Sadly he's not always available on time.

Comment on attachment 8812498 [details] [diff] [review] Convert nsIMsgFilter::searchTerms from nsISupportsArray to nsIMutableArray v1.2 Suite part: > --- a/suite/mailnews/commandglue.js > +++ b/suite/mailnews/commandglue.js > const nsIMsgSearchTerm = Components.interfaces.nsIMsgSearchTerm; > - for (let term in fixIterator(termsArray, nsIMsgSearchTerm)) { > + for (let term in fixIterator(searchTerms, nsIMsgSearchTerm)) { Hmm. Should this be "of" rather than "in"? Assuming "of" works. > - var numEntries = searchTermsArray.Count(); > + var numEntries = searchTermsArray.length; > for (var i = 0; i < numEntries; i++) { > - var searchTerm = searchTermsArray.GetElementAt(i).QueryInterface(Components.interfaces.nsIMsgSearchTerm); > + let searchTerm = searchTermsArray.queryElementAt(i, Components.interfaces.nsIMsgSearchTerm); Since you're already changing this chunk of code, please use for (let i ...) > --- a/suite/mailnews/msgViewPickerOverlay.js > +++ b/suite/mailnews/msgViewPickerOverlay.js > - var isupports = null; > - var searchTerm; > - var termsArray = virtualFolderSearchTerms.QueryInterface(Components.interfaces.nsISupportsArray); > - for (var i = 0; i < termsArray.Count(); i++) > + for (var i = 0; i < termsArray.length; i++) again var -> let. Other parts: > --- a/mailnews/base/search/content/searchTermOverlay.js > +++ b/mailnews/base/search/content/searchTermOverlay.js > - searchTerms.SetElementAt(i, searchTerm); > + searchTerms.replaceElementAt(searchTerm, i, false); nitpick searchTerms.replaceElementAt(searchTerm, i, /* weak */ false)

(In reply to Philip Chee from comment #40) > > const nsIMsgSearchTerm = Components.interfaces.nsIMsgSearchTerm; > > - for (let term in fixIterator(termsArray, nsIMsgSearchTerm)) { > > + for (let term in fixIterator(searchTerms, nsIMsgSearchTerm)) { > Hmm. Should this be "of" rather than "in"? Assuming "of" works. Both versions work for now. But yes, we can gradually modernize the lines we touch.

Should be the final version. Try run at: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2dd487750576044449a05b1dd95faf3659898497

Tests look successful.

https://hg.mozilla.org/comm-central/rev/07171d0818412e1162ff63e6978fa63204b75649 Thanks to all involved! I assumed a=Ratty for SM.

Just writing a backup / restore feature message filters for quickFilters, testing in Thunderbird 58, I see a lot of code failing that does: searchTerms.QueryInterface(Components.interfaces.nsICollection) ... it appears to be expecting nsIMutableArray instead now, which will necessitate writing some shim code. were there recent changes in the code base that can explain this?

I think nsICollection have been completely removed, see bug 1315812, bug 1317040 and bug 792209 and friends, although I can't see where nsICollection was removed.

I don't understand the question. In this patch searchTerms were clearly change to nsIMutableArray so that is what you get. I don't think nsIMutableArray inherits from nsICollection (it may be nsISupportsArray did). I don't know of recent changes, I think the bug here caused what you say. You can iterate searchTerms via fixIterator to be safe, then there would be no changes necessary after this patch.

(In reply to :aceman from comment #47) > I don't understand the question. In this patch searchTerms were clearly > change to nsIMutableArray so that is what you get. > I don't think nsIMutableArray inherits from nsICollection (it may be > nsISupportsArray did). > > I don't know of recent changes, I think the bug here caused what you say. I saw the behavior change between Tb 56 (current release) and Tb 58 beta, which is strange if the patch landed a year ago? > You can iterate searchTerms via fixIterator to be safe, then there would be > no changes necessary after this patch. OK, maybe I will do that. At the moment for backwards compatibility I wrote some (fairly clunky) wrappers, like so: querySearchTermsArray: function querySearchTermsArray(searchTerms) { if (searchTerms.QueryElementAt) return searchTerms.QueryInterface(Components.interfaces.nsICollection); // old version if (searchTerms.queryElementAt) return searchTerms.QueryInterface(Components.interfaces.nsIMutableArray); return null; } , querySearchTermsAt: function querySearchTermsAt(searchTerms, i) { if (searchTerms.QueryElementAt) return searchTerms.QueryElementAt(i, Components.interfaces.nsIMsgSearchTerm); if (searchTerms.queryElementAt) return searchTerms.queryElementAt(i, Components.interfaces.nsIMsgSearchTerm); return null; } ,