Closed Bug 296430 Opened 15 years ago Closed 15 years ago

Allow extensions to ship searchplugins

Categories

(Toolkit :: Startup and Profile System, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta3

People

(Reporter: mkaply, Assigned: benjamin)

References

Details

Attachments

(3 files, 2 obsolete files)

Similar to bug 295247, extensions should be able to ship searchplugins.
bsmedberg: 

we discussed this.
This allows directory service providers to be registered (in no particular
order) in the xpcom-directory-providers category, and will be used after
autoreg but before xpcom-startup or any real work is done. This means that they
cannot add new component directories, but they can add pretty much anything
else including chrome directories and our future searchplugin directories.
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
Attachment #185289 - Flags: first-review?(darin)
Comment on attachment 185289 [details] [diff] [review]
Directory service changes, rev. 1

I should test before attaching.
Attachment #185289 - Attachment is obsolete: true
Attachment #185289 - Flags: first-review?(darin)
ok, this one works
Attachment #185300 - Flags: first-review?(darin)
Comment on attachment 185298 [details] [diff] [review]
Add a directory list for internetsearchservice, rev. 1

mkaply, I have no clue who a good reviewer for this is: I suggest one of your
IBM minions.
Attachment #185298 - Flags: first-review?(mozilla)
Comment on attachment 185289 [details] [diff] [review]
Directory service changes, rev. 1

>Index: xpcom/build/nsXPComInit.cpp

>-    NS_IF_RELEASE(gDirectoryService);
>+    NS_IF_RELEASE(nsDirectoryService::gService);

this name change seems a bit gratuitous to me.	personally, i don't
mind the global gDirectoryService since it is unique enough (as 
unique as nsDirectoryService::gService at any rate).


>Index: xpcom/io/nsDirectoryService.cpp

> nsresult
> nsDirectoryService::Init()
> {
>+    NS_NOTREACHED("Don't call me, I was for internal use only!");
>+    return NS_OK;
>+}

In fact, shouldn't this function be declared with NS_IMETHODIMP?
It would seem that calling this function on Windows might crash.


>+    NS_ADDREF(gService = self);

Can this be used?

  self.swap(gService);


>+    nsCOMPtr<nsIUTF8StringEnumerator> strings (do_QueryInterface(entries));

nit: kill space between variable name and open paran.
Attachment #185289 - Attachment is obsolete: false
Attachment #185289 - Flags: first-review+
Comment on attachment 185289 [details] [diff] [review]
Directory service changes, rev. 1

whoops.. wrong patch
Attachment #185289 - Attachment is obsolete: true
Attachment #185289 - Flags: first-review+
Comment on attachment 185300 [details] [diff] [review]
Directory service changes, rev. 1

Should the "xpcom-directory-providers" category be declared in some header
file?
Attachment #185300 - Flags: first-review?(darin) → first-review+
> this name change seems a bit gratuitous to me.	personally, i don't

It's not as much a name change as a combining of nsXPCOMInit.cpp
gDirectoryService and (static) nsDirectoryService::mService.

I'm happy to add this category to a header file. Are we comfortable with
freezing it right now and putting it in nsXPCOMCID.h ?
Attachment #185300 - Flags: approval-aviary1.1a2?
Darin, this adds a #define in nsXPCOM.h and also documents a few of our
categories and observer topics that I know are frozen.
Attachment #185478 - Flags: first-review?(darin)
Attachment #185300 - Attachment is obsolete: true
Attachment #185300 - Flags: approval-aviary1.1a2?
I wanted to write this in JS, but that would have required throwing exceptions
instead of setting Components.returnCode because of bug 287107. Blech.
Depends on: 287107
So is the first "add a directory list" patch obsolete? It has totally different
content than the old one, touching different files...
Comment on attachment 185478 [details] [diff] [review]
Directory service changes, rev. 2 [checked in]

Whoops, no, attachment 185478 [details] [diff] [review] is a new version of attachment 185300 [details] [diff] [review].
Attachment #185478 - Attachment description: Add a directory list for internetsearchservice, rev. 2 → Directory service changes, rev. 2
Priority: -- → P2
Comment on attachment 185289 [details] [diff] [review]
Directory service changes, rev. 1

How come the mac code isn't needed anymore?
The XP_MAC code is OS/9 only.
Comment on attachment 185478 [details] [diff] [review]
Directory service changes, rev. 2 [checked in]

nsDirectoryService::RegisterCategoryProviders should use the new #defines that
you added to nsXPCOM.h

otherwise, this patch looks good to me.  r=darin
Attachment #185478 - Flags: first-review?(darin) → first-review+
Comment on attachment 185489 [details] [diff] [review]
Use a Firefox-specific dirprovider to provide the searchplugins list, rev. 1 [checked in]

>Index: toolkit/xre/nsXULAppAPI.h

>+#define XRE_EXTENSIONS_DIR_LIST "ExtDL"

Hopefully this never collides with something defined by XPCOM.
Should we prefix with some kind of namespace?


>Index: browser/components/dirprovider/nsBrowserDirectoryProvider.cpp


>+  nsCOMPtr<nsIFile> defaults;
>+  rv = NS_GetSpecialDirectory(NS_APP_PROFILE_DEFAULTS_50_DIR,
>+                              getter_AddRefs(defaults));
>+  if (NS_FAILED(rv))
>+    return;
>+
>+  defaults->AppendNative(aLeafName);

up above, you get the profile directory, and you clone it before
using it.  presumably in that case you did not need to clone it,
or you need to clone it here.  I think it's the case that you did
not need to clone it up above.	Too bad documentation on the 
directory service is so poor :(


r=darin
Attachment #185489 - Flags: first-review?(darin) → first-review+
Comment on attachment 185489 [details] [diff] [review]
Use a Firefox-specific dirprovider to provide the searchplugins list, rev. 1 [checked in]

I will rename the key to "XREExtDL".

And the cloning is correct: I needed both the profile directory and the
"target" file to pass to the Ensure... function.
Attachment #185489 - Flags: approval-aviary1.1a2?
Attachment #185478 - Flags: approval-aviary1.1a2?
so are those defines in nsXPCOM.h frozen? if so should you add an @status FROZEN
to it?
AIUI everything in nsXPCOM.h is frozen. I can add explicit @status FROZEN if
desired.
the functions in this file all seem to explicitly mention @status FROZEN.
Attachment #185478 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
Attachment #185489 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
Attachment #185478 - Attachment description: Directory service changes, rev. 2 → Directory service changes, rev. 2 [checked in]
Attachment #185489 - Attachment description: Use a Firefox-specific dirprovider to provide the searchplugins list, rev. 1 → Use a Firefox-specific dirprovider to provide the searchplugins list, rev. 1 [checked in]
http://lxr.mozilla.org/seamonkey/source/browser/components/dirprovider/nsBrowserDirectoryProvider.cpp#105

now claims that our bookmarks file is called bookmarks.rdf instead of .html :-/.
s/bookmarks.rdf/bookmarks.html/ checked in *blush*.
Comment on attachment 185298 [details] [diff] [review]
Add a directory list for internetsearchservice, rev. 1

r=mkaply

Next time do the Mac removal stuff in a separate patch :)
Attachment #185298 - Flags: first-review?(mozilla) → first-review+
Attachment #185298 - Flags: approval-aviary1.1a2?
Attachment #185298 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
Can an extension that comes with search plugins overwrite existing search
plugins to lead the use to a malicious site?
> Can an extension that comes with search plugins overwrite existing search
> plugins to lead the use to a malicious site?

No, the search plugins are additive. But since extensions can be as malicious as
they want (to the point of replacing the search box entirely), it's a mostly
moot point.
Fixed on trunk for 1.8b3
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8beta3
is stuff like this documented anywhere?
I mean how to add the search plugins

should they just be in a subdir named "searchplugins" under my extension?

Do I need to register them anywhere? In the install.rdf
Component: XRE Startup → Startup and Profile System
QA Contact: nobody → startup
You need to log in before you can comment on or make changes to this bug.