1163559 - Search engine icons are always displayed at low resolution

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Florian Quèze [:florian]]

Bug 1121802 changed PlacesUtils.getImageURLForResolution so that it now only works for URLs finishing with .ico or .ICO.

Unfortunately, that means it broke for engine icons that are data URLs.

[Tracking Requested - why for this release]: very visible (on retina screens) regression.

Comment 2

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Do you know whether we used hidpi search icons on Windows before this regression kicked in?

Comment 3

[Florian Quèze [:florian]]

(In reply to Philipp Sackl [:phlsa] please use needinfo from comment #2)
> Do you know whether we used hidpi search icons on Windows before this
> regression kicked in?

I think we did until Firefox 35 (the regression shipped in 36).

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch

There are two ways that I can see to fix this: a) either do the math (let size = 16 * window.devicePixelRatio) and add the -moz-resolution in each place we're setting the icon, or b) add an optional parameter to PlaceUtils.getImageURLForResolution that makes it bypass the .ico check.

I like b) better, and am attaching a patch for it. It seems like this stuff will get cleared up once the downscale-during-decode stuff lands anyway. If you want a), please let me know with an r- :)
Also, if you have a better suggestion for that parameter than "aNotIco", I'd be grateful ;)

Wasn't sure who to r? on this, so I r?'d bwinton because he reviewed the patch on bug 1121802, and florian because of the search changes. If I should be requesting r? from someone else let me know.

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

By the way, this patch assumes that we are storing the favicons of the search providers somewhere and that they are indeed ico's that will benefit from downscale-during-decode. I'm going to do some research and see if this assumption is good in a while, and possibly think of an alternative fix if it's bad. If you reach your r- stamp before I figure it out, that's fine too.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8610885 [details] [diff] [review]
Patch

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

