1299524 - Tweak favicon appearance in Activity Stream topsites

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[Andrzej Hunt :ahunt]

To match the topsites design, a few adjustments are needed, probably within FaviconView:

- No rounded corners (CardView will round the upper corners for us, the bottom should be completely flat).
- Avoid upscaling generated favicons: we seem to make generated favicon 2x size if the FaviconView is larger than expected.
- Ensure generated favicons gave a background (this might happen automatically with sebastian's Favicon improvements in Bug 1290014)

Possibly also worthwhile:
- investigate why we get white as the dominant colour (i.e. background colour) for the BBC and NZZ icons, where are mostly black.

Comment 1

[Andrzej Hunt :ahunt]

I'm planning to wait for Bug 1290014 to land before looking into this (I do have a few hacky patches for some of the issues locally - but it's likely most of them conflict with the code in Bug 1290014).

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Andrzej Hunt :ahunt from comment #0)
> Possibly also worthwhile:
> - investigate why we get white as the dominant colour (i.e. background
> colour) for the BBC and NZZ icons, where are mostly black.

One reason could be that we treat black (0) as "no dominant color" afaik.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: topsites_icons.png

This is a screenshot of some top sites pages with the new icon code. Colors are fixed and scaling looks better too.

Comment 4

[Andrzej Hunt :ahunt]

Attachment: preview_favicons.png

I've started playing with both the sizing, and dominant colour processing: I'm wondering whether we should consider increasing the default favicon caching size to fill more of the topsites view. I've also managed to improve handling for black favicons.

I've attached a screenshot with larger icons: in some cases making the icon fill the view would be ideal (facebook), in other cases making the icon smaller is better. In some cases not fading the background colour is better (e.g. facebook), in some cases it seems to be worse (spiegel).

I'm going to experiment with some further algorithms: maybe if there's one dominant border colour we can use that for the background (no fading), if there are multiple colours we could fade instead?

(The new favicon code is great btw, icons actually appear reliably!)

Comment 5

[Andrzej Hunt :ahunt]

Attachment: Bug 1299524 - Allow disabling FaviconView corner rounding via enableRoundCorners attribute

Review commit: https://reviewboard.mozilla.org/r/77002/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77002/

Comment 6

[Andrzej Hunt :ahunt]

Attachment: Bug 1299524 - Disable FaviconView corner rounding in Activity Stream topsites

We use the FaviconView to fill the majority of the card (i.e. full width, and approx 75% of the height)
- in that scenario rounding the corners looks odd.

Review commit: https://reviewboard.mozilla.org/r/77004/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77004/

Comment 7

[Andrzej Hunt :ahunt]

I'll leave the favicon code for now, the use of Palette has improved handling quite a bit - this is something we could always revisit in future, but it seems like premature optimisation to spend too much time on this right now while we're still implementing basic functionality.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8788550 [details]
Bug 1299524 - Allow disabling FaviconView corner rounding via enableRoundCorners attribute

https://reviewboard.mozilla.org/r/77002/#review75158

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8788551 [details]
Bug 1299524 - Disable FaviconView corner rounding in Activity Stream topsites

https://reviewboard.mozilla.org/r/77004/#review75160

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae2cab55008a
Allow disabling FaviconView corner rounding via enableRoundCorners attribute r=sebastian
https://hg.mozilla.org/integration/autoland/rev/f14745862e04
Disable FaviconView corner rounding in Activity Stream topsites r=sebastian

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ae2cab55008a
https://hg.mozilla.org/mozilla-central/rev/f14745862e04