Low quality favicons in new search preferences

VERIFIED FIXED in Firefox 36

Status

()

Firefox
Search
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: Sören Hentzschel, Assigned: abdelrhman, Mentored)

Tracking

Trunk
Firefox 37
All
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox36 verified, firefox37 verified, firefox-esr38 affected)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8538466 [details]
search-favicons.png

The favicons of the search engines are low quality, please compare with the favicons in the search field.

(OS X HiDPI)
See Also: → bug 1055879

Updated

4 years ago
Whiteboard: [good first bug][lang=js]
Shubham, would you like to work on this bug?
Flags: needinfo?(shubhamjindal18)
(Assignee)

Comment 3

4 years ago
Created attachment 8539654 [details] [diff] [review]
rev 1 - Low quality favicons
Attachment #8539654 - Flags: review?(florian)
Yes. Sorry, I missed the comment before when ntim suggested me this bug. Though I see a patch being uploaded and marked for review.
Flags: needinfo?(shubhamjindal18)
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).
Attachment #8539654 - Flags: review?(florian) → feedback+
(Assignee)

Comment 6

4 years ago
Created attachment 8539684 [details] [diff] [review]
rev 2 - Low quality favicons
Attachment #8539684 - Flags: review?(florian)
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.
Attachment #8539684 - Flags: review?(florian) → feedback+
Assignee: nobody → codo.abdo
Attachment #8539654 - Attachment is obsolete: true
(Assignee)

Comment 8

4 years ago
Created attachment 8539700 [details] [diff] [review]
rev 3 - Low quality favicons
Attachment #8539700 - Flags: review?(florian)
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.
Attachment #8539700 - Flags: review?(florian) → review+
https://hg.mozilla.org/integration/fx-team/rev/9604b7cc6432

Would you like me to show you other search bugs I could mentor?
(Assignee)

Comment 11

4 years ago
Yes, sure
Currently there are bug 1102961 (I think it can only be reproduced on Windows) and bug 1109854.
Attachment #8539684 - Attachment is obsolete: true
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.
Attachment #8539700 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/9604b7cc6432
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
status-firefox36: --- → affected
status-firefox37: --- → fixed
Attachment #8539700 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
status-firefox36: fixed → verified
status-firefox37: fixed → verified
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!
status-firefox-esr38: --- → affected
Flags: needinfo?(florian)
(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?
Flags: needinfo?(florian) → needinfo?(petruta.rasa)
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.
Flags: needinfo?(petruta.rasa)
(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.
You need to log in before you can comment on or make changes to this bug.