Ship both autocomplete impls with the new-toolkit (<textbox type="autocomplete-xpfe">) to aid transition, #ifdef-hell

RESOLVED WONTFIX

Status

()

enhancement
P2
normal
RESOLVED WONTFIX
15 years ago
10 years ago

People

(Reporter: benjamin, Unassigned)

Tracking

unspecified
mozilla1.9alpha1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 2 obsolete attachments)

Reporter

Description

15 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?
Product: Core → Mozilla Application Suite
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 ?
Severity: normal → enhancement
Reporter

Updated

14 years ago
Blocks: 299986
Component: XP Apps: Autocomplete → Autocomplete
Priority: -- → P2
Product: Mozilla Application Suite → Toolkit
Target Milestone: --- → mozilla1.9alpha1
Version: Trunk → unspecified
Reporter

Updated

14 years ago
Blocks: 306324
Reporter

Comment 2

14 years ago
This patch makes both autocomplete impls little standalone components, but doesn't change who is building them (yet).
Attachment #201013 - Flags: first-review?(neil.parkwaycc.co.uk)

Updated

14 years ago
Attachment #201013 - Flags: first-review?(neil.parkwaycc.co.uk) → first-review+
Reporter

Comment 3

14 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).
Attachment #201420 - Flags: first-review?(dmose)
Reporter

Updated

14 years ago
Attachment #201013 - Attachment description: Make both autocomplete impls independent little components → Make both autocomplete impls independent little components [checked in]
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.
Attachment #201420 - Flags: first-review?(dmose)
Reporter

Comment 5

14 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?
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
cvs diff: xpfe/components/autocomplete/public/nsILDAPAutoCompFormatter.idl was removed, no comparison available

might wnat to diff with -N to show removed files
Reporter

Comment 8

14 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).
this broke camino's autocomplete. i guess we need to move where we pick up the old autocomplete's component?
Reporter

Comment 10

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

Updated

14 years ago
Blocks: 314612
Not shown are
a) toolkit autocomplete idl files copied to xpfe
b) toolkit filepicker.xul changes (no js changes required)
Reporter

Comment 12

14 years ago
Attachment #201420 - Attachment is obsolete: true
Attachment #201545 - Flags: first-review?(dmose)
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?
Attachment #201545 - Flags: first-review?(dmose) → first-review+
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.
(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.
Reporter

Updated

14 years ago
Depends on: 314830
Reporter

Comment 16

14 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.
Attachment #201545 - Attachment description: Move LDAPAutocomplete to mailnews/addrbook → Move LDAPAutocomplete to mailnews/addrbook [checked in]
Reporter

Comment 17

14 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.
Attachment #206523 - Flags: first-review?(neil.parkwaycc.co.uk)
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?
Reporter

Comment 19

14 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).
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.
Attachment #206523 - Flags: first-review?(neil.parkwaycc.co.uk) → first-review-
Reporter

Updated

14 years ago
Depends on: 282328
Reporter

Comment 21

14 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.
Attachment #201013 - Flags: approval1.8.1?
Reporter

Updated

14 years ago
Attachment #201013 - Flags: approval1.8.1? → branch-1.8.1?(bryner)
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.
Attachment #201013 - Flags: branch-1.8.1?(bryner) → branch-1.8.1+
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...
Attachment #210176 - Flags: first-review?(benjamin)
Reporter

Updated

14 years ago
Attachment #210176 - Flags: first-review?(benjamin) → first-review+
Attachment #210176 - Attachment description: Remove some redundant MOZ_LDAP_XPCOM defines & checks → Remove some redundant MOZ_LDAP_XPCOM defines & checks [checked in]
Minimal prerequisite to attachment 201532 [details] [diff] [review] as there's been no activity recently.
Attachment #214108 - Flags: first-review?(benjamin)
Reporter

Comment 25

13 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.
Attachment #214108 - Flags: first-review?(benjamin) → first-review+
Note: The makefile changes already have r=bsmedberg from the previous patch.
Attachment #214108 - Attachment is obsolete: true
Attachment #215900 - Flags: second-review?(dveditz)
Attachment #215900 - Flags: first-review?(ajschult)

Comment 27

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

r=ajschult
Attachment #215900 - Flags: first-review?(ajschult) → first-review+
Comment on attachment 215900 [details] [diff] [review]
Updated for review comments

r=dveditz
Attachment #215900 - Flags: second-review?(dveditz) → second-review+

Updated

13 years ago
Attachment #215900 - Flags: approval-branch-1.8.1?(benjamin)
Reporter

Comment 29

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

What is the deleteThisFile() call in os2/browser.jst for?
Attachment #215900 - Flags: approval-branch-1.8.1?(benjamin) → approval-branch-1.8.1+
Blocks: 344143

Updated

13 years ago
Blocks: 360648
Reporter

Comment 30

13 years ago
I'm no longer actively working on this.
Assignee: benjamin → nobody
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.
Reporter

Comment 32

13 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.
(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...

Comment 34

12 years ago
Is this still the planned course of action?
No longer blocks: 360648
No longer blocks: 306324
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...
(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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX

Updated

10 years ago
QA Contact: autocomplete
You need to log in before you can comment on or make changes to this bug.