Resync' Directory Provider from Firefox

RESOLVED FIXED in seamonkey2.18

Status

SeaMonkey
General
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: ewong)

Tracking

Trunk
seamonkey2.18

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

7 years ago
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

7 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

7 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

7 years ago
Hey Jag, are you interested in taking this bug?
(Assignee)

Updated

5 years ago
Assignee: nobody → ewong
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 4

5 years ago
Created attachment 696950 [details] [diff] [review]
Resync Directory Provider from Firefox (patch 1) (v1)
Attachment #696950 - Flags: review?(neil)
(Assignee)

Comment 5

5 years ago
Created attachment 696952 [details] [diff] [review]
Resync Directory Provider from Firefox (patch 2) (v1)

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

5 years ago
Created attachment 696955 [details] [diff] [review]
Resync Directory Provider from Firefox (patch 2) (v2)
Attachment #696952 - Attachment is obsolete: true
Attachment #696952 - Flags: review?(neil)
Attachment #696955 - Flags: review?(neil)

Comment 7

5 years ago
> -  else if (!strcmp(aKey, NS_APP_USER_PANELS_50_FILE))
> -    leafName = "panels.rdf";
We are still using this one.
(Assignee)

Comment 8

5 years ago
Created attachment 696962 [details] [diff] [review]
Resync Directory Provider from Firefox (patch 2) (v3)
Attachment #696955 - Attachment is obsolete: true
Attachment #696955 - Flags: review?(neil)
Attachment #696962 - Flags: review?(neil)

Comment 9

5 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

5 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 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-
(Assignee)

Comment 13

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

5 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

5 years ago
Created attachment 698502 [details] [diff] [review]
Resync Directory Provider from Firefox (patch 2) (v4)
Attachment #696962 - Attachment is obsolete: true
Attachment #698502 - Flags: review+
(Assignee)

Comment 17

5 years ago
Created attachment 698503 [details] [diff] [review]
Resync Directory Provider from Firefox (patch 1) (v2)
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-
(Assignee)

Comment 19

5 years ago
Created attachment 699060 [details] [diff] [review]
Resync Directory Provider from Firefox (patch 1) (v3)
Attachment #698503 - Attachment is obsolete: true
Attachment #699060 - Flags: review?(neil)
(Assignee)

Comment 20

5 years ago
Created attachment 699511 [details] [diff] [review]
Resync Directory Provider from Firefox (patch 1) (v4)
Attachment #699060 - Attachment is obsolete: true
Attachment #699060 - Flags: review?(neil)
Attachment #699511 - Flags: review?(neil)

Updated

5 years ago
Attachment #699511 - Flags: review?(neil) → review+
(Assignee)

Comment 21

5 years ago
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/11987db79dcd
http://hg.mozilla.org/comm-central/rev/ccc5dae566c9

Updated

5 years ago
Target Milestone: --- → seamonkey2.18
(Assignee)

Comment 22

5 years ago
I'm going on a limb here and saying this bug is resolved.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
No longer blocks: 467530
You need to log in before you can comment on or make changes to this bug.