Closed
Bug 631897
Opened 13 years ago
Closed 11 years ago
Resync' Directory Provider from Firefox
Categories
(SeaMonkey :: General, defect)
SeaMonkey
General
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.18
People
(Reporter: sgautherie, Assigned: ewong)
References
()
Details
Attachments
(2 files, 6 obsolete files)
3.24 KB,
patch
|
ewong
:
review+
|
Details | Diff | Splinter Review |
8.80 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
While checking bug 623110, I noticed SeaMonkey DirectoryProvider was (almost) not updated since ages. I wonder whether we should port (some) updates from Firefox. *** http://mxr.mozilla.org/mozilla/source/browser/components/dirprovider/nsBrowserDirectoryProvider.cpp http://mxr.mozilla.org/seamonkey/source/suite/profile/nsSuiteDirectoryProvider.cpp
Comment 1•13 years ago
|
||
1. Bug 364297. We don't have contract requirements with Google, so we don't need to make Google our default home page and search provider in CJKT locales. 2. Bug 392771. We don't have search partners or requirements from Linux distros where we need distro specific search plugins. It might be good to take though. CC'ing some Council members for a policy decision. 3. Bug 344159 - Ensure FF2 search service interacts appropriately with CCK extensions for partner and corporate deployment. Dunno might be useful. CCK does work with SeaMonkey after all. 4. Bug 366442 - search.rdf is obsolete. Yeah we are now using openSearch 5. Bug 296430 - Allow extensions to ship searchplugins. I think we want this if we don't already have this
Comment 2•13 years ago
|
||
(In reply to comment #1) > 2. Bug 392771. We don't have search partners or requirements from Linux distros > where we need distro specific search plugins. It might be good to take though. > CC'ing some Council members for a policy decision. distribution/ doesn't mean Linux distros, but means someone else distributing a specialized version of our binaries with some stuff like add-ons, searchplugins, etc. added. And yes, supporting that would be nice, even if we don't have an immediate need right now.
Comment 3•13 years ago
|
||
Hey Jag, are you interested in taking this bug?
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → ewong
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #696950 -
Flags: review?(neil)
Assignee | ||
Comment 5•11 years ago
|
||
This patch is for: 4. Bug 366442 - search.rdf is obsolete. Yeah we are now using openSearch
Attachment #696952 -
Flags: review?(neil)
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #696952 -
Attachment is obsolete: true
Attachment #696952 -
Flags: review?(neil)
Attachment #696955 -
Flags: review?(neil)
Comment 7•11 years ago
|
||
> - else if (!strcmp(aKey, NS_APP_USER_PANELS_50_FILE))
> - leafName = "panels.rdf";
We are still using this one.
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #696955 -
Attachment is obsolete: true
Attachment #696955 -
Flags: review?(neil)
Attachment #696962 -
Flags: review?(neil)
Comment 9•11 years ago
|
||
Additional candidates: Bug 515232 - Try getting general.useragent.locale as a complex value first in DirectoryProvider.cpp Bug 623110 - prioritise the loading of user-installed search engines over app-shipped engines
Assignee | ||
Comment 10•11 years ago
|
||
Comment on attachment 696950 [details] [diff] [review] Resync Directory Provider from Firefox (patch 1) (v1) Apparently this patch also fixes bug #3
Comment 11•11 years ago
|
||
Comment on attachment 696962 [details] [diff] [review] Resync Directory Provider from Firefox (patch 2) (v3) >-defaults/profile/search.rdf Don't change this, we need it for people upgrading from 2.0.x. r=me with that fixed.
Attachment #696962 -
Flags: review?(neil) → review+
Comment 12•11 years ago
|
||
Comment on attachment 696950 [details] [diff] [review] Resync Directory Provider from Firefox (patch 1) (v1) >+#include "nsCOMArray.h" >+#include "nsIFile.h" > #include "nsSuiteDirectoryProvider.h" Don't bother including stuff that nsSuiteDirectoryProvider.h already includes. >+ nsCOMArray<nsIFile> baseFiles; Some of the patch must be missing, because you never use this once you've appended to it. >+ if (prefs) { >+ Nit: no blank lines after { please. >+#include "nsIPrefBranch.h" >+#include "nsDirectoryServiceDefs.h" >+#include "nsIPrefLocalizedString.h" >+#include "nsIPrefService.h" Don't include these, you don't use them here.
Attachment #696950 -
Flags: review?(neil) → review-
Assignee | ||
Comment 13•11 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #12) > Comment on attachment 696950 [details] [diff] [review] > Resync Directory Provider from Firefox (patch 1) (v1) > > >+#include "nsCOMArray.h" > >+#include "nsIFile.h" > > #include "nsSuiteDirectoryProvider.h" > Don't bother including stuff that nsSuiteDirectoryProvider.h already > includes. ok > > >+ nsCOMArray<nsIFile> baseFiles; > Some of the patch must be missing, because you never use this once you've > appended to it. > But it's used a couple of lines down: + nsCOMArray<nsIFile> baseFiles; + + /** + * We want to preserve the following order, since the search service loads + * engines in first-loaded-wins order. + * - extension search plugin locations (prepended below using + * NS_NewUnionEnumerator) + * - distro search plugin locations + * - user search plugin locations (profile) + * - app search plugin location (shipped engines) + */ + AppendDistroSearchDirs(dirSvc, baseFiles); + AppendFileKey(NS_APP_USER_SEARCH_DIR, dirSvc, baseFiles); + AppendFileKey(NS_APP_SEARCH_DIR, dirSvc, baseFiles); + Or did I misunderstand? > >+#include "nsIPrefBranch.h" > >+#include "nsDirectoryServiceDefs.h" > >+#include "nsIPrefLocalizedString.h" > >+#include "nsIPrefService.h" > Don't include these, you don't use them here. But it doesn't compile without these..
Comment 14•11 years ago
|
||
(In reply to Edmund Wong from comment #13) > (In reply to comment #12) > > (From update of attachment 696950 [details] [diff] [review]) > > >+ nsCOMArray<nsIFile> baseFiles; > > Some of the patch must be missing, because you never use this once you've > > appended to it. > > > But it's used a couple of lines down: It's appended a couple of lines down, but once you've appended to it, you never use it. > > >+#include "nsIPrefBranch.h" > > >+#include "nsDirectoryServiceDefs.h" > > >+#include "nsIPrefLocalizedString.h" > > >+#include "nsIPrefService.h" > > Don't include these, you don't use them here. > But it doesn't compile without these.. Well, it's unclear because you added them to both files, but I only meant you to remove these four from the .h file, and remove the other three from the .cpp file.
Assignee | ||
Comment 15•11 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #14) > (In reply to Edmund Wong from comment #13) > > (In reply to comment #12) > > > (From update of attachment 696950 [details] [diff] [review]) > > > >+ nsCOMArray<nsIFile> baseFiles; > > > Some of the patch must be missing, because you never use this once you've > > > appended to it. > > > > > But it's used a couple of lines down: > It's appended a couple of lines down, but once you've appended to it, you > never use it. > haha .. copy/paste fail.. > > > >+#include "nsIPrefBranch.h" > > > >+#include "nsDirectoryServiceDefs.h" > > > >+#include "nsIPrefLocalizedString.h" > > > >+#include "nsIPrefService.h" > > > Don't include these, you don't use them here. > > But it doesn't compile without these.. > Well, it's unclear because you added them to both files, but I only meant > you to remove these four from the .h file, and remove the other three from > the .cpp file. Ah. Ok.
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #696962 -
Attachment is obsolete: true
Attachment #698502 -
Flags: review+
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #696950 -
Attachment is obsolete: true
Attachment #698503 -
Flags: review?(neil)
Comment 18•11 years ago
|
||
Comment on attachment 698503 [details] [diff] [review] Resync Directory Provider from Firefox (patch 1) (v2) >+#include "nsIProperties.h" [Actually we don't need this in the .cpp either because it's in the .h] >+ nsCOMPtr<nsISimpleEnumerator> baseEnum; >+ rv = NS_NewArrayEnumerator(getter_AddRefs(baseEnum), baseFiles); Good news! You're now using baseFiles. Bad news! You're not using baseEnum yet. Note that there's a catch because our version of AppendingEnumerator is simpler than Firefox's, so you can't quite just copy and paste.
Attachment #698503 -
Flags: review?(neil) → review-
Assignee | ||
Comment 19•11 years ago
|
||
Attachment #698503 -
Attachment is obsolete: true
Attachment #699060 -
Flags: review?(neil)
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #699060 -
Attachment is obsolete: true
Attachment #699060 -
Flags: review?(neil)
Attachment #699511 -
Flags: review?(neil)
Updated•11 years ago
|
Attachment #699511 -
Flags: review?(neil) → review+
Assignee | ||
Comment 21•11 years ago
|
||
Pushed to comm-central: http://hg.mozilla.org/comm-central/rev/11987db79dcd http://hg.mozilla.org/comm-central/rev/ccc5dae566c9
Updated•11 years ago
|
Target Milestone: --- → seamonkey2.18
Assignee | ||
Comment 22•11 years ago
|
||
I'm going on a limb here and saying this bug is resolved.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•