795495 - Use 32x32 search engine icons in HiDPI mode when available

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Stephen Horlander [:shorlander]]

Bug 781327 updates most of the main window UI for HiDPI with the exception of the icon in the search field. The image is defined in the search plugin XML file as a data URI so the method used in Bug 781327 doesn't appear to work here.

Comment 1

[Frank Yan (:fryn)]

I will take this.
We could either replace the data URI with a 32x32 one, or we could expand the XML file format to contain more data URIs. I prefer the latter, because it avoids degradation on non-HiDPI displays, but it requires more code changes obviously.

Comment 2

[Justin Dolske [:Dolske]]

Seems like there are a few different possible ways to fix this:

* Extend OpenSearch to include a <ImageHiDPI> tag, or something handwavy with providing multiple (existing) <Image> tags and we figure out which is which.

* Change these data URIs to chrome://, and do some CSS/override/mumble magic to make the right thing happen

* Change the icons to 32x32 and hope they scale down OK to 16x16 on lodpi displays?

* A totally gross temporary hack in the code to just check which built-in provider is selected, and switcharoo the image for the hidpi flavor. Possibly the easiest was to fix things for this cycle?

Comment 3

[Justin Dolske [:Dolske]]

* Switch from PNG to ICO, and fix our ICO decoder to prefer 32x32 when in hidpi?

Comment 4

[Stephen Horlander [:shorlander]]

(In reply to Justin Dolske [:Dolske] from comment #3)
> * Switch from PNG to ICO, and fix our ICO decoder to prefer 32x32 when in
> hidpi?

I like this approach because we should do that anyway for the tab favicon ;)

Comment 5

[Frank Yan (:fryn)]

