Can't add Search Engines with http referenced images in the xml and support adding/removing engines

RESOLVED FIXED in Thunderbird 15.0

Status

Thunderbird
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: standard8, Assigned: Irving)

Tracking

13 Branch
Thunderbird 15.0
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
In a search xml file, if we put <image>http://[url to an icon]</image>, then Thunderbird won't load it, and generally it breaks the UI. The comment on the error console complains that engine.iconURI is undefined.
(Reporter)

Comment 1

5 years ago
Irving, can you take a look at this?

Here's roughly what to do.

- http://hg.mozilla.org/comm-central/annotate/d3f4108df62e/mail/base/content/webSearchTab.js#l119 is where it is failing initially, that needs a check for engine.iconURI being null.

- We'll then need a listener function to listen for search engine updates and tear down and rebuild the list, similar to:

http://hg.mozilla.org/mozilla-central/annotate/d698e656b1e0/browser/components/search/content/search.xml#l202

In doing that listener, we should basically make any of the add/remove/change rebuild the list, and by adding the facility for add/remove, will help us complete the code a bit more.

A tear down/rebuild is enough on its own, as this isn't going to be a frequent operation (typically just when a user installs an engine or clears a cache maybe).
Assignee: nobody → irving
Summary: Can't add Search Engines with http referenced images in the xml → Can't add Search Engines with http referenced images in the xml and support adding/removing engines
For example: http://bwinton.latte.ca/Work/google.xml
Status: NEW → ASSIGNED
The engine.iconURI == null problem arises because of a special case check at http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/search/nsSearchService.js#1715. The openSearch icon handling only fills out its internal data structure if the <Image> element in the search.xml file is explicitly annotated with width="16" height="16", e.g. <Image width="16" height="16">http://search.service/favicon.jpg</Image>
Created attachment 620394 [details] [diff] [review]
Null-check before trying to attach image to search engine selection button

This leaves the button blank; the fact that there's a button with its image missing isn't obvious. Requesting ui-r from bwinton in case he has suggestions.
Attachment #620394 - Flags: ui-review?(bwinton)
Attachment #620394 - Flags: review?(mbanner)
Comment on attachment 620394 [details] [diff] [review]
Null-check before trying to attach image to search engine selection button

That sounds way too hidden to me.  Could we use the broken-image-image instead?
Attachment #620394 - Flags: ui-review?(bwinton) → ui-review-
Bug 335781 is making progress on moving the search engine management UI into the add-ons manager, which will give us user interface for changing search engines.
See Also: → bug 335781
Created attachment 622014 [details] [diff] [review]
Better handling of bad image links in OpenSearch xml files

Log a warning and substitute the broken-image if the search service's image link isn't suitable in an OpenSearch XML spec. This involves allowing resource: URLs in the search image spec; I don't think this should be dangerous, but if it's deemed unacceptable we can look at another approach.

Also cleaned up some (but not all) of the javascript strict warnings.

To the TB folks, note that this patch is relative to the mozilla/ subdirectory

bwinton gave this a verbal ui-r+ for Thunderbird.
Attachment #620394 - Attachment is obsolete: true
Attachment #620394 - Flags: review?(mbanner)
Attachment #622014 - Flags: review?(gavin.sharp)
Created attachment 622431 [details] [diff] [review]
Add observer to update search pages when list of engines changes

Also has an if-statement to protect us from broken images even if the nsSearchService patch doesn't land, and fixes one javascript strict warning.
Attachment #622431 - Flags: review?(mbanner)
Comment on attachment 622014 [details] [diff] [review]
Better handling of bad image links in OpenSearch xml files

I don't think the fallback is a good idea - it seems useful for search service consumers to be able to detect that there was no valid image specified. That also means that they need to handle iconURI being null - this seems to already be documented in the IDL. As a consumer of this API, you can provide a fallback icon yourself if desired.

The other changes (strict warning fixes and more explicit logging for the failure) look good, though you should use LOG instead of ULOG for the warning.
Attachment #622014 - Flags: review?(gavin.sharp) → review-
Created attachment 623700 [details] [diff] [review]
Log a warning if OpenSearch image is not acceptable

Updated based on Gavin's comments, removed the change that inserted the broken image when an icon was incorrectly specified, squashed another JavaScript strict warning.
Attachment #622014 - Attachment is obsolete: true
Attachment #623700 - Flags: review?(gavin.sharp)
Comment on attachment 623700 [details] [diff] [review]
Log a warning if OpenSearch image is not acceptable

r=me without the else-after-return in the iconURI getter :)
Attachment #623700 - Flags: review?(gavin.sharp) → review+
Created attachment 623718 [details] [diff] [review]
Log a warning if OpenSearch image is not acceptable, with nit picked

Else-after-return is gone. r=gavin.sharp
Attachment #623700 - Attachment is obsolete: true
Attachment #623718 - Flags: review+
(Reporter)

Comment 13

5 years ago
Comment on attachment 622431 [details] [diff] [review]
Add observer to update search pages when list of engines changes

Review of attachment 622431 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good r=Standard8
Attachment #622431 - Flags: review?(mbanner) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/f1779f8d47e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/70d194df378f

Leaving open until the second patch lands on m-c.
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 15.0
https://hg.mozilla.org/mozilla-central/rev/70d194df378f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.