Closed Bug 715258 Opened 8 years ago Closed 7 years ago

Favicons are of low quality for local/internal pages

Categories

(Firefox for Android :: General, defect, P3)

11 Branch
x86
macOS
defect

Tracking

()

VERIFIED FIXED
Firefox 20
Tracking Status
firefox20 --- verified
blocking-fennec1.0 --- -
fennec 19+ ---

People

(Reporter: padamczyk, Assigned: wesj)

References

Details

Attachments

(4 files, 4 obsolete files)

Attached image 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
Unassigning myself as this is ready for dev to pick up
Assignee: ibarlow → nobody
Attached image 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: --- → ?
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...)
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+
Attached patch Patch (obsolete) — Splinter Review
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?
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).
Attached patch Patch (obsolete) — Splinter Review
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
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+
Flags: needinfo?(wjohnston)
Wes, ping again.
I am updating the patch with my comments.
Flags: needinfo?(wjohnston)
Attached patch updated patchSplinter Review
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
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
You need to log in before you can comment on or make changes to this bug.