Closed Bug 1362394 Opened 3 years ago Closed 2 years ago
Some favicons in bookmarks sidebar/library use ugly rescale
18.39 KB, image/png
18.73 KB, image/png
59 bytes, text/x-review-board-request
STR: 1. Go to http://www.cnn.co.jp/ and bookmark the page AR: Favicon image in Bookmarks sidebar is damaged. ER: It should be normal as same as in history/bookmarks menu or tab icon. mozregression: 60:14.48 INFO: Last good revision: b01f44c896b2472b154a48c68cf343ae2289d6bd 60:14.48 INFO: First bad revision: d80c517658f5048491b7bf27da25246a297ab50b 60:14.48 INFO: Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b01f44c896b2472b154a48c68cf343ae2289d6bd&tochange=d80c517658f5048491b7bf27da25246a297ab50b This happened only on http://www.cnn.co.jp/ as for me. Favicon url is http://www.cnn.co.jp/media/cnn/images/icons/favicon-160x160.png
Likely another case where bug 1352459 would help by collecting more than one icon... In my db I can only find http://www.cnn.co.jp/media/cnn/images/icons/favicon-160x160.png stored as 144x144, even if the page gives out many more favicons. So we are basically rescaling. I'm not sure why the sidebar looks so bad, sounds like bad rescaling instead of resampling, the other views are not suffering that. Before we were not suffering this because we were rescaling the image before storing it so the treeview didn't have to do the rescaling. Timothy, do you know if the tree implementation uses rescaling instead of resampling, or at least where that code lies? I suppose it's somewhere in nsTreeBodyFrame::PaintImage
Depends on: 1352459
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Summary: A favicon in bookmarks sidebar is being damaged → Some favicons in bookmarks sidebar/library use ugly rescale
Not sure if this is the right thing to do, but it solves the problem. replacing ni? with r?.
Comment on attachment 8865104 [details] Bug 1362394 - Favicons in bookmarks treeviews don't use high quality scaling. https://reviewboard.mozilla.org/r/136776/#review139824 This seems right. But we need to only pass the high quality scaling flag if we are painting to the window (as opposed to doing a drawWindow call). This requires access to the nsDisplayListBuilder so we can call builder->IsPaintingToWindow(). Which requires passing the builder through a bunch of the tree painting functions. We should probably also apply the same flag to all the other DrawImage calls in this file.
Attachment #8865104 - Flags: review?(tnikkel)
OK, I'll look into comment 6 hints one the P1s are gone. Since this currently only happens for a few icons and it doesn't break any major functionality, we are not in a hurry.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Comment on attachment 8865104 [details] Bug 1362394 - Favicons in bookmarks treeviews don't use high quality scaling. https://reviewboard.mozilla.org/r/136776/#review142340 I'd actually prefer that we pass down a nsDisplayListBuilder pointer. That is how it is done in all other painting code. It has the advantage if any other context is needed in these functions it will be available from the builder. Sorry to make you rewrite it!
Attachment #8865104 - Flags: review?(tnikkel) → review-
Ok, it sounded cleaner to pass a bool, since I could put a null check in a single point and in the future, when there will effectively be the need for other options, it could have been easily converted. I don't usually code for things that may not ever happen. I'll just change the patch when I have some time along this week, thank you for the feedback.
Comment on attachment 8865104 [details] Bug 1362394 - Favicons in bookmarks treeviews don't use high quality scaling. https://reviewboard.mozilla.org/r/136776/#review144306 Thanks!
Attachment #8865104 - Flags: review?(tnikkel) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/ab73eb9aae41 Favicons in bookmarks treeviews don't use high quality scaling. r=tnikkel
I can reproduce this bug with Firefox Nightly 55.0a1 (2017-05-05) (64-bit) on Deepin 15.4, 64bit. I can verify that this bug is fixed in latest Nightly Build ID 20170524100215 User Agent Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0 [bugday-20170524]
I have successfully reproduced the bug in firefox Nightly 55.0a1 (2017-05-05) (32-bit)with windows 10 (32 bit) Verified as fixed with latest nightly 55.0a1 (2017-05-24) (32-bit) Build ID: 20170524030204 Mozilla/5.0 (Windows NT 10.0; rv:55.0) Gecko/20100101 Firefox/55.0
QA Whiteboard: [bugday-20170524]
Verified as fixed on Mac OS X 10.12 on latest Nightly 55.0a1 (2017/05/28)
You need to log in before you can comment on or make changes to this bug.