Remove dead code in LDAP
Categories
(Thunderbird :: Address Book, task)
Tracking
(thunderbird_esr78 wontfix, thunderbird84 wontfix)
People
(Reporter: benc, Assigned: benc)
References
Details
Attachments
(2 files)
(deleted),
patch
|
mkmelin
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
There seems to be a goodly chunk of LDAP-related code that isn't actually used. I don't know if it works or not, but I'd be inclined to remove it to avoid any confusion.
There are two main ones which stand out:
Firstly, nsLDAPSyncQuery
isn't used anywhere in the C-C codebase.
The moz.build file includes it conditionally via if CONFIG['MOZ_PREF_EXTENSIONS']:
, which suggests it might be used by extensions?
Secondly, there's nsILDAPService
and nsILDAPServer
. Originally it seems the idea was that nsILDAPService would have a list of LDAP server details and cache LDAP connections. But that doesn't seem to be used at all any more.
The LDAP server list is now (I think) just addressbook directories of type nsIAbLDAPDirectory
.
I think the only nsILDAPService
usage is in AbLDAPAutoCompleteSearch
, which uses nsILDAPService.createFilter()
.
There are a bunch of historical bugs related to nsILDAPService
which could probably be closed here too: Bug 75954, Bug 75989, Bug 75999, Bug 76755, Bug 76887
Assignee | ||
Comment 1•4 years ago
|
||
Geoff: Magnus mentioned you had some ideas on LDAP stuff which could be trimmed or reimplemented in JS?
Not sure if any of that falls into the "dead code" bag, but if so, here's a bug to hang it off :-)
Comment 2•4 years ago
|
||
What I want to do is get rid of the C++ LDAP stuff in mailnews/addrbook and replace it with a JS equivalent. It only really does two things: searches the remote address book (so the same as what the autocomplete does but with more arbitrary parameters), and fetches all of the contacts from the server and stores them locally for offline use. If there's anything else I've not come across it.
Ultimately we should be able to replace all of the LDAP C++ code and replace it with something much simpler. For now though I agree, let's throw away anything we're not using.
Assignee | ||
Comment 3•4 years ago
|
||
(In reply to Geoff Lankow (:darktrojan) [away until 16/11] from comment #2)
What I want to do is get rid of the C++ LDAP stuff in mailnews/addrbook and replace it with a JS equivalent. It only really does two things: searches the remote address book (so the same as what the autocomplete does but with more arbitrary parameters), and fetches all of the contacts from the server and stores them locally for offline use. If there's anything else I've not come across it.
(just musing here - not expecting any response :-)
Maybe a good approach would be to define the JS interface you'd like to see for LDAP functionality. We could implement that on top of what's there already, then replace it at leisure, with new C++, JS or (most likely) a combination. Like so many of the protocols there's enough byte-fiddling that I'm inclined to think C++ is a better fit down there (I've tried doing some stuff with JS byte arrays and found them to be a pretty blunt tool). But as soon as you bootstrap up to something workable, JS really comes into it's own.
(and at the very least, I'd like to merge the libprldap and nsLDAPSecurityGlue layers. Lots of extra obfuscation there.)
Assignee | ||
Comment 4•4 years ago
|
||
Nothing currently uses nsLDAPSyncQuery, and I don't even know if it works.
But I don't know what the situation is with extensions. I don't see it show up in bugzilla, so it's either not being used by any extensions, or it just works perfectly and everyone is really happy with it :-)
try run here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9759f0151b0dcc1e5660c9bdde0dcbfc11458594
Comment 5•4 years ago
|
||
Assignee | ||
Comment 6•4 years ago
|
||
Ahh, it didn't even occur to me that any M-C code might rely on C-C code...
Shifting attention to Bug 1662433 instead.
Assignee | ||
Comment 7•4 years ago
|
||
This removes all the connection management in nsILDAPService, which isn't used as far as I can tell.
createFilter()
is the only one left. It is only used in AbLDAPAutoCompleteSearch
, and could probably be reimplemented easily enough in javascript - only a small subset of the filter creation syntax is required. But for now, since the rest of the patch is a simple set of cuts, I'll leave this C++ implementation in place.
Doubt there's any test coverage, but there's a try run here:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=f45e1cddeaf160e4d82d4f43a45d8c17812dd616
The try run isn't this exact patch. After I submitted it I snipped out a bunch of now-redundant #includes (which shouldn't affect anything - it still compiles fine).
Assignee | ||
Comment 8•4 years ago
|
||
(In reply to Ben Campbell from comment #7)
The try run isn't this exact patch. After I submitted it I snipped out a bunch of now-redundant #includes (which shouldn't affect anything - it still compiles fine).
Errm. And I removed a couple of unused vars which the compiler warned me about locally but cause the try build to fail :-)
Let's have another go:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=46d8deba291069c5e551cd703feda81fe17861c6
Comment 9•4 years ago
|
||
Comment 10•4 years ago
|
||
Test failures looks unrelated.
Comment 11•4 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/75d7bbd322cc
Remove unused nsILDAPService methods. r=mkmelin
Assignee | ||
Updated•4 years ago
|
Updated•3 years ago
|
Description
•