Last Comment Bug 531289 - Firefox doesn't obey system dpi settings anymore
: Firefox doesn't obey system dpi settings anymore
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Mark Finkle (:mfinkle) (use needinfo?)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 530931
Blocks: 519108
  Show dependency treegraph
 
Reported: 2009-11-26 08:36 PST by Pascal Chevrel:pascalc
Modified: 2010-01-05 16:19 PST (History)
8 users (show)
roc: blocking1.9.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta5-fixed


Attachments
screenshot of font problem (99.79 KB, image/png)
2009-11-26 08:36 PST, Pascal Chevrel:pascalc
no flags Details
patch (2.04 KB, patch)
2009-11-27 19:46 PST, Mark Finkle (:mfinkle) (use needinfo?)
roc: review+
Details | Diff | Splinter Review
revert to full behavior (1.12 KB, patch)
2009-11-30 14:38 PST, Mark Finkle (:mfinkle) (use needinfo?)
dbaron: review+
Details | Diff | Splinter Review

Description Pascal Chevrel:pascalc 2009-11-26 08:36:23 PST
Created attachment 414738 [details]
screenshot of font problem

With today's nightlies, the UI fonts for Firefox trunk has become big.

See the attached screenshot to see the difference

Normal fonts with:
Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.3a1pre) Gecko/20091125 Minefield/3.7a1pre

Bad font size with:
Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.3a1pre) Gecko/20091126 Minefield/3.7a1pre


platform is Ubuntu Karmic, Gnome desktop.
Comment 1 Pascal Chevrel:pascalc 2009-11-26 08:42:30 PST
Bug #530931 is fonts related for GTK and landed code change in the regression window, could be the cause of this bug?
Comment 2 Henrik Skupin (:whimboo) 2009-11-26 14:15:27 PST
Mark or Roc, could this be a regression from bug 530931?

Pascal, can you give us the changeset id's of both builds so we can check for all the checkins in this time frame?
Comment 3 Pascal Chevrel:pascalc 2009-11-26 14:28:27 PST
Henrik, I woudln't know how to provide you this information but I am open to instructions for that :)
Comment 4 Henrik Skupin (:whimboo) 2009-11-26 14:41:38 PST
Just open about:buildconfig. You will see those links there.
Comment 5 Pascal Chevrel:pascalc 2009-11-27 06:56:23 PST
20091125 build:
http://hg.mozilla.org/mozilla-central/rev/d76583175408

20091126 build:
http://hg.mozilla.org/mozilla-central/rev/77136b3d68fc
Comment 6 Henrik Skupin (:whimboo) 2009-11-27 07:27:46 PST
Thanks Pascal, here all the checkins in that time frame:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d76583175408&tochange=77136b3d68fc

I was able to reproduce the bug when testing builds from 11/25 and 11/26. That's the time range when the patch on bug 530931 has been checked into 1.9.2.

Here steps how to reproduce this issue:
1. Open "System | Preferences | Fonts | Details" on Ubuntu
2. Change the DPI setting from 96dpi to e.g. 80dpi
3. Start latest Namoroka or Minefield build

With step 3 you can see that we don't obey the system dpi settings anymore. We always use a font size which applies to 96dpi. This bug will be highly visible on all laptops which mostly have another default dpi. As Pascal told me on IRC his laptop is using 90dpi per default.

Requesting blocking1.9.2.
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2009-11-27 19:05:56 PST
The code is currently enforcing a minimum DPI:
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/src/gfxPlatformGtk.cpp#535

When I originally refactored the DPI code, this minimum-check only affected the Thebes drawing code, not the system/pango font code, as it does in m-192 and m-c:
http://mxr.mozilla.org/mozilla1.9.1/source/gfx/src/thebes/nsThebesDeviceContext.cpp#204

I'm not sure why GTK is the only platform that enforces the minimum, but I can add it back into the thebes code only. I could remove the minimum too, but I assume it's there for a reason.

Robert, what do you think?
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2009-11-27 19:43:32 PST
I could also bring back the PlatformDPI, which would be 96dpi and used for the font code. That would leave Thebes to be the only consumer of the display DPI.
Comment 9 Mark Finkle (:mfinkle) (use needinfo?) 2009-11-27 19:46:25 PST
Created attachment 414892 [details] [diff] [review]
patch

Simple fix. Uses a platform define in thebes code to enforce the minimum DPI only for GTK. This is very similar to the original DPI code, still found in 1.9.1

This is probably the safest thing to do for 1.9.2

Maybe we could file a followup for cleaning up trunk.
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-29 13:58:57 PST
Comment on attachment 414892 [details] [diff] [review]
patch

This is good for 1.9.2.

