410955 - Support AddSearchProvider JS call

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Stuart Morgan]

To really make the new expandable search engine system as extensible as users are going to want, we need to support the window.external.AddSearchProvider(engineURL) JavaScript call that pages like the MozDev Mycroft listing.

If we could get it for 1.6, that would be ideal, since otherwise there may be some user confusion around how to add things (given that lots of popular sites don't have an OpenSearch link on their page).

Comment 1

[Stuart Morgan]

Attachment: fix

This hooks us up to handle AddSearchProvider, which means that users can get searches for pretty much every site ever created from Mycroft.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 296292 [details] [diff] [review]
fix

>-"SearchPluginInstallationErrorMessage" = "Camino was unable to add %@ to the web search field. The search plugin could be unavailable or in an unsupported format.";
>+"SearchPluginInstallationErrorMessage" = "Camino was unable to add “%@” to the web search field. The search plugin could be unavailable or in an unsupported format.";
>+"UnknownSearchPluginName" = "Unknown Plugin";

I'm not sure how I missed this before (or the missing quotes :( ), but Camino style is "plug-in" rather than "plugin".

>+"SearchPluginInstallationCancelButton" = "Don't Add";

This single quote looks straight instead of curly.

Comment 3

[Mark Mentovai]

Comment on attachment 296292 [details] [diff] [review]
fix

Your first XPCOM component?  Congratulations!

Sorry, I guess I didn't understand your question yesterday - I could have helped you out with this, but I thought you were asking about something tied more closely to Spidermonkey.  I guess you wouldn't have known the magic thing to ask unless you've been through this once before.  :(  Looks like you've got it all figured out now, though.

>Index: Makefile.in

> DIRS = \
>+	idl \
> 	flashblock \

Alphabetize?

>+	idl/_xpidlgen \

Nope.  First of all, cleaning things in camino/idl should be handled by the Makefile in camino/idl.  Because idl is in DIRS, "make clean" will automatically recurse into idl and "make clean" in there, too.  Second of all, rules.mk automatically adds _xpidlgen to GARBAGE_DIRS whenever there are XPIDLSRCS.  It Just Works!

>===================================================================
>RCS file: /cvsroot/mozilla/camino/config/Camino.xcconfig,v

I didn't try to apply this patch, but there's no Index line here.  Did I catch you hand-editing patches again?  :)

>+HEADER_SEARCH_PATHS = ../dist/include/ ../dist/include/appcomps ../dist/include/camino ../dist/include/caps [...]

Might as well drop the trailing slash from that first ../dist/include/ while you're changing this line.

>Index: resources/localized/English.lproj/Localizable.strings.in

Smokey's our man here.

>--- /dev/null	2008-01-09 20:19:24.000000000 -0800
>+++ idl/Makefile.in	2008-01-09 20:14:31.000000000 -0800

Oh, now it just looks like you're diffing stuff by hand.  No biggie, but I thought I'd advertise "cvs diff -N" in case you didn't know about it.  It works with files that you've "cvs add"ed.

>+include $(topsrcdir)/config/rules.mk
>+

I'm seeing extra newlines at the ends of each of your new files.

>+// Provides the interface supported by Firefox and IE for installing OpenSearch

I think /* */ comments are traditional in .idl files.

>+  void AddSearchProvider(in AString aDescriptionURL);

Firefox calls it aDescriptionURL, but is that really the best name we can come up with?  Same in the .mm.

>+++ src/websearch/AddSearchProviderHandler.h	2008-01-09 20:15:56.000000000 -0800

>+
>+

>+#import <AppKit/AppKit.h>

I don't see any reason for this to be here.  Move it to the .mm.  And you've got two blank lines above this, but only one between the license and the includes in the .mm - I know I'm being anal, but as long as I"m mentioning this line...

>+++ src/websearch/AddSearchProviderHandler.mm	2008-01-09 20:18:34.000000000 -0800

>+#include "nsMemory.h"

Wow, really?  That's a surprise.

>+NS_IMETHODIMP AddSearchProviderHandler::AddSearchProvider(const nsAString &aDescriptionURL)

>+  BOOL parseSucceeded = [pluginParser parseSearchPluginAtURL:[NSURL URLWithString:searchDescriptionURL]];
>+  if (!parseSucceeded) {

We don't really need the variable.

>+    [NSMenu cancelAllTracking];

NICE!

>+  // If got this far, everything checked out, so add it.

Revise.

I'm on the fence about moving the code to build the three alerts out to its own shareable function.  It seems like duplicated work, but most of the stuff that goes into the dialogs is different, so the only thing it'd really gain us is insurance that the dialogs remain built up the same way when changes are made in the future.  Your call.

TODO: find the underlying window to let the dialogs shown by this method be sheets.

>+void AddSearchProviderHandler::InstallHandler()

One nice thing about Objective-C compared to C++ is that you always see the + or - in both the interface and the implementation.  Put a "static" comment above this line.

>+  NS_GetComponentRegistrar(getter_AddRefs(cr));
>+  if (!cr)
>+    return;

You can check the return value of NS_GetComponentRegistrar instead of checking for cr's nullness.  Also, since you have NS_ASSERTIONs for other failures in this function, you should add one matching this early return, too.

Comment 4

[Stuart Morgan]

Landed on trunk and MOZILLA_1_8_BRANCH, with issues from comments 2 and 3 addressed.