(In reply to Stephen Horlander from comment #4)
> (In reply to Justin Dolske [:Dolske] from comment #3)
> > * Switch from PNG to ICO, and fix our ICO decoder to prefer 32x32 when in
> > hidpi?
> 
> I like this approach because we should do that anyway for the tab favicon ;)

I third this, and I'm happy to (try to) write the decoder code for it too. Is this do-able in a reasonable time frame?

Comment 6

[Matthew N. [:MattN]]

Bug 419588 is for the ICO decoder support

Comment 7

[Frank Yan (:fryn)]

Attachment: patch

I figured out a way to support this cleanly without requiring bug 419588 to be fixed, and here it is! :D

Comment 8

[Frank Yan (:fryn)]

Attachment: patch

Forgot that I had bitrotted myself. This fixes it.

Comment 9

[Frank Yan (:fryn)]

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

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

::: toolkit/components/search/nsSearchService.js
@@ +1602,5 @@
>        this._setIcon(aElement.textContent, true);
>      }
> +    else if (getIntPref("gfx.hidpi.enabled", 0) &&
> +             aElement.getAttribute("width")  == "32" &&
> +             aElement.getAttribute("height") == "32") {

Hmm, I think making getIntPref be the last condition is better, performance-wise. If the patch is good aside from this, I can change it before pushing.

Comment 10

[Frank Yan (:fryn)]

(In reply to Justin Dolske [:Dolske] from comment #2)
> with providing multiple (existing) <Image> tags and we figure out which is
> which.

Ah, I guess you did mention this. :)

Google and Amazon are the only search providers that have 32x32 icons in their favicon.ico, and none of the others seem to provide 32x32 icons via their <link/> elements either, so those two are the only ones updated in this patch.

Comment 11

[Stephen Horlander [:shorlander]]

We have a 32x32 Twitter icon in mobile from bug 773641.

Comment 12

[Frank Yan (:fryn)]

Attachment: patch v2

(In reply to Stephen Horlander from comment #11)
> We have a 32x32 Twitter icon in mobile from bug 773641.

Thanks for reminding me that I can use the icons from mobile/.../searchplugins/

Since Bing and eBay aren't available in that directory, I had to find them online, and they both made it rather difficult. I had to spoof IE9's user agent to get the 32x32 icons. :| But I got them all, and here it is!

Margaret, the icons that mobile is using in mobile/.../searchplugins/ are much larger in file (data url) size than they need to be. I recommend using the data urls that I'm using in this patch. I converted them all to data/png and pngcrush'd them. I can make a mobile patch for them if you'd like.

Comment 13

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

(In reply to Frank Yan (:fryn) from comment #12)
> Margaret, the icons that mobile is using in mobile/.../searchplugins/ are
> much larger in file (data url) size than they need to be. I recommend using
> the data urls that I'm using in this patch. I converted them all to data/png
> and pngcrush'd them. I can make a mobile patch for them if you'd like.

Be careful with this - if the source image is PNG and you're just using the default pngcrush options that's fine, but we shouldn't be making any lossy optimizations to the images we receive from providers just for the sake of filesize.

Comment 14

[Dão Gottwald [::dao]]

PNG uses lossless data compression. Even if the source isn't PNG, converting to PNG shouldn't lose any relevant data.

Comment 15

[Justin Dolske [:Dolske]]

Comment on attachment 666176 [details] [diff] [review]
patch v2

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

::: toolkit/components/search/nsSearchService.js
@@ +1603,5 @@
>      }
> +    else if (aElement.getAttribute("width")  == "32" &&
> +             aElement.getAttribute("height") == "32" &&
> +             getIntPref("gfx.hidpi.enabled", 0)) {
> +      this._setIcon(aElement.textContent, true);

Hmm. This would mean systems which are _capable_ of hidpi will also end up picking (and scaling to 16x16 screen pixels) the 32x32 image, no?

Seems like we want a check to see if we're actually using HiDPI in order to use the larger image. [Ideally image-set would make this easy.]

I think I would take either a 1-time runtime check (looking at the current display, even if it later changes), or a bit of a CSS trick to control display of (e.g.) the .icon16 or .icon32 image. (Probably not anything you can do about the dropdown, since that's Hard in xul?)

Comment 16

[Justin Dolske [:Dolske]]

(In reply to Justin Dolske [:Dolske] from comment #15)

> I think I would take either a 1-time runtime check ...

Like && window.matchMedia("(min-resolution: 2dppx)"), from your other patch. ;)

Comment 17

[Frank Yan (:fryn)]

(In reply to Justin Dolske [:Dolske] from comment #15)
> ::: toolkit/components/search/nsSearchService.js
> @@ +1603,5 @@
> >      }
> > +    else if (aElement.getAttribute("width")  == "32" &&
> > +             aElement.getAttribute("height") == "32" &&
> > +             getIntPref("gfx.hidpi.enabled", 0)) {
> > +      this._setIcon(aElement.textContent, true);
> 
> Hmm. This would mean systems which are _capable_ of hidpi will also end up
> picking (and scaling to 16x16 screen pixels) the 32x32 image, no?

> Like && window.matchMedia("(min-resolution: 2dppx)"), from your other patch.
> ;)

I don't think that this is a big deal, because downscaling from 32x32 to 16x16 in practice does not make images look much worse than the 16x16 originals.

Also, as I understand it, the window object is not available in nsSearchService.js.

I realize that this solution is not ideal, but it's the cleanest way (minimal, isolated code changes) to get 32x32 icons to the user's screen in HiDPI mode for now.

Comment 18

[Frank Yan (:fryn)]

Comment on attachment 666176 [details] [diff] [review]
patch v2

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

After doing a full side-by-side comparison of 16x16 originals and 32x32 icons downscaled to 16x16 and discussing with dolske and shorlander, I'm giving myself a ui-review- on this, because we shouldn't prioritize HiDPI over non-HiDPI for now.
https://people.mozilla.com/~fyan/tmp/icon-downscaling.png

Comment 19

[Frank Yan (:fryn)]

Attachment: patch v2 w/ updated ebay icons

Updating the patch on this bug, so we have a patch with updated, optimized data URLs for future reference.

Comment 20

[Frank Yan (:fryn)]

I guess the best available approach for now is to carry both sizes all the way to the front-end, where CSS or window.matchMedia can be used to filter for HiDPI, but I'm relinquishing this bug for the time being.

Comment 21

[Frank Yan (:fryn)]

Attachment: alternate patch

Comment 22

[Frank Yan (:fryn)]

Attachment: patch

Comment 23

[Frank Yan (:fryn)]

Attachment: patch

Changed setIcon signature to be more object-oriented.

Comment 24

[Frank Yan (:fryn)]

Attachment: patch

Manually created ICO files byte by byte to make them contain PNGs, because the automated converters kept stuffing the outputted ICOs with giant BMPs.

Comment 25

[Justin Dolske [:Dolske]]

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

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

r+ with comments addressed.

I also thought a bit about maybe having this code live in <menuitem>, so that this would work anywhere, but I'm not really sure how useful that is -- generally we control what chrome URI we stuff in there, and this is kind of an exceptional case. So let's not.

::: browser/components/search/content/search.xml
@@ +275,5 @@
> +        <body><![CDATA[
> +          if (uri) {
> +            let size = Math.round(16 * window.devicePixelRatio);
> +            uri += (uri.contains("#") ? "&" : "#") +
> +                   "-moz-resolution=" + size + "," + size;

Eew, did we really add support for hash syntax for -moz-resolution?

I'd probably just keep this simple, since I wouldn't be expecting hashes here in the first place. And if something _is_ using them, I'd be astonished if they supported any obscure syntax we've tried to impose.

  if (uri && !uri.contains('#')) {
    uri += "#-moz-resolution=" + ...
  }

@@ +348,5 @@
>                menuitem.setAttribute("label", labelStr);
>                menuitem.setAttribute("tooltiptext", engineInfo.uri);
>                menuitem.setAttribute("uri", engineInfo.uri);
>                if (engineInfo.icon)
> +                this.setIcon.call(menuitem, engineInfo.icon);

This is a little hacky, I think you were better off with the approach in attachment 731017 [details] [diff] [review] (passing in an |element|). One would expect |this| in a binding's method to always be the bound element, when writing a helper function just write it the plain way without relying upon magic in the caller.

Comment 26

[Frank Yan (:fryn)]

(In reply to Justin Dolske [:Dolske] from comment #25)

Thank you for the review. :)

> Eew, did we really add support for hash syntax for -moz-resolution?

Yes, it's like "media fragments", and it's ugly. :|

> I'd probably just keep this simple, since I wouldn't be expecting hashes
> here in the first place. And if something _is_ using them, I'd be astonished
> if they supported any obscure syntax we've tried to impose.
> 
>   if (uri && !uri.contains('#')) {
>     uri += "#-moz-resolution=" + ...
>   }

It's not the server that needs to support the hash syntax. The server does not even receive the hash portion of URIs. This is a directive for Firefox's decoder only. We have the more robust concatenation in tabbrowser.xml already: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#713


Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75f108f790a1

Comment 27

[Phil Ringnalda (:philor)]

Remember browser_google.js, that browser-chrome test you broke when you updated the Google icon because it tests against a hard-coded data URI?

Yeah, backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/74354f979ea8

Comment 28

[Frank Yan (:fryn)]

Ugh, that data URI again.

My bad. Fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bfd5cff709b
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b2d5c652043

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1bfd5cff709b
https://hg.mozilla.org/mozilla-central/rev/6b2d5c652043