1318185 - fix remaining/incorrect QueryElementAt() to queryElementAt() in c-c

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[:aceman]

It seems to me there are some remaining QueryElementAt() calls on arrays, that are no longer nsISupportsArray. For nsIArray, the name seems to be queryElementAt() (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIArray).

https://dxr.mozilla.org/comm-central/search?q=QueryElementAt+%2Bfile%3A.js&redirect=true
mail/base/content/mailCommands.js

306
addressList = addressList + (i > 0 ? ",":"") + addresses.QueryElementAt(i,Components.interfaces.nsISupportsString).data;
mail/components/compose/content/addressingWidgetOverlay.js

1163
addressToAdd = searchResultsForSession.items.QueryElementAt(searchResultsForSession.defaultItemIndex, Components.interfaces.nsIAutoCompleteItem).value;

1176
addressToAdd = searchResultsForSession.items.QueryElementAt(0, Components.interfaces.nsIAutoCompleteItem).value;
mailnews/base/search/content/FilterEditor.js

142
var searchTerm = copiedFilter.searchTerms.QueryElementAt(i,
mailnews/base/search/content/searchTermOverlay.js

177
var searchTerm = searchTerms.QueryElementAt(i, nsIMsgSearchTerm);
mailnews/base/util/iteratorUtils.jsm

114
yield aEnum.QueryElementAt(i, face);
suite/mailnews/commandglue.js

883
var term = searchTerms.QueryElementAt(searchIndex++, Components.interfaces.nsIMsgSearchTerm);
suite/mailnews/compose/addressingWidgetOverlay.js

1107
addressToAdd = searchResultsForSession.items.QueryElementAt(searchResultsForSession.defaultItemIndex, Components.interfaces.nsIAutoCompleteItem).value;

1120
addressToAdd = searchResultsForSession.items.QueryElementAt(0, Components.interfaces.nsIAutoCompleteItem).value;
suite/mailnews/mailCommands.js

257
addressList = addressList + (i > 0 ? ",":"") + addresses.QueryElementAt(i,Components.interfaces.nsISupportsString).data;

I skip the seearchTerms and fixIterator cases as those still use nsISupportsArray.

Comment 1

[:aceman]

Attachment: patch

The case in mailCommands is that addresses is nsIAbView.selectedAddresses which returns a nsIArray.

The case in addressingWidgetOverlay.js is stranger. It seems searchResultsForSession is a plain JS array. I don't think .items works on that so the [qQ]ueryElementAt may be the least of its problems. But I think that is dead code anyway as any autocomplete is disabled per the comment above that place. So once somebody reenables that code, he will see what needs to be done there.

Comment 2

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #0)
> It seems to me there are some remaining QueryElementAt() calls on arrays,
> that are no longer nsISupportsArray. For nsIArray, the name seems to be queryElementAt()

FYI.

Change from QueryElementAt() etc. to queryElementAt() etc. was done between Tb 19 and Tb 20.
I wrote Wrapper like next for account object, identity object etc. in my extension for Thunderbird testing, and it still works well in Tb 45. 
> // **********************************************
> // for compatibility between Tb 19 and Tb 20/current Tb
> // **********************************************
>    var Common_Account_Length; var Common_Account_GetNext;
>    if( All_Accounts.length || 0==All_Accounts.length ) // Tb 20 and newer
>    {
>       Common_Account_Length  = All_Accounts.length;
>       Common_Account_GetNext = function (nn) { var obj = All_Accounts.queryElementAt(nn,Components.interfaces.nsIMsgAccount); return obj; }
>    }
>    else // until Tb 19
>    {
>       Common_Account_Length  = All_Accounts.Count();
>       Common_Account_GetNext = function (nn) { var obj = All_Accounts.QueryElementAt(nn,Components.interfaces.nsIMsgAccount); return obj; } 
>    }
> // **********************************************
> // for compatibility betwen Tb 19 and Tb 20/current Tb
> // **********************************************

At least change of Count()=>length is needed in addition to change of QueryElementAt=>queryElementAt.
Sorry but I don't know about hasMore()/HasMore()/hasmore() like function.

Comment 3

[:aceman]

(In reply to WADA from comment #2)
> At least change of Count()=>length is needed in addition to change of
> QueryElementAt=>queryElementAt.
> Sorry but I don't know about hasMore()/HasMore()/hasmore() like function.

I did some convertions from Count() to .length in bug 857230, but am not planning a wholesale conversion across the tree yet.
For some reason Count() is defined in a nsIMutableArray (in addition to the inherited .length):
https://dxr.mozilla.org/comm-central/source/mozilla/xpcom/ds/nsIArrayExtensions.idl#28
Maybe it is to ease the transition from nsISupportsArray to nsIMutableArray.

Eric, would you know if Count() is planned to go away or if we can safely leave it in?

Comment 4

[Eric Rahm [:erahm]]

(In reply to :aceman from comment #3)
> (In reply to WADA from comment #2)
> > At least change of Count()=>length is needed in addition to change of
> > QueryElementAt=>queryElementAt.
> > Sorry but I don't know about hasMore()/HasMore()/hasmore() like function.
> 
> I did some convertions from Count() to .length in bug 857230, but am not
> planning a wholesale conversion across the tree yet.
> For some reason Count() is defined in a nsIMutableArray (in addition to the
> inherited .length):
> https://dxr.mozilla.org/comm-central/source/mozilla/xpcom/ds/
> nsIArrayExtensions.idl#28
> Maybe it is to ease the transition from nsISupportsArray to nsIMutableArray.
> 
> Eric, would you know if Count() is planned to go away or if we can safely
> leave it in?

It's intended to ease the transition, particularly for add-ons. I don't think we'll get rid of it, but it might get folded into nsIArray.

Comment 5

[:aceman]

The searchTerm and searchResults* cases were fixed in bug 857230 so the 3 cases per app I have in the patch are the last occurences. The iteratorUtils case is intentionally left in as it really is a supportsArray and we can leave it until nsISupportsArray is removed from m-c.

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 8811538 [details] [diff] [review]
patch

Aceman, sorry to cancel your review. Could you please add the cases in nsDragAndDrop.js (mail/ and suite/) that you mentioned in bug 1317871 comment #8 into this patch?

This way we get them reviewed in the correct context.

Comment 7

[:aceman]

Attachment: patch v2

Removes also all GetElementAt() calls, which only exist for nsIMutableArray (actually nsIArrayExtensions that is a subclass of nsIArray and parent class of nsIMutableArray). So it again does not exist on pure nsIArray which is returned e.g. by getEmbeddedObjects() (looks like I missed e.g. the usage in /suite/debugQA when I converted that function).

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 8814570 [details] [diff] [review]
patch v2

Review of attachment 8814570 [details] [diff] [review]:
-----------------------------------------------------------------

Have you tested the mailCommands.js and addressingWidgetOverlay.js changes? I guess I'll have to test the drag and drop.

::: suite/debugQA/content/debugQAEditorOverlay.js
@@ +221,5 @@
>    try {
>      var objectArray = GetCurrentEditor().getEmbeddedObjects();
> +    dump(objectArray.length + " embedded objects\n");
> +    for (var i=0; i < objectArray.length; ++i)
> +      dump(objectArray.queryElementAt(i, Components.interfaces.nsIDOMNode) + "\n");

Is this going to dump something useful?
Perhaps better dump .localName or something similar.

Comment 9

[Jorg K (CEST = GMT+2)]

Grrr, I overwrote Ratty's r+.

Comment 10

[:aceman]

(In reply to Jorg K (GMT+1) from comment #8)
> Have you tested the mailCommands.js and addressingWidgetOverlay.js changes?
> I guess I'll have to test the drag and drop.

No, I have not run the currently disabled code in addressingWidgetOverlay.js, as said in comment 1.
I do not remember how I tested the mailCommands.js code, but the old code should be wrong as nsIAbView.selectedAddresses returns nsIArray, not nsISupportsArray. It's probably some rare code that nobody has hit yet (no reports of console errors).

I'll try it again once my build is done.

Comment 11

[:aceman]

I noticed I must also convert the .Count() to .length at those places as Count also does not exist in nsIArray.

Comment 12

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #11)
> I noticed I must also convert the .Count() to .length at those places as
> Count also does not exist in nsIArray.
Are you saying that you'll be sending a new patch?

Meanwhile I tried to see whether I can confirm this working. Sadly, after adding some debug I didn't see any of the code that's changed by the patch being run.

Comment 13

[:aceman]

(In reply to Jorg K (GMT+1) from comment #12)
> Are you saying that you'll be sending a new patch?

Yes.

> Meanwhile I tried to see whether I can confirm this working. Sadly, after
> adding some debug I didn't see any of the code that's changed by the patch
> being run.

It seems also the NewMessageToSelectedAddresses() may be dead code.
There does not seem any place that would set "selectedAddresses" attribute.
Even if it would I didn't find a case we would call ComposeMessage() and abResultsTree would be open so the function NewMessageToSelectedAddresses can't get any addresses from the AbView.

The addressing widget code is disabled so do not try to run it ;)

Comment 14

[:aceman]

Attachment: patch v3

Comment 15

[Jorg K (CEST = GMT+2)]

I'm a little reluctant to review anything I can't run. Why are we fixing dead code instead of just removing it?

processAllResults() has a few call sites in the file, so how do I know it's disabled?

"selectedAddresses" is used, it's an attribute here:
mailnews/addrbook/public/nsIAbView.idl
108 readonly attribute nsIArray selectedAddresses;

And the drag&drop stuff is just a mystery to me.

Ratty, you reviewed this, do you have further insights here?

Sure, I can rs+ this since it looks right, but do I do anyone a favour if if blows up later?

Comment 16

[:aceman]

(In reply to Jorg K (GMT+1) from comment #15)
> I'm a little reluctant to review anything I can't run. Why are we fixing
> dead code instead of just removing it?

Because e.g. the parts of processAllResults are only temporarily disabled. 
 
> processAllResults() has a few call sites in the file, so how do I know it's
> disabled?

It is not disabled in full. Only the autocomplete part is never run, because we do not start it in autoCompleteNextAddress. So this.searchResults is empty and we never run the code I change. But I said if we reenable that code once, the dev will notice if the lines are working or not.

> "selectedAddresses" is used, it's an attribute here:
> mailnews/addrbook/public/nsIAbView.idl
> 108 readonly attribute nsIArray selectedAddresses;

No I did not mean the attribute of nsIAbView, but the XUL attribute of "selectedaddresses" for this code:
      if (document.commandDispatcher.focusedWindow &&
          document.commandDispatcher.focusedWindow
                  .document.documentElement.hasAttribute("selectedaddresses"))
 
But I mistyped the case (it is only "selectedaddresses", not "selectedAddresses") so I found now where it is set:
https://dxr.mozilla.org/comm-central/rev/4f8486123b817d6b18bf2684a0f7e5facf725ce6/mail/components/addrbook/content/abContactsPanel.xul#21

But I still didn't find a path how ComposeMessage can be called from the contacts panel. There is a single compose window opening in abCommon.js and that used direct MailServices.compose.OpenComposeWindowWithParams(), not ComposeMessage.

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 8814636 [details] [diff] [review]
patch v3

OK, I don't want to me mean although I'm not convinced.
Put r=philip.chee, rs=jorgk.

Perhaps Ratty can repeat his r+ which I accidentally removed from a previous version and enlighten us on how he convinced himself of the goodness which is presented here ;-)

Comment 18

[Philip Chee]

Comment on attachment 8814636 [details] [diff] [review]
patch v3

(In reply to Jorg K (GMT+1) from comment #17)
> Perhaps Ratty can repeat his r+ which I accidentally removed from a previous
> version and enlighten us on how he convinced himself of the goodness which
> is presented here ;-)

This patch is quite reasonable, non-intrusive, and avoids mission creep. Hence r+

>    var params = Components.classes["@mozilla.org/messengercompose/composeparams;1"].createInstance(Components.interfaces.nsIMsgComposeParams);
>    if (params) {
>      params.type = type;
>      params.format = format;
>      params.identity = identity;
>      var composeFields = Components.classes["@mozilla.org/messengercompose/composefields;1"].createInstance(Components.interfaces.nsIMsgCompFields);
>      if (composeFields) {
>        var addressList = "";
> -      for (var i = 0; i < addresses.Count(); i++) {
> -        addressList = addressList + (i > 0 ? ",":"") + addresses.QueryElementAt(i,Components.interfaces.nsISupportsString).data;
> +      for (var i = 0; i < addresses.length; i++) {
> +        addressList = addressList + (i > 0 ? ",":"") +
> +          addresses.queryElementAt(i, Components.interfaces.nsISupportsString).data;
>        }

* mission creep warning*

      var addressList = [];
      const nsISupportsString = Components.interfaces.nsISupportsString;
      for (var i = 0; i < addresses.length; i++) {
        addressList.push(
          addresses.queryElementAt(i, nsISupportsString).data);
      }
      composeFields.to = addressList.join(",");
.....

> -          var trans = array.GetElementAt(i);
> +          var trans = array.queryElementAt(i, Components.interfaces.nsITransferable);
>            if (!trans) continue;
> -          trans = trans.QueryInterface(Components.interfaces.nsITransferable);

Since you are touching this code anyway, you might want to wrap this properly:

if (!trans)
  continue;

Comment 19

[Jorg K (CEST = GMT+2)]

Aceman, would you mind incorporating the "mission creep" here an get this landed.

Comment 20

[:aceman]

Thanks, done:
https://hg.mozilla.org/comm-central/rev/5c2bf1aadbe2a8d41477ad67277cd0a93bdea6cc