900137 - Add support for multple icon sizes to the search service

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

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

The search service currently only reads 16x16 and exposes icons from OpenSearch files. We should fix that, and adjust the nsISearchEngine API to expose all icons specified in the file somehow.

Comment 1

[:Margaret Leibovic]

This would be the long-term solution to bug 895423.

Comment 3

[Jonathan Wilde [:jwilde]]

Over on Metro, we /also/ need the ability to support multiple sizes of higher-resolution icons for search providers.

Currently, the OpenSearch 1.1 Draft 6 spec contains a way to specify a series of search providers with different widths and heights:

  http://www.opensearch.org/Specifications/OpenSearch/1.1/Draft_3#The_.22Image.22_element

One way to implement this would be to keep the `nsISearchEngine` `iconURI` field unchanged, but add an additional `icons` field that contains a list of image objects that each have a nsIURI, numeric width, and numeric height.

Comment 4

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

(In reply to Jonathan Wilde [:jwilde] from comment #3)
> One way to implement this would be to keep the `nsISearchEngine` `iconURI`
> field unchanged, but add an additional `icons` field that contains a list of
> image objects that each have a nsIURI, numeric width, and numeric height.

Sounds good to me! Want to take this bug? :)

Comment 5

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: WIP

WIP

Giving this a whirl since it blocks bug 895520.

For metro we're using a 74x74 image which is non-standard, but we also don't support adding engines - you only get the built-in ones.

The 64x64 image is part of the spec[1] and should be available.

This patch adds an imageLarge (64x64) and imageMetro (74x74) fields to the engines. It only works when reading form an opensearch xml.

An image array would make it more flexible but would also make using the API more complex and could be overkill. 

Feedback greatly appreciated.

[1] - https://developer.mozilla.org/en/docs/Creating_OpenSearch_plugins_for_Firefox#OpenSearch_description_file

Comment 6

[:Margaret Leibovic]

Comment on attachment 808888 [details] [diff] [review]
WIP

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

::: netwerk/base/public/nsIBrowserSearchService.idl
@@ +118,5 @@
>    readonly attribute nsIURI iconURI;
>  
> +
> +  readonly attribute nsIURI imageLarge;
> +  readonly attribute nsIURI imageMetro;

Drive-by: Is there a more generic term we can use here instead of "Metro"? I think we should try to avoid including product-specific variable names in the platform code.

I also don't know that imageLarge is a great name, if it's going to be associated with 64x64 icons, since those are already not as large as ideal icons would be for Fennec on high DPI devices.

I think we should consider creating a more generic map of sizes to images, since that would be a more future-proof solution to support a larger set of icon sizes.

Comment 7

[Chris Kitching [:ckitching]]

Wild idea: Could we include an ICO file and use that to provide the multiple sizes we need?

Comment 8

[Rodrigo Silveira [:rsilveira] [:rodms]]

