61491 - Autocomplete newsgroup names

Status: RESOLVED FIXED

(MailNews Core :: Composition, enhancement, P3)

Description

[(not reading, please use [e-mail] instead)]

feature enhancement

get autocomplete to work when I'm addressing a message to "Newsgroup:"

Comment 1

[laurel]

*** Bug 39626 has been marked as a duplicate of this bug. ***

Comment 2

[Fabian Guisset]

*** Bug 11046 has been marked as a duplicate of this bug. ***

Comment 3

[[e-mail] (gone - use [e-mail] instead)]

*** Bug 65498 has been marked as a duplicate of this bug. ***

Comment 4

[laurel]

*** Bug 101994 has been marked as a duplicate of this bug. ***

Comment 5

[[e-mail] (gone - use [e-mail] instead)]

*** Bug 105558 has been marked as a duplicate of this bug. ***

Comment 6

[[e-mail] (gone - use [e-mail] instead)]

*** Bug 174030 has been marked as a duplicate of this bug. ***

Comment 7

[Mike Cowperthwaite]

*** Bug 194153 has been marked as a duplicate of this bug. ***

Comment 8

[Mike Cowperthwaite]

*** Bug 265164 has been marked as a duplicate of this bug. ***

Comment 9

[Mike Cowperthwaite]

*** Bug 273540 has been marked as a duplicate of this bug. ***

Comment 10

[Karsten Düsterloh]

*** Bug 363563 has been marked as a duplicate of this bug. ***

Comment 11

[(not reading, please use [e-mail] instead)]

sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.

Comment 12

[Serge Gautherie (:sgautherie)]

Filter on "Nobody_NScomTLD_20080620"

Comment 14

[Mark Banner (:standard8)]

This could be a nice little student project. Here's a quick summary:

There are autocomplete modules implementing different types of autocomplete for compose [1]. The simplest is nsAbAutoCompleteMyDomain.js. There is also documentation on devmo [2].

To autocomplete for newsgroups would require implementing a module (js would be the easiest). It would have to iterate through the newsgroup names probably getting the list of accounts via nsIMsgAccountManager and then checking for newsgroup only-accounts.

[1] http://mxr.mozilla.org/comm-central/find?text=&kind=text&string=nsAbAutoCom
[2] https://developer.mozilla.org/en/How_to_implement_custom_autocomplete_search_component

Comment 16

[Lu Wei]

Can't believe this "little student project" has bugged for nearly 9 years! Maybe it's not as easy as it looks like?

Comment 17

[Jim Porter (:squib)]

I'm taking this, though it probably won't get done by the time 3.0 is released.

Comment 18

[Jim Porter (:squib)]

Attachment: (Very) rough patch