::: toolkit/components/places/PlacesUtils.jsm
@@ +1618,5 @@
>     */
>    getImageURLForResolution:
> +  function PU_getImageURLForResolution(aWindow, aURL, aWidth = 16, aHeight = 16,
> +                                       aNotIco) {
> +    if (!aNotIco && !aURL.endsWith('.ico') && !aURL.endsWith('.ICO')) {

Aren't you passing a data URI as aURL? Why don't you instead check if the URL contains the mimetype string e.g. "image/x-icon" from the data URI? While you're fixing this logic, can you also just do a .includes(".ico") instead of endsWith so it handles query parameters after the extension as people have been complaining about this. I don't think we should add a new argument to this method since these problems affect website favicons too and we don't need special treatment here IMO.

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8610885 [details] [diff] [review]
Patch

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

Like Matthew said, please don't add another parameter. I think you can parse the string into a URI, and then check .schemeIs("data"). Also, it would be nice to change the .ico check to ignore the query parameter, and the hash. This is easier to do with an URI object.

I'm not sure checking the mime type is useful, as for data urls we'll never be causing an additional network request anyway, but I guess it doesn't hurt.

Comment 8

[Blake Winton (:bwinton) (:☕️)]

(In reply to Matthew N. [:MattN] from comment #6)
> While you're fixing this logic, can you also just do a .includes(".ico")
> instead of endsWith so it handles query parameters after the extension as
> people have been complaining about this.

What do you all think about using .endsWith on the url spec as mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1121802#c19 instead of using .includes?

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v2

This parses aURL into an nsIURI and checks the scheme. It also puts the ".ico" extension checks on the uri's spec.

Comment 10

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v2.1

I noticed I can just use Cc and Ci.

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8611281 [details] [diff] [review]
Patch v2.1

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

::: toolkit/components/places/PlacesUtils.jsm
@@ +1613,5 @@
>     */
>    getImageURLForResolution:
>    function PU_getImageURLForResolution(aWindow, aURL, aWidth = 16, aHeight = 16) {
> +    let uri = Cc["@mozilla.org/network/io-service;1"]
> +                .getService(Ci.nsIIOService)

Service.io

@@ +1616,5 @@
> +    let uri = Cc["@mozilla.org/network/io-service;1"]
> +                .getService(Ci.nsIIOService)
> +                .newURI(aURL, null, null);
> +    if (!uri.schemeIs("data") && !uri.spec.endsWith('.ico') &&
> +        !uri.spec.endsWith('.ICO')) {

I don't think uri.spec gives you what you want.

What about uri.fileExtension.toLowerCase() ?

(Note: fileExtension is part of the nsIURL interface which isn't implemented by all nsIURI instances, so you'll need to do an |uri instanceof Ci.nsIURL| check first)

Comment 12

[Matthew N. [:MattN]]

(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> I'm not sure checking the mime type is useful, as for data urls we'll never
> be causing an additional network request anyway, but I guess it doesn't hurt.

I thought it was because of memory usage and something related to the image cache so I thought that it would still help to not cause other images as a data URI to use extra memory.

Comment 13

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v3

Uses Services.io and uri.fileExtension.

Comment 14

[Florian Quèze [:florian]]

(In reply to Matthew N. [:MattN] from comment #12)
> (In reply to Florian Quèze [:florian] [:flo] from comment #7)
> > I'm not sure checking the mime type is useful, as for data urls we'll never
> > be causing an additional network request anyway, but I guess it doesn't hurt.
> 
> I thought it was because of memory usage and something related to the image
> cache so I thought that it would still help to not cause other images as a
> data URI to use extra memory.

Err, that's also true. I guess I only remembered the first reason given in bug 1121802.

So... do you think the current patch is fine, or should we check more specifically for data urls and their mime type?

Comment 15

[Matthew N. [:MattN]]

I would probably still try to do a simple check for the mime type in data URIs so we don't bring back some of the problems we were trying to solve with the regressing patch.

Comment 16

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v4

Check mime-type for data URIs.
I didn't bother checking the scheme of the URI and stuff, because all data URIs start with "data:<mediatype>" anyway (https://tools.ietf.org/html/rfc2397).

Comment 17

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v5

The early return logic was broken (thanks flo), this fixes that and makes it work with the image/x-icon, image/icon, and image/ico media types.

Comment 18

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v5.1

Adds a comment for the early return.

Comment 19

[Florian Quèze [:florian]]

Comment on attachment 8611473 [details] [diff] [review]
Patch v5.1

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

I think this version should work :-).

::: toolkit/components/places/PlacesUtils.jsm
@@ +1616,5 @@
> +    // We only want to modify the URL when the file extension is ".ico" or
> +    // it's a data URI with an icon media-type.
> +    let uri = Services.io.newURI(aURL, null, null);
> +    if ((!(uri instanceof Ci.nsIURL) || uri.fileExtension.toLowerCase() != "ico") &&
> +        !/^data:image\/(x-icon|icon|ico)/.test(aURL)) {

Please change this to non-capturing parentheses (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp#grouping-back-references)

Comment 20

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v6

Use non-capturing parentheses.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/dee1e303d67e

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dee1e303d67e

Comment 23

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Tracking for 39 and 40. Looks like this has been an issue since Firefox 38. 

MattN or Nihath, do you want to request uplift to aurora to fix this in 40?

Comment 24

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8611576 [details] [diff] [review]
Patch v6

Approval Request Comment
[User impact if declined]: Low resolution icons on retina displays! Looks bad. :)
[Risks and why]: Low risk, self contained patch.

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Adding a tracking flag for Firefox40 as this is a regression.

Comment 26

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8611576 [details] [diff] [review]
Patch v6

Approved for uplift to beta; regression from 38, pretty simple fix, noticeable to users

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bbb59a00669f

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8ecfee4a2185

Comment 29

[Cornel Ionce [:noni] [Hubs QA]]

Reproduced this issue with Firefox 38.0.5.

Confirming the fix using:
- Firefox 39 RC, build ID: 20150622181234;
- latest Aurora, build ID: 20150622004006;
- latest Nightly, build ID: 20150622052959.