(In reply to :Margaret Leibovic from comment #6)
> Comment on attachment 808888 [details] [diff] [review]
> WIP
> 
> Review of attachment 808888 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/public/nsIBrowserSearchService.idl
> @@ +118,5 @@
> >    readonly attribute nsIURI iconURI;
> >  
> > +
> > +  readonly attribute nsIURI imageLarge;
> > +  readonly attribute nsIURI imageMetro;
> 
> Drive-by: Is there a more generic term we can use here instead of "Metro"? I
> think we should try to avoid including product-specific variable names in
> the platform code.
> 
> I also don't know that imageLarge is a great name, if it's going to be
> associated with 64x64 icons, since those are already not as large as ideal
> icons would be for Fennec on high DPI devices.
> 
> I think we should consider creating a more generic map of sizes to images,
> since that would be a more future-proof solution to support a larger set of
> icon sizes.

Yeah, totally agree. We'll actually need high-dpi images for metro too. Makes sense to have a more generic API, the need already exists.

I'll start with a method to get image by size. Later on we can add a method to get the image array that can be used for things like finding the best available image.

Comment 9

[Rodrigo Silveira [:rsilveira] [:rodms]]

(In reply to Chris Kitching [:ckitching] from comment #7)
> Wild idea: Could we include an ICO file and use that to provide the multiple
> sizes we need?

Yes, that would be nice. There are some bugs around that like bug 795495 and bug 492172. I'll take a look and see what support we already have, -moz-resolution[1] looks like a good first step.

[1] - https://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsMediaFragmentURIParser.cpp#379

Comment 10

[Axel Hecht [:Pike]]

Using multiple images within an ICO is cool, but only if we have a tool that allows us to create those easily. Right now, we're pointing localizers to http://software.hixie.ch/utilities/cgi/data/data to create the icons.

Also, is there a way to review the various images within an ICO effectively? Again, right now, I just open the data URI in Firefox, and I don't know if that covers multiple images within the ICO file.

Comment 11

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: WIP v2

Change API to getIconURIBySize and getIcons.

This will only work for search engines read from opensearch/mozsearch format. An <Image> entry with the appropriate width/height is needed for each size.

There are other entry points like addEngine and addEngineWithDetails that aren't changed.

f? Gavin as he reviewed the latest patches in this area. I'd like to know if this is in the right direction before adding tests and comments.

Comment 12

[Chris Kitching [:ckitching]]

(In reply to Axel Hecht [:Pike] from comment #10)
> Using multiple images within an ICO is cool, but only if we have a tool that
> allows us to create those easily. Right now, we're pointing localizers to
> http://software.hixie.ch/utilities/cgi/data/data to create the icons.
> 
> Also, is there a way to review the various images within an ICO effectively?
> Again, right now, I just open the data URI in Firefox, and I don't know if
> that covers multiple images within the ICO file.

GIMP can do this. 
Firefox can open an image from an ICO file of a specified size by doing something like:
http://www.nvidia.com/favicon.ico#-moz-resolution=256,256

(Thanks, Wes!)

Support for decoding ICOs properly and making best use of their contents is covered by the ongoing favicon system work in Bug 914296 and Bug 748100 - which will hopefully land before the thermal death of the universe.

Comment 13

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

Comment on attachment 811401 [details] [diff] [review]
WIP v2

Looks good overall!

>diff --git a/netwerk/base/public/nsIBrowserSearchService.idl b/netwerk/base/public/nsIBrowserSearchService.idl

>+  nsIURI getIconURIBySize(in unsigned long width, in unsigned long height);

unsigned? probably not necessary :)

>+  void getIcons(
>+            [optional] out unsigned long count,
>+            [retval, array, size_is(count)] out nsISearchEngineIcon result);

These could use some documentation. E.g. what does getIconURIBySize do if you pass it a size that doesn't exist/is invalid?

I wonder whether we should just make this JS-friendly and have it return a JS array directly as a jsval, rather than using the old xpcom outparam convention. It's annoying for C++ callers, but we probably won't really have any of those.

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>+  _getIconKey: function SRCH_ENG_getIconKey(aWidth, aHeight) {
>+    let keyObj = {
>+     width: aWidth,
>+     height: aHeight
>+    };
>+
>+    return JSON.stringify(keyObj);

Why not just aWidth+"-"+aHeight, or some such?

>+  _addIconToMap: function SRCH_ENG_addIconToMap(aWidth, aHeight, aURISpec) {
>+    this._iconMap = this._iconMap || {};

Use an actual Map()? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map

>   _parseImage: function SRCH_ENG_parseImage(aElement) {

>+    let width = parseInt(aElement.getAttribute("width"));
>+    let height = parseInt(aElement.getAttribute("height"));
>+    let isPrefered = width == 16 && height == 16;
>+
>+    this._setIcon(aElement.textContent, isPrefered, width, height);

Seems like we should probably validate the width/height values and reject obviously invalid ones.

Comment 14

[Rodrigo Silveira [:rsilveira] [:rodms]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #13)
> Comment on attachment 811401 [details] [diff] [review]
> WIP v2
> 
> Looks good overall!

Thanks for the fast feedback!

> >+  void getIcons(
> >+            [optional] out unsigned long count,
> >+            [retval, array, size_is(count)] out nsISearchEngineIcon result);
> 
> These could use some documentation. E.g. what does getIconURIBySize do if
> you pass it a size that doesn't exist/is invalid?
> 

Added the documentation.

> I wonder whether we should just make this JS-friendly and have it return a
> JS array directly as a jsval, rather than using the old xpcom outparam
> convention. It's annoying for C++ callers, but we probably won't really have
> any of those.
> 

Changed to jsval, one less interface needed!

> >diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js
> 
> >+  _getIconKey: function SRCH_ENG_getIconKey(aWidth, aHeight) {
> >+    let keyObj = {
> >+     width: aWidth,
> >+     height: aHeight
> >+    };
> >+
> >+    return JSON.stringify(keyObj);
> 
> Why not just aWidth+"-"+aHeight, or some such?
> 

This is useful when I parse the keys to retrieve width/height information in getIcons().

> >+  _addIconToMap: function SRCH_ENG_addIconToMap(aWidth, aHeight, aURISpec) {
> >+    this._iconMap = this._iconMap || {};
> 
> Use an actual Map()?
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/Map
> 

I was actually using a Map at first, but it doesn't serialize using JSON.stringify(). We need to serialize it since we parse the opensearch xml once and generate a json file for next runs.

> >   _parseImage: function SRCH_ENG_parseImage(aElement) {
> 
> >+    let width = parseInt(aElement.getAttribute("width"));
> >+    let height = parseInt(aElement.getAttribute("height"));
> >+    let isPrefered = width == 16 && height == 16;
> >+
> >+    this._setIcon(aElement.textContent, isPrefered, width, height);
> 
> Seems like we should probably validate the width/height values and reject
> obviously invalid ones.

I added a validation to check if they're <= 0. I'm not sure if the width/height attributes are optional though, didn't want to add too many restrictions.

Will post updated patch.

Comment 15

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Patch v1

Updated after feedback comments.

Will add some tests too.

Comment 16

[Axel Hecht [:Pike]]

I'd really like to get a review tool for the ICO functionality into Firefox Desktop before starting to use this.

We're loosing basically all our established review tools with using parts of data that needs to be reverse engineered by guessing or firing up involved out-of-browser or -bugzilla toolchains.

Comment 17

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

Huh? This bug isn't about using multi-dimension ICO files, it's about supporting multiple <Image>s specified in the OpenSearch description.

Comment 18

[Axel Hecht [:Pike]]

Probably got confused by comments 7, 9, 10, 12 - sorry.

Comment 19

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: 900137_test.patch

xpcshell tests for new APIs.

Comment 20

[Rodrigo Silveira [:rsilveira] [:rodms]]

Ping. Feel free to forward to another toolkit peer.

Comment 21

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

Comment on attachment 812293 [details] [diff] [review]
Patch v1

I'd like to SR this before it lands, but hopefully someone else can help with the review. Apologies for the delay.

Comment 22

[David Teller [:Yoric] - still alive but not very active]

I probably won't have time to review this patch before going to PTO. I'll try and review it next week, when I'm back.

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 812293 [details] [diff] [review]
Patch v1

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

Looks good to me, with a few changes below.

::: netwerk/base/public/nsIBrowserSearchService.idl
@@ +87,5 @@
> +   * Returns a nsIURI to an engine's icon matching both width and height.
> +   * Will return null if icon with specified dimension is not found.
> +   *
> +   * Note: Only icons defined using the OpenSearch format with dimensions
> +   * defined will be available through this API.

Nit: Present tense. Also below.

::: toolkit/components/search/nsSearchService.js
@@ +1469,5 @@
> +   * @param aWidth
> +   *        Width of the icon.
> +   * @param aHeight
> +   *        Height of the icon.
> +   * @returns key string

I don't follow, what's the point of serializing this to string?

@@ +1475,5 @@
> +  _getIconKey: function SRCH_ENG_getIconKey(aWidth, aHeight) {
> +    let keyObj = {
> +     width: aWidth,
> +     height: aHeight
> +    };

I would be more comfortable if you could either make this a Map or prefix your keys to avoid any possible confusion, e.g. "icon:".

@@ +1488,5 @@
> +   *        Width of the icon.
> +   * @param aHeight
> +   *        Height of the icon.
> +   * @param aURISpec
> +   *        nsURI of the icon.

Looks to me like it's a string, rather than a nsURI.

@@ +1510,5 @@
>     *         override non-preferred icons.
> +   *  @param aWidth (optional)
> +   *         Width of the icon.
> +   *  @param aHeight (optional)
> +   *         Height of the icon.

Please document what happens if aWidth and/or aHeight is not provided.

@@ +1515,2 @@
>     */
> +  _setIcon: function SRCH_ENG_setIcon(aIconURL, aIsPreferred, aWidth, aHeight) {

Please give default values to aWidth and aHeight.

@@ +1531,5 @@
> +          this._hasPreferredIcon = aIsPreferred;
> +        }
> +
> +        if (aWidth && aHeight) {
> +          this._addIconToMap(aWidth, aHeight, aIconURL)

So we only add icons to the map if they are provided as data:? Is that what what you mean by « Only icons defined using the OpenSearch format [...] will be available through this API »? Regardless, a small clarification in the documentation would be useful.

@@ +1741,5 @@
>    _parseImage: function SRCH_ENG_parseImage(aElement) {
>      LOG("_parseImage: Image textContent: \"" + limitURILength(aElement.textContent) + "\"");
> +
> +    let width = parseInt(aElement.getAttribute("width"), 10);
> +    let height = parseInt(aElement.getAttribute("height"), 10);

Could you take the opportunity to handle parse errors gracefully?

@@ +1745,5 @@
> +    let height = parseInt(aElement.getAttribute("height"), 10);
> +    let isPrefered = width == 16 && height == 16;
> +
> +    if (width <= 0 || height <=0) {
> +      LOG("OpenSearch image element must have positive width and height.");

You removed aElement.textContent from the log, is that by design?

@@ +2626,5 @@
> +   * Returns a nsIURI to an engine's icon matching both width and height.
> +   * Will return null if icon with specified dimension is not found.
> +   *
> +   * Note: Only icons defined using the OpenSearch format with dimensions
> +   * defined will be available through this API.

Nit: Present tense.

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 815162 [details] [diff] [review]
900137_test.patch

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

Looks good to me.

::: toolkit/components/search/tests/xpcshell/test_multipleIcons.js
@@ +23,5 @@
> +    false,
> +    {
> +      onSuccess: test_multiIcon,
> +      onError: errorCode => do_throw("Failed to add engine with errorCode " + errorCode)
> +    });

Could you wait until Services.search is initialized before proceeding with your test? Here, you're using the deprecated synchronous fallback.

@@ +45,5 @@
> +
> +    do_check_eq(allIcons.length, 3);
> +
> +    let expectedWidths = [16, 32, 74];
> +    do_check_true(allIcons.every((item) => {

Nit: Could you add a do_print to make logs a little clearer?

Comment 25

[Rodrigo Silveira [:rsilveira] [:rodms]]

(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #23)
> Comment on attachment 812293 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 812293 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, with a few changes below.
> 

Thanks for the review!

> ::: toolkit/components/search/nsSearchService.js
> @@ +1469,5 @@
> > +   * @param aWidth
> > +   *        Width of the icon.
> > +   * @param aHeight
> > +   *        Height of the icon.
> > +   * @returns key string
> 
> I don't follow, what's the point of serializing this to string?
>

This is so that I can easily de-serialize the key in getIcons().

> @@ +1475,5 @@
> > +  _getIconKey: function SRCH_ENG_getIconKey(aWidth, aHeight) {
> > +    let keyObj = {
> > +     width: aWidth,
> > +     height: aHeight
> > +    };
> 
> I would be more comfortable if you could either make this a Map or prefix
> your keys to avoid any possible confusion, e.g. "icon:".
> 

I was actually using a Map at first, but it doesn't serialize using JSON.stringify(). We need to serialize it since we parse the opensearch xml once and generate a json file for next runs. See _serializeToJSON(). I'd rather not add an "icon:" prefix because I'd have to remove it when de-serializing in getIcons().

> @@ +1515,2 @@
> >     */
> > +  _setIcon: function SRCH_ENG_setIcon(aIconURL, aIsPreferred, aWidth, aHeight) {
> 
> Please give default values to aWidth and aHeight.
> 

If I add it to the method signature I won't be able to detect when it's not defined and I don't want to assume it's 16x16 when it's not defined.

> @@ +1745,5 @@
> > +    let height = parseInt(aElement.getAttribute("height"), 10);
> > +    let isPrefered = width == 16 && height == 16;
> > +
> > +    if (width <= 0 || height <=0) {
> > +      LOG("OpenSearch image element must have positive width and height.");
> 
> You removed aElement.textContent from the log, is that by design?
> 

Yes, at the beginning of the function there's a log showing textContent already. And it has limitURILength() witch is useful now that we have bigger images.

I'll post a patch shortly with the comments addressed. Thanks!

Comment 26

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Patch v2

Carrying over r+ from :yoric.

Comment 27

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

Comment on attachment 827718 [details] [diff] [review]
Patch v2

>diff --git a/netwerk/base/public/nsIBrowserSearchService.idl b/netwerk/base/public/nsIBrowserSearchService.idl

>+   * Returns a nsIURI to an engine's icon matching both width and height.
>+   * Returns null if icon with specified dimension is not found.
>+   *
>+   * Note: Only icons defined using the OpenSearch format with dimensions
>+   * defined are available through this API.

I'm not sure what this means, can you clarify? Which icons are excluded? (same issue with the comment for getIcons)

>+  /**
>+   * Gets an array of all available icons. Each entry is an object with
>+   * width, height and uri properties. Width and height are numeric and
>+   * represent the icon's dimensions. uri is the nsURI of the icon.

nsURI -> nsIURI

Though I think it makes more sense to just return a string - feels like most callers will just want to load it, which usually involves e.g. by setting a 'src' attribute, and it's easy enough for the caller to call createURI themselves if that's not what they want. I suppose for consistency that means getIconURIBySize should return a string too. Feel free to push back if you think my sense of the needs of callers is wrong :) 

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>+   * Sets the .iconURI property of the engine. If both aWidth and aHeight are
>+   * provided and the icon is not remote (i.e. provided by a data: uri), an
>+   * entry will be added to _iconMap that will enable accessing icon's data
>+   * through getIcons() and getIconURIBySize() APIs.

This still doesn't clarify - why are data: URI icons handled differently?

nit: iconMap is a bit of a confusing name since we have actual Map()s now and this is not one of them. iconMapObj?

Comment 28

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Test Patch v2

Addressed review comments.

Comment 29

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: 900137_test.patch

Oops, forgot to refresh.

Comment 30

[Rodrigo Silveira [:rsilveira] [:rodms]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #27)
> Comment on attachment 827718 [details] [diff] [review]
> Patch v2
> 
> >diff --git a/netwerk/base/public/nsIBrowserSearchService.idl b/netwerk/base/public/nsIBrowserSearchService.idl
> 
> >+   * Returns a nsIURI to an engine's icon matching both width and height.
> >+   * Returns null if icon with specified dimension is not found.
> >+   *
> >+   * Note: Only icons defined using the OpenSearch format with dimensions
> >+   * defined are available through this API.
> 
> I'm not sure what this means, can you clarify? Which icons are excluded?
> (same issue with the comment for getIcons)
>

We support MozSearch, OpenSearch and Sherlock format for defining search engines. MozSeach is parsed the same as OpenSearch in code. It's just that there is no way to define width and height of the icon in Sherlock, so it's not supported for the new APIs. I removed the mention of engine format from the note.

If we really care for sherlock support we could get dimensions from the image itself. But AFAIK that would require rendering it to a canvas or similar solution, which would be more involved than just supporting opensearch attributes.

> >+  /**
> >+   * Gets an array of all available icons. Each entry is an object with
> >+   * width, height and uri properties. Width and height are numeric and
> >+   * represent the icon's dimensions. uri is the nsURI of the icon.
> 
> nsURI -> nsIURI
> 
> Though I think it makes more sense to just return a string - feels like most
> callers will just want to load it, which usually involves e.g. by setting a
> 'src' attribute, and it's easy enough for the caller to call createURI
> themselves if that's not what they want. I suppose for consistency that
> means getIconURIBySize should return a string too. Feel free to push back if
> you think my sense of the needs of callers is wrong :) 
>

Makes sense, the nsIURI was not adding any value. I've changed it to string and renamed getIcon*URI*BySize to getIcon*URL*BySize .

> >diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js
> 
> >+   * Sets the .iconURI property of the engine. If both aWidth and aHeight are
> >+   * provided and the icon is not remote (i.e. provided by a data: uri), an
> >+   * entry will be added to _iconMap that will enable accessing icon's data
> >+   * through getIcons() and getIconURIBySize() APIs.
> 
> This still doesn't clarify - why are data: URI icons handled differently?
> 

That was because my patch only handled data uris, pure laziness :). I added support for http/https/ftp, it was already being converted to data uris anyway was just a matter of calling _addIconToMap.

> nit: iconMap is a bit of a confusing name since we have actual Map()s now
> and this is not one of them. iconMapObj?

Done. Also added a comment about why an obj and not a Map().

Thanks for the review! I'll post the updated patch with the above changes and sr? again.

Comment 31

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Patch v3

Comment 32

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

We're killing Sherlock support (bug 862137), so we don't need to worry about that particular issue.

Comment 33

[Rodrigo Silveira [:rsilveira] [:rodms]]

Awesome, one less worry.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=f7465205e7a2

Comment 34

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

Comment on attachment 828202 [details] [diff] [review]
Patch v3

>diff --git a/netwerk/base/public/nsIBrowserSearchService.idl b/netwerk/base/public/nsIBrowserSearchService.idl

>+   * Note: Only icons with both width and height attributes defined are
>+   * available through this API.

Per comment 32, I would just drop this comment (from getIcons() too).

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>+  getIcons: function SRCH_ENG_getIcons() {

>+    for(let key of Object.keys(this._iconMapObj)) {

ubernit: space after "for" :)

Thanks for your work on this!

Comment 35

[Rodrigo Silveira [:rsilveira] [:rodms]]

Try run looked good. Landed on fx-team \o/:
https://hg.mozilla.org/integration/fx-team/rev/4ed1121bf0f4
https://hg.mozilla.org/integration/fx-team/rev/9f1168eae1b7

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ed1121bf0f4
https://hg.mozilla.org/mozilla-central/rev/9f1168eae1b7

Comment 37

[Ioana (away)]

https://hg.mozilla.org/mozilla-central/diff/9f1168eae1b7/toolkit/components/search/tests/xpcshell/test_multipleIcons.js