The favicons of the search engines are low quality, please compare with the favicons in the search field. (OS X HiDPI)

See http://hg.mozilla.org/mozilla-central/annotate/5c7a6294b82a/browser/base/content/urlbarBindings.xml#l1074 for how the search panel gets the right URL for the hidpi icons. The code that needs to be modified is the code that touches "iconURI" in http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/search.js and http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/search.js

Shubham, would you like to work on this bug?

Yes. Sorry, I missed the comment before when ntim suggested me this bug. Though I see a patch being uploaded and marked for review.

Comment on attachment 8539654 [details] [diff] [review] rev 1 - Low quality favicons Review of attachment 8539654 [details] [diff] [review]: ----------------------------------------------------------------- Hi Abdelrhman, thanks for the patch! This correctly fixes the icons in the dropdown to select the default search engine. More changes will be needed to also fix the icons in the tree displayed with the list of one-click search engines, see the getImageSrc method (http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/search.js#435 and similar in the file for the in-content version).

Comment on attachment 8539684 [details] [diff] [review] rev 2 - Low quality favicons Review of attachment 8539684 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for updating the patch quickly! :-) This new version works well, there's just a tiny coding style detail that could be improved and then the patch will be ready for check-in. ::: browser/components/preferences/in-content/search.js @@ +367,5 @@ > getImageSrc: function(index, column) { > + if (column.id == "engineName" && this._engineStore.engines[index].iconURI) { > + let uri = PlacesUtils.getImageURLForResolution(window, > + this._engineStore.engines[index].iconURI.spec); > + return uri; These 3 lines would be slightly more readable if reformatted like this: let uri = this._engineStore.engines[index].iconURI.spec; return PlacesUtils.getImageURLForResolution(window, uri); ::: browser/components/preferences/search.js @@ +438,5 @@ > getImageSrc: function(index, column) { > + if (column.id == "engineName" && this._engineStore.engines[index].iconURI) { > + let uri = PlacesUtils.getImageURLForResolution(window, > + this._engineStore.engines[index].iconURI.spec); > + return uri; Same comment here.

Comment on attachment 8539700 [details] [diff] [review] rev 3 - Low quality favicons Thanks! For some reason this patch has only 3 lines of context. This is fine here as I only looked for the change requested in comment 7, but usually 8 lines of context (like your previous 2 attachments) is more practical for review.

https://hg.mozilla.org/integration/fx-team/rev/9604b7cc6432 Would you like me to show you other search bugs I could mentor?

Yes, sure

Currently there are bug 1102961 (I think it can only be reproduced on Windows) and bug 1109854.

Comment on attachment 8539700 [details] [diff] [review] rev 3 - Low quality favicons Approval Request Comment [Feature/regressing bug #]: New UI implemented in bug 1088660 and bug 1106559 [User impact if declined]: pixelated icons on retina screen [Describe test coverage new/current, TBPL]: currently green on fx-team [Risks and why]: low risk, self contained patch. [String/UUID change made/needed]: none.

Reproduced using Firefox 35 beta 8 under Mac OS X 10.9.5. Verified as fixed using Developer Edition 36.0a2 and Nightly 37.0a1 2014-01-05.

Florian, the search icons (in search panel and about:preferences#search) are not hidpi in Firefox 38.6 ESR build, OS X 10.9.5. Shouldn't this be uplifted there too? Thank you!

(In reply to Petruta Rasa [QA] [:petruta] from comment #17) > Florian, the search icons (in search panel and about:preferences#search) are > not hidpi in Firefox 38.6 ESR build, OS X 10.9.5. Shouldn't this be uplifted > there too? Thank you! This bug was fixed in Firefox 37, so 38.* should already have the fix. Is this a recent regression? Did it work in the non-ESR Firefox 38? Did it work in the previous 38.* ESR versions?

Thanks, Florian! I checked the earlier versions and also run mozregression and it seems that this fix didn't enter 37.0 release, mozregressions points to this changeset https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=369a8f14ccf8&tochange=0c454540fc2b from 2015-01-18 that "unfixed" the icons. From 39.0 the icons are hidpi again, so they will enter the next 45.0ESR. Should I try to find when this was fixed again between 38 and 39? So far I had no luck with mozregression.

(In reply to Petruta Rasa [QA] [:petruta] from comment #19) > Thanks, Florian! > > I checked the earlier versions and also run mozregression and it seems that > this fix didn't enter 37.0 release, mozregressions points to this changeset > https://hg.mozilla.org/mozilla-central/ > pushloghtml?fromchange=369a8f14ccf8&tochange=0c454540fc2b from 2015-01-18 > that "unfixed" the icons. Ok, the regression range makes it easy to see what happened. You are seeing bug 1163559 which was caused by bug 1121802.