Closed Bug 1362394 Opened 3 years ago Closed 2 years ago

Some favicons in bookmarks sidebar/library use ugly rescale

Categories

(Firefox :: Bookmarks & History, defect, P2, minor)

55 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
firefox55 --- verified

People

(Reporter: euthanasia_waltz, Assigned: mak)

References

Details

(Whiteboard: [fxsearch])

Attachments

(3 files)

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
Flags: needinfo?(tnikkel)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Whiteboard: [fxsearch]
Duplicate of this bug: 1362628
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?.
Flags: needinfo?(tnikkel)
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 mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/ab73eb9aae41
Favicons in bookmarks treeviews don't use high quality scaling. r=tnikkel
https://hg.mozilla.org/mozilla-central/rev/ab73eb9aae41
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.