1125506 - Autocomplete when creating mailinglist in address book produces a warning about params.idKey

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[:aceman]

When creating a new mailinglist under Personal AB and typing the first member email address, I still get:
Warning: ReferenceError: reference to undefined property params.idKey
Source File:
file:///TB-bin/dist/bin/components/nsAbAutoCompleteSearch.js
Line: 393

Comment 1

[Magnus Melin [:mkmelin]]

WFM

Comment 2

[:aceman]

I still see it in current trunk.

Comment 3

[:aceman]

It can also be seen when adding addresses to the mailing list.
It can also be seen in the output of test mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch4.js.

Comment 4

[:aceman]

Attachment: patch

UseForAutocomplete() method of various addressbook types either ignore passed in identity key, or check if it is empty and behave accordingly.
So we can pass in empty string, just silence the warning.

Comment 5

[Magnus Melin [:mkmelin]]

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

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

I think I'd prefer if you use 

"idKey" in params

... at the one place it's used instead. That also saves you a function call.

Comment 6

[:aceman]

I wanted to avoid that check at all potential callers in the future :) But today there is only a single one so I can do what you say.

Comment 7

[:aceman]

Attachment: patch v2

Comment 8

[Magnus Melin [:mkmelin]]

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

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

::: mailnews/addrbook/src/nsAbAutoCompleteSearch.js
@@ +422,5 @@
>        // just going to find duplicates.
>        while (allABs.hasMoreElements()) {
>          let dir = allABs.getNext();
>          if (dir instanceof Components.interfaces.nsIAbDirectory &&
> +            dir.useForAutocomplete(("idKey" in params) ? params.idKey : "")) {

why not just

dir instanceof Components.interfaces.nsIAbDirectory &&
("idKey" in params) && dir.useForAutocomplete(params.idKey)

Comment 9

[:aceman]

Because it is not the same. We do not want to skip calling useForAutocomplete() is there is no idKey. As I said, some of the variants of the function detect if idKey is "" and do something. Even those that do not check idKey value, may return true (as some ignore idKey completely). So we would be skipping the subsequent call this._searchCards(), which would be incorrect (at least it would change the behaviour).

Comment 10

[Magnus Melin [:mkmelin]]

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

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

::: mailnews/addrbook/src/nsAbAutoCompleteSearch.js
@@ +422,5 @@
>        // just going to find duplicates.
>        while (allABs.hasMoreElements()) {
>          let dir = allABs.getNext();
>          if (dir instanceof Components.interfaces.nsIAbDirectory &&
> +            dir.useForAutocomplete(("idKey" in params) ? params.idKey : "")) {

come to think of it, null is probably more appropriate than ""

Comment 11

[:aceman]

The argument to useForAutocomplete() is a string. I'd be afraid to send it something else that does convert to who-knows-what. I still remember the problem where a boolean true is converted to string "true" when set as an element attribute. Or something like that.
Are you sure null is passed as "" into the C++ and will be so forever?

Comment 12

[Magnus Melin [:mkmelin]]

Well we've been passing it undefined, so it should be pretty safe to pass null instead ;)

It's not that the null is passed as "" into c++, it's that null and "" mozilla strings are treated equally. That part is not going to change.

Comment 13

[:aceman]

OK, pushed with that:
https://hg.mozilla.org/comm-central/rev/eda1e289e6ea3f82bc3b5790002100ca8be4f9ae