Benjamin Smedberg Reporter
	Description • 20 years ago

I'm trying to dig xpfe/components/Makefile.in and toolkit/components/Makefile.in
out of ifdef-hell, and I think an easy first step would be to ship the toolkit
with both flavors of autocomplete. Then <textbox type="autocomplete"> would be
the new interface, and <textbox type="xpfe-autocomplete"> would be the old type.
Objections?

	Serge Gautherie (:sgautherie)
	Comment 1 • 20 years ago

One positive way to look at it is that theee was no objection comment added in 5
months :->

Benjamin, what is the current status of this proposal ?

	Benjamin Smedberg Reporter
	Comment 2 • 19 years ago

This patch makes both autocomplete impls little standalone components, but doesn't change who is building them (yet).

	Benjamin Smedberg Reporter
	Comment 3 • 19 years ago

This moves the LDAP-specific portions of autocomplete to directory/xpcom to avoid feature ifdefs in tier 50. It also moves the mailnews-specific ldapAutoCompErrs.properties file into mailnews (it was already forked into mail/locales, but wrongly, this brings everything back into synch).

	Dan Mosedale (:dmosedale, :dmose)
	Comment 4 • 19 years ago

Comment on attachment 201420 [details] [diff] [review]
Move LDAPAutocomplete to directory/xpcom

bsmedberg: come find me on IRC tomorrow and we can discuss this.  I don't think this factoring is really the right one.  LDAP autocomplete is not mailnews specific, and directory/xpcom is low-level enough that it doesn't want to depend on appcomps.

	Benjamin Smedberg Reporter
	Comment 5 • 19 years ago

Dan, this patch has almost nothing to do with mailnews: the ldapAutoCompErrs.properties file is specific to addrbook and just lived in the wrong place in the source tree. As for directory/xpcom being low-level (not tier-50 or higher), why?

	neil@parkwaycc.co.uk
	Comment 6 • 19 years ago

Comment on attachment 201420 [details] [diff] [review]
Move LDAPAutocomplete to directory/xpcom

>cvs diff: Diffing xpfe/components/alerts
>cvs diff: Diffing xpfe/components/alerts/public
>cvs diff: Diffing xpfe/components/alerts/resources
>cvs diff: Diffing xpfe/components/alerts/resources/content
>cvs diff: Diffing xpfe/components/alerts/src
Tip: don't cvs diff 2>&1

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 7 • 19 years ago

cvs diff: xpfe/components/autocomplete/public/nsILDAPAutoCompFormatter.idl was removed, no comparison available

might wnat to diff with -N to show removed files

	Benjamin Smedberg Reporter
	Comment 8 • 19 years ago

I diffed 2>&1 to show where I had moved files without actually including them in the patch (no changes in any moved files).

	Mike Pinkerton (not reading bugmail)
	Comment 9 • 19 years ago

this broke camino's autocomplete. i guess we need to move where we pick up the old autocomplete's component?

	Benjamin Smedberg Reporter
	Comment 10 • 19 years ago

Yes, if you have a manifest which copies components, or a list of static components, that will need to be updated.

	neil@parkwaycc.co.uk
	Comment 11 • 19 years ago

Not shown are
a) toolkit autocomplete idl files copied to xpfe
b) toolkit filepicker.xul changes (no js changes required)

	Dan Mosedale (:dmosedale, :dmose)
	Comment 13 • 19 years ago

Comment on attachment 201545 [details] [diff] [review]
Move LDAPAutocomplete to mailnews/addrbook [checked in]

r=dmose, conditional on Standard8 being okay with this change.  Mark?

	Dan Mosedale (:dmosedale, :dmose)
	Comment 14 • 19 years ago

Also, when moving these files, I'd suggest asking the CVS admins to do it via symlinkery behind the scenes so that CVS history is preserved.

	Mark Banner (:standard8)
	Comment 15 • 19 years ago

