Closed Bug 1113096 Opened 9 years ago Closed 9 years ago

Low quality favicons in new search preferences

Categories

(Firefox :: Search, defect)

All
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 37
Tracking Status
firefox36 --- verified
firefox37 --- verified
firefox-esr38 --- affected

People

(Reporter: soeren.hentzschel, Assigned: abdelrahman, Mentored)

References

Details

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

Attachments

(2 files, 2 obsolete files)

Attached image 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: → 1055879
Whiteboard: [good first bug][lang=js]
Shubham, would you like to work on this bug?
Flags: needinfo?(shubhamjindal18)
Attached patch rev 1 - Low quality favicons (obsolete) — Splinter Review
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+
Attached patch rev 2 - Low quality favicons (obsolete) — Splinter Review
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
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?
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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
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
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!
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.