Add support for multple icon sizes to the search service

RESOLVED FIXED in Firefox 28

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Gavin, Assigned: rsilveira)

Tracking

(Blocks 1 bug)

Trunk
Firefox 28
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

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.
This would be the long-term solution to bug 895423.
Blocks: 895423
Duplicate of this bug: 903650
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.
Blocks: 833192
(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? :)

Updated

6 years ago
Blocks: 903292
Blocks: 895520
Posted patch WIP (obsolete) — Splinter Review
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
Assignee: nobody → rsilveira
Status: NEW → ASSIGNED
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.
Wild idea: Could we include an ICO file and use that to provide the multiple sizes we need?
(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.
(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
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.
Posted patch WIP v2 (obsolete) — Splinter Review
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.
Attachment #808888 - Attachment is obsolete: true
Attachment #811401 - Flags: feedback?(gavin.sharp)
(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 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.
Attachment #811401 - Flags: feedback?(gavin.sharp) → feedback+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Posted patch Patch v1 (obsolete) — Splinter Review
Updated after feedback comments.

Will add some tests too.
Attachment #811401 - Attachment is obsolete: true
Attachment #812293 - Flags: review?(gavin.sharp)
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.
Huh? This bug isn't about using multi-dimension ICO files, it's about supporting multiple <Image>s specified in the OpenSearch description.
Probably got confused by comments 7, 9, 10, 12 - sorry.
Posted patch 900137_test.patch (obsolete) — Splinter Review
xpcshell tests for new APIs.
Attachment #815162 - Flags: review?(gavin.sharp)
Ping. Feel free to forward to another toolkit peer.
Flags: needinfo?(gavin.sharp)
Blocks: metrobacklog
No longer blocks: 833192, 895520, 903292, 895423
Summary: add support for multple icon sizes to the search service → Change - Add support for multple icon sizes to the search service
Whiteboard: feature=change c=tbd u=tbd p=0
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.
Attachment #812293 - Flags: review?(mnoorenberghe+bmo)
Attachment #812293 - Flags: review?(gavin.sharp)
Attachment #812293 - Flags: review?(dteller)
Attachment #815162 - Flags: review?(mnoorenberghe+bmo)
Attachment #815162 - Flags: review?(gavin.sharp)
Attachment #815162 - Flags: review?(dteller)
Flags: needinfo?(gavin.sharp)
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 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.
Attachment #812293 - Flags: review?(dteller) → review+
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?
Attachment #815162 - Flags: review?(dteller) → review+
Attachment #812293 - Flags: superreview?(gavin.sharp)
Attachment #812293 - Flags: review?(MattN+bmo)
Attachment #815162 - Flags: review?(MattN+bmo)
(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!
Posted patch Patch v2 (obsolete) — Splinter Review
Carrying over r+ from :yoric.
Attachment #812293 - Attachment is obsolete: true
Attachment #812293 - Flags: superreview?(gavin.sharp)
Attachment #827718 - Flags: superreview?(gavin.sharp)
Attachment #827718 - Flags: review+
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?
Attachment #827718 - Flags: superreview?(gavin.sharp) → superreview+
Posted patch Test Patch v2 (obsolete) — Splinter Review
Addressed review comments.
Attachment #815162 - Attachment is obsolete: true
Attachment #827871 - Flags: review+
Oops, forgot to refresh.
Attachment #827871 - Attachment is obsolete: true
Attachment #827874 - Flags: review+
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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.
No longer blocks: metrobacklog
Summary: Change - Add support for multple icon sizes to the search service → Add support for multple icon sizes to the search service
Whiteboard: feature=change c=tbd u=tbd p=0
Posted patch Patch v3Splinter Review
Attachment #827718 - Attachment is obsolete: true
Attachment #828202 - Flags: superreview?(gavin.sharp)
We're killing Sherlock support (bug 862137), so we don't need to worry about that particular issue.
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!
Attachment #828202 - Flags: superreview?(gavin.sharp) → superreview+
https://hg.mozilla.org/mozilla-central/rev/4ed1121bf0f4
https://hg.mozilla.org/mozilla-central/rev/9f1168eae1b7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Blocks: 895520

Updated

6 years ago
Blocks: 937747

Updated

6 years ago
Keywords: verifyme

Updated

4 years ago
See Also: → 1130181
You need to log in before you can comment on or make changes to this bug.