(In reply to comment #13)
> (From update of attachment 201545 [details] [diff] [review] [edit])
> r=dmose, conditional on Standard8 being okay with this change.  Mark?

Yep, its fine by me.

	Benjamin Smedberg Reporter
	Comment 16 • 19 years ago

Comment on attachment 201545 [details] [diff] [review]
Move LDAPAutocomplete to mailnews/addrbook [checked in]

Part 2 (moving ldap-autocomplete to mailnews) checked in.

	Benjamin Smedberg Reporter
	Comment 17 • 19 years ago

This patch renames the XPFE classes associated with autocomplete and the "autocomplete.xml" file; this is necessary because the two autocomplete impls share classnames and filenames but they do not mean the same thing.

	neil@parkwaycc.co.uk
	Comment 18 • 19 years ago

Comment on attachment 206523 [details] [diff] [review]
Rename xpfe autocomplete classes, rev. 1

How many ifdefs is this supposed to save? I see none in the patch, shouldn't there be at least one removed?

	Benjamin Smedberg Reporter
	Comment 19 • 19 years ago

This patch itself doesn't remove ifdefs. That will be the next patch where I turn on both autocomplete impls in both toolkits and edit the xul.css to match (basically including *both* halves of the ifdef at http://lxr.mozilla.org/mozilla/source/toolkit/content/xul.css#694 in both toolkits).

	neil@parkwaycc.co.uk
	Comment 20 • 19 years ago

Comment on attachment 206523 [details] [diff] [review]
Rename xpfe autocomplete classes, rev. 1

You don't need to rename any files, they live at different chrome URLs anyway (you also forgot to update toolkit xul.css with the new names for Thunderbird). I'm not convinced about the class renaming either, I think I'd prefer all the xpfe autocomplete to gain an xpfe class e.g. class="xpfe autocomplete-result-popup" in the XBL and .xpfe.autocomplete-result-popup in the CSS, but only necessarily in those portions of CSS where it makes a difference, of course.

	Benjamin Smedberg Reporter
	Comment 21 • 19 years ago

Comment on attachment 201013 [details] [diff] [review]
Make both autocomplete impls independent little components [checked in]

I'd like this on 1.8 branch mainly to avoid forkage related to places.

	Brian Ryner (not reading)
	Comment 22 • 19 years ago

Comment on attachment 201013 [details] [diff] [review]
Make both autocomplete impls independent little components [checked in]

sounds ok.  you'll have a bit of merging to do thanks to some places code that already landed on the branch.

	Mark Banner (:standard8)
	Comment 23 • 19 years ago

This patch removes some MOZ_LDAP_XPCOM checks and defines that I believe are redundant after the patch to move LDAPAutocomplete to mailnews/addrbook.

dmose & I believe that the additional checks in the cpp files aren't required anymore as 1) we only compile them if MOZ_LDAP_XPCOM is defined in the makefile system (mailnews/addrbook/src/Makefile.in) and 2) we don't support OS X 9 anymore which we believe this was originally put in to work around for...

If I need a second review before checkin let me know please...

	neil@parkwaycc.co.uk
	Comment 24 • 19 years ago

Minimal prerequisite to attachment 201532 [details] [diff] [review] as there's been no activity recently.

	Benjamin Smedberg Reporter
	Comment 25 • 19 years ago

Comment on attachment 214108 [details] [diff] [review]
Build both autocomplete interfaces for suite

You're going to need to update the install manifests as well.

	neil@parkwaycc.co.uk
	Comment 26 • 19 years ago

Note: The makefile changes already have r=bsmedberg from the previous patch.

	Andrew Schultz
	Comment 27 • 19 years ago

Comment on attachment 215900 [details] [diff] [review]
Updated for review comments

r=ajschult

	Daniel Veditz [:dveditz]
	Comment 28 • 19 years ago

Comment on attachment 215900 [details] [diff] [review]
Updated for review comments

r=dveditz

	Benjamin Smedberg Reporter
	Comment 29 • 19 years ago

Comment on attachment 215900 [details] [diff] [review]
Updated for review comments

What is the deleteThisFile() call in os2/browser.jst for?

	Benjamin Smedberg Reporter
	Comment 30 • 18 years ago

I'm no longer actively working on this.

	Mark Banner (:standard8)
	Comment 31 • 18 years ago

What needs doing to drive this home? I think SeaMonkey (and Thunderbird) will need it in the short term to pick up more of toolkit.

	Benjamin Smedberg Reporter
	Comment 32 • 18 years ago

Mark, the major problem I ran into is that when I tried to distinguish between old-style autocomplete and new-style in xul.css using the following rule:

textbox[type="autocomplete"][searchSessions] { /* binding for old-style autocomplete */

The problem is that xul.css is parsed as all lowercase (HTML), so this rule never matches anything. Filed as bug 282328, but it's beyond me to fix.

	neil@parkwaycc.co.uk
	Comment 33 • 18 years ago

(In reply to comment #32)
>textbox[type="autocomplete"][searchSessions] { /* binding for old-style autocomplete */
Another problem is that we now only use searchSessions in the compose window...

	Robert Kaiser
	Comment 34 • 18 years ago

Is this still the planned course of action?

	Shawn Wilsher :sdwilsh
	Comment 35 • 16 years ago

Is this still needed by anyone?  Since it's been dead for a year and a half, I'm guessing not and we can close it...

	Mark Banner (:standard8)
	Comment 36 • 15 years ago

(In reply to comment #35)
> Is this still needed by anyone?  Since it's been dead for a year and a half,
> I'm guessing not and we can close it...

The only things stopping removing the ifdefs are some remaining changes in Thunderbird and SeaMonkey - namely to pick up the new toolkit interfaces for LDAP complete. This is covered by bug 452232 and friends. Once this is complete we won't need to ship both implementations (and we don't anyway).

I agree we can close it. I guess wontfix is the nearest appropriate resolution.