430070 - Give a specific error message for POST search engines

Status: RESOLVED FIXED

(Camino Graveyard :: General, enhancement)

Description

[Stuart Morgan]

Until we support post (bug 430067), perhaps we should have a specific error message so that it's clear that it's not a bug or a broken plugin, just something we don't support yet.

We could have the parser pass out NSErrors for specific known failure cases, and use that text when it's present.

Comment 1

[Sean Murphy]

Yes, I'm with you on this.

At one point, I did work up a version of the parser here that populated an NSError object in addition to the all encompassing BOOL return value.  I did so originally in SearchEngineManager to address the addition of duplicate engines, but strayed away since the AddSearchProviderHandler required direct use of XMLSearchPluginParser and there was no single entry point for adding a new engine.

As for the parsing, though, if Smokey also confirms, I can definitely integrate more informative error handling into the class.

Comment 2

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

Given that POST is likely a 2.0 feature, and this would be 1 (or perhaps a couple if there are other "errors" that really are useful to the user), I think we should take this for 1.6.1.  

The l10n hit for 1 new string in Localizable.strings shouldn't be *too* bad, especially if we have adequate lead-time.

Comment 3

[Mycroft Project (CC)]

In the mean time, can you confirm if the 'POST test' at:
http://mycroft.mozdev.org/download.html?name=allmusic&sherlock=yes&opensearch=yes&skipcache=yes&submitform=Search

is filtering out Camino as planned.
(Note to myself, will have to alter this test to accept IE7/Camino bejond some version if/when POST is supported - will cc myself to 430067)

Comment 4

[Stuart Morgan]

Yep, looks good, thanks!

Comment 5

[Sean Murphy]

Attachment: Patch

Adds NSError support to XMLSearchPluginParser.  Additionally, since the BrowserWindowController only talks to SearchEngineManger, I also added NSError there as well, which basically echoes the parsing error but does allow for the manager to add it's own in the future.

I'm never sure about model objects populating errors with localized strings ultimately meant for display in the interface, but doing so in this case simplified matters.  It prevented the BWC and AddSearchProviderHandler from both having to duplicate the process of examining the parsing error's status code and fetching the proper string for that code.  In the alert, I have us re-use the same first sentence in the information text for all errors and then append a specific message as the second sentence.

As far as the POST error is concerned, I actually chose to store the http method as a parsed attribute for the search engine regardless of browser support.  Since multiple <Url> elements are allowed by the OS spec, I couldn't force the parsing to stop immediately upon seeing it.  Instead, the http method is looked at more generally by the superclass when parsing is finished.  

I thought about adding a +[WebSearchField supportsHTTPMethod:] class method, to make it easy to fix bug 430067 without digging around in the XMLSearchPluginParser implementation; those attributes are already obtained and the parsing will automatically succeed when POST support is indicated.  I backed away from placing this in WebSearchField though, afraid to lightly couple the parser with a browser/view class and also because XMLSearchPluginParser still had the similar MIME type support question to answer.  During review, feel free to comment on this approach.

The alert dialog's informative text now consists of: 
> "SearchPluginInstallationErrorMessage" = "Camino was unable to add “%@” to the web search field. %@";

With one of these inserted at that last token:

> /* XMLSearchPluginParser Error Descriptions: */
> "XMLSearchPluginParserInvalidPluginFormat" = "The plug-in is in an unrecognized or unsupported format.";
> "XMLSearchPluginParserPluginNotFound" = "The plug-in could not be found on the server.";
> "XMLSearchPluginParserUnsupportedSearchURL" = "The search method used by this plug-in is not supported.";
> "XMLSearchPluginParserUnknownError" = "The search plug-in could be unavailable or in an unsupported format.";

Finally, because I added an accessor method to XMLSearchPluginParser, I also went ahead and changed the others to the safer approach of returning an autoreleased object.

Comment 6

[Stuart Morgan]

Comment on attachment 318307 [details] [diff] [review]
Patch

Architecturally this looks good. I think this is a perfectly reasonable level to add the error strings, since it has a complete picture of the failure condition and the messages are client-neutral. As for the method support communication from outside, we'd need to change things slightly either way if we wanted this class to be completely client-independent, and leaving things as they are (as your patch does) seems best for the time being.

>+"SearchPluginInstallationErrorMessage" = "Camino was unable to add “%@” to the web search field. %@";

The format markers should be %1$@ and %2$@ respectively (just for good habits in the code base, since these wouldn't ever really be reversed during localization).

>-    NSString* searchPluginName = [searchPlugin valueForKey:kWebSearchPluginNameKey];
>     NSAlert* alert = [[[NSAlert alloc] init] autorelease];
>     [alert addButtonWithTitle:NSLocalizedString(@"OKButtonText", nil)];
>     [alert setMessageText:NSLocalizedString(@"SearchPluginInstallationErrorTitle", nil)];
>-    [alert setInformativeText:[NSString stringWithFormat:NSLocalizedString(@"SearchPluginInstallationErrorMessage", nil), searchPluginName]];
>+    NSString* explanatoryText = [NSString stringWithFormat:NSLocalizedString(@"SearchPluginInstallationErrorMessage", nil),
>+                                                           [searchPlugin valueForKey:kWebSearchPluginNameKey],
>+                                                           [parsingError localizedDescription]];
>+    [alert setInformativeText:explanatoryText];

I know this isn't new, but since you are touching searchPluginName it occurs to me: if the parse failed, there's no guarantee that there's a name there to get (unless I'm confused about the flow here). If that's the case, then it should stay a local variable, and if it's nil should be set to the localized string UnknownSearchPluginName.

>+    // The spec alows multiple <Url> elements, so we can't just abort parsing if this one isn't supported.
>+    if ([self browserSupportsSearchQueryURLWithMIMEType:mimeType] &&
>         [attributeDict objectForKey:@"template"])
>     {
>       NSMutableString *searchURLTemplate = [NSMutableString stringWithString:[attributeDict objectForKey:@"template"]];
>       [self insertValuesForParametersInURLTemplate:searchURLTemplate];
>       [self setSearchEngineURL:searchURLTemplate];
>+      [self setSearchEngineURLRequestMethod:method];
>     }

Doesn't this mean that a plugin with a GET <Url>, then an alternative POST <Url>, would come out as unsupported? I imagine that's highly unlikely, but we should probably try to handle that case better if we believe it's possible--a follow-up bug is fine for that though as it's low priority.

>-- (BOOL)addSearchEngineFromPlugin:(NSDictionary *)searchPluginInfoDict
>+- (BOOL)addSearchEngineFromPlugin:(NSDictionary *)searchPluginInfoDict error:(NSError **)outError

Callers will expect (as your code demonstrates ;) that they can pass in an uninitialized NSError, so it should always come out either a valid NSError or nil; the very first thing in this method should be
  if (outError)
    *outError = nil;
to prevent ever accidentally giving back garbage.

>   XMLSearchPluginParser *pluginParser = [XMLSearchPluginParser searchPluginParserWithMIMEType:[searchPluginInfoDict objectForKey:kWebSearchPluginMIMETypeKey]];
>   if (!pluginParser)
>     return NO;

This is a NO return with no NSError, which contradicts your documentation; this case should be XMLSearchPluginParserInvalidPluginFormat.

>+typedef enum {
>+  // The search query URL template used by the plugin is not supported by the browser (e.g. it uses a POST method type):
>+  eXMLSearchPluginParserUnsupportedSearchURLError = 1,
>+  // The search plugin description file could not be found on the server:
>+  eXMLSearchPluginParserPluginNotFoundError       = 2,
>+  // Indicates a parsing error, meaning the plugin is invalid for the MIME type it represents:
>+  eXMLSearchPluginParserInvalidPluginFormatError  = 3
>+} EXMLSearchPluginParserErrorCode;

Why assign these explicit values, rather letting the enum give them defaults?

>+- (BOOL)parseSearchPluginAtURL:(NSURL *)searchPluginURL error:(NSError**)outError

Again, nil *outError immediately.

>   // |...WithContentsOfURL| methods throughout the Foundation will fail whenever the 
>   // requested web server offers to return a gzipped data stream.  To work around 
>   // this issue, we have to use NSURLConnection instead (which will automatically
>   // decompress gzipped data when necessary).
>   if (!searchPluginURL)
>     return NO;

I know this is programmer error if it happens, but it still needs some kind of NSError per your docs. I'd suggest XMLSearchPluginParserPluginNotFound, rather than making up a new error for a case that shouldn't happen.

>-- (BOOL)parseSearchPluginData:(NSData *)pluginData
>+- (BOOL)parseSearchPluginData:(NSData *)pluginData error:(NSError **)outError

You know the drill ;)


And, as always, I encourage breaking long lines that have obvious break points, but I leave that to your discretion.

Comment 7

[Sean Murphy]

Attachment: Patch, v2

(In reply to comment #6)
> (From update of attachment 318307 [details] [diff] [review])

> >-    NSString* searchPluginName = [searchPlugin valueForKey:kWebSearchPluginNameKey];
> >     NSAlert* alert = [[[NSAlert alloc] init] autorelease];
> >     [alert addButtonWithTitle:NSLocalizedString(@"OKButtonText", nil)];
> >     [alert setMessageText:NSLocalizedString(@"SearchPluginInstallationErrorTitle", nil)];
> >-    [alert setInformativeText:[NSString stringWithFormat:NSLocalizedString(@"SearchPluginInstallationErrorMessage", nil), searchPluginName]];
> >+    NSString* explanatoryText = [NSString stringWithFormat:NSLocalizedString(@"SearchPluginInstallationErrorMessage", nil),
> >+                                                           [searchPlugin valueForKey:kWebSearchPluginNameKey],
> >+                                                           [parsingError localizedDescription]];
> >+    [alert setInformativeText:explanatoryText];
> 
> I know this isn't new, but since you are touching searchPluginName it occurs to
> me: if the parse failed, there's no guarantee that there's a name there to get
> (unless I'm confused about the flow here). If that's the case, then it should
> stay a local variable, and if it's nil should be set to the localized string
> UnknownSearchPluginName.

The |searchPlugin| object is actually obtained not from the XMLSearchPluginParser but rather the WebSearchField menu.  This means the name actually not dependent on the search plugin definition (and thus parsing success), and is merely supplied from the autodiscovery <link> on the web page.  Nevertheless, I decided to be pedantic and just check for a nil name anyway, since we already have the localized 'Unknown Plug-In' string to take advantage of.

> >+    // The spec alows multiple <Url> elements, so we can't just abort parsing if this one isn't supported.
> >+    if ([self browserSupportsSearchQueryURLWithMIMEType:mimeType] &&
> >         [attributeDict objectForKey:@"template"])
> >     {
> >       NSMutableString *searchURLTemplate = [NSMutableString stringWithString:[attributeDict objectForKey:@"template"]];
> >       [self insertValuesForParametersInURLTemplate:searchURLTemplate];
> >       [self setSearchEngineURL:searchURLTemplate];
> >+      [self setSearchEngineURLRequestMethod:method];
> >     }
> 
> Doesn't this mean that a plugin with a GET <Url>, then an alternative POST
> <Url>, would come out as unsupported? I imagine that's highly unlikely, but we
> should probably try to handle that case better if we believe it's possible--a
> follow-up bug is fine for that though as it's low priority.

Ah geez, good point.  Filed as bug 431685.

> >-- (BOOL)addSearchEngineFromPlugin:(NSDictionary *)searchPluginInfoDict
> >+- (BOOL)addSearchEngineFromPlugin:(NSDictionary *)searchPluginInfoDict error:(NSError **)outError
> 
> Callers will expect (as your code demonstrates ;) that they can pass in an
> uninitialized NSError, so it should always come out either a valid NSError or
> nil; the very first thing in this method should be
>   if (outError)
>     *outError = nil;
> to prevent ever accidentally giving back garbage.

This issue was something I debated amongst myself, and I made my decision in attempt to be consistent with Apple's behavior.  Since my documentation does not indicate the value of |outError| on success, and only mentions that upon NO it is set to a valid object, I felt leaving the error undefined was reasonable.

Bill Bumgarner: "As the caller of your own method, do not rely on *anError being set to nil.  Doing so creates an impedance mismatch with the rest of Cocoa's behaviors and, thus, is simply asking for trouble."
Thread @ <http://www.cocoabuilder.com/archive/message/cocoa/2008/4/19/204669>

Now that I think about this some more though, Bill's comment was most likely more concerned about the calling code, not necessarily the behavior of the actual error returning method (so I guess my consistency goal just means the code in BWC shouldn't actually expect nil on success).  I did make this change, then.

> >   XMLSearchPluginParser *pluginParser = [XMLSearchPluginParser searchPluginParserWithMIMEType:[searchPluginInfoDict objectForKey:kWebSearchPluginMIMETypeKey]];
> >   if (!pluginParser)
> >     return NO;
> 
> This is a NO return with no NSError, which contradicts your documentation; this
> case should be XMLSearchPluginParserInvalidPluginFormat.
> 
> >+typedef enum {
> >+  // The search query URL template used by the plugin is not supported by the browser (e.g. it uses a POST method type):
> >+  eXMLSearchPluginParserUnsupportedSearchURLError = 1,
> >+  // The search plugin description file could not be found on the server:
> >+  eXMLSearchPluginParserPluginNotFoundError       = 2,
> >+  // Indicates a parsing error, meaning the plugin is invalid for the MIME type it represents:
> >+  eXMLSearchPluginParserInvalidPluginFormatError  = 3
> >+} EXMLSearchPluginParserErrorCode;
> 
> Why assign these explicit values, rather letting the enum give them defaults?

Again, debated this too, ended up following what Apple does (See NSXMLParser.h, for example).
Except for the "Apple does it" reason, I don't see it as necessary and made the change ;)

Thanks for catching the other issues.

Comment 8

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

Comment on attachment 318826 [details] [diff] [review]
Patch, v2

>Index: resources/localized/English.lproj/Localizable.strings.in
>===================================================================
>+/* XMLSearchPluginParser Error Descriptions: */
>+"XMLSearchPluginParserInvalidPluginFormat" = "The plug-in is in an unrecognized or unsupported format.";
>+"XMLSearchPluginParserPluginNotFound" = "The plug-in could not be found on the server.";
>+"XMLSearchPluginParserUnsupportedSearchURL" = "The search method used by this plug-in is not supported.";
>+"XMLSearchPluginParserUnknownError" = "The search plug-in could be unavailable or in an unsupported format.";

Nit: Make these consistent and use "The search plug-in" in all three cases instead of just XMLSearchPluginParserUnknownError.

Comment 9

[Stuart Morgan]

Comment on attachment 318826 [details] [diff] [review]
Patch, v2

>+- (BOOL)browserSupportsSearchQueryURLWithHTTPMethod:(NSString *)httpMethod;

Since you call this a request method everywhere else, it's probably better to use that term here as well for consistency.
 
>+  NSError *parsingError;
>+  BOOL parsedOK = [self parseSearchPluginData:xmlData error:&parsingError];
>+
>+  if (!parsedOK && outError)
>+    *outError = parsingError;

Since you don't use parsingError locally (and don't have a pre-set error that you are trying to preserve), isn't:
  BOOL parsedOK = [self parseSearchPluginData:xmlData error:outError];
equivalent?


r=me with those changes and comment 8 addressed.

Comment 10

[Sean Murphy]

Attachment: Patch, v3

Changes from the previous two comments are addressed.

Comment 11

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 319321 [details] [diff] [review]
Patch, v3

+"XMLSearchPluginParserUnsupportedSearchURL" = "The search method used by this plug-in is not supported.";

do we want to mention (maybe in parens afterwards) about POST so people know a bit more about the problem?

Comment 12

[Stuart Morgan]

I talked pink out of putting "POST" in the dialog; I don't think the added jargon is worthwhile given how rarely the user seeing this would be someone with any control over the source. We did think it would be good to say "not yet supported" to make it clear we intend to add support.

We could maybe add a FAQ or something on the error so that someone who does want to know why can search the web for the error string and find an explanation (and a reassurance that we plan to support it eventually).

Comment 13

[Sean Murphy]

(In reply to comment #12) 
> We could maybe add a FAQ or something on the error so that someone who does
> want to know why can search the web for the error string and find an
> explanation (and a reassurance that we plan to support it eventually).

In the alert, we could advertise more help is available via the purple question-mark button by calling -[NSAlert setShowsHelp:YES] and then load up the specific FAQ item in the alertShowHelp: delegate method.

Comment 14

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

I'm not keen on landing "new" stuff on the branch alone at this point, but I talked to Sam, and since this is holding up both bug 429718 and the l10n work on this bug and he couldn't get to boxset tonight, I was going to go ahead and land this on the branch tonight so we could get a build to l10n.

Unfortunately, the patch doesn't apply cleanly on the branch (there's an extra 
- (NSString *)simplifiedNameForElement:(NSString *)fullElementName;
in the last block of hunk 1 of XMLSearchPluginParser.mm on branch).  

After I applied Hunk 1 manually and built, I tested against a search plug-in that I'd modified to claim it used POST, but I still got the same old generic error message.

Sean, can you spin a branch version of this patch (plus the "not yet supported" string change from comment 12) and verify that it's spitting out the right error messages for you?  If everything's working OK and we still don't have resolution on boxset by night, I'll branch-only this so we can keep 1.6.1 moving.

Comment 15

[Sean Murphy]

Attachment: Branch Patch

Tested and works properly on the branch.

Comment 16

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

I went ahead and landed this on the branch so that we can get a build with the changes to l10n.

Comment 17

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

Landed on the trunk.