848150 - Update about:home favicons

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Frank Yan (:fryn)]

Attachment: patch

Currently, we're using specialized 16x16 images as about:home favicons, but UX actually prefers the appearance of the 32x32 images even when downscaled to 16x16.

Also, we're using an outdated Google search engine icon in the search bar. The updated 32x32 image that Google is now using looks great even when downscaled to 16x16 on non-HiDPI displays, so let's use that.

Not all the other search engine icons downscale as well, so I'm leaving them as 16x16, but enabling `image-rendering: -moz-crisp-edges;` on OS X HiDPI mode for upscaling like we do for favicons.

We've tested all of these images on both HiDPI and non-HiDPI hardware.
With these changes, all of our primary UI will look crisp on both HiDPI and non-HiDPI hardware!

Comment 1

[Justin Dolske [:Dolske]]

Didn't you start this in another bug? Or was that in one of the mega hidpi fixes, and we just dropped it?

Got screenshots of the FF and Google icons on lowdpi with this patch?

Comment 2

[Frank Yan (:fryn)]

(In reply to Justin Dolske [:Dolske] from comment #1)
> Didn't you start this in another bug? Or was that in one of the mega hidpi
> fixes, and we just dropped it?

I wrote a different patch that tried to provide HiDPI versions of all the search engine icons in bug 795495, but that didn't work out, so I've put that aside for now. This bug is just for the about:home favicons and the Google icon only, which downscale well.

> Got screenshots of the FF and Google icons on lowdpi with this patch?

Limi will post them shortly.

Comment 3

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 721481 [details] [diff] [review]
patch

Yes, please!

(In reply to Justin Dolske [:Dolske] from comment #1)
> Didn't you start this in another bug? Or was that in one of the mega hidpi
> fixes, and we just dropped it?
> 
> Got screenshots of the FF and Google icons on lowdpi with this patch?

Firefox 32px icon scaled to 16px by the browser:
http://cl.ly/image/2B0V1E1l1e1S

Google 32px icon scaled to 16px by the browser:
http://cl.ly/image/2q0M2v341C2M

Looks great!

Comment 4

[Justin Dolske [:Dolske]]

Attachment: lodpi screenshot

(In reply to Alex Limi (:limi) — Firefox UX Team from comment #3)

> > Got screenshots of the FF and Google icons on lowdpi with this patch?
> 
> Firefox 32px icon scaled to 16px by the browser:
> http://cl.ly/image/2B0V1E1l1e1S
> 
> Google 32px icon scaled to 16px by the browser:
> http://cl.ly/image/2q0M2v341C2M
> 
> Looks great!

That's not what I see at all. Attached is a screenshot showing the 2 icons for this patch on a non-Retina display; with (from left to right) 16x16, 32x32 (scaled with -moz-crisp-edges), and 32x32 (with regular scaling).

Both of the scaled 32x32 icons looks noticeably worse.

I don't think a shortcut is going to work well here, we're actually going to need to show the right icon for the right screen. :(

Comment 5

[Justin Dolske [:Dolske]]

Oh, wait, what am I thinking. The crisp-edge scaling isn't used on non-Retina pages.

Comment 6

[Frank Yan (:fryn)]

(In reply to Justin Dolske [:Dolske] from comment #5)
> Oh, wait, what am I thinking. The crisp-edge scaling isn't used on
> non-Retina pages.

That is correct if you replace the word "pages" with "displays".

For the Firefox logo, the downscaled version is preferred because it looks more like the full-size Firefox logo. The specially made 16×16 icon attempted to make the fox more defined at the heavy cost of being less visually balanced and arguably closer to our old logo than the new one and less recognizable.

For the Google logo, the downscaled version is either exactly or almost exactly what Google is using on www.google.com, not our outdated version.

Comment 7

[Justin Dolske [:Dolske]]

Comment on attachment 721481 [details] [diff] [review]
patch

The Google icon (with regular scaling) isn't great, but it's not a very big delta. I'm more concerned about the Firefox icon, since the 16x16 version is heavily tweaked. But I'll defer to Steven since he has final say on visual stuff.

[Bugzilla doesn't do multiple ui-review requests, so the clearing of limi's ui-r+ here isn't meant as a some kind of passive-aggressive thing. :-)]

Comment 8

[Justin Dolske [:Dolske]]

Comment on attachment 721481 [details] [diff] [review]
patch

[Let me try that again.]

Comment 9

[Stephen Horlander [:shorlander]]

Comment on attachment 721481 [details] [diff] [review]
patch

Review of attachment 721481 [details] [diff] [review]:
-----------------------------------------------------------------

This looks better to me, or at least worth the trade-off.

Comment 10

[Frank Yan (:fryn)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6690c6da6cc6

Comment 11

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6c3ea4720ab9 - you wouldn't think we'd do such a thing, but we actually have a test, browser_google.js, to be sure you haven't changed the Google search icon.

Comment 12

[Frank Yan (:fryn)]

Wow, the test literally compares the data url in the product to the data url in the test, string to string. :|

Pushed a one-line test fix with the patch.

Patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/f4beba4909b0
Test fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/1666ca9b946b

Comment 13

[Mats Palmgren (inactive)]

Looks like Ms2ger backed this out again due to 'bc' test failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37b4a5f15295

Comment 14

[Frank Yan (:fryn)]

(In reply to Mats Palmgren [:mats] from comment #13)
> Looks like Ms2ger backed this out again due to 'bc' test failures:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/37b4a5f15295

Ugh, it got backed out when it shouldn't have been.
These two changesets weren't the ones causing any of the failures. I'll push again when inbound reopens.

Comment 15

[Frank Yan (:fryn)]

Hmm, it does seem to fail reliably on linux64 (Fedora64) opt mochitest-bc on Mozilla-Inbound, but it's failing immediately right at the beginning of the first mochitest-bc test.

Here is the log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=20726056&tree=Mozilla-Inbound
Full log if needed:
https://tbpl.mozilla.org/php/getParsedLog.php?id=20726056&tree=Mozilla-Inbound&full=1

Since it only fails on linux64, and I don't have a linux64 box, I pushed a few changes to Tryserver to see what's up.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Frank Yan (:fryn) from comment #14)
> (In reply to Mats Palmgren [:mats] from comment #13)
> > Looks like Ms2ger backed this out again due to 'bc' test failures:
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/37b4a5f15295
> 
> Ugh, it got backed out when it shouldn't have been.
> These two changesets weren't the ones causing any of the failures. I'll push
> again when inbound reopens.

They may not have been, but all Fedora64 opt and pgo builds failed the exact same test, starting from your push, and ending with my backout.

Comment 17

[Frank Yan (:fryn)]

(In reply to :Ms2ger from comment #16)
> They may not have been, but all Fedora64 opt and pgo builds failed the exact
> same test, starting from your push, and ending with my backout.

After reading through TBPL and the logs, I see that your backout was the right decision. I'm investigating it. See comment 15. I won't push again until I figure it out.

Comment 18

[Frank Yan (:fryn)]

Findings:

Making the Google search engine icon not 16×16 is what causes mochitest-bc to fail almost every time, and it fails near the beginning of the first mochitest-bc test:
chrome://mochitests/content/browser/browser/base/content/test/browser_CTPScriptPlugin.js
logging the following TEST-UNEXPECTED-FAIL:
should have a click-to-play notification (about:blank)
instead of the following TEST-PASS:
should have a click-to-play notification (plugin_test_noScriptNoPopup.html)

I have no idea why this is.
In fact, the test actually passed once, despite the image size change, which is even more baffling:
https://tbpl.mozilla.org/?tree=Try&rev=4f5b0beb1528

Perhaps, I'll split off the Google search engine icon change into a separate bug, so I can land the rest of the patch and close this bug.

Comment 19

[Frank Yan (:fryn)]

Pushed everything except the Google search engine icon change to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e69bc37414dd

Renamed bug to reflect this.
Filed bug 851953 for the remaining Google search engine icon part.

Comment 20

[Jim Jeffery not reading bug-mail 1/2/11]

Why did this patch add a scrollbar to the Downloads button/Manager with only 1 (one) download in the list?

Screenshot:
http://i.imgur.com/bAo0Qat.jpg

Comment 21

[Frank Yan (:fryn)]

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #20)

Why do you think this patch had anything to do with that?

Comment 22

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to Frank Yan (:fryn) from comment #21)
> (In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #20)
> 
> Why do you think this patch had anything to do with that?

From regression testing...scrollbar does not appear until I load the build from cset:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e69bc37414dd

Comment 23

[Marco Bonardo [:mak]]

Interesting, may be somehow related to the position of the search bar, may be a rounding px problem?

Comment 24

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to Marco Bonardo [:mak] from comment #23)
> Interesting, may be somehow related to the position of the search bar, may
> be a rounding px problem?

I have nothing modified or moved,resized.  I believe in testing builds with the barest setup possible and everything pretty muck 'stock'.  Although my old eyes can't detect 1px shifts as a general rule. 

For the record all testing was done either with m-c or m-i hourly builds.
win7 x64 and I run the browser maximized.

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e69bc37414dd