Closed Bug 631897 Opened 11 years ago Closed 9 years ago

Resync' Directory Provider from Firefox

Categories

(SeaMonkey :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.18

People

(Reporter: sgautherie, Assigned: ewong)

References

()

Details

Attachments

(2 files, 6 obsolete files)

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
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
(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.
Hey Jag, are you interested in taking this bug?
Assignee: nobody → ewong
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #696950 - Flags: review?(neil)
This patch is for:

 4. Bug 366442 - search.rdf is obsolete. Yeah we are now using openSearch
Attachment #696952 - Flags: review?(neil)
Attachment #696952 - Attachment is obsolete: true
Attachment #696952 - Flags: review?(neil)
Attachment #696955 - Flags: review?(neil)
> -  else if (!strcmp(aKey, NS_APP_USER_PANELS_50_FILE))
> -    leafName = "panels.rdf";
We are still using this one.
Attachment #696955 - Attachment is obsolete: true
Attachment #696955 - Flags: review?(neil)
Attachment #696962 - Flags: review?(neil)
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
Comment on attachment 696950 [details] [diff] [review]
Resync Directory Provider from Firefox (patch 1) (v1)

Apparently this patch also fixes bug #3
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 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-
(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..
(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.
(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.
Attachment #696962 - Attachment is obsolete: true
Attachment #698502 - Flags: review+
Attachment #696950 - Attachment is obsolete: true
Attachment #698503 - Flags: review?(neil)
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-
Attachment #698503 - Attachment is obsolete: true
Attachment #699060 - Flags: review?(neil)
Attachment #699060 - Attachment is obsolete: true
Attachment #699060 - Flags: review?(neil)
Attachment #699511 - Flags: review?(neil)
Attachment #699511 - Flags: review?(neil) → review+
Target Milestone: --- → seamonkey2.18
I'm going on a limb here and saying this bug is resolved.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
No longer blocks: FF2SM
You need to log in before you can comment on or make changes to this bug.