1362394 - Some favicons in bookmarks sidebar/library use ugly rescale

Status: VERIFIED FIXED

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

Description

[atlanto]

Attachment: favicon in sidebar:NG, menu:OK, tab:OK

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

Comment 1

[atlanto]

Attachment: expected normal image taken on build 20170411030208

Comment 2

[Marco Bonardo [:mak]]

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

Comment 4

[Marco Bonardo [:mak]]

Attachment: Bug 1362394 - Favicons in bookmarks treeviews don't use high quality scaling.

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

Comment 5

[Marco Bonardo [:mak]]

Not sure if this is the right thing to do, but it solves the problem. replacing ni? with r?.

Comment 6

[Timothy Nikkel (: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.

Comment 7

[Marco Bonardo [:mak]]

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.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8865104 [details]
Bug 1362394 - Favicons in bookmarks treeviews don't use high quality scaling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136776/diff/1-2/

Comment 9

[Timothy Nikkel (:tnikkel)]

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!

Comment 10

[Marco Bonardo [:mak]]

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 11

[Marco Bonardo [:mak]]

Comment on attachment 8865104 [details]
Bug 1362394 - Favicons in bookmarks treeviews don't use high quality scaling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136776/diff/2-3/

Comment 12

[Timothy Nikkel (:tnikkel)]

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!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab73eb9aae41
Favicons in bookmarks treeviews don't use high quality scaling. r=tnikkel

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Sayed [:LiquidO2]]

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]

Comment 16

[Md. Rahimul Islam]

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

Comment 17

[Brindusa Tot, Desktop QA]

Verified as fixed on Mac OS X 10.12 on latest Nightly 55.0a1 (2017/05/28)