On trunk, can we just stop enforcing a minimum DPI on GTK2?
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2009-11-30 13:08:53 PST
(In reply to comment #10)
> (From update of attachment 414892 [details] [diff] [review])
> This is good for 1.9.2.

OK, I'll and this on 1.9.2

> On trunk, can we just stop enforcing a minimum DPI on GTK2?

I think that's a good idea. If there is any fallout, we can adjust it.
Comment 12 David Baron :dbaron: ⌚️UTC-10 2009-11-30 13:39:18 PST
I think we want to enforce the minimum 96dpi; Web pages become unusable at smaller DPIs because they specify font sizes in 'pt' that map to numbers of pixels that are too small to be legible at less than 96dpi.

We just don't want to apply that change to the GTK DPI that we use for interacting with GTK/GDK/pango.
Comment 13 David Baron :dbaron: ⌚️UTC-10 2009-11-30 13:42:33 PST
And also see the comment just above the code you're modifying:

        // A value of -1 means use the minimum of 96 and the system DPI.
        // A value of 0 means use the system DPI. A positive value is used as the DPI.
        // This sets the physical size of a device pixel and thus controls the
        // interpretation of physical units such as "pt".

which is no longer what that code is doing.


Maybe we should just back out the whole series of patches given that the purpose of the most recent patch was to undo the change but leave the refactoring... maybe we just want to undo the whole thing?
Comment 14 David Baron :dbaron: ⌚️UTC-10 2009-11-30 13:48:05 PST
(In reply to comment #13)
> Maybe we should just back out the whole series of patches given that the
> purpose of the most recent patch was to undo the change but leave the
> refactoring... maybe we just want to undo the whole thing?

Actually, never mind this.  I think the refactoring is useful.

But I want to go through and review the whole set of patches and see what the overall diff looks like, because there have been a lot of patches and I'm pretty confused.
Comment 15 Mark Finkle (:mfinkle) (use needinfo?) 2009-11-30 13:56:57 PST
(In reply to comment #13)

> And also see the comment just above the code you're modifying:
> 
>         // A value of -1 means use the minimum of 96 and the system DPI.
>         // A value of 0 means use the system DPI. A positive value is used as
> the DPI.
>         // This sets the physical size of a device pixel and thus controls the
>         // interpretation of physical units such as "pt".
> 
> which is no longer what that code is doing.

Sorry I missed this comment before pushing. Yes, the patch makes -1 and 0 act like -1 on 192. The patch removes the minimum, so -1 and 0 work like 0 on trunk.

I can:
 1. just update comments to be correct on 192 and trunk
 2. add the full behavior back to 192
 3. add full behavior back to trunk
 4. 2 & 3

Thoughts?
Comment 16 David Baron :dbaron: ⌚️UTC-10 2009-11-30 14:03:39 PST
I'd say either:
 (1) land the full patch (attached and reviewed above) on both mozilla-central and 1.9.2
or
 (2) restore the full behavior for both mozilla-central and 1.9.2, for at least GTK (also fixes bug 519108)

I'd slightly prefer (2).  But I think dropping the 96dpi clamping is not acceptable; there's a good reason for it.
Comment 17 David Baron :dbaron: ⌚️UTC-10 2009-11-30 14:15:41 PST
But (1)'s fine too; that just means landing the rest of what you've already landed on 1.9.2 on m-c as well.
Comment 18 Mark Finkle (:mfinkle) (use needinfo?) 2009-11-30 14:38:59 PST
Created attachment 415255 [details] [diff] [review]
revert to full behavior

This patch is for 192 and trunk. It puts back the full -1, 0, > 0 behavior for GTK, which appears to be the only code that used the -1 clamping mode. Other platforms only use the 0 (use system dpi) and the > 0 (override dpi) modes.
Comment 19 David Baron :dbaron: ⌚️UTC-10 2009-11-30 14:48:51 PST
Comment on attachment 415255 [details] [diff] [review]
revert to full behavior

>+#ifdef MOZ_ENABLE_GTK2
>+        if (prefDPI == -1) // Clamp the minimum dpi is 96dpi


I'd make this if (prefDPI < 0) for consistency with the old code.

r=dbaron with that, and thanks for fixing this
Comment 20 Mark Finkle (:mfinkle) (use needinfo?) 2009-11-30 15:12:28 PST
first patch

pushed m-c:
http://hg.mozilla.org/mozilla-central/rev/08321912dd62

pushed m-192:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2bdbd5e609bf
Comment 21 Mark Finkle (:mfinkle) (use needinfo?) 2009-11-30 15:13:38 PST
second patch

pushed m-c:
http://hg.mozilla.org/mozilla-central/rev/18633dc54642

pushed m-192:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/eb059e3ff71d
Comment 22 Pascal Chevrel:pascalc 2009-12-02 04:37:17 PST
I confirm that UI fonts behaviour is back to normal in Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.3a1pre) Gecko/20091201 Minefield/3.7a1pre

Thanks

Note You need to log in before you can comment on or make changes to this bug.