Closed Bug 1163559 Opened 9 years ago Closed 9 years ago

Search engine icons are always displayed at low resolution

Categories

(Firefox :: Search, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 41
Tracking Status
firefox38 --- wontfix
firefox39 + verified
firefox40 + verified
firefox41 --- verified

People

(Reporter: florian, Assigned: nhnt11)

References

Details

(Keywords: regression)

Attachments

(1 file, 7 obsolete files)

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.
Flags: firefox-backlog+
Do you know whether we used hidpi search icons on Windows before this regression kicked in?
(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).
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Attachment #8610885 - Flags: review?(florian)
Attachment #8610885 - Flags: review?(bwinton)
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 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.
Attachment #8610885 - Flags: feedback-
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.
Attachment #8610885 - Flags: review?(florian)
Attachment #8610885 - Flags: review?(bwinton)
Attachment #8610885 - Flags: review-
(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?
Attached patch Patch v2 (obsolete) — Splinter Review
This parses aURL into an nsIURI and checks the scheme. It also puts the ".ico" extension checks on the uri's spec.
Attachment #8610885 - Attachment is obsolete: true
Attachment #8611279 - Flags: review?(florian)
Attached patch Patch v2.1 (obsolete) — Splinter Review
I noticed I can just use Cc and Ci.
Attachment #8611279 - Attachment is obsolete: true
Attachment #8611279 - Flags: review?(florian)
Attachment #8611281 - Flags: review?(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)
Attachment #8611281 - Flags: review?(florian) → review-
(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.
Attached patch Patch v3 (obsolete) — Splinter Review
Uses Services.io and uri.fileExtension.
Attachment #8611281 - Attachment is obsolete: true
Attachment #8611340 - Flags: review?(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?
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(MattN+bmo)
Attached patch Patch v4 (obsolete) — Splinter Review
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).
Attachment #8611340 - Attachment is obsolete: true
Attachment #8611340 - Flags: review?(florian)
Attachment #8611409 - Flags: review?(florian)
Attached patch Patch v5 (obsolete) — Splinter Review
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.
Attachment #8611409 - Attachment is obsolete: true
Attachment #8611409 - Flags: review?(florian)
Attachment #8611468 - Flags: review?(florian)
Attached patch Patch v5.1 (obsolete) — Splinter Review
Adds a comment for the early return.
Attachment #8611468 - Attachment is obsolete: true
Attachment #8611468 - Flags: review?(florian)
Attachment #8611473 - Flags: review?(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)
Attachment #8611473 - Flags: review?(florian) → review+
Attached patch Patch v6Splinter Review
Use non-capturing parentheses.
Attachment #8611473 - Attachment is obsolete: true
Attachment #8611576 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/dee1e303d67e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
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?
Flags: needinfo?(nhnt11)
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(nhnt11)
Flags: needinfo?(MattN+bmo)
Attachment #8611576 - Flags: approval-mozilla-beta?
Attachment #8611576 - Flags: approval-mozilla-aurora?
Adding a tracking flag for Firefox40 as this is a regression.
Flags: qe-verify+
Comment on attachment 8611576 [details] [diff] [review]
Patch v6

Approved for uplift to beta; regression from 38, pretty simple fix, noticeable to users
Attachment #8611576 - Flags: approval-mozilla-beta?
Attachment #8611576 - Flags: approval-mozilla-beta+
Attachment #8611576 - Flags: approval-mozilla-aurora?
Attachment #8611576 - Flags: approval-mozilla-aurora+
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.
You need to log in before you can comment on or make changes to this bug.