Closed Bug 795495 Opened 10 years ago Closed 10 years ago

Use 32x32 search engine icons in HiDPI mode when available

Categories

(Firefox :: Theme, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 23

People

(Reporter: shorlander, Assigned: fryn)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 7 obsolete files)

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.
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.
Assignee: nobody → fryn
Status: NEW → ASSIGNED
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?
* Switch from PNG to ICO, and fix our ICO decoder to prefer 32x32 when in hidpi?
(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 ;)
(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?
Bug 419588 is for the ICO decoder support
Depends on: 419588
Summary: Update Search Engine Icon for 2x HiDPI Display → Use 32x32 search engine icons in HiDPI mode when available
Attached patch patch (obsolete) — Splinter Review
I figured out a way to support this cleanly without requiring bug 419588 to be fixed, and here it is! :D
Attachment #666164 - Flags: review?(dolske)
Attached patch patch (obsolete) — Splinter Review
Forgot that I had bitrotted myself. This fixes it.
Attachment #666164 - Attachment is obsolete: true
Attachment #666164 - Flags: review?(dolske)
Attachment #666166 - Flags: review?(dolske)
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.
Blocks: osx-hidpi
(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.
We have a 32x32 Twitter icon in mobile from bug 773641.
Attached patch patch v2 (obsolete) — Splinter Review
(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.
Attachment #666166 - Attachment is obsolete: true
Attachment #666166 - Flags: review?(dolske)
Attachment #666176 - Flags: review?(dolske)
(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.
PNG uses lossless data compression. Even if the source isn't PNG, converting to PNG shouldn't lose any relevant data.
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?)
Attachment #666176 - Flags: review?(dolske) → review-
(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. ;)
(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 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
Attachment #666176 - Flags: ui-review-
Attached patch patch v2 w/ updated ebay icons (obsolete) — Splinter Review
Updating the patch on this bug, so we have a patch with updated, optimized data URLs for future reference.
Attachment #666176 - Attachment is obsolete: true
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.
Assignee: fryn → nobody
Status: ASSIGNED → NEW
Depends on: 828508
No longer depends on: 828508
Version: unspecified → Trunk
Depends on: 851953
Attached patch alternate patch (obsolete) — Splinter Review
Attachment #687945 - Attachment is obsolete: true
Attachment #726507 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → fyan
Status: NEW → ASSIGNED
Attachment #731017 - Flags: review?(dolske)
Attached patch patch (obsolete) — Splinter Review
Changed setIcon signature to be more object-oriented.
Attachment #731017 - Attachment is obsolete: true
Attachment #731017 - Flags: review?(dolske)
Attachment #731049 - Flags: review?(dolske)
Attached patch patchSplinter Review
Manually created ICO files byte by byte to make them contain PNGs, because the automated converters kept stuffing the outputted ICOs with giant BMPs.
Attachment #731049 - Attachment is obsolete: true
Attachment #731049 - Flags: review?(dolske)
Attachment #731087 - Flags: review?(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.
Attachment #731087 - Flags: review?(dolske) → review+
(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
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
https://hg.mozilla.org/mozilla-central/rev/1bfd5cff709b
https://hg.mozilla.org/mozilla-central/rev/6b2d5c652043
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
You need to log in before you can comment on or make changes to this bug.