Closed
Bug 263042
Opened 20 years ago
Closed 15 years ago
Ship both autocomplete impls with the new-toolkit (<textbox type="autocomplete-xpfe">) to aid transition, #ifdef-hell
Categories
(Toolkit :: Autocomplete, enhancement, P2)
Toolkit
Autocomplete
Tracking
()
RESOLVED
WONTFIX
mozilla1.9alpha1
People
(Reporter: benjamin, Unassigned)
References
Details
Attachments
(6 files, 2 obsolete files)
(deleted),
patch
|
neil
:
first-review+
bryner
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
dmosedale
:
first-review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
neil
:
first-review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
benjamin
:
first-review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ajschult784
:
first-review+
dveditz
:
second-review+
benjamin
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
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?
Updated•20 years ago
|
Product: Core → Mozilla Application Suite
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 ?
Severity: normal → enhancement
Reporter | ||
Updated•19 years ago
|
Blocks: 299986
Component: XP Apps: Autocomplete → Autocomplete
Priority: -- → P2
Product: Mozilla Application Suite → Toolkit
Target Milestone: --- → mozilla1.9alpha1
Version: Trunk → unspecified
Reporter | ||
Comment 2•19 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•19 years ago
|
Attachment #201013 -
Flags: first-review?(neil.parkwaycc.co.uk) → first-review+
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).
Attachment #201420 -
Flags: first-review?(dmose)
Reporter | ||
Updated•19 years ago
|
Attachment #201013 -
Attachment description: Make both autocomplete impls independent little components → Make both autocomplete impls independent little components [checked in]
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.
Attachment #201420 -
Flags: first-review?(dmose)
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?
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
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
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).
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?
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.
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)
Reporter | ||
Comment 12•19 years ago
|
||
Attachment #201420 -
Attachment is obsolete: true
Attachment #201545 -
Flags: first-review?(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?
Attachment #201545 -
Flags: first-review?(dmose) → first-review+
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.
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.
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.
Attachment #201545 -
Attachment description: Move LDAPAutocomplete to mailnews/addrbook → Move LDAPAutocomplete to mailnews/addrbook [checked in]
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.
Attachment #206523 -
Flags: first-review?(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?
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).
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.
Attachment #206523 -
Flags: first-review?(neil.parkwaycc.co.uk) → first-review-
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.
Attachment #201013 -
Flags: approval1.8.1?
Reporter | ||
Updated•19 years ago
|
Attachment #201013 -
Flags: approval1.8.1? → branch-1.8.1?(bryner)
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.
Attachment #201013 -
Flags: branch-1.8.1?(bryner) → branch-1.8.1+
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...
Attachment #210176 -
Flags: first-review?(benjamin)
Reporter | ||
Updated•19 years ago
|
Attachment #210176 -
Flags: first-review?(benjamin) → first-review+
Updated•19 years ago
|
Attachment #210176 -
Attachment description: Remove some redundant MOZ_LDAP_XPCOM defines & checks → Remove some redundant MOZ_LDAP_XPCOM defines & checks [checked in]
Comment 24•19 years ago
|
||
Minimal prerequisite to attachment 201532 [details] [diff] [review] as there's been no activity recently.
Attachment #214108 -
Flags: first-review?(benjamin)
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.
Attachment #214108 -
Flags: first-review?(benjamin) → first-review+
Comment 26•19 years ago
|
||
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•19 years ago
|
||
Comment on attachment 215900 [details] [diff] [review] Updated for review comments r=ajschult
Attachment #215900 -
Flags: first-review?(ajschult) → first-review+
Comment 28•19 years ago
|
||
Comment on attachment 215900 [details] [diff] [review] Updated for review comments r=dveditz
Attachment #215900 -
Flags: second-review?(dveditz) → second-review+
Updated•19 years ago
|
Attachment #215900 -
Flags: approval-branch-1.8.1?(benjamin)
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?
Attachment #215900 -
Flags: approval-branch-1.8.1?(benjamin) → approval-branch-1.8.1+
Reporter | ||
Comment 30•18 years ago
|
||
I'm no longer actively working on this.
Assignee: benjamin → nobody
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.
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.
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...
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...
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.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•