Last Comment Bug 742662 - Can't add Search Engines with http referenced images in the xml and support adding/removing engines
: Can't add Search Engines with http referenced images in the xml and support a...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: General (show other bugs)
: 13 Branch
: All All
: -- normal with 1 vote (vote)
: Thunderbird 15.0
Assigned To: :Irving Reid (No longer working on Firefox)
:
Mentors:
data:text/html;charset=utf-8,<!DOCTYP...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-05 02:56 PDT by Mark Banner (:standard8) (afk until 26th July)
Modified: 2012-05-25 08:26 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Null-check before trying to attach image to search engine selection button (1.08 KB, patch)
2012-05-02 11:32 PDT, :Irving Reid (No longer working on Firefox)
bwinton: ui‑review-
Details | Diff | Splinter Review
Better handling of bad image links in OpenSearch xml files (3.29 KB, patch)
2012-05-08 09:46 PDT, :Irving Reid (No longer working on Firefox)
gavin.sharp: review-
Details | Diff | Splinter Review
Add observer to update search pages when list of engines changes (6.06 KB, patch)
2012-05-09 11:31 PDT, :Irving Reid (No longer working on Firefox)
standard8: review+
Details | Diff | Splinter Review
Log a warning if OpenSearch image is not acceptable (3.11 KB, patch)
2012-05-14 09:22 PDT, :Irving Reid (No longer working on Firefox)
gavin.sharp: review+
Details | Diff | Splinter Review
Log a warning if OpenSearch image is not acceptable, with nit picked (3.10 KB, patch)
2012-05-14 10:31 PDT, :Irving Reid (No longer working on Firefox)
irving: review+
Details | Diff | Splinter Review

Description Mark Banner (:standard8) (afk until 26th July) 2012-04-05 02:56:02 PDT
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.
Comment 1 Mark Banner (:standard8) (afk until 26th July) 2012-04-17 14:56:11 PDT
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).
Comment 2 :Irving Reid (No longer working on Firefox) 2012-05-01 09:54:18 PDT
For example: http://bwinton.latte.ca/Work/google.xml
Comment 3 :Irving Reid (No longer working on Firefox) 2012-05-01 13:53:51 PDT
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>
Comment 4 :Irving Reid (No longer working on Firefox) 2012-05-02 11:32:43 PDT
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.
Comment 5 Blake Winton (:bwinton) (:☕️) 2012-05-02 13:33:49 PDT
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?
Comment 6 :Irving Reid (No longer working on Firefox) 2012-05-08 09:16:43 PDT
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.
Comment 7 :Irving Reid (No longer working on Firefox) 2012-05-08 09:46:57 PDT
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.
Comment 8 :Irving Reid (No longer working on Firefox) 2012-05-09 11:31:21 PDT
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.
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-10 11:20:10 PDT
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.
Comment 10 :Irving Reid (No longer working on Firefox) 2012-05-14 09:22:02 PDT
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.
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-14 10:11:44 PDT
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 :)
Comment 12 :Irving Reid (No longer working on Firefox) 2012-05-14 10:31:01 PDT
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
Comment 13 Mark Banner (:standard8) (afk until 26th July) 2012-05-24 14:25:52 PDT
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
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-05-24 15:50:55 PDT
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.
Comment 15 Ed Morley [:emorley] 2012-05-25 08:26:56 PDT
https://hg.mozilla.org/mozilla-central/rev/70d194df378f

Note You need to log in before you can comment on or make changes to this bug.