If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Camino1.6

Status

Camino Graveyard
General
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Jeff Dlouhy, Assigned: Sean Murphy)

Tracking

({fixed1.8.1.13})

unspecified
Camino1.6
x86
Mac OS X
fixed1.8.1.13

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

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

10 years ago
(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.
Right now we're matching against the name, which we have to fetch on every pageload, too; what's different?

Comment 3

10 years ago
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.
(Assignee)

Comment 4

10 years ago
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."
Assignee: nobody → murph
(Assignee)

Comment 5

10 years ago
Created attachment 301545 [details] [diff] [review]
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.
Attachment #301545 - Flags: review?(Jeff.Dlouhy)
(Reporter)

Comment 6

10 years ago
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.
Attachment #301545 - Flags: review?(Jeff.Dlouhy) → review+
(Assignee)

Comment 7

10 years ago
Created attachment 302826 [details] [diff] [review]
Patch v1.1

Thanks for the review Jeff.  This patch addresses your comments.
Attachment #301545 - Attachment is obsolete: true
Attachment #302826 - Flags: superreview?(stuart.morgan)
Moving this to 1.6 since we have a patch here.
Target Milestone: --- → Camino1.6

Comment 9

10 years ago
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.
Attachment #302826 - Flags: superreview?(stuart.morgan) → superreview-
(Assignee)

Comment 10

10 years ago
Created attachment 305410 [details] [diff] [review]
Patch, v2

Fixes everything mentioned in comment #9.
Attachment #302826 - Attachment is obsolete: true
Attachment #305410 - Flags: superreview?(stuart.morgan)
(Assignee)

Comment 11

10 years ago
Created attachment 305421 [details] [diff] [review]
Patch, v2

Sorry, I left out Localizable.strings entries in the last patch.
Attachment #305410 - Attachment is obsolete: true
Attachment #305421 - Flags: superreview?(stuart.morgan)
Attachment #305410 - Flags: superreview?(stuart.morgan)
(Assignee)

Comment 12

10 years ago
Created attachment 305427 [details] [diff] [review]
Patch, v2

Geez, I accidentally attached the same patch last time...
Attachment #305421 - Attachment is obsolete: true
Attachment #305427 - Flags: superreview?(stuart.morgan)
Attachment #305421 - Flags: superreview?(stuart.morgan)

Comment 13

10 years ago
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.
Attachment #305427 - Flags: superreview?(stuart.morgan) → superreview+
Those wording changes sound good to me.
(Assignee)

Comment 15

10 years ago
Created attachment 305793 [details] [diff] [review]
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.
Attachment #305427 - Attachment is obsolete: true

Comment 16

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

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

10 years ago
I'll fix those on checkin in the morning.

Comment 19

10 years ago
Landed on trunk and MOZILLA_1_8_BRANCH with comment 17 tweaks.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: fixed1.8.1.13
Resolution: --- → FIXED
(Assignee)

Comment 20

10 years ago
(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.
Keywords: fixed1.8.1.13
Keywords: fixed1.8.1.13
You need to log in before you can comment on or make changes to this bug.