Closed
Bug 1163559
Opened 9 years ago
Closed 9 years ago
Search engine icons are always displayed at low resolution
Categories
(Firefox :: Search, defect)
Firefox
Search
Tracking
()
VERIFIED
FIXED
Firefox 41
People
(Reporter: florian, Assigned: nhnt11)
References
Details
(Keywords: regression)
Attachments
(1 file, 7 obsolete files)
1.32 KB,
patch
|
nhnt11
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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+
Comment 2•9 years ago
|
||
Do you know whether we used hidpi search icons on Windows before this regression kicked in?
Reporter | ||
Comment 3•9 years ago
|
||
(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).
Assignee | ||
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
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•9 years ago
|
||
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-
Reporter | ||
Comment 7•9 years ago
|
||
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-
Comment 8•9 years ago
|
||
(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?
Assignee | ||
Comment 9•9 years ago
|
||
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)
Assignee | ||
Comment 10•9 years ago
|
||
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)
Reporter | ||
Comment 11•9 years ago
|
||
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-
Comment 12•9 years ago
|
||
(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.
Assignee | ||
Comment 13•9 years ago
|
||
Uses Services.io and uri.fileExtension.
Attachment #8611281 -
Attachment is obsolete: true
Attachment #8611340 -
Flags: review?(florian)
Reporter | ||
Comment 14•9 years ago
|
||
(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)
Comment 15•9 years ago
|
||
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)
Assignee | ||
Comment 16•9 years ago
|
||
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)
Assignee | ||
Comment 17•9 years ago
|
||
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)
Assignee | ||
Comment 18•9 years ago
|
||
Adds a comment for the early return.
Attachment #8611468 -
Attachment is obsolete: true
Attachment #8611468 -
Flags: review?(florian)
Attachment #8611473 -
Flags: review?(florian)
Reporter | ||
Comment 19•9 years ago
|
||
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+
Assignee | ||
Comment 20•9 years ago
|
||
Use non-capturing parentheses.
Attachment #8611473 -
Attachment is obsolete: true
Attachment #8611576 -
Flags: review+
Comment 22•9 years ago
|
||
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?
Assignee | ||
Comment 24•9 years ago
|
||
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+
Comment 29•9 years ago
|
||
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.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•