This sort of works. It causes a regression in that it autocompletes email and newsgroups no matter which type of address you're typing, but I don't know how to change the autocompletesearch value dynamically (trying to do so doesn't actually have any effect). There's probably a Better Way to do that...

There are lots of other issues too (mostly in that it brute-forces the problem instead of trying to be smart with narrowing down results).

Comment 19

[Mark Banner (:standard8)]

Using the autocomplete search parameter would seem the obvious way, but we already use that for passing the selected identity to mydomain and addrbook. Maybe you could make it a duel-value parameter or something.

Comment 20

[[e-mail]]

(In reply to comment #19)
> Using the autocomplete search parameter would seem the obvious way, but we
> already use that for passing the selected identity to mydomain and addrbook.
> Maybe you could make it a duel-value parameter or something.
Something like "identity,addr_type" perhaps, and split it up in the component?
Someone could then add an autocomplete for addr_other to make it easier to add values for other headers.

Don't forget that the newsgroup names should only be from the current server.
(Well, the server belonging to the same account as the selected identity.)

Comment 21

[Jim Porter (:squib)]

Attachment: Cleaned up patch

Ok, this patch handles identities sensibly and only autocompletes for appropriate types (i.e. newsgroup autocompletion only happens when the field is Newsgroup: or Followup-To:). The autocompletion matching is a little bit braindead (it just does a simple substring search), but I'm not sure what the ideal search method is.

This is probably ready for review, too. Who should I assign it to?

Comment 22

[Mark Banner (:standard8)]

I can have a look, but it may not be for a week or two yet. Another expert in this area is Neil (one of us could do r, the other sr).

Comment 23

[Jim Porter (:squib)]

Attachment: Patch for review

Let's try a patch that doesn't have my debug code stowing away inside it...

Comment 24

[Lu Wei]

A simple substring search may suffice, just like "subscribe to groups" interface. Few people need RegEx I think?

Comment 25

[Jim Porter (:squib)]

Well, the issue is that, currently, if I type "e" to autocomplete a newsgroup, *every* newsgroup containing an "e" anywhere will show up. If I type "e" to autocomplete an email, it will only show addresses/names starting with "e".

I don't think I'd want to match just the beginning of the newsgroup either, though. The beginning of a newsgroup name is often the least useful part. For example, if I wanted to access the C++ newsgroup, I'd rather type "c++" than have to start typing "comp.lang.c++" until it gets narrowed down enough. Maybe matching at the beginning of each name piece would work.

Comment 26

[[e-mail]]

Comment on attachment 409189 [details] [diff] [review]
Patch for review

You rather inconveniently failed to make the equivalent changes to the suite/mailnews/compose versions of the mail/components/compose files...

>+          let params = JSON.parse(awGetInputElement(i).getAttribute("autocompletesearchparam") || '{}');
You could put the "{}" in the XUL as the default attribute value.

>+    if(["addr_to", "addr_cc", "addr_bcc", "addr_reply"].indexOf(params.type) == -1)
Nit: make this array a global constant or member variable.

>+		nsNntpAutoCompleteSearch.js \
Nit: call this "News" perhaps, not "Nntp"?

>+ * Contributor(s):
>+ *   Mark Banner <[e-mail]>
You're a contributor too!

>+  getStyleAt: function getStyleAt(aIndex) {
>+    return "local-news";
I think the local/remote distinction only makes sense for abook, not news.
Also, you need to add CSS to messengercompose.css for the -moz-tree-image.

>+    let numAccounts = msgAccountManager.accounts.Count();
Please put msgAccountManager.accounts in its own local variable.

>+    let ident = msgAccountManager.getIdentity(idKey);
Not used?

>+      let account = msgAccountManager.accounts.GetElementAt(i)
>+        .QueryInterface(Components.interfaces.nsIMsgAccount);
QueryElementAt(i, Components.interfaces.nsIMsgAccount);

>+        let numIdents = account.identities.Count();
Please cache identities too.

>+          if (account.identities.GetElementAt(j).QueryInterface(
>+                Components.interfaces.nsIMsgIdentity).key == idKey) {
QueryElementAt again.

>+      let groups = this.cachedServer.rootFolder.subFolders;
I wonder whether it's worth searching unsubscribed groups too? (Presumably as a follow-up, as I don't see an easy way to do it.)

>+            value: curr.prettiestName,
>+            comment: this.cachedServer.prettyName
Remind me what the difference between the two is please?

Comment 27

[Jim Porter (:squib)]

Ok, I addressed most of the issues, but I have a couple of questions before I upload the revised patch:

(In reply to comment #26)
> >+  getStyleAt: function getStyleAt(aIndex) {
> >+    return "local-news";
> I think the local/remote distinction only makes sense for abook, not news.
> Also, you need to add CSS to messengercompose.css for the -moz-tree-image.

Would "local-news"/"remote-news" be a good distinction for subscribed/unsubscribed newsgroups? Maybe "(un)subscribed-news" would be better names for them, though. (This is aside from the fact that I don't look at unsubscribed groups yet).

> >+        let numIdents = account.identities.Count();
> Please cache identities too.

Is there a way of invalidating the cached identities when necessary? That function should only be executing if you changed the currently-selected identity.

> >+            value: curr.prettiestName,
> >+            comment: this.cachedServer.prettyName
> Remind me what the difference between the two is please?

The value is the name of the group, and the comment is the name of the server. I suppose the comment isn't strictly necessary, since we're only getting results from one server, but I thought it would help clarify which newsgroup you're actually sending to. I can remove it though; I'm not particularly attached to it.

Comment 28

[[e-mail]]

I found an issue indirectly caused by your patch; minResultsForPopup currently depends on a) whether autocompleteToMyDomain is on or b) LDAP is enabled; when it's set to 3 you'll never see the second newsgroup if exactly two match.

(In reply to comment #27)
> (In reply to comment #26)
> > >+  getStyleAt: function getStyleAt(aIndex) {
> > >+    return "local-news";
> > I think the local/remote distinction only makes sense for abook, not news.
> > Also, you need to add CSS to messengercompose.css for the -moz-tree-image.
> Would "local-news"/"remote-news" be a good distinction for
> subscribed/unsubscribed newsgroups? Maybe "(un)subscribed-news" would be better
> names for them, though. (This is aside from the fact that I don't look at
> unsubscribed groups yet).
Not sure; I'd have to ask Standard8 to chime in on that.

> > >+        let numIdents = account.identities.Count();
> > Please cache identities too.
I meant in a local variable. Sorry for the confusion.

> > >+            value: curr.prettiestName,
> > >+            comment: this.cachedServer.prettyName
> > Remind me what the difference between the two is please?
And I misread this - I thought the comment was the group's pretty name; the reason I mention this is that I wasn't sure which name you were completing. (Netscape 4 supported something called that it called the prettiest name which was a human-readable description of the newsgroup.)

Comment 29

[Jim Porter (:squib)]

(In reply to comment #28)
> I found an issue indirectly caused by your patch; minResultsForPopup currently
> depends on a) whether autocompleteToMyDomain is on or b) LDAP is enabled; when
> it's set to 3 you'll never see the second newsgroup if exactly two match.

Looking through the code, it seems that minResultsForPopup is only set to 3 when the field would autocomplete for your domain, presumably so that the autocompleteToMyDomain doesn't show up if you have exactly one match in your address book. The easy way to fix this would be to eliminate that entirely and just always set minResultsForPopup to 2. Granted, this is a change in behavior, but 1) autocompleteToMyDomain is disabled by default, and 2) if you explicitly enable it, I'd suppose that you want it to show up all the time. If nothing else, it would simplify the code a fair bit. :)

Comment 30

[Mark Banner (:standard8)]

Sorry for not getting to look at this earlier.

(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > >+  getStyleAt: function getStyleAt(aIndex) {
> > > >+    return "local-news";
> > > I think the local/remote distinction only makes sense for abook, not news.
> > > Also, you need to add CSS to messengercompose.css for the -moz-tree-image.
> > Would "local-news"/"remote-news" be a good distinction for
> > subscribed/unsubscribed newsgroups? Maybe "(un)subscribed-news" would be better
> > names for them, though. (This is aside from the fact that I don't look at
> > unsubscribed groups yet).
> Not sure; I'd have to ask Standard8 to chime in on that.

I'm not sure I see a real case for unsubscribed newsgroups, personally, I definitely wouldn't want it by default as there are many newsgroups I don't subscribe to on a server and wouldn't want to have my autocomplete 'polluted' by them. I guess there could be a hidden option though.

(In reply to comment #29)
> (In reply to comment #28)
> > I found an issue indirectly caused by your patch; minResultsForPopup currently
> > depends on a) whether autocompleteToMyDomain is on or b) LDAP is enabled; when
> > it's set to 3 you'll never see the second newsgroup if exactly two match.
> 
> Looking through the code, it seems that minResultsForPopup is only set to 3
> when the field would autocomplete for your domain, presumably so that the
> autocompleteToMyDomain doesn't show up if you have exactly one match in your
> address book. The easy way to fix this would be to eliminate that entirely and
> just always set minResultsForPopup to 2. Granted, this is a change in behavior,
> but 1) autocompleteToMyDomain is disabled by default, and 2) if you explicitly
> enable it, I'd suppose that you want it to show up all the time. If nothing
> else, it would simplify the code a fair bit. :)

Could we modify it so that if a newsgroup is selected then we minResultsForPopup is 2 (otherwise 3)?


Also, this patch currently breaks the autocomplete tests for the address book (in mailnews/addrbook/test). You need to fix those up, and it would be good to get some tests for your newsgroup specific code as well - you should be able to base them on the existing address book tests and some of the newsgroup code (ask if you need help).

Comment 31

[Magnus Melin [:mkmelin]]

(In reply to comment #30)
> I'm not sure I see a real case for unsubscribed newsgroups, personally, I
> definitely wouldn't want it by default as there are many newsgroups I don't
> subscribe to on a server and wouldn't want to have my autocomplete 'polluted'
> by them. I guess there could be a hidden option though.

Non-subscribed groups could be pretty useful for setting follow-ups. I wonder if this couldn't be handled by simply giving the subscribed groups priority so they show up higher in the autocomplete results.

Comment 32

[Mark Banner (:standard8)]

Jim, would you still be able to get a new patch up?

If you need help with the tests, just let me know.

Comment 33

[Jens Müller (:tessarakt)]

(In reply to comment #28)
> (Netscape 4 supported something called that it called the prettiest name which
> was a human-readable description of the newsgroup.)

I think this is usually called "tagline" (the charter is AFAIK not available from the NNTP server).

Comment 34

[Jim Porter (:squib)]

(In reply to comment #32)
> Jim, would you still be able to get a new patch up?
> 
> If you need help with the tests, just let me know.

I've been tinkering with this again, and the situation has gotten worse. For reasons I haven't quite been able to figure out, the autocomplete search param is always the param for the first row. This makes changing the autocomplete type based on the selected address type difficult/impossible.

Comment 37

[Jim Porter (:squib)]

Unassigning myself from this, since I'm unlikely to work on it in the near future.

Comment 38

[Jens Müller (:tessarakt)]

Would it be a good approach to take Jim's latest patch and try to get it working? I'd like to get involved into Thunderbird a bit, and this might be a good start.

Another thing: Is there a description of the expected visible behavior of this change? Where I work, we call it "system description", and it is quite useful.

Comment 39

[:aceman]

Bwinton is the current UI guy for Thunderbird, you can ask him what he would expect it to look.

Comment 40

[Jens Müller (:tessarakt)]

Attachment: Jim Porter's patch, de-bitrotted

Just de-bitrotted - not ready for review!

Comment 41

[Jens Müller (:tessarakt)]

(In reply to [e-mail] from comment #26)
> >+ * Contributor(s):
> >+ *   Mark Banner <[e-mail]>
> You're a contributor too!

I have been told on #maildev that the new license header should be used now:

/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Do I have to ask someone (especially, Jim Porter) to do that? The license block in Jim's latest patch mentioned original code developed by Mark Banner and Mozilla Messaging. From a legal perspective, can these notes be safely removed?

Thanks!

Comment 42

[Patrick Cloke [:clokep]]

(In reply to Jens Müller from comment #41)
> Do I have to ask someone (especially, Jim Porter) to do that? The license
> block in Jim's latest patch mentioned original code developed by Mark Banner
> and Mozilla Messaging. From a legal perspective, can these notes be safely
> removed?
> 
> Thanks!

No you do not need to ask, all of c-c was changed to the MPL 2 at one point. MPL 1.1 states you can use any later version.

Comment 43

[Jens Müller (:tessarakt)]

And to properly record the answer to the other part of the question, answered on IRC: The change to MPL 2.0 also removed the requirement to list the name of authors/contributors, so that part of the license block in the old patch is obsolete as well.

Comment 44

[Jens Müller (:tessarakt)]

Attachment: De-bitrotted patch, with most review comments applied

Comment 45

[Jens Müller (:tessarakt)]

I de-bitrotted the patch and applied most review comments.

Open issues are:
* narrow done search results more intelligently
* set minResultsForPopup to 2 everywhere?
* As Jim already noted, the _first_ row is used to determine the kind of autocomplete used. So when first row is "To:" and second is "Newsgroup:", newsgroup autocompletion won't work. The same probably applies vice versa. And that would be a really common case: Writing a newsgroup posting and putting someone on Cc: 
* equivalent changes to the suite/mailnews/compose
* possibly remove the server name. It is not _that_ useful indeed.

Related issues to be tackled in follow-up bugs:
 * Also autocomplete to names of unsubscribed newsgroup
 * Download taglines from the server, and use them at various places (subscription dialog, tooltip in the three-pane-window, and autocompletion).
 * possibly only match at start of newsgroup name components. I guess we should wait for user feedback on the solution with its current scope.
 * When pressing "Write" while on an newsgroup account, pre-set the first row in the compose window to "Newsgroup". This is something I noticed while testing.

Comment 46

[Jens Müller (:tessarakt)]

(In reply to [e-mail] from comment #26)
> Also, you need to add CSS to messengercompose.css for the -moz-tree-image.

All of 
mail/themes/pinstripe/mail/compose/messengercompose.css
mail/themes/qute/mail/compose/messengercompose.css
mail/themes/qute/mail/compose/messengercompose-aero.css
mail/themes/gnomestripe/mail/compose/messengercompose.css
?

(And when ported to suite: All of
suite/themes/classic/mac/messenger/messengercompose/messengercompose.css
suite/themes/classic/messenger/messengercompose/messengercompose.css
suite/themes/modern/messenger/messengercompose/messengercompose.css
?)

Comment 47

[Jens Müller (:tessarakt)]

(In reply to Jens Müller from comment #46)
> (In reply to [e-mail] from comment #26)
> > Also, you need to add CSS to messengercompose.css for the -moz-tree-image.

Umm, when exactly is that icon shown? I didn't manage to get any icons (for the other types) shown ... I guess they should be left of the autocomplete-result list entries?

And which icon to use here? Is there an existing icon where I can just enter the path? And is there a list of all available icons somewhere? I guess theme authors would need such a list as well ...

Comment 48

[Jens Müller (:tessarakt)]

(In reply to Jim Porter (:squib) from comment #34)
> (In reply to comment #32)
> > Jim, would you still be able to get a new patch up?
> > 
> > If you need help with the tests, just let me know.
> 
> I've been tinkering with this again, and the situation has gotten worse. For
> reasons I haven't quite been able to figure out, the autocomplete search
> param is always the param for the first row. This makes changing the
> autocomplete type based on the selected address type difficult/impossible.

I found this in addressingWidgetOverlay.js:

       //this copies the autocomplete sessions list from recipient#1
       input[0].syncSessions(document.getElementById('addressCol2#1'));

and it looked kind of fishy to me. So I commented out the second line - now it works!

Does someone now what this is supposed to do? Should I do anything else beyond just removing it?

Best,

Jens

Comment 49

[Jens Müller (:tessarakt)]

Documentation:
https://developer.mozilla.org/en-US/docs/XUL/Method/syncSessions (very short)

Here it is explained/defined:
http://mxr.mozilla.org/comm-central/source/mozilla/xpfe/components/autocomplete/resources/content/autocomplete.xml

609       <!-- make this widget listen to all of the same autocomplete sessions 
610            from another autocomplete widget -->

I don't know why one would want that. Is it for performance or for functional purposes?

Comment 50

[[e-mail]]

(In reply to Jim Porter from comment #34)
> I've been tinkering with this again, and the situation has gotten worse. For
> reasons I haven't quite been able to figure out, the autocomplete search
> param is always the param for the first row. This makes changing the
> autocomplete type based on the selected address type difficult/impossible.
This is because the standard autocomplete options (mydomain/addrbook) use the param to work out what they should be autocompleting.

(In reply to Jens Müller from comment #48)
> I found this in addressingWidgetOverlay.js:
> 
>        //this copies the autocomplete sessions list from recipient#1
>        input[0].syncSessions(document.getElementById('addressCol2#1'));
> 
> Does someone now what this is supposed to do?

syncSessions exists so that LDAP autocomplete works on all rows, not just the first. This is with old-style LDAP autocomplete of course; --enable-incomplete-toolkit-ldap-autocomplete uses the autocomplete param instead, to match mydomain/addrbook autocomplete.

Note that these autocompletes expect param to be the identity key, and this is reset whenever you choose an identity from the From: dropdown (see LoadIdentity in MsgComposeCommands.js).

Comment 51

[Jens Müller (:tessarakt)]

(In reply to [e-mail] from comment #50)
> (In reply to Jim Porter from comment #34)
> > I've been tinkering with this again, and the situation has gotten worse. For
> > reasons I haven't quite been able to figure out, the autocomplete search
> > param is always the param for the first row. This makes changing the
> > autocomplete type based on the selected address type difficult/impossible.
> This is because the standard autocomplete options (mydomain/addrbook) use
> the param to work out what they should be autocompleting.

Jim's patch set the param fine, IMO. I think it just had no effect due to the syncSessions.

> (In reply to Jens Müller from comment #48)
> > I found this in addressingWidgetOverlay.js:
> > 
> >        //this copies the autocomplete sessions list from recipient#1
> >        input[0].syncSessions(document.getElementById('addressCol2#1'));
> > 
> > Does someone now what this is supposed to do?
> 
> syncSessions exists so that LDAP autocomplete works on all rows, not just
> the first. This is with old-style LDAP autocomplete of course;
> --enable-incomplete-toolkit-ldap-autocomplete uses the autocomplete param
> instead, to match mydomain/addrbook autocomplete.

Is that bug 452232? 

Will it be finished soon, or should I try a more complicated solution that works with the old LDAP autocomplete?

> Note that these autocompletes expect param to be the identity key, and this
> is reset whenever you choose an identity from the From: dropdown (see
> LoadIdentity in MsgComposeCommands.js).

I will look at home. The current patch adjusts autocompleteparams in different functions, so I hope that case is already handled as well.

 -- Jens

Comment 52

[[e-mail]]

(In reply to Jens Müller from comment #51)
> (In reply to comment #50)
> > (In reply to Jim Porter from comment #34)
> > > I've been tinkering with this again, and the situation has gotten worse. For
> > > reasons I haven't quite been able to figure out, the autocomplete search
> > > param is always the param for the first row. This makes changing the
> > > autocomplete type based on the selected address type difficult/impossible.
> > This is because the standard autocomplete options (mydomain/addrbook) use
> > the param to work out what they should be autocompleting.
> Jim's patch set the param fine, IMO. I think it just had no effect due to
> the syncSessions.
You're right, there is a major bug in syncSessions that means that all the autocompletes use the param from the first one. This means bad news if you ever delete the first row. Oops.

> > (In reply to Jens Müller from comment #48)
> > > I found this in addressingWidgetOverlay.js:
> > > 
> > >        //this copies the autocomplete sessions list from recipient#1
> > >        input[0].syncSessions(document.getElementById('addressCol2#1'));
> > > 
> > > Does someone now what this is supposed to do?
> > syncSessions exists so that LDAP autocomplete works on all rows, not just
> > the first. This is with old-style LDAP autocomplete of course;
> > --enable-incomplete-toolkit-ldap-autocomplete uses the autocomplete param
> > instead, to match mydomain/addrbook autocomplete.
> Will it be finished soon, or should I try a more complicated solution that
> works with the old LDAP autocomplete?
No, it won't be finished soon, unless you finish it (joke!)

> > Note that these autocompletes expect param to be the identity key, and this
> > is reset whenever you choose an identity from the From: dropdown (see
> > LoadIdentity in MsgComposeCommands.js).
> I will look at home. The current patch adjusts autocompleteparams in
> different functions, so I hope that case is already handled as well.
Indeed, this wasn't the problem after all.

Comment 53

[[e-mail]]

Attachment: Possible syncSessions fix

This might fix the syncSessions bug, but I'm not 100% sure it works with LDAP.

Comment 54

[Jens Müller (:tessarakt)]

(In reply to [e-mail] from comment #53)
> Created attachment 662104 [details] [diff] [review]
> Possible syncSessions fix
> 
> This might fix the syncSessions bug, but I'm not 100% sure it works with
> LDAP.

I hope it does - that is the whole reason we cannot just drop the syncSessions ...

Do you know a (dummy) LDAP server I could use for testing?

 -- Jens

Comment 55

[[e-mail]]

(In reply to Jens Müller from comment #54)
> Do you know a (dummy) LDAP server I could use for testing?
David Bienvenu used to use the University of Washington
Hostname: directory.washington.edu Base DN: O=University of Washington,C=US

Comment 56

[Jens Müller (:tessarakt)]

(In reply to [e-mail] from comment #53)
> Created attachment 662104 [details] [diff] [review]
> Possible syncSessions fix
> 
> This might fix the syncSessions bug, but I'm not 100% sure it works with
> LDAP.

I applied it, enabled LDAP, and it produces very weird results. I might manage to write them down some time later ... Basically, newsgroup lines autocomplete to LDAP addresses in many cases. I have tried to achieve the opposite, but that doesn't seem to occur.

On stdout/stderr I get messages like "search "ldap" not found - skipping." and "###!!! ASSERTION: We don't know what went wrong with the ldap operation: 'Error', file /home/jens/devel/mozilla-comm-central/ldap/xpcom/src/nsLDAPConnection.cpp, line 661".

Any ideas?

Best,

Jens

Comment 57

[Jens Müller (:tessarakt)]

Scenario 1:
* Compose new message
* Change first address row to "Newsgroup"
* enter "John"
-> auto-completion to a John from the LDAP directory

Scenario 2:
* Compose new message
* leave first row at "To", enter John, choose an LDAP entry, press Enter
* In the second line, change to "Newsgroup"
* Enter "John"
 -> autocompleted to a name from LDAP
* enter a string that occurs in a newsgroup name
 -> newsgroup entries appear, but also a name from LDAP

My assumption is that LDAP autocomplete somehow gets "hardwired" to the elements and never gets disconnected.

So my questions now are:
 * How does the LDAP autocomplete functionality (listener or whatever) get connected to the textbox in the first place?
 * How is it possible to disconnect it?

If I know that, I can provide the appropriate explicit actions when address-row types are changed etc.

Comment 58

[Jens Müller (:tessarakt)]

OK, got it:

setupLdapAutocompleteSession() sets up an LDAP autocomplete listener for the first row (if LDAP autocompletion is enabled).

For each input element, the following happens:

let autoCompleteWidget = document.getElementById("addressCol2#" + i);
if (autoCompleteWidget)
{
  autoCompleteWidget.addSession(LDAPSession);
}

In ReleaseAutoCompleteState(),
for (let i = 1; i <= maxRecipients; i++)
    document.getElementById("addressCol2#" + i).removeSession(gLDAPSession);
is done.

So I have to just _create_ the LDAP session in those two functions (and cache it in a global variable, as is already done), and call addSession/removeSession in the same functions where the params are also set.

A call to syncSessions will no longer be necessary.

Comment 59

[[e-mail]]

(In reply to Jens Müller from comment #58)
> So I have to just _create_ the LDAP session in those two functions (and
> cache it in a global variable, as is already done), and call
> addSession/removeSession in the same functions where the params are also set.
You already have gLDAPSession, no need for a new one.

> A call to syncSessions will no longer be necessary.
Presumably because you will manually manipulate the LDAP sessions in _awSetAutoComplete() and setupLdapAutocompleteSession() instead?

Comment 60

[Jens Müller (:tessarakt)]

Attachment: working LDAP autocomplete

LDAP autocomplete now works properly. LDAP sessions are now added/removed manually.

Comment 61

[Jens Müller (:tessarakt)]

Changes for SeaMonkey were straightforward.

Now I am wondering about the CSS (comment #46, comment #47).

Comment 62

[Jens Müller (:tessarakt)]

(In reply to Mark Banner (:standard8) from comment #30)
> > Looking through the code, it seems that minResultsForPopup is only set to 3
> > when the field would autocomplete for your domain, presumably so that the
> > autocompleteToMyDomain doesn't show up if you have exactly one match in your
> > address book. The easy way to fix this would be to eliminate that entirely and
> > just always set minResultsForPopup to 2. Granted, this is a change in behavior,
> > but 1) autocompleteToMyDomain is disabled by default, and 2) if you explicitly
> > enable it, I'd suppose that you want it to show up all the time. If nothing
> > else, it would simplify the code a fair bit. :)
> 
> Could we modify it so that if a newsgroup is selected then we
> minResultsForPopup is 2 (otherwise 3)?


Currently, I have always set minResultsForPopup to 2. Is this ok? Or should I put back the old behavior?

Comment 63

[Jens Müller (:tessarakt)]

The icons are only shown on Mac in Thunderbird, and in all themes in Seamonkey. I add the style for treechildren::-moz-tree-image(subscribed-news) in those places.

Open issues are:

1. Is it ok to set minResultsForPopup to 2 in all cases?

2. https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIAutoCompleteSearch says:

"Results can be sent to the listener either synchronously or asynchronously, depending on the implementation."

Do I read this correctly as requiring that the listener MUST (or at least should) be called in any case?

The original patch introduced checks at the beginning of several mailnews-related autocomplete components that just return, without calling the listener. IMO they should call the observer. I am not sure, however, whether RESULT_IGNORED or RESULT_FAILURE or even RESULT_NOMATCH should be used.

On a side note, https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIAutoCompleteResult is very rudimentary ...

Comment 64

[[e-mail]]

(In reply to Jens Müller from comment #63)
> 1. Is it ok to set minResultsForPopup to 2 in all cases?
[You'll need ui-review, but this is an edge case, as you would need to turn autocomplete my domain on, and then the difference is that when you have a single addressbook match, you would still get the popup.]

> Do I read this correctly as requiring that the listener MUST (or at least
> should) be called in any case?
I believe it must, otherwise for instance the red "no match" styling won't get applied.

> I am not sure, however, whether RESULT_IGNORED or RESULT_FAILURE or even
> RESULT_NOMATCH should be used.
Any of them will work. Returning no results also works. Even returning results for the wrong search string works. My preference is for RESULT_IGNORED.

Comment 65

[Jens Müller (:tessarakt)]

Attachment: Jim Porter's patch improved a bit and working, ready for review

Jim, thanks for the original patch. It's now working, has unit tests, also addresses Seamonkey, and has CSS for Mac and Seamonkey for the new autocomplete result types.

Neil, you already reviewed the old patch. So can you do this review as well, please?

Comment 66

[Jens Müller (:tessarakt)]

Autocomplete works in Seamonkey, too.

However, LDAP autocomplete works only for the first address row in Seamonkey. Any ideas why this is so? I have not found differences between my changes to Thunderbird and Semonkey ...

A followup bug for Seamonkey, btw, is that an address card is shown for all kinds of fields, including newsgroups (not in the autocomplete result list, but afterwards in the field).

Comment 67

[[e-mail]]

Comment on attachment 663102 [details] [diff] [review]
Jim Porter's patch improved a bit and working, ready for review

I haven't looked at anything called mail/* or */test*.

>+    var applicable = (this.applicableHeaders.indexOf(params.type) != -1)
[Not sure this was worth its own variable. Anyway, you forgot the semicolon!]

>+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>+
>+
>+const ACR = Components.interfaces.nsIAutoCompleteResult;
>+const nsIAutoCompleteResult = Components.interfaces.nsIAutoCompleteResult;
What's going on here? Also, double blank line.

>+  getValueAt: function getValueAt(aIndex) {
>+    return this._searchResults[aIndex].value;
>+  },
There's a new method, getLabelAt, which we should implement. (Not sure if we actually use it yet.)

>+  /**
>+   * Find the newsgroup server associated with the current identity.
>+   *
>+   * @param idKey  The key of the current identity.
>+   * @return       The incoming news server (or null if one does not exist).
>+   */
We should just pass in the selected account in the params, like we do for the identity.

>+  // For component registration
>+  classDescription: "Newsgroup Autocomplete",
>+  classID: Components.ID("e9bb3330-ac7e-11de-8a39-0800200c9a66"),
>+  contractID: "@mozilla.org/autocomplete/search;1?name=news",
Only the classID is needed these days.

>+    if(["addr_newsgroups", "addr_followup"].indexOf(params.type) == -1) {
>+      result.searchResult = ACR.RESULT_SUCCESS;
Are you sure?

>+      let groups = this.cachedServer.rootFolder.subFolders;
[There is almost a way of searching all folders on the server, using the setSearchValue, rowCount and getCellText functions. Unfortunately you need a tree to be able to call getCellText...]

>+      result.defaultIndex = 0;
Is this a good idea? (Assuming it makes any difference.)

>diff --git a/run-tests.sh b/run-tests.sh
>new file mode 100755
>index 0000000..7db9eef
>--- /dev/null
>+++ b/run-tests.sh
>@@ -0,0 +1,6 @@
>+#!/bin/bash
>+cd obj-x86_64-unknown-linux-gnu
>+make -C mailnews/addrbook/test/ xpcshell-tests
>+make -C mailnews/news/test/ xpcshell-tests
>+cd ..
???

>+                  if (autoCompleteWidget && 
>+                    ["addr_newsgroups", "addr_followup"].indexOf(typeWidget.value) == -1 )
Nit: no space before )

>+          let params = JSON.parse(awGetInputElement(i).getAttribute("autocompletesearchparam"));
>+          params.idKey = idKey;
>+          awGetInputElement(i).setAttribute("autocompletesearchparam", JSON.stringify(params));
>+          awGetInputElement(i).removeSession(gLDAPSession);
Why does the session get removed in this case?

>       //this copies the autocomplete sessions list from recipient#1 
>-      input[0].syncSessions(document.getElementById('addressCol2#1'));
>+      //input[0].syncSessions(document.getElementById('addressCol2#1'));
Don't just comment it out, delete it!

>+    if ( ["addr_newsgroups", "addr_followup"].indexOf(selectElem.value) == -1 ){
>+      inputElem.addSession(gLDAPSession);
>+    } else {
>+      inputElem.removeSession(gLDAPSession);
>+    }
I'm not sure that the element likes you adding the session multiple times. Probably safest to remove the session, then add it if necessary.

I don't expect you to agree but it might simplify the patch to split it into pieces - switching autocompletesearchparam to JSON, adding the address type to the JSON, adding the account key to the JSON, adding the LDAP session manually, adding the news autocomplete.

Comment 68

[Jens Müller (:tessarakt)]

I'm working on this patch again. It's quite hard to split it up to split it up into so many parts, and see which detail belongs where ... But maybe it's a good exercise.

Comment 69

[Jens Müller (:tessarakt)]

Mark, can you confirm that handling LDAP sessions manually here has become obsolete with the landing of bug 452232? I am currently debitrotting the patch here.

Comment 70

[Jens Müller (:tessarakt)]

Attachment: Part 1: Switch autocompletesearchparam to JSON

Comment 71

[Jens Müller (:tessarakt)]

Attachment: Part 2: Add address type to JSON

Comment 72

[Jens Müller (:tessarakt)]

Attachment: Part 3: Change minresultsforpopup

Comment 73

[Jens Müller (:tessarakt)]

Attachment: Part 4: Switch existing autocomplete modules to JSON, let them check address type instead of manually enabling/disabling autocompletion

Comment 74

[Jens Müller (:tessarakt)]

Attachment: Part 5: Implement autocomplete module for news

Comment 75

[Jens Müller (:tessarakt)]

Attachment: Part 6: Adjust existing tests; test for new functionality

Comment 76

[Jens Müller (:tessarakt)]

Hi Neil,

(In reply to [e-mail] from comment #67)
> Comment on attachment 663102 [details] [diff] [review]
> Jim Porter's patch improved a bit and working, ready for review
> 
> I haven't looked at anything called mail/* or */test*.
> 
> >+    var applicable = (this.applicableHeaders.indexOf(params.type) != -1)
> [Not sure this was worth its own variable. Anyway, you forgot the semicolon!]

I think it is much more readable with it. Do you think it is a big performance hit? Semicolon is fixed.

> >+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> >+
> >+
> >+const ACR = Components.interfaces.nsIAutoCompleteResult;
> >+const nsIAutoCompleteResult = Components.interfaces.nsIAutoCompleteResult;
> What's going on here? Also, double blank line.

Only ACR remains now.

> >+  getValueAt: function getValueAt(aIndex) {
> >+    return this._searchResults[aIndex].value;
> >+  },
> There's a new method, getLabelAt, which we should implement. (Not sure if we
> actually use it yet.)

I added it, and have it return the same value as getValueAt.

> >+  /**
> >+   * Find the newsgroup server associated with the current identity.
> >+   *
> >+   * @param idKey  The key of the current identity.
> >+   * @return       The incoming news server (or null if one does not exist).
> >+   */
> We should just pass in the selected account in the params, like we do for
> the identity.

Done.

> >+  // For component registration
> >+  classDescription: "Newsgroup Autocomplete",
> >+  classID: Components.ID("e9bb3330-ac7e-11de-8a39-0800200c9a66"),
> >+  contractID: "@mozilla.org/autocomplete/search;1?name=news",
> Only the classID is needed these days.

I removed the contractID.

> >+    if(["addr_newsgroups", "addr_followup"].indexOf(params.type) == -1) {
> >+      result.searchResult = ACR.RESULT_SUCCESS;
> Are you sure?

Replaced with ACR.RESULT_IGNORED.

> >+      let groups = this.cachedServer.rootFolder.subFolders;
> [There is almost a way of searching all folders on the server, using the
> setSearchValue, rowCount and getCellText functions. Unfortunately you need a
> tree to be able to call getCellText...]

So no need to change anything?

> >+      result.defaultIndex = 0;
> Is this a good idea? (Assuming it makes any difference.)

"Index of the default item that should be entered if none is selected"/

Your point is that maybe there should be no default item at all?

> >diff --git a/run-tests.sh b/run-tests.sh
> [...]
> ???

removed.

> >+                  if (autoCompleteWidget && 
> >+                    ["addr_newsgroups", "addr_followup"].indexOf(typeWidget.value) == -1 )
> Nit: no space before )

Fixed.

> [stuff related to LDAP sessions]

This has become obsolete with bug 452232.

> 
> I don't expect you to agree but it might simplify the patch to split it into
> pieces - switching autocompletesearchparam to JSON, adding the address type
> to the JSON, adding the account key to the JSON, adding the LDAP session
> manually, adding the news autocomplete.

I did it similar to the structure you suggest here. However, adjusting existing autocomplete modules to the changes done in Part 1 and Part 2 is only done in Part 4. Tests for everything are fixed in Part 6.

If you want me to move the relevant changes from Part 4 and 6 into Part 1 and 2, I could do so as well.

Thanks for the review of the first patch.

Regards,

Jens

Comment 77

[Jens Müller (:tessarakt)]

As far as I can see, all this is Compose-related, so asking review from you, Mike.

Blake, what is the preferred way to do UI review? I see three relevant points:
 * The change of minresultsforpopup in all cases.
 * The search behaviour of the new autocomplete feature for newsgroup names. Currently, the typed text has to appear as a substring anywhere in the newsgroup name. Perhaps it should rather match only at the beginning?
 * As the icon for the newsgroup autocomplete entries, I used the existing folder-newsgroup.png.

BR, Jens

Comment 78

[Jens Müller (:tessarakt)]

Attachment: For convenience: Part 1 to 6 in one patch.

Comment 79

[Jens Müller (:tessarakt)]

Arxy, can you push attachment 785465 [details] [diff] [review] to tryserver for me, please?

Comment 80

[Jens Müller (:tessarakt)]

Attachment: For convenience: Only the Seamonkey-specific parts

Comment 81

[Jens Müller (:tessarakt)]

Ian,

I am giving this to you as the owner of the Composition sub-module in Seamonkey. Please reassign the review if appropriate. The patch I set the r? on contains only the changes in suite/. To understand things, it might be easier to look at the entire patches, which are split up into 6 parts.

BR, Jens

Comment 82

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed to Thunderbird-Try as https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=0a30e8488e9d

Comment 83

[Jens Müller (:tessarakt)]

(In reply to Archaeopteryx [:aryx] from comment #82)
> Pushed to Thunderbird-Try as
> https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=0a30e8488e9d

Thanks, aryx.

Build and tests have run through now. As far as I can see, the tests show only issues for which bugs have already been filed, i.e., that were not caused by this patch.

Comment 84

[Jens Müller (:tessarakt)]

Magnus, the wanted-thunderbird3+ is obviously obsolete. Can you check whether some other wanted flag or target should be set instead?

Comment 85

[Blake Winton (:bwinton) (:☕️)]

(In reply to Jens Müller (:tessarakt) from comment #77)
> As far as I can see, all this is Compose-related, so asking review from you,
> Mike.
> 
> Blake, what is the preferred way to do UI review? I see three relevant
> points:

Set ui-r? someone, and mention the points you mentioned.  ;)

(mconley might be a decent choice, since he's already reviewing the patches…)

For what it's worth, I gave the try-build a test run, and couldn't get newsgroup names to autocomplete, so a set of steps that demonstrates the functionality would make it way easier to ui-review…

Comment 86

[Jens Müller (:tessarakt)]

I still have different issues :-(

- In the autocomplete popup list, no icons are shown.
- The address type for the first row seems to determine the applicable autocomplete modules for all rows

I hope that will not be too difficult to find.

Comment 87

[Jens Müller (:tessarakt)]

Attachment: Part 1: Switch autocompletesearchparam to JSON

Comment 88

[Jens Müller (:tessarakt)]

Attachment: Part 2: Add address type to JSON

Comment 89

[Jens Müller (:tessarakt)]

(In reply to Jens Müller (:tessarakt) from comment #86)
> - The address type for the first row seems to determine the applicable
> autocomplete modules for all rows

See bug 452232 comment 90.

When I remove that line with the call to syncSessions, everything works fine. I think this was overlooked in bug 452232 (or one of its related cases).

Comment 90

[Jens Müller (:tessarakt)]

(In reply to Jens Müller (:tessarakt) from comment #86)
> - In the autocomplete popup list, no icons are shown.

Apparently, the themes have changed considerably ...

Multiple icons are now combined into one .png and then chosen like this:

  list-style-image: url("chrome://messenger/skin/icons/folder-pane.png");
  -moz-image-region: rect(208px 16px 224px 0px);

I will have to go carefully through all the .css files again. Until then, please ignore them (especially the icon locations) in Part 5.

Comment 91

[Mark Banner (:standard8)]

(In reply to Jens Müller (:tessarakt) from comment #69)
> Mark, can you confirm that handling LDAP sessions manually here has become
> obsolete with the landing of bug 452232? I am currently debitrotting the
> patch here.

Yes they are obsolete, as are any references to gLDAPSession.

(In reply to Jens Müller (:tessarakt) from comment #84)
> Magnus, the wanted-thunderbird3+ is obviously obsolete. Can you check
> whether some other wanted flag or target should be set instead?

We don't generally use the wanted-* flags any more - although they were set, they didn't really provide much use.

Comment 92

[Jens Müller (:tessarakt)]

Attachment: Part 5: Implement autocomplete module for news

This now contains the proper icons in the CSS.

The CSS properties taken from the folderPane.css in the respective theme. The only exception is suite/themes/classic/mac/messenger/messengercompose/messengercompose.css, where no icon for newsgroups is defined in folderPane.css - therefore I have just copied the properties from the non-mac classic theme.

Comment 93

[Jens Müller (:tessarakt)]

Attachment: For convenience: Part 1 to 6 in one patch.

Comment 94

[Jens Müller (:tessarakt)]

Attachment: For convenience: Only the Seamonkey-specific parts

Comment 95

[Jens Müller (:tessarakt)]

Attachment: For convenience: Part 1 to 6 in one patch.

Comment 96

[Jens Müller (:tessarakt)]

Attachment: For convenience: Only the Seamonkey-specific parts

Comment 97

[Jens Müller (:tessarakt)]

Aryx, can you push another try build for me?

again
try: -b do -p all -u all -t none

Patches to apply:
 1. https://bug899822.bugzilla.mozilla.org/attachment.cgi?id=783416
 2. https://bug61491.bugzilla.mozilla.org/attachment.cgi?id=785858

Thanks! BR, Jens

Comment 98

[Jens Müller (:tessarakt)]

Attachment: Screenshot of autocompletion in action

The screenshot shows the icon used and that match is done on any substring.

Comment 99

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed as https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=383dffee295c - Thunderbird-Trunk is closed due to bustage, you might want to check out Standard8's xpcshell debug Try runs.

Comment 100

[Jens Müller (:tessarakt)]

Comment on attachment 785861 [details] [diff] [review]
For convenience: Only the Seamonkey-specific parts

I need to make some small corrections for the SeaMonkey part (I was not able to build SeaMonkey before yesterday so could not test). I will update the individual parts of the Patch accordingly.

Comment 101

[Jens Müller (:tessarakt)]

Attachment: Part 1: Switch autocompletesearchparam to JSON

Comment 102

[Jens Müller (:tessarakt)]

Attachment: Part 5: Implement autocomplete module for news

Comment 103

[Jens Müller (:tessarakt)]

Attachment: For convenience: Part 1 to 6 in one patch.

Comment 104

[Jens Müller (:tessarakt)]

Attachment: For convenience: Only the Seamonkey-specific parts

Comment 105

[Jens Müller (:tessarakt)]

Attachment: For convenience: Part 1 to 6 in one patch.

Comment 106

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 786480 [details] [diff] [review]
For convenience: Part 1 to 6 in one patch.

Ok, now that I've examined each piece, I'm going to try reviewing just the combined patch.

Comment 107

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 786480 [details] [diff] [review]
For convenience: Part 1 to 6 in one patch.

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

I've identified mostly style nits, but I have to say that most of this looks very sane (although I've yet to actually test it).

I'll probably test it tomorrow evening, when I'm back in my TB environment, but in the meantime, these style nits need to be fixed.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3787,4 @@
>          var idKey = identityElement.value;
>          gCurrentIdentity = MailServices.accounts.getIdentity(idKey);
>  
> +        var accountKey = null;

let, not var

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +774,4 @@
>  
>  function _awSetAutoComplete(selectElem, inputElem)
>  {
> +  var params = JSON.parse(inputElem.getAttribute('autocompletesearchparam'));

let, not var

::: mailnews/addrbook/src/nsAbAutoCompleteMyDomain.js
@@ +16,3 @@
>    cachedIdentity: null,
>  
> +  applicableHeaders: ["addr_to", "addr_cc", "addr_bcc", "addr_reply"],

Maybe it's just me, but I prefer a Javascript Set for this sort of thing. That way, instead of using indexOf == -1, we can just do:

someSet.has(x)

Also, is there a common place we could put such a Set? I'm seeing the same 4 things defined in an Array several times.

@@ +17,5 @@
>  
> +  applicableHeaders: ["addr_to", "addr_cc", "addr_bcc", "addr_reply"],
> +
> +  startSearch: function(aString, aSearchParam, aResult, aListener) {
> +    var params = JSON.parse(aSearchParam);

Use let, not var

::: mailnews/addrbook/src/nsAbAutoCompleteSearch.js
@@ +344,4 @@
>     */
>    startSearch: function startSearch(aSearchString, aSearchParam,
>                                      aPreviousResult, aListener) {
> +    var params = JSON.parse(aSearchParam);

let, not var. Apply this to all of your changes - I'll stop mentioning this now.

::: mailnews/addrbook/src/nsAbLDAPAutoCompleteSearch.js
@@ +173,4 @@
>      // result ignored.
>      // The comma check is so that we don't autocomplete against the user
>      // entering multiple addresses.
> +    if ( !applicable || !aSearchString || aSearchString.contains(",")) {

No space after (

::: mailnews/addrbook/test/unit/test_nsAbAutoCompleteMyDomain.js
@@ +37,5 @@
> +  // Set up autocomplete parameters
> +  let params = JSON.stringify({ idKey: identity.key, type: "addr_to" });
> +  let paramsNews = JSON.stringify({ idKey: identity.key, type: "addr_newsgroups" });
> +  let paramsFollowup = JSON.stringify({ idKey: identity.key, type: "addr_followup" });
> +  

Nit, trailing whitespace. There are more of these in here - use the Splinter view to see them all, and please omit those in your next patch.

@@ +96,5 @@
>    do_check_eq(obs._result.getImageAt(0), null);
>  
> +  // No autocomplete for addr_newsgroups!
> +  acs.startSearch("a@b", paramsNews, null, obsNews);
> +  do_check_true(obsNews._result==null || obsNews._result.matchCount==0 );

Spaces on either side of == - no space before )

@@ +100,5 @@
> +  do_check_true(obsNews._result==null || obsNews._result.matchCount==0 );
> +  
> +  // No autocomplete for addr_followup!
> +  acs.startSearch("a@b", paramsFollowup, null, obsFollowup);
> +  do_check_true(obsFollowup._result==null || obsFollowup._result.matchCount==0 );

Same as above

::: mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch1.js
@@ +178,5 @@
>    do_check_eq(obs._result.getImageAt(0), "");
>  
> +  // quick-check that nothing is found for addr_newsgroups
> +  acs.startSearch("email", paramNews, null, obsNews);
> +  do_check_true(obsNews._result==null || obsNews._result.matchCount==0 );

Spaces on either side of ==, no space before ). Same below.

::: mailnews/news/src/Makefile.in
@@ +21,4 @@
>  OS_CXXFLAGS += -DNOMINMAX
>  endif
>  
> +EXTRA_COMPONENTS += \

These need to go into the moz.build now.

::: mailnews/news/src/nsNewsAutoCompleteSearch.js
@@ +1,5 @@
> +/* -*- Mode: Javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ 
> +

Let's define Cu, Ci, and Cc up here, for brevity's sake.

@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ 
> +
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +const ACR = Components.interfaces.nsIAutoCompleteResult;

Either all consts should start with k, like kACR and kSupportedTypes, or be all caps. Please pick one and stick with it.

@@ +11,5 @@
> +
> +function nsNewsAutoCompleteResult(aSearchString) {
> +  // Can't create this in the prototype as we'd get the same array for
> +  // all instances
> +  this._searchResults = new Array();

this._searchResults = [];

@@ +74,5 @@
> +   * @param accountKey  The key of the account.
> +   * @return            The incoming news server (or null if one does not exist).
> +   */
> +  _findServer: function _findServer(accountKey) {
> +    let msgAccountManager =

MailServices.accounts should be used instead. Search mxr for usages of MailServices.

@@ +97,5 @@
> +  startSearch: function startSearch(aSearchString, aSearchParam,
> +                                    aPreviousResult, aListener) {
> +    var params = JSON.parse(aSearchParam);
> +    var result = new nsNewsAutoCompleteResult(aSearchString);
> +    if( !params.type || !params.accountKey || 

Space between if and (. No space after (.

@@ +104,5 @@
> +      aListener.onSearchResult(this, result);
> +      return;
> +    }
> +
> +    if(params.accountKey != this.cachedAccountKey) {

Space after if

@@ +117,5 @@
> +        if (curr.prettiestName.indexOf(aSearchString) != -1) {
> +          result._searchResults.push({
> +            value: curr.prettiestName,
> +            comment: this.cachedServer.prettyName
> +            });

Wrong indentation.

::: mailnews/news/test/unit/test_newsAutocomplete.js
@@ +30,5 @@
> +  let server = makeServer(NNTP_RFC977_handler, daemon);
> +  server.start(NNTP_PORT);
> +  
> +  // create identity
> +  var acctmgr = Cc["@mozilla.org/messenger/account-manager;1"]

MailServices.accounts

Comment 108

[Jens Müller (:tessarakt)]

(In reply to Mike Conley (:mconley) from comment #107)

> ::: mailnews/addrbook/src/nsAbAutoCompleteMyDomain.js
> @@ +16,3 @@
> >    cachedIdentity: null,
> >  
> > +  applicableHeaders: ["addr_to", "addr_cc", "addr_bcc", "addr_reply"],
> 
> Maybe it's just me, but I prefer a Javascript Set for this sort of thing.
> That way, instead of using indexOf == -1, we can just do:
> 
> someSet.has(x)


I just found https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set on this, and it says: 

"Warning: The SpiderMonkey Set implementation is a prototype and the Set API and semantics specifications are unstable. The SpiderMonkey implementation may not reflect the latest specification draft. It is subject to change any time. It is provided as an experimental feature. Do not rely on it for production code."

Is this really something that should be used already?
 
> Also, is there a common place we could put such a Set? I'm seeing the same 4
> things defined in an Array several times.

Yeah, in all the autocomplete components for mail (search, my domain, LDAP). They all import resource:///modules/mailServices.js, but I don't think that is a proper place ...

The values are defined in mail/components/compose/content/messengercompose.xul (and its suite counterpart) - 4 of them apply to mail and 2 apply to news.

Comment 109

[Mike Conley (:mconley) (:⚙️)]

(In reply to Jens Müller (:tessarakt) from comment #108)
> (In reply to Mike Conley (:mconley) from comment #107)
> 
> > ::: mailnews/addrbook/src/nsAbAutoCompleteMyDomain.js
> > @@ +16,3 @@
> > >    cachedIdentity: null,
> > >  
> > > +  applicableHeaders: ["addr_to", "addr_cc", "addr_bcc", "addr_reply"],
> > 
> > Maybe it's just me, but I prefer a Javascript Set for this sort of thing.
> > That way, instead of using indexOf == -1, we can just do:
> > 
> > someSet.has(x)
> 
> 
> I just found
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/Set on this, and it says: 
> 
> "Warning: The SpiderMonkey Set implementation is a prototype and the Set API
> and semantics specifications are unstable. The SpiderMonkey implementation
> may not reflect the latest specification draft. It is subject to change any
> time. It is provided as an experimental feature. Do not rely on it for
> production code."
> 
> Is this really something that should be used already?

I think so, yes. I can tell you that they're used quite liberally within Firefox's code.

>  
> > Also, is there a common place we could put such a Set? I'm seeing the same 4
> > things defined in an Array several times.
> 
> Yeah, in all the autocomplete components for mail (search, my domain, LDAP).
> They all import resource:///modules/mailServices.js, but I don't think that
> is a proper place ...
> 
> The values are defined in
> mail/components/compose/content/messengercompose.xul (and its suite
> counterpart) - 4 of them apply to mail and 2 apply to news.

Ok, I wouldn't push too hard on that then. It just seemed a bit silly to redefine the same collections over and over again.

Comment 110

[Jens Müller (:tessarakt)]

Attachment: Autocomplete newsgroup names (based on a patch by Jim Porter)

Comment 111

[Jens Müller (:tessarakt)]

I addressed the review comments (except for removing the multiple definitions of the set with the address types) and created a new patch. I also pushed it to the tryserver: https://hg.mozilla.org/try-comm-central/rev/2bcf17850020

Comment 112

[Jens Müller (:tessarakt)]

Comment on attachment 804114 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter)

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

::: mailnews/test/resources/localAccountUtils.js
@@ +97,4 @@
>      }
>      server.valid = true;
>  
> +    return { server: server, account: account};

OK, I guess I found another nit myself ...

No space after the {, right?

Comment 113

[Jens Müller (:tessarakt)]

Attachment: Autocomplete newsgroup names (based on a patch by Jim Porter)

Comment 114

[[e-mail]]

Comment on attachment 804897 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter)

Do the results display in a predictable order?

>+        if (curr.prettiestName.indexOf(aSearchString) != -1) {
Nit: use .contains instead

>+  inputElem.setAttribute('autocompletesearchparam',JSON.stringify(params));
Nit: space after comma

Comment 115

[Jens Müller (:tessarakt)]

(In reply to [e-mail] from comment #114)
> Comment on attachment 804897 [details] [diff] [review]
> Autocomplete newsgroup names (based on a patch by Jim Porter)
> 
> Do the results display in a predictable order?

I tried it out: It is exactly the same order in which the newsgroups are shown in the folder tree (which is the order in the newsrc file).

> >+        if (curr.prettiestName.indexOf(aSearchString) != -1) {
> Nit: use .contains instead
> 
> >+  inputElem.setAttribute('autocompletesearchparam',JSON.stringify(params));
> Nit: space after comma

Fixed.

Comment 116

[Jens Müller (:tessarakt)]

Attachment: Autocomplete newsgroup names (based on a patch by Jim Porter)

Comment 117

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 804938 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter)

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

Just some style nits, and you'll probably want a mailnews reviewer to give their blessing, but this is looking really good. r=me with the nits fixed and the tests all passing.

Can you please get this pushed to try to ensure that the tests pass on our infrastructure?[1]

Thanks,

-Mike

[1]: https://wiki.mozilla.org/ReleaseEngineering/ThunderbirdTryServer

::: mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch1.js
@@ +114,5 @@
>  
> +  let param = JSON.stringify({ type: "addr_to"  });
> +  let paramNews = JSON.stringify({ type: "addr_newsgroups"  });
> +  let paramFollowup = JSON.stringify({ type: "addr_followup"  });
> +  

Trailing whitespace.

::: mailnews/news/src/nsNewsAutoCompleteSearch.js
@@ +1,4 @@
> +/* -*- Mode: Javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ 

Trailing whitespace

@@ +36,5 @@
> +
> +  getValueAt: function getValueAt(aIndex) {
> +    return this._searchResults[aIndex].value;
> +  },
> +  

Trailing ws

::: mailnews/news/test/unit/head_server_setup.js
@@ +93,4 @@
>  const NNTP_PORT = 1024+119;
>  
>  var _server = null;
> +var _account = null;

let, not var

@@ +110,4 @@
>  function setupLocalServer(port) {
>    if (_server != null)
>      return _server;
> +  let serverAndAccount = 

trailing whitespace

::: mailnews/news/test/unit/test_newsAutocomplete.js
@@ +8,5 @@
> +
> +Components.utils.import("resource:///modules/mailServices.js");
> +
> +// The basic daemon to use for testing nntpd.js implementations
> +var daemon = setupNNTPDaemon();

let, not var. This is a global, and so it should be prefixed with a g, like gDaemon.

@@ +38,5 @@
> +  let acs = Components.classes["@mozilla.org/autocomplete/search;1?name=news"]
> +    .getService(Components.interfaces.nsIAutoCompleteSearch);
> +  let obs;
> +
> +  let paramsN = JSON.stringify({ 

Trailing whitespace on this line, 46 and 48

@@ +54,5 @@
> +
> +  // misc.test is not subscribed
> +  obs = new acObserver();
> +  acs.startSearch("misc", paramsN, null, obs);
> +  do_check_true(obs._result==null || obs._result.matchCount==0 );

Spaces on either side of the =='s on line 58, 62, 66, 80.

No spaces before ).

Comment 118

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 785877 [details]
Screenshot of autocompletion in action

I don't think this patch warrants a UI review, as it's really just re-using a pre-existing mechanism that is well understood - just making it include more stuff.

Comment 119

[Jens Müller (:tessarakt)]

Attachment: Autocomplete newsgroup names (based on a patch by Jim Porter)

Comment 120

[Jens Müller (:tessarakt)]

(In reply to Mike Conley (:mconley) from comment #117)
> Comment on attachment 804938 [details] [diff] [review]
> Autocomplete newsgroup names (based on a patch by Jim Porter)
> 
> Review of attachment 804938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just some style nits, and you'll probably want a mailnews reviewer to give
> their blessing, but this is looking really good. r=me with the nits fixed
> and the tests all passing.

All nits are fixed. I uploaded a new patch.

> Can you please get this pushed to try to ensure that the tests pass on our
> infrastructure?[1]

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=764c3467d1b7
 
> Thanks,
> 
> -Mike

Thanks for the diligent review, and sorry for all the nits you had to point out.

 - Jens

Comment 121

[Jens Müller (:tessarakt)]

Comment on attachment 806093 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter)

Neil, you reviewed an earlier version of this patch.

Could you also review the (hopefully) final version, with the hats of both a Mail and News Backend and a SeaMonkey Peer?

The patch (without the nit fixes) already got r+ from mconley.

Comment 122

[Jens Müller (:tessarakt)]

Attachment: Autocomplete newsgroup names (based on a patch by Jim Porter) - v5

Comment 123

[Jens Müller (:tessarakt)]

(In reply to Jens Müller (:tessarakt) from comment #120)
> https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=764c3467d1b7

Another try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=d1b757109b45

Comment 124

[Jens Müller (:tessarakt)]

Attachment: Autocomplete newsgroup names (based on a patch by Jim Porter) - v6

I had to add packaging specs for the new JS module ... 

Now tests seem to be running through:  https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=9717acb524e9 (not finished yet on all platforms).

Comment 125

[Jens Müller (:tessarakt)]

Comment on attachment 806402 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v6

mconley: As I had to add packaging declarations, you might want to have another look:  https://bugzilla.mozilla.org/attachment.cgi?oldid=804938&action=interdiff&newid=806402&headers=1

Comment 126

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 806402 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v6

Still looks good. Thanks Jens.

Comment 127

[[e-mail]]

Comment on attachment 806402 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v6

>-  cachedParam: "",
>+  cachedIdKey: "",
You renamed cachedParam to cachedIdKey...

>+      if (params.idKey != this.cachedParam) {
>+        this.cachedIdentity = MailServices.accounts.getIdentity(params.idKey);
>+        this.cachedParam = params.idKey;
... but you didn't rename the uses.

>+const kSupportedTypes = Set(["addr_newsgroups","addr_followup"]);
Nit: space after comma.

>+    for (let i = 0; i < numAccounts; i++) {
>+      let account = accounts.queryElementAt(i, Ci.nsIMsgAccount);
>+      if (account.key == accountKey)
What's wrong with MailServices.accounts.getAccount(accountKey)?

>+        let accountKey = null;
>+        if (identityElement.selectedItem)
>+          accountKey = identityElement.selectedItem.getAttribute("accountkey");
When does the identityElement not have a selectedItem?

>+          let params = JSON.parse(awGetInputElement(i).getAttribute("autocompletesearchparam"));
>+          params.idKey = idKey;
>+          params.accountKey = accountKey;
>+          awGetInputElement(i).setAttribute("autocompletesearchparam", JSON.stringify(params));
Please use the searchParam property.

>-    autoCompleteWidget.minResultsForPopup = 2;
You didn't change the default value for SeaMonkey.

>+  let params = JSON.parse(inputElem.getAttribute('autocompletesearchparam'));
>+  params.type = selectElem.value;
>+  inputElem.setAttribute('autocompletesearchparam', JSON.stringify(params));
searchParam again please.

Comment 128

[Jens Müller (:tessarakt)]

Hi Neil,

(In reply to [e-mail] from comment #127)
> Comment on attachment 806402 [details] [diff] [review]
> Autocomplete newsgroup names (based on a patch by Jim Porter) - v6
> 
> >-  cachedParam: "",
> >+  cachedIdKey: "",
> You renamed cachedParam to cachedIdKey...
> 
> >+      if (params.idKey != this.cachedParam) {
> >+        this.cachedIdentity = MailServices.accounts.getIdentity(params.idKey);
> >+        this.cachedParam = params.idKey;
> ... but you didn't rename the uses.
> 
> >+const kSupportedTypes = Set(["addr_newsgroups","addr_followup"]);
> Nit: space after comma.

All fixed.

> >+    for (let i = 0; i < numAccounts; i++) {
> >+      let account = accounts.queryElementAt(i, Ci.nsIMsgAccount);
> >+      if (account.key == accountKey)
> What's wrong with MailServices.accounts.getAccount(accountKey)?

Nothing, presumably. I changed it and will test it.

> >+        let accountKey = null;
> >+        if (identityElement.selectedItem)
> >+          accountKey = identityElement.selectedItem.getAttribute("accountkey");
> When does the identityElement not have a selectedItem?

If there are no accounts, maybe? Or is that not possible?

> >+          let params = JSON.parse(awGetInputElement(i).getAttribute("autocompletesearchparam"));
> >+          params.idKey = idKey;
> >+          params.accountKey = accountKey;
> >+          awGetInputElement(i).setAttribute("autocompletesearchparam", JSON.stringify(params));
> Please use the searchParam property.

I beg your pardon? Could you explain a little bit more? Which searchParam property?

> >-    autoCompleteWidget.minResultsForPopup = 2;
> You didn't change the default value for SeaMonkey.

I do it now.
 
> >+  let params = JSON.parse(inputElem.getAttribute('autocompletesearchparam'));
> >+  params.type = selectElem.value;
> >+  inputElem.setAttribute('autocompletesearchparam', JSON.stringify(params));
> searchParam again please.

Same question as above ...

Thanks a lot for the quick review!

Jens

Comment 129

[[e-mail]]

(In reply to Jens Müller from comment #128)
> (In reply to comment #127)
> > >+        let accountKey = null;
> > >+        if (identityElement.selectedItem)
> > >+          accountKey = identityElement.selectedItem.getAttribute("accountkey");
> > When does the identityElement not have a selectedItem?
> If there are no accounts, maybe? Or is that not possible?
Well, what I thought happens, is that if there are no accounts then you get prompted to add an account, and if there are still no accounts then the window closes. But I don't know whether this code runs before or after that.

> > >+          let params = JSON.parse(awGetInputElement(i).getAttribute("autocompletesearchparam"));
> > >+          params.idKey = idKey;
> > >+          params.accountKey = accountKey;
> > >+          awGetInputElement(i).setAttribute("autocompletesearchparam", JSON.stringify(params));
> > Please use the searchParam property.
> I beg your pardon? Could you explain a little bit more? Which searchParam
> property?
I meant this one, but it seems that MDN isn't quite up-to-date with textbox XPFE autocomplete properties so I need to add it and others in:
https://developer.mozilla.org/en-US/docs/XUL/Property/searchParam

Comment 130

[Jens Müller (:tessarakt)]

(In reply to [e-mail] from comment #129)
> (In reply to Jens Müller from comment #128)
> > (In reply to comment #127)
> > > >+        let accountKey = null;
> > > >+        if (identityElement.selectedItem)
> > > >+          accountKey = identityElement.selectedItem.getAttribute("accountkey");
> > > When does the identityElement not have a selectedItem?
> > If there are no accounts, maybe? Or is that not possible?
> Well, what I thought happens, is that if there are no accounts then you get
> prompted to add an account, and if there are still no accounts then the
> window closes. But I don't know whether this code runs before or after that.

I added an else branch with a dump, and no message was output. So apparently this code runs after the prompt.

> > > >+          let params = JSON.parse(awGetInputElement(i).getAttribute("autocompletesearchparam"));
> > > >+          params.idKey = idKey;
> > > >+          params.accountKey = accountKey;
> > > >+          awGetInputElement(i).setAttribute("autocompletesearchparam", JSON.stringify(params));
> > > Please use the searchParam property.
> > I beg your pardon? Could you explain a little bit more? Which searchParam
> > property?
> I meant this one, but it seems that MDN isn't quite up-to-date with textbox
> XPFE autocomplete properties so I need to add it and others in:
> https://developer.mozilla.org/en-US/docs/XUL/Property/searchParam

Thanks - fixed.

Comment 131

[Jens Müller (:tessarakt)]

Attachment: Autocomplete newsgroup names (based on a patch by Jim Porter) - v7

Comment 132

[Jens Müller (:tessarakt)]

Pushed to try once again: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=9bd82cd89fd4

Difference to previous patch: https://bugzilla.mozilla.org/attachment.cgi?oldid=806402&action=interdiff&newid=807888&headers=1

Comment 133

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 807888 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v7

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

::: mailnews/news/test/unit/test_newsAutocomplete.js
@@ +44,5 @@
> +    accountKey: _account.key,
> +    type: "addr_newsgroups" });
> +  let paramsF = JSON.stringify({
> +    idKey: identity.key,
> +    accountKey: _account.key, 

trailing whitespace

Comment 134

[Jens Müller (:tessarakt)]

Attachment: Autocomplete newsgroup names (based on a patch by Jim Porter) - v8

Comment 135

[Jens Müller (:tessarakt)]

Comment on attachment 808045 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v8

The xpcshell tests still seem to be fine: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=6acf55f2343c

Neil, I addressed all your review comments.

Mike, I changed two lines in mail/components/compose/content/MsgComposeCommands.js based on Neil's suggestions, so technically I need another Thunderbird review ...

Diff to the last version of the patch is here: https://bugzilla.mozilla.org/attachment.cgi?oldid=806402&action=interdiff&newid=808045&headers=1

Comment 136

[Magnus Melin [:mkmelin]]

Comment on attachment 808045 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v8

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

(In reply to Jens Müller (:tessarakt) from comment #135)
> Mike, I changed two lines in
> mail/components/compose/content/MsgComposeCommands.js based on Neil's
> suggestions, so technically I need another Thunderbird review ...

For such cases it's fine to just carry forward the review.

Comment 137

[[e-mail]]

Comment on attachment 808045 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v8

>+treechildren::-moz-tree-image(subscribed-news) {
>+  margin-top: 2px;
>+  margin-bottom: 2px;
>+  -moz-margin-start: 2px;
>+  -moz-margin-end: -3px;
>+  list-style-image: url("chrome://messenger/skin/icons/folder-newsgroup.png");
>+}
So, you seem to have over-zealously copied and pasted here. In particular the existing icons are all over the shop when it comes to image size, and so the margins don't make sense for the newsgroup icon. In particular the top and bottom margins cause the image to get clipped. r=me with those removed.

(The start and end margins aren't right either, but that's the problem of the remote-abook settings that you copied from, so it's OK to keep those for consistency.)

Comment 138

[Jens Müller (:tessarakt)]

(In reply to [e-mail] from comment #137)
> Comment on attachment 808045 [details] [diff] [review]
> Autocomplete newsgroup names (based on a patch by Jim Porter) - v8
> 
> >+treechildren::-moz-tree-image(subscribed-news) {
> >+  margin-top: 2px;
> >+  margin-bottom: 2px;
> >+  -moz-margin-start: 2px;
> >+  -moz-margin-end: -3px;
> >+  list-style-image: url("chrome://messenger/skin/icons/folder-newsgroup.png");
> >+}
> So, you seem to have over-zealously copied and pasted here. In particular
> the existing icons are all over the shop when it comes to image size, and so
> the margins don't make sense for the newsgroup icon. In particular the top
> and bottom margins cause the image to get clipped. r=me with those removed.

This block is identical in suite/themes/classic/mac/messenger/messengercompose/messengercompose.css and suite/themes/classic/messenger/messengercompose/messengercompose.css so I assume I have to make the change in both files.

In suite/themes/modern/messenger/messengercompose/messengercompose.css I use url("chrome://messenger/skin/icons/folder-newsgroup.gif"), with the same margin values. Both images have identical size (16x16 pixels), so I assume I have to remove margin-top and margin-bottom there as well.

Comment 139

[Jens Müller (:tessarakt)]

Attachment: Autocomplete newsgroup names (based on a patch by Jim Porter) - v9

Comment 140

[Jens Müller (:tessarakt)]

Comment on attachment 810010 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v9

Carrying forward review by Neil and mconley.

According to http://www.seamonkey-project.org/dev/review-and-flags, I also need moa from a SeaMonkey MailNews module owner or peer (unless one of them says otherwise) ....

Comment 141

[Jens Müller (:tessarakt)]

(Neil confirmed on IRC that his r+ also includes moa+.)

Comment 142

[Mark Banner (:standard8)]

Comment on attachment 810010 [details] [diff] [review]
Autocomplete newsgroup names (based on a patch by Jim Porter) - v9

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

Looks good, sr=Standard8

Comment 143

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/5ac1dfd37ebf

Comment 144

[Ryan VanderMeulen [:RyanVM]]

Backed out for Linux debug xpcshell crashes.
https://hg.mozilla.org/comm-central/rev/723f9a787538

https://tbpl.mozilla.org/php/getParsedLog.php?id=29730060&tree=Thunderbird-Trunk

Comment 145

[Jens Müller (:tessarakt)]

Thanks, Ryan.

I could not produce this locally until now.

When I upgraded the patch to recent c-c, I noticed it will not cleanly apply due to this one line: http://hg.mozilla.org/comm-central/annotate/cd7065b3430f/mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch4.js#l130 . Accordingly, I will provide a new patch file and carry forward the existing r+ and sr+. 

As I cannot reproduce locally, I will push the patch to try again ...

Comment 146

[Jens Müller (:tessarakt)]

Pushed to try again: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=4c77a703edbe

Comment 147

[Jens Müller (:tessarakt)]

(In reply to Jens Müller (:tessarakt) from comment #146)
> Pushed to try again:
> https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=4c77a703edbe

Sorry, https://hg.mozilla.org/try-comm-central/rev/d0cfdb9d7787

Forgot to include xpcshell tests ...

Comment 148

[Jens Müller (:tessarakt)]

The problem appeared again on try, so apparently this is valid.


The NS_ASSERTION occurs here: http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgMailSession.cpp#68 - nsMsgMailSession::AddFolderListener is called with a listener that already exists.

This is called from here: http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgAccountManager.cpp#1226 within nsMsgAccountManager::LoadAccounts(). What seems a bit strange to me is the way possible reentrant calls to this function are handled ... There is a quick-out depending on m_accountsLoaded at the very beginning of the method, but this flag is only set at the end of the function ... So one thing to try out is add some debug output indicating calls to and successful completions of this function ...

Next step up in the stacktrace is nsMsgAccountManager::FindServerByURI at http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgAccountManager.cpp#1824 - I cannot see anything unusual here.


The test script test_newsAutocomplete.js seems to finish fine: 
TEST-INFO | (xpcshell/head.js) | test MAIN run_test finished (1)
TEST-INFO | (xpcshell/head.js) | exiting test
TEST-PASS | (xpcshell/head.js) | 11 (+ 0) check(s) passed
TEST-INFO | (xpcshell/head.js) | 0 check(s) todo


And according to these log entries:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file ../../../../mailnews/base/src/nsMsgAccountManager.cpp, line 1825
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file ../../../../mailnews/base/util/nsMsgDBFolder.cpp, line 3194
###!!! ASSERTION: tried to add duplicate listener: 'index == -1', file ../../../../mailnews/base/src/nsMsgMailSession.cpp, line 68

there apparently _are_ multiple calls to LoadAccounts() ... First, one is not successful, causing the NS_ENSURE_SUCCESS in line 1825 to fail. The call to nsMsgAccountManager::LoadAccounts() (which subsequently leads to a call to nsMsgMailSession::AddFolderListener and then the failed NS_ASSERTION) is already in line 1824, so this is a different call.

So this is the second thing to debug: Why does LoadAccounts() return an error code the first time? There is only one return statement not returning NS_OK, so it has to be that one:

1229   // If we have code trying to do things after we've unloaded accounts,
1230   // ignore it.
1231   if (m_shutdownInProgress || m_haveShutdown)
1232     return NS_ERROR_FAILURE;

Comment 149

[Jens Müller (:tessarakt)]

I added some debug output - let's see what happens ...

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=916a41329c94

Comment 150

[Jens Müller (:tessarakt)]

jcranmer provided some helpful information on IRC.

He pointed out that the legacy NNTP credentials migration is being triggered during shutdown. This should not happen normally, it does happen in the tests because the event loop is not executed throughout the whole test, only at the end when the fakeserver is shutdown.

He further gave the advice to not start up and shutdown the fakeserver if I am not querying it. In fact, I AM NOT querying it in the test - I just blindly subscribe to newsgroups without ever retrieving the newsgroup list from the server.

So what I did now is to remove "server.start(NNTP_PORT);" and "server.stop();" from the test.

I pushed another try build: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=7c949c33ae4f - let's see what happens.

Comment 151

[Jens Müller (:tessarakt)]

P.S.:

I do not claim to completely understand the problem, but is this a real solution or just a workaround for a more complex underlying issue? 

In particular, I have not yet understood whether this is deterministic or a timing problem ...

Comment 152

[Jens Müller (:tessarakt)]

Attachment: Autocomplete newsgroup names (based on a patch by Jim Porter) - v10

Comment 153

[Jens Müller (:tessarakt)]

Diffing the patches shows this as the only "material" difference, in mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch4.js:

    function checkSearch(element, index, array) {
--    acs.startSearch(element, null, null, obs);
+     print("Checking " + element);
+-    acs.startSearch(element, null, null, obs)
 +    acs.startSearch(element, JSON.stringify({ type: "addr_to"  }), null, obs);

diff also finds a fake difference which does not really exist ...

Well, I trust the person who will check in the patch again will be able to resolve that conflict manually :)

Comment 154

[Jens Müller (:tessarakt)]

(In reply to Jens Müller (:tessarakt) from comment #150)
> I pushed another try build:
> https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=7c949c33ae4f - let's see
> what happens.

The first crash (failing assert) is gone - but now there is a new one:

###!!! ASSERTION: nsNSSComponent relies on profile manager to wait for synchronous shutdown of all network activity: 'mIsNetworkDown', file ../../../../../../mozilla/security/manager/ssl/src/nsNSSComponent.cpp, line 1869

Bug 819746 seems related.

Comment 155

[Jens Müller (:tessarakt)]

I got it narrowed down: The crash occurs while loading mailnews/news/test/unit/tail_news.js, so probably _in_ that file ...

Comment 156

[Jens Müller (:tessarakt)]

(In reply to Jens Müller (:tessarakt) from comment #155)
> I got it narrowed down: The crash occurs while loading
> mailnews/news/test/unit/tail_news.js, so probably _in_ that file ...

Sorry, it's not the original crash that occurs there, it's the construction of the nsNSSComponent.

Apparently, it occurs within mailnews/test/resources/mailShutdown.js, which is loaded from there, in the call to Services.obs.notifyObservers(null, "profile-change-net-teardown", null).

More precisely:

postShutdownNotifications() started.
nsObserverService::NotifyObservers: Notifying observers about quit-application-requested
nsObserverList::NotifyObservers: Notiying 1 observers about quit-application-requested
nsObserverService::NotifyObservers: Notified observers about quit-application-requested
observers notified about quit-application-requested.
Notifying observers about quit-application
nsObserverService::NotifyObservers: Notifying observers about quit-application
nsObserverList::NotifyObservers: Notiying 1 observers about quit-application
nsObserverService::NotifyObservers: Notified observers about quit-application
Notified observers about quit-application
Notifying observers about profile-change-net-teardown
nsObserverService::NotifyObservers: Notifying observers about profile-change-net-teardown
nsObserverList::NotifyObservers: Notiying 1 observers about profile-change-net-teardown
nsObserverService::NotifyObservers: Notifying observers about ipc:network:set-offline
nsObserverService::NotifyObservers: Notified observers about ipc:network:set-offline
nsObserverService::NotifyObservers: Notifying observers about network:offline-about-to-go-offline
nsObserverList::NotifyObservers: Notiying 1 observers about network:offline-about-to-go-offline
Creating instance of {e6f68040-c7ec-11d3-8cda-0060b0fc14a3}
Creating instance of {6476378a-da09-11d3-8cda-0060b0fc14a3}
nsObserverService::NotifyObservers: Notified observers about network:offline-about-to-go-offline
nsObserverService::NotifyObservers: Notifying observers about network:offline-status-changed
nsObserverService::NotifyObservers: Notified observers about network:offline-status-changed
Creating instance of {138ad1b2-c694-41cc-b201-333ce936d8b8}
Creating instance of {1419aa16-f134-4154-9886-00c7c5147a13}
Creating instance of {1419aa16-f134-4154-9886-00c7c5147a13}
Creating instance of {1419aa16-f134-4154-9886-00c7c5147a13}
Creating instance of {338c8597-1e32-4682-b5c7-cf8142c0bd1d}
Creating instance of {6ffbb526-205b-49c5-ae3f-5959c084075e}
###!!! ASSERTION: nsNSSComponent::nsNSSComponent() started: 'false', file /home/jens/devel/releases-comm-central/mozilla/security/manager/ssl/src/nsNSSComponent.cpp, line 22

So whatever it is that is notified about profile-change-net-teardown creates 6 XPCOM component instances, the last of which is nsNSSComponent ...

Quite ironic that this happens exactly for the topic which nsNSSComponent would have needed to have observed itself ...

Comment 157

[Jens Müller (:tessarakt)]

(In reply to Jens Müller (:tessarakt) from comment #156)
> So whatever it is that is notified about profile-change-net-teardown creates
> 6 XPCOM component instances, the last of which is nsNSSComponent ...

The observer called is nsIOService:
- http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/src/nsIOService.cpp#910
- http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/src/nsIOService.cpp#659

All 6 component instances are created within this call to mSocketTransportService->Shutdown():
- http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/src/nsIOService.cpp#748

mSocketTransportService is a nsPISocketTransportService ("@mozilla.org/network/socket-transport-service;1"), retrieved here:
 - mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/src/nsIOService.cpp#225

Comment 158

[Jens Müller (:tessarakt)]

Indeed the first creation of nsNSSComponent happens in mozilla::net::NetworkActivityMonitor::Shutdown(). 

More precisely: In the invocation of mThread->Shutdown().

Comment 159

[Jens Müller (:tessarakt)]

On IRC, Standard8 pointed me to this:

> http://mxr.mozilla.org/comm-central/source/mailnews/news/test/unit/test_nntpPassword.js#51 let any events cycle before getting into the tail

Indeed, this seems to work - I will prepare a new try build now.

Comment 160

[Jens Müller (:tessarakt)]

Pushed to Try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=ce500ce4aca6

Comment 161

[Jens Müller (:tessarakt)]

Attachment: Followup: Fixes for shutdown problems in xpcshell test

Comment 162

[Jens Müller (:tessarakt)]

Green X on Try for all platforms (except Windows, which does not build at all at the moment ...).

Comment 163

[Ryan VanderMeulen [:RyanVM]]

Can you please attach a roll-up patch that contains proper commit information and mark the others as obsolete?

Comment 164

[Jens Müller (:tessarakt)]

Attachment: Autocomplete newsgroup names (based on a patch by Jim Porter) - v11

Comment 165

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/2523fe46615f

Comment 166

[Magnus Melin [:mkmelin]]

Sweet! Nice job pulling this one through, Jens!