Favicons are of low quality for local/internal pages

VERIFIED FIXED in Firefox 20

Status

()

Firefox for Android
General
P3
normal
VERIFIED FIXED
6 years ago
2 years ago

People

(Reporter: patryk, Assigned: wesj)

Tracking

11 Branch
Firefox 20
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox20 verified, blocking-fennec1.0 -, fennec19+)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 585836 [details]
Low quality favicons

Get / use high quality favicons for in-content UI:
+ Add-ons Manager
+ About: Firefox
+ About: Home
... etc.
We should be using a high quality version of the _Firefox_ icon here, rather than the "no favicon" favicon. We should also have a higher quality version of the no favicon favicon (the dotted box), of course.
Summary: Favicons are of low quality for → Favicons are of low quality for local/internal pages
Assignee: nobody → ibarlow
tracking-fennec: --- → 11+
Priority: -- → P3
Engineers: What format would you prefer? ico? png?
(In reply to Ian Barlow (:ibarlow) from comment #2)
> Engineers: What format would you prefer? ico? png?

PNG
Created attachment 586521 [details]
A range of favicon sizes for Firefox, Aurora, and Nightly
Unassigning myself as this is ready for dev to pick up
Assignee: ibarlow → nobody
Created attachment 592151 [details]
screenshot

Just a friendly ping to see if anyone was available to pick this up. Seeing a blank favicon in the start page header is not ideal.
(In reply to Ian Barlow (:ibarlow) from comment #6)
> Created attachment 592151 [details]
> screenshot
> 
> Just a friendly ping to see if anyone was available to pick this up. Seeing
> a blank favicon in the start page header is not ideal.

Blank favicons for internal pages is bug 709250
(In reply to Mark Finkle (:mfinkle) from comment #7)
> (In reply to Ian Barlow (:ibarlow) from comment #6)
> > Created attachment 592151 [details]
> > screenshot
> > 
> > Just a friendly ping to see if anyone was available to pick this up. Seeing
> > a blank favicon in the start page header is not ideal.
> 
> Blank favicons for internal pages is bug 709250

Meaning that even if we fixed this bug, we still couldn't see the favicons
Soft blocking nom?

Now that bug 709250 is knocked off, maybe we can revisit this
blocking-fennec1.0: --- → ?
(Assignee)

Comment 10

6 years ago
Created attachment 613724 [details] [diff] [review]
Patch 2/2 - Use high quality favicons

I have these patches from bug 739215 that mfinkle hated there.

This one uses high res favicons on internal pages (not the ones ibarlow put up here though...)
(Assignee)

Comment 11

6 years ago
Created attachment 613725 [details] [diff] [review]
Patch 1/2 - Default bookmarks cleanup

This gives us the ability to specify a jar location for favicons for our default bookmarks. Makes it possible to remove some duplication we currently have.
blocking-fennec1.0: ? → soft
tracking-fennec: 11+ → 15+
blocking-fennec1.0: soft → -
Assignee: nobody → wjohnston
tracking-fennec: 15+ → 17+
(Assignee)

Comment 12

5 years ago
Created attachment 667276 [details] [diff] [review]
Patch

Rolled up patches with new icons. I'm only including the 64x64 icons. Not sure if we need more or not (48x48 seems like overkill, 128x128?)
Attachment #613724 - Attachment is obsolete: true
Attachment #613725 - Attachment is obsolete: true
Attachment #667276 - Flags: review?(mark.finkle)
Comment on attachment 667276 [details] [diff] [review]
Patch

Why would we want to add two <link> to our HTML files? It seems like that would cause more data to be sent to Java than we care about. Wouldn't we always want to show the larger icon? Or am I missing something?
(Assignee)

Comment 14

5 years ago
Just me being correct. On LDPI devices, the 32x32 one is actually what we would probably want (but it looks like our current code just takes the last one in the file and uses its largest size?). If we ever fix that bug, these things would just work (correctly).
(Assignee)

Comment 15

5 years ago
Created attachment 668078 [details] [diff] [review]
Patch

Remove the 32x32 bits.

I mentioned yesterday, but this also moves us to using:

http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/global/icons/blacklist_large.png

for blocked pages, as opposed to:

http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/global/icons/blacklist_favicon.png
Attachment #667276 - Attachment is obsolete: true
Attachment #667276 - Flags: review?(mark.finkle)
Attachment #668078 - Flags: review?(mark.finkle)
Comment on attachment 668078 [details] [diff] [review]
Patch

>diff --git a/mobile/android/base/Makefile.in b/mobile/android/base/Makefile.in

>-RESOURCES=$(RES_LAYOUT) $(RES_LAYOUT_LAND_V14) $(RES_LAYOUT_LARGE_V11) $(RES_LAYOUT_XLARGE_V11) $(RES_VALUES) $(RES_VALUES_LAND) $(RES_VALUES_V11) $(RES_VALUES_LARGE_V11) $(RES_VALUES_XLARGE_V11) $(RES_VALUES_LAND_V14) $(RES_XML) $(RES_ANIM) $(RES_DRAWABLE_NODPI) $(RES_DRAWABLE_BASE) $(RES_DRAWABLE_LDPI) $(RES_DRAWABLE_HDPI) $(RES_DRAWABLE_XHDPI) $(RES_DRAWABLE_MDPI_V11) $(RES_DRAWABLE_HDPI_V11) $(RES_DRAWABLE_XHDPI_V11) $(RES_DRAWABLE_LAND_V14) $(RES_DRAWABLE_LAND_MDPI_V14) $(RES_DRAWABLE_LAND_HDPI_V14) $(RES_DRAWABLE_LAND_XHDPI_V14) $(RES_DRAWABLE_LARGE_MDPI_V11) $(RES_DRAWABLE_LARGE_HDPI_V11) $(RES_DRAWABLE_LARGE_XHDPI_V11) $(RES_DRAWABLE_XLARGE_MDPI_V11) $(RES_DRAWABLE_XLARGE_HDPI_V11) $(RES_DRAWABLE_XLARGE_XHDPI_V11) $(RES_COLOR) $(RES_MENU)
>+RESOURCES=$(RES_LAYOUT) $(RES_LAYOUT_LAND_V14) $(RES_LAYOUT_LARGE_V11) $(RES_LAYOUT_XLARGE_V11) $(RES_VALUES) $(RES_VALUES_LAND) $(RES_VALUES_V11) $(RES_VALUES_LARGE_V11) $(RES_VALUES_XLARGE_V11) $(RES_VALUES_LAND_V14) $(RES_VALUES_LARGE) $(RES_VALUES_XLARGE) $(RES_XML) $(RES_ANIM) $(RES_DRAWABLE_NODPI) $(RES_DRAWABLE_BASE) $(RES_DRAWABLE_LDPI) $(RES_DRAWABLE_HDPI) $(RES_DRAWABLE_XHDPI) $(RES_DRAWABLE_MDPI_V11) $(RES_DRAWABLE_HDPI_V11) $(RES_DRAWABLE_XHDPI_V11) $(RES_DRAWABLE_LAND_V14) $(RES_DRAWABLE_LAND_MDPI_V14) $(RES_DRAWABLE_LAND_HDPI_V14) $(RES_DRAWABLE_LAND_XHDPI_V14) $(RES_DRAWABLE_LARGE_MDPI_V11) $(RES_DRAWABLE_LARGE_HDPI_V11) $(RES_DRAWABLE_LARGE_XHDPI_V11) $(RES_DRAWABLE_XLARGE_MDPI_V11) $(RES_DRAWABLE_XLARGE_HDPI_V11) $(RES_DRAWABLE_XLARGE_XHDPI_V11) $(RES_COLOR) $(RES_MENU)

You added values here, but I don't see them defined: $(RES_VALUES_LARGE) $(RES_VALUES_XLARGE)

>diff --git a/mobile/android/branding/aurora/content/fennec_48x48.png b/mobile/android/branding/aurora/content/fennec_48x48.png
>deleted file mode 100644
>Binary file mobile/android/branding/aurora/content/fennec_48x48.png has changed

I don't think you want to delete fennec_48x48.png from aurora branding


Let me know if these changes are needed
(Assignee)

Comment 17

5 years ago
Not needed. I'll throw up a new patch.
Comment on attachment 668078 [details] [diff] [review]
Patch

Waiting for new patch
Attachment #668078 - Flags: review?(mark.finkle) → review-
Wes - Ping?
tracking-fennec: 17+ → 19+

Updated

5 years ago
Flags: needinfo?(wjohnston)
Wes, ping again.
I am updating the patch with my comments.
Flags: needinfo?(wjohnston)
Created attachment 692167 [details] [diff] [review]
updated patch

I updated Wes' patch with the two changes I requested and unbitrotted. Tested and it seems to work as expected.
Attachment #668078 - Attachment is obsolete: true
Attachment #692167 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6fb67e58bad6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20

Comment 27

5 years ago
Firefox 20.0a1 (2012-12-17)
Device: Galaxy Nexus
OS: Android 4.1.1

The favicons has proper quality and are displayed in Nightly (Fx 20) for pages like about:home, about:firefox, about:addons 
Marking bug as VERIFIED FIXED.
Status: RESOLVED → VERIFIED
status-firefox20: --- → verified
You need to log in before you can comment on or make changes to this bug.