715258 - Favicons are of low quality for local/internal pages

Status: VERIFIED FIXED

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

Description

[Patryk Adamczyk [:patryk] UX]

Attachment: Low quality favicons

Get / use high quality favicons for in-content UI:
+ Add-ons Manager
+ About: Firefox
+ About: Home
... etc.

Comment 1

[Madhava Enros [:madhava]]

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.

Comment 2

[Ian Barlow (:ibarlow)]

Engineers: What format would you prefer? ico? png?

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Ian Barlow (:ibarlow) from comment #2)
> Engineers: What format would you prefer? ico? png?

PNG

Comment 4

[Ian Barlow (:ibarlow)]

Attachment: A range of favicon sizes for Firefox, Aurora, and Nightly

Comment 5

[Ian Barlow (:ibarlow)]

Unassigning myself as this is ready for dev to pick up

Comment 6

[Ian Barlow (:ibarlow)]

Attachment: 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.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 9

[Ian Barlow (:ibarlow)]

Soft blocking nom?

Now that bug 709250 is knocked off, maybe we can revisit this

Comment 10

[Wesley Johnston (:wesj)]

Attachment: 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...)

Comment 11

[Wesley Johnston (:wesj)]

Attachment: 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.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: 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?)

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 14

[Wesley Johnston (:wesj)]

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).

Comment 15

[Wesley Johnston (:wesj)]

Attachment: 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

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 17

[Wesley Johnston (:wesj)]

Not needed. I'll throw up a new patch.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 668078 [details] [diff] [review]
Patch

Waiting for new patch

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Wes - Ping?

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

Wes, ping again.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

I am updating the patch with my comments.

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: updated patch

I updated Wes' patch with the two changes I requested and unbitrotted. Tested and it seems to work as expected.

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/042b44652822

Comment 24

[Ed Morley [:emorley]]

Backed out for breaking robocop & troboprovider:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=robo&rev=042b44652822

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1fb4089fa9b

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

Fixed test breakage:
https://tbpl.mozilla.org/?tree=Try&rev=6e8d4a279a35

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fb67e58bad6

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6fb67e58bad6

Comment 27

[Ioana Chiorean]

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.