993302 - Restore the scaling of favicons when they are saved to disk

Status: NEW

(Core :: Widget: Win32, defect)

Description

[Jonathan Watt [:jwatt]]

Prior to bug 875609 we used to scale favicons when saving them to disk. The code at the |GetSystemMetrics(SM_CXSMICON);| call in the patch on that bug is broken though. Nothing does the scaling any more and, if scaling is required, we will end up working on a surface of a different size to |size|! Depending on whether we are scaling up or down we may then bail out at this sanity check:

http://mxr.mozilla.org/mozilla-central/source/image/encoders/ico/nsICOEncoder.cpp?rev=aa774ab6eb72#58

For reasons I don't currently understand this issue didn't show up until bug 981430 landed (I think).

We acknowledged that the scaling is broken and removed the broken code in bug 983745 comment 3.

It's not clear to me if (or why) it's actually desirable to do this scaling, but this patch would do it if it is. I've put the most convincing argument I can think of for doing the scaling in the comments in the patch, but I'm unsure if the implemented scaling actually does what the comment claims (scales to the size that we will (most commonly) paint at).

Perhaps it would be better to use a size returned by nsWindowGfx::GetIconMetrics for the scaling? I'm not sure if it's relevant, but of the three current callers of that function two pass nsWindowGfx::kSmallIcon and one passes nsWindowGfx::kMediumIcon.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

To be clear I'm interested in feedback on whether we should scale, and to what (rather than the implementation of the code that does the scaling).

Comment 3

[Brian R. Bondy [:bbondy]]

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

I think if we want to scale, this is correct. I'm not sure if scaling is right though either.  Back when I wrote the original code that did work with EncodeAndScaleImage I believed that we got better quality from our imaglib than by letting windows do the scaling. I'm not sure if that's true or not though. 

If you have time it would be good to try and put a favicon on a page and fake your favicon sqlite db that has a different dimension, and then compare how it looks with Windows scaling to this patch's scaling. 

If you don't have time for that maybe we can ask someone like kjozwiak from QA to do that comparison.

Comment 4

[Brian R. Bondy [:bbondy]]

I agree that this can happen only on trunk too btw.

Comment 5

[Jonathan Watt [:jwatt]]

Do you have a good idea of all the places that use the icons saved to disk by AsyncFaviconDataReady::OnComplete when mURLShortcut==false? Having such a list would allow for a more educated decision. I'm assuming we use such saved favicons in tabs in the browser's tab strip, and beside bookmarks in lists of bookmarks. Not sure where else.

The quality of the scaling is something that we should consider in our decision, but I'm also wondering where we use the icons. In places where we draw a lot of favicons at once (tab strip and bookmarks menu) scaling the icons may have a perf impact. If that is so then we should consider trying to save the icons in the size that they will be used.

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Brian R. Bondy [:bbondy] from comment #3)
> and then compare
> how it looks with Windows scaling to this patch's scaling. 

When you say "Windows scaling", what do you mean? Scaling that occurs when the icons are used outside the browser? Or scaling in the browser? If the latter, what Window's API is being used to do the scaling, and where is it being called?

> If you don't have time for that maybe we can ask someone like kjozwiak from
> QA to do that comparison.

I don't have time to spend on this right now, but it is something I'd like to revisit once we figure out answers to some of the questions above.

Comment 7

[Brian R. Bondy [:bbondy]]

By windows scaling I mean what happens if you have for example a 32x32 ico favicon.
Then compare these 2 things:
1) A jumplist entry that uses a 32x32 favicon directly (Windows will visually scale it for us)
2) A jumplist entry that uses a 32x32 image scaled by us to a 16x16 favicon. 

I think we only use these images for the taskbar jump list in win7 and up.

Comment 8

[Jonathan Watt [:jwatt]]

I'm not familiar with how the jump list is painted. I'd assumed that gecko does the painting, but it sounds like you're saying we pass off paths and text to some Windows API and Windows creates and draws the jump list from that?

Comment 9

[Brian R. Bondy [:bbondy]]

We just tell windows which ico to use right. So I'm saying I think our scaling from 16x16->24x24 for example is better visually than we'd get if we gave windows a 16x16 and we were on a dpi setting that needed 24x24 (in which case Windows will resize before displaying to 24x24)

Comment 10

[Jonathan Watt [:jwatt]]

(In reply to Brian R. Bondy [:bbondy] from comment #9)
> We just tell windows which ico to use right.

Okay, I see. This is all to do with JumpListBuilder passing off stuff to its mJumpListMgr.

> So I'm saying I think our
> scaling from 16x16->24x24 for example is better visually than we'd get if we
> gave windows a 16x16 and we were on a dpi setting that needed 24x24 (in
> which case Windows will resize before displaying to 24x24)

Yup, I understood that bit. :)

Assuming that these icon files are indeed only used in the jump list then I agree that probably it's only image quality that matters, and perf is probably not a concern. In that case I'm happy enough for someone (kjozwiak?) to go ahead and do some quality comparisons with and without this bugs patch and base our decision on that.

Comment 11

[Brian R. Bondy [:bbondy]]

This is not super high priority because we already have a work around in a different bug, but if you have time between projects kjozwiak, then it would be good to test out the image quality difference for this.  The steps that need to be done:

Do these steps with each of these 2 builds:
i) Latest Nightly build
ii) Make a try build with the patch in this bug.

1. Make a page that offers a favicon of 64x64 only (some .ico contain multiple sizes so make sure it only has a 64x64 in it)
2. Manually modify your favicon db to fake the num visits for that site to be really high.
3. Clear you jumpListCache folder from within your profile
4. Toggle ("browser.taskbar.lists.enabled", true); to regenerate the icons
5. Wait 120 seconds. New icons will be on the taskbar.

Comment 12

[Kamil Jozwiak [:kjozwiak]]

Attachment: Image Comparisons (Twitter & Google ICO)

Went through the verification process using the following Nightly build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-05-08-03-02-03-mozilla-central/

- Installed the latest Nightly build
- Browsed to http://localhost:8080 that's using the 64x64 .ICO (used "python -m SimpleHTTPServer 8080" to launch my local web server)
- Installed "SQLite Manager" from about:addons and opened "places.sqlite"
- Selected the "moz_places" table, selected the "Browse & Search" tab and changed the "visit_count" value to "500" that's associated with the localhost:8080 website
- Saved the database and ensured that the localhost:8080 appeared under the Nightly JumpList
- Removed everything from the following folder: C:\Users\mozilla\AppData\Local\Mozilla\Firefox\Profiles\ir6bplni.default\jumpListCache
- Toggled "browser.taskbar.lists.enabled" and waited for the new .ICO to appear under the Nightly JumpList

I went through the same steps that are listed above with a Nightly build that had the above patch applied and took a screenshot of the JumpList before and after the patch was applied. The quality looks the same and I can't really tell if there's a difference.

Jonathan, Brian.. Please let me know if you see any mistakes or something that was incorrectly done.. If not, this patch can probably be applied as there's no decrease in quality from what I can see.

Comment 13

[Brian R. Bondy [:bbondy]]

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

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

Thanks jwatt and kjozwiak!

Comment 14

[Jonathan Watt [:jwatt]]

I'm a bit confused by the r+. If the image quality is the same, why would we bother with this patch?

Comment 15

[Brian R. Bondy [:bbondy]]

I'm actually fine either way, but the only benefit I can think of is that we store smaller file sizes to disk.  See bug 724423 for a case where some users have a large number of them.