339343 - Base64 png image in opensearch plugin doesn't show up in search box dropdown

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Description

[Paul Millar]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1a2) Gecko/20060526 BonEcho/2.0a2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1a2) Gecko/20060526 BonEcho/2.0a2

If I install an opensearch search plugin with a Base64 PNG image as the icon, then the icon doesn't show up in the drop down search list.

Reproducible: Always

Steps to Reproduce:
1. Goto http://www.edazzle.net/os/pluginlist.aspx?q=bbc
2. Click on 'Add BBC News and Sport'

Actual Results:  
No icon displayed

Expected Results:  
Icon should be displayed

Comment 1

[:Gavin Sharp [email: [e-mail]]]

I reproduced this. There are a couple problems:
1) The auto-detection code tries <pageurl>/favicon.ico rather randomly as an icon value.
2) The search code doesn't handle failed HTTP requests all that great (it tries to use the 404 page from <http://www.edazzle.net/os/favicon.ico> as an icon)
3) The search code has no mechanism to deal with multiple icons, for example an icon specified in the addEngine call, and an icon found in the XML. The last icon wins. In this case the 404 page is often the last one since the request takes a bit of time.

I must say: I really appreciate all the testing you're doing on this OpenSearch code, Paul!

Comment 2

[Paul Millar]

No problem. :) All because I'm setting up my own opensearch search plugin repository and generation code. So I'm trying to work out bugs with my own code and those with the browser.

Off topic from this bug - is it possible to add OpenSearch plugins to Firefox Bon Echo using javascript (with IE7 it's window.external.AddSearchProvider(url)) - is there anything like this yet in Firefox and is it working?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

This patch was tested with the five engines at http://gavinsharp.com/os/ .

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #2)
> Off topic from this bug - is it possible to add OpenSearch plugins to Firefox
> Bon Echo using javascript (with IE7 it's
> window.external.AddSearchProvider(url)) - is there anything like this yet in
> Firefox and is it working?

No, not yet. That's bug 337780.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 223632 [details] [diff] [review]
patch

This fixes parts 2 and 3 of comment 1.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 223632 [details] [diff] [review]
patch

Measure (test!) twice, cut once. This isn't quite right.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 223632 [details] [diff] [review]
patch

Oops, I thought this was bug 339551. This patch is fine!

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 223632 [details] [diff] [review]
patch


>-    }
>+    } else
>+      this._callback(this._bytes, this._engine);

   }
   else
     this... 

instead, please.


>       case "data":
>         this._iconURI = uri;
>+        notifyAction(this, SEARCH_ENGINE_CHANGED);
>+        this._hasPreferredIcon = !!aIsPreferred;

>             notifyAction(aEngine, SEARCH_ENGINE_CHANGED);
>+            aEngine._hasPreferredIcon = !!aIsPreferred;

zany double negations are evil.  kill these, make the caller be specific.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
>    }
>    else
> instead, please.

Checked in branch and trunk, without this nit which I discussed with mconnor on IRC.

Comment 10

[Michael Ventnor]

>+ const searchHrefRegex = /^https?:\/\//i;

Forgive me if I'm wrong, but shouldn't this regex also support FTP?

Comment 11

[Phil Ringnalda (:philor)]

Should, and did in bug 342010 - lxr.mozilla.org will give you a better idea of the current state of the code than reading random month-old patches. 

Comment 12

[Michael Ventnor]

(In reply to comment #11)
> Should, and did in bug 342010 - lxr.mozilla.org will give you a better idea of
> the current state of the code than reading random month-old patches. 
> 

Ah I see. That site completely slipped my mind. :P Thank you.