413191 - OpenSearch for specific site is rediscovered when renamed by user in editor

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Jeff Dlouhy]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9b3pre) Gecko/2008011600 Camino/2.0a1pre (like Firefox/3.0b3pre)
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9b3pre) Gecko/2008011600 Camino/2.0a1pre (like Firefox/3.0b3pre)

After adding a site via OpenSearch to the search engine editor and then renaming it causes the blue search bar icon to come back and suggest adding the old search engine name.

Reproducible: Always

Steps to Reproduce:
1. Go to http://en.wikipedia.org/wiki/Main_Page
2. Add 'Wikipedia (English)'
3. Rename to 'Wikipedia'
4. Go to http://en.wikipedia.org/wiki/Dynabook
Actual Results:  
Blue search icon returns on wikipedia pages after returning to a page, wanting to add 'Wikipedia (English)'.

Expected Results:  
The search bar should check against the url for the OpenSearch plugin and not the name.

Comment 1

[Stuart Morgan]

(In reply to comment #0)
> The search bar should check against the url for the OpenSearch plugin and not
> the name.

That would require us actually going and fetching the OpenSearch description on every pageload of a site supporting OpenSearch, which I don't think we want to do.

Comment 2

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

Right now we're matching against the name, which we have to fetch on every pageload, too; what's different?

Comment 3

[Stuart Morgan]

The title is part of the <link> element on the page that we are loading; the search URL is part of the xml description file that we'd have to get by fetching and parsing it, which we currently don't do until the user tries to install it.

I guess we could store and match against the URL of the description, rather than the search URL, which might be more robust than the <link> title.

Comment 4

[Sean Murphy]

Yeah, I definitely agree this behavior causes our search fields to appear unintelligent.  During the initial design I struggled with this tradeoff and commented briefly about it here:

<http://mxr.mozilla.org/seamonkey/source/camino/src/browser/BrowserWindowController.mm#4758>

As Stuart said, I knew there's no way we could download, parse, and re-format the xml to obtain an actual search URL on a whim like that.  I like the aforementioned idea of working with the information we do have, without parsing, and I think I have a nice way to implement such a plan.

Taking an approach to remedy this will also fix the issue which I actually worried about above, where an engine's <link> element indicates a different plugin name than they chose to list in the description.  The OpenSearch documents that these should be the same, but as usual there are many sites I noticed using a <link> name like "Yahoo Search" and an installed name of just "Yahoo."

Comment 5

[Sean Murphy]

Attachment: Patch

I added an optional key for use in the installed search engine dictionaries to hold metadata about where the plugin came from, similar to Spotlight's kMDItemWhereFroms attribute.

This allows our WebSearchField's to appear a lot more intelligent in regard to realizing whether a detected plugin is already installed.  We now won't be tricked when <link> name and parsed name differ, and also gained is the ability to identify an engine installed from a plugin even after it is renamed.

Comment 6

[Jeff Dlouhy]

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

This patch looks good. The results were as suspected once I removed previously renamed engines. I just have a few nits.

>+- (BOOL)hasSearchEngineInstalledFromPlugin:(NSDictionary *)searchPluginInfoDict;

You seemed to have left a ; when copying it from the header file. :-)


>+  NSMutableDictionary *renamingSearchEngine = [NSMutableDictionary dictionaryWithDictionary:[mInstalledSearchEngines objectAtIndex:index]];

|renamingSearchEngine| seems like a odd name, but maybe I am missing the point of this variable. It sounds more like a name of a function than a variable. Maybe it should be called |renamedSearchEngine|?


Other than that r=jeff.

Comment 7

[Sean Murphy]

Attachment: Patch v1.1

Thanks for the review Jeff.  This patch addresses your comments.

Comment 8

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

Moving this to 1.6 since we have a patch here.

Comment 9

[Stuart Morgan]

Comment on attachment 302826 [details] [diff] [review]
Patch v1.1

AddSearchProviderHandler::AddSearchProvider needs to be updated to store WhereFrom, and to check for duplicates based on it (providing appropriate feedback if there is a dupe) rather than name.

Why do addSearchEngineWithName:url: and addSearchEngineWithName:url:atIndex: still exist? Presumably we don't want to have engines added without source information.

Lastly, given that we enforce uniqueness of names on launch, we shouldn't allow the creation of duplicate names. addSearchEngineFromPlugin needs to do something to unique-ify any colliding name before adding it to the list.

