21.48 KB, image/png
52.76 KB, image/png
16.16 KB, image/png
12.28 KB, image/png
963 bytes, patch
|Details | Diff | Splinter Review|
29.26 KB, text/html
2.09 KB, patch
|Details | Diff | Splinter Review|
Created attachment 347194 [details] screenshot 1: ugly question mark used in Firefox dialog STR: 1. Open "Tools" menu, and select "Private Browsing 2. Observe icon used in the dialog that appears. ACTUAL RESULTS: Dialog uses crappy-looking pixelated question mark icon. (see upcoming "screenshot 1") EXPECTED RESULTS: Dialog should use clean-looking question mark icon. (see upcoming "screenshot 2") (Note: This probably isn't a private-browsing-specific bug, AFAIK -- that's just where I've noticed this dialog recently.) OS: Ubuntu 8.10 Browser: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081109 Minefield/3.1b2pre
Component: Build Config → General
QA Contact: build.config → general
Created attachment 347195 [details] screenshot 2: pretty question mark, used in Gnome wallpaper app Here's a screenshot of the clean-looking question mark, as used in the Gnome wallpaper app's file-picker. This icon is slightly larger than the broken one in the Firefox dialog -- perhaps we're using this same icon, but we're scaling it down slightly, making it look broken...?
Gavin says in IRC the issue is probably that my icons are 48x48, but Firefox's theme has them hardcoded to 40x40. e.g. if I load moz-icon://stock/gtk-dialog-question?size=dialog or moz-icon://stock/gtk-dialog-info?size=dialog and then right-click and select 'properties', it says "Image Dimensions: 48px x 48px"
http://hg.mozilla.org/mozilla-central/annotate/d8295ec827fa/toolkit/themes/gnomestripe/global/global.css#l91 We hardcode the size to 40x40, but apparently they're actually 48x48 in dholbert's theme.
Component: General → Themes
Product: Firefox → Toolkit
QA Contact: general → themes
Summary: "Start Private Browsing" dialog uses pixelated GTK question mark icon → GTK common dialog icons (e.g. "Start Private Browsing" dialog) are pixelated
I wonder if they're a different size in 8.10 than they were in 8.04?
40x40 is a very bad idea, gtk/freedesktop only support 32x32 or 48x48. Note that GTK themes can change this size so what Firefox should do is get the actual GTK_STOCK_DIALOG (?) size and use this size.
Are we doing this correctly with the quit dialog?
Created attachment 347473 [details] screenshot 3: ugly question mark in 'quit' dialog (In reply to comment #6) > Are we doing this correctly with the quit dialog? No -- it's ugly there, as well. (see newly-attached screenshot)
My Ubuntu 8.04 says they were 48x48 then, too.
Created attachment 347718 [details] screenshot I've the ugly icon as well, but this is on 8.04 with some updates.
Looks like the justification for 40x40 is in bug 402839 comment 7. Does removing the explicit size really cause problems with the text?
pretty icon: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081104 Minefield/3.1b2pre bad icon: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081105 Minefield/3.1b2pre
Perhaps this is a regression from bug 458487, then?
(In reply to comment #12) > Perhaps this is a regression from bug 458487, then? Confirmed regression from that bug. hg revert --all -r 2d35868314cf --> Pretty icon on Private Browsing dialog hg revert --all -r 49dd935efff3 --> Bad icon on on Private Browsing dialog (that second change ID is bug 458487's checkin, and the first is its parent)
Maybe bug 458487 triggered it but the underlying problem is the rescaling of the icon from 48x48 to 40x40, right?
Right -- perhaps before that, we were *supposed* to rescale, but we were failing at doing it (and thus inadvertently succeeding at keeping pretty icons).
(In reply to comment #3) > http://hg.mozilla.org/mozilla-central/annotate/d8295ec827fa/toolkit/themes/gnomestripe/global/global.css#l91 > > We hardcode the size to 40x40, but apparently they're actually 48x48 in > dholbert's theme. The change to 40x40 was made recently, in bug Bug 402839. (before that, it was 32x32) --> Adding dependency.
Why did we regress the good image downscaling? The decision for 40x40 was made ages ago. We chose that size because we can't detect actual size so that if a theme has 32x32 dialog icons (and they do exist) the upscaling doesnt look as bad in 40x40 as 48x48. But GTK generally has larger icons for dialogs and 32x32 really didn't look good as a dialog icon in the environment.
We need to fix bug 458487 rather than the image size. Fixing the dialog image size doesn't fix the look of every other downscaled image on the web that now looks revolting on Linux.
What happened before was that downscaling images used the GOOD filter and upscaling images used the FAST filter, which worked perfectly fine. I can't find anything in nsThebesImage::Draw that measures the scale anymore, though...
Created attachment 347954 [details] [diff] [review] Fix downscaling This re-re-re-fixes the smooth downscaling on Linux. Honestly, why do the gfx guys always regress this? Don't they ever test their patches on Linux? If only I could think of a way of doing so, I would LOVE to write a test for this to light a fire under them if they regress it again.
Did I mention this is a regression from FF3? Just an incentive for the blocker flag, is all...
Just because we're downscaling doesn't mean we won't sample outside the image, AFAIK.
Comment on attachment 347954 [details] [diff] [review] Fix downscaling However, restoring that dubious old code probably is the right thing to do for now.
(In reply to comment #22) > Just because we're downscaling doesn't mean we won't sample outside the image, > AFAIK. I've never actually seen a case where sampling outside the image on downscaling was a problem for a web page. But, it didn't take me 3 sites to always find a page where images now looked unreasonably and unintentionally pixellated because we got rid of this downscaling filtering.
Component: Themes → GFX: Thebes
Product: Toolkit → Core
QA Contact: themes → thebes
(In reply to comment #20) > Created an attachment (id=347954) [details] > Fix downscaling > > This re-re-re-fixes the smooth downscaling on Linux. Honestly, why do the gfx > guys always regress this? Don't they ever test their patches on Linux? > > If only I could think of a way of doing so, I would LOVE to write a test for > this to light a fire under them if they regress it again. Yes, because bluster and vitriol are exactly what's needed here. Or something. Perhaps you can redirect it towards fixing the broken crap upstream so that we can stop having to do workarounds and workarounds for said linux-only broken crap.
Attachment #347954 - Flags: review?(vladimir) → review+
Duplicate of this bug: 464798
Also, it was me, not the "gfx guys", who regressed it.
Sorry, I guess I could have worded that post better as I didn't mean for it to sound rude. But this has been regressed many times in the past and am still wondering why nearest neighbour should be applied to downscaling whenever this code is changed.
Also, what is needed for EXTEND_PAD to be possible on X11? Is it an XRender deficiency?
Comment on attachment 347954 [details] [diff] [review] Fix downscaling One last thing, would it still be possible to get this in beta 2? If we don't we could probably expect a lot of duplicates about ugly icons and images.
Attachment #347954 - Flags: approval1.9.1b2?
Flags: blocking1.9.1? → blocking1.9.1+
Target Milestone: --- → mozilla1.9.1
Comment on attachment 347954 [details] [diff] [review] Fix downscaling a1.9.1b2=beltzner, but we won't hold b2 for this; it does block final release.
Attachment #347954 - Flags: approval1.9.1b2? → approval1.9.1b2+
I'd be happy to land this, with/after the other a1.9b2 patch that I'm landing on https://wiki.mozilla.org/1.9.1_beta_2_checkins .
Assignee: nobody → ventnor.bugzilla
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(In reply to comment #29) > Also, what is needed for EXTEND_PAD to be possible on X11? Is it an XRender > deficiency? Yes.
Target Milestone: mozilla1.9.1 → mozilla1.9.1b2
Created attachment 348241 [details] reftest failures (including screenshots of tests) This patch made these two reftests failed on Linux -- I may back out soon, unless it's determined that the tests are broken.
The reftests are: http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/403181-1.xml http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/421885-1.xml Note that these tests just use *up* scaling (one by 2.5, one by 2)... which is bad, because in theory this bug's patch was supposed to only change down-scaling, I think. Backing out & reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla1.9.1b2 → ---
Attachment #348241 - Attachment description: reftest failures → reftest failures (including screenshots of tests)
In the reftest failures (in attachment 348241 [details]), it looks like the reference cases have solid black shapes, whereas the testcases have shapes with a 1px fuzzy gray border.
Roc, does aFill or aSubimage represent the source/destination sizes? What the function used to do was calculate the scale itself, which seemed to work well. I could also be misunderstanding what the values in the transform matrix mean (see, this is why I want to avoid the gfx code :) )
aFill may be what I want, but that seems to be transformed somehow since using that still doesn't fix the reftests. What I need is the untransformed source image and destination rect sizes and so far I haven't found a way of doing that without adding parameters to Draw() which I want to avoid. roc, are these values available in the Draw() function?
I'm dumb. Let me write the patch for this.
Created attachment 348464 [details] [diff] [review] fix When we're downscaling, the transform from device space to image space is actually scaling *up* coordinates. E.g. if you're scaling down by a factor of 2 and no translation is needed, then device coordinate (1,1) maps to image pixel (2,2). Michael, please test this. If it works, and passes reftests, get review from vlad.
The 403181 reftest still doesn't pass. 421885 does though. I'm wondering why it fails, 2.5 scale using foreignObject is supposed to scale up, isn't it?
Yes. We can work on this when you get here next week if you like.
Attachment #347954 - Flags: approval1.9.1b2+
Robert, it seems 403181 fails on my machine anyway even without your patch. In fact, when comparing the unexpected fails on my machine with your patch vs w/o they are exactly the same. We should take your fix. r=me (ok, not really)
Attachment #348464 - Flags: review?(vladimir) → review+
Whiteboard: [needs landing]
Pushed to trunk as 8e22374a4c8a
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago → 9 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
(sorry, that fixed1.9.1 was added in error - as per comments this has yet to land on 1.9.1)
Pushed e54a9f162ee1 to 1.9.1
Whiteboard: [needs 191 landing]
You need to log in before you can comment on or make changes to this bug.