Comment 10

[Sean Murphy]

Attachment: Patch, v2

Fixes everything mentioned in comment #9.

Comment 11

[Sean Murphy]

Attachment: Patch, v2

Sorry, I left out Localizable.strings entries in the last patch.

Comment 12

[Sean Murphy]

Attachment: Patch, v2

Geez, I accidentally attached the same patch last time...

Comment 13

[Stuart Morgan]

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

>+"SearchPluginAlreadyInstalledMessage" = "Camino did not add the search engine because “%@” was already installed from this plug-in.";

I think we can just say:
"The search engine “%@” is already installed."
which is shorter, and I think just as clear as to the effect. The title would then become something like "Duplicate search engine." to avoid redundancy.

>+- (BOOL)hasSearchEngineInstalledFromPluginURL:(NSString *)pluginURL;

Just hasSearchEngineFromPluginURL:; if it's there, it's clearly installed.

>+- (NSDictionary *)searchEngineInstalledFromPluginURL:(NSString *)pluginURL;

Same here.

>+- (void)addSearchEngineWithName:(NSString *)engineName url:(NSString *)engineURL whereFromAttribute:(NSString *)whereFrom;

addSearchEngineWithName:searchURL:pluginURL: (or ...:fromPluginURL)

> [...] @"InstalledFromPluginAtURL";

@"PluginURL"

>-- (BOOL)addSearchEngineFromPlugin:(NSDictionary *)searchPluginInfoDict
>+- (BOOL)addSearchEngineFromPlugin:(NSDictionary *)searchPluginInfoDict;
> {

A semi-colon snuck in here somehow.

>+  return [self searchEngineInstalledFromPluginURL:pluginURL] ? YES : NO;

We generally use the style:
  return ([self searchEngineInstalledFromPluginURL:pluginURL] != nil);

>-  NSDictionary *existingRenamingSearchEngine = [mInstalledSearchEngines objectAtIndex:index];
>+  NSMutableDictionary *renamedSearchEngine = [NSMutableDictionary dictionaryWithDictionary:[mInstalledSearchEngines objectAtIndex:index]];

renamedSearchEngine is a potentially confusing name for something that hasn't been renamed yet. I think within the context of this method, |searchEngine| is sufficiently clear.

sr=smorgan with the renamings, assuming Smokey is fine with the suggested wording.

Comment 14

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

Those wording changes sound good to me.

Comment 15

[Sean Murphy]

Attachment: Patch, for Checkin

Updated to address everything in comment #13.

> >+"SearchPluginAlreadyInstalledMessage" = "Camino did not add the search engine because “%@” was already installed from this plug-in.";
> 
> I think we can just say:
> "The search engine “%@” is already installed."
> which is shorter, and I think just as clear as to the effect. The title would
> then become something like "Duplicate search engine." to avoid redundancy.

Part of the reason I worded the description that way is because the inserted engine name could have actually been renamed since it was installed, and in that case I wanted the user to be able to associate the renamed engine with the plugin name even though they're different. I don't have any problem with the simpler version, though, but just want to ensure it makes sense with a renamed engine.

Comment 16

[Stuart Morgan]

Users aren't seeing the plugin name in the case where this dialog is shown, just link text, so they have nothing to compare against whether they have renamed it or not. I think it's reasonable to expect that if a user has installed a plugin, a dialog that says "hey, you have this already" is sufficiently clear.

Comment 17

[Stuart Morgan]

Comment on attachment 305793 [details] [diff] [review]
Patch, for Checkin

A couple of the changes were missed:

>+- (void)addSearchEngineWithName:(NSString *)engineName url:(NSString *)engineURL pluginURL:(NSString *)pluginURL;

...:searchURL:..., not just url:

>+NSString *const kWebSearchEngineWhereFromKey = @"InstalledFromPluginAtURL";

@"PluginURL"

Comment 18

[Stuart Morgan]

I'll fix those on checkin in the morning.

Comment 19

[Stuart Morgan]

Landed on trunk and MOZILLA_1_8_BRANCH with comment 17 tweaks.

Comment 20

[Sean Murphy]

(In reply to comment #16)

On hindsight, that does make enough sense; I just wanted to be sure.

(In reply to comment #17)

My fault for missing out on those changes, and thanks for taking care of them on checkin.