Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(317)

Issue 929733002: Fix Pango font rendering with HiDPi displays on Linux. (Closed)

Created:
5 years, 10 months ago by stapelberg
Modified:
5 years, 9 months ago
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix Pango font rendering with HiDPi displays on Linux. Don’t scale fonts up twice when running with device_scale_factor > 1.0. Chrome’s UI is scaled up to device_scale_factor (e.g. 2x) by calling SkCanvas->scale(). See Chrome’s gfx::Canvas class which calls canvas.scale(image_scale, image_scale): https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/canvas.cc Here is a skia fiddle which renders text with .setTextSize(21): https://fiddle.skia.org/c/098cb32edac7e394aba5b9ff4c08b06c Here is the same fiddle, but calling canvas.Scale(2.0, 2.0) before rendering to simulate what Chrome does: https://fiddle.skia.org/c/a7b456338776239278e398741e1098d9 As you can see, the text is twice as large. Therefore, this commit changes Chrome to take into account the DPI that GTK provides, but set it in perspective to the device scale factor. This preserves the same behavior when device scale factor == 1 but yields correct rendering when device scale factor > 1.0 (hi-dpi displays). Before this change: http://t.zekjur.net/chrome-hidpi-enabled.png After this change: http://t.zekjur.net/chrome-hidpi-enabled-scalefix.png This change also enables subpixel rendering on HiDPi displays, which is what Chrome OS also does. Without subpixel rendering: http://t.zekjur.net/without-subpixel.png With subpixel rendering: http://t.zekjur.net/with-subpixel.png BUG=143619 Committed: https://crrev.com/aa051ee8d1a7ff725225d783d1a96663d5aedb08 Cr-Commit-Position: refs/heads/master@{#321457}

Patch Set 1 #

Patch Set 2 : Don’t scale fonts up twice when running with device_scale_factor > 1.0. #

Total comments: 23

Patch Set 3 : Fix Pango font rendering with HiDPi displays on Linux #

Total comments: 6

Patch Set 4 : Fix Pango font rendering with HiDPi displays on Linux. #

Total comments: 2

Patch Set 5 : Fix Pango font rendering with HiDPi displays on Linux. #

Patch Set 6 : Fix Pango font rendering with HiDPi displays on Linux. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+42 lines, -16 lines) Patch
M chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc View 1 2 1 chunk +6 lines, -2 lines 0 comments Download
M chrome/browser/ui/libgtk2ui/gtk2_ui.h View 1 2 2 chunks +5 lines, -0 lines 0 comments Download
M chrome/browser/ui/libgtk2ui/gtk2_ui.cc View 1 2 5 chunks +14 lines, -3 lines 0 comments Download
M ui/gfx/font_render_params_linux.cc View 1 2 3 4 5 4 chunks +13 lines, -11 lines 0 comments Download
M ui/views/linux_ui/linux_ui.h View 1 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 41 (9 generated)
stapelberg
5 years, 10 months ago (2015-02-14 21:17:46 UTC) #2
Evan Stade
based on the before and after screencaps, I think Chrome's already working as intended.
5 years, 10 months ago (2015-02-17 17:41:24 UTC) #3
chromium-reviews
With all due respect, I totally disagree. The fonts are HUGE in Chrome without my ...
5 years, 10 months ago (2015-02-17 17:54:00 UTC) #4
sadrul
On 2015/02/17 17:54:00, chromium-reviews wrote: > With all due respect, I totally disagree. The fonts ...
5 years, 9 months ago (2015-03-10 20:23:56 UTC) #5
Daniel Erat
On 2015/03/10 20:23:56, sadrul wrote: > On 2015/02/17 17:54:00, chromium-reviews wrote: > > With all ...
5 years, 9 months ago (2015-03-10 20:32:43 UTC) #6
stapelberg
On 2015/03/10 20:23:56, sadrul wrote: > On 2015/02/17 17:54:00, chromium-reviews wrote: > > With all ...
5 years, 9 months ago (2015-03-10 21:59:21 UTC) #7
stapelberg
On 2015/03/10 20:32:43, Daniel Erat wrote: > On 2015/03/10 20:23:56, sadrul wrote: > > On ...
5 years, 9 months ago (2015-03-10 22:09:03 UTC) #8
sadrul
On 2015/03/10 21:59:21, stapelberg wrote: > On 2015/03/10 20:23:56, sadrul wrote: > > On 2015/02/17 ...
5 years, 9 months ago (2015-03-10 22:12:36 UTC) #9
sadrul
[ +derat@ ]
5 years, 9 months ago (2015-03-10 22:13:28 UTC) #11
stapelberg
On 2015/03/10 22:12:36, sadrul wrote: > On 2015/03/10 21:59:21, stapelberg wrote: > > On 2015/03/10 ...
5 years, 9 months ago (2015-03-10 22:27:53 UTC) #12
stapelberg
On 2015/03/10 22:27:53, stapelberg wrote: > On 2015/03/10 22:12:36, sadrul wrote: > > On 2015/03/10 ...
5 years, 9 months ago (2015-03-12 09:15:31 UTC) #13
Daniel Erat
On 2015/03/12 09:15:31, stapelberg wrote: > On 2015/03/10 22:27:53, stapelberg wrote: > > On 2015/03/10 ...
5 years, 9 months ago (2015-03-12 13:48:22 UTC) #15
oshima
On 2015/03/12 13:48:22, Daniel Erat wrote: > On 2015/03/12 09:15:31, stapelberg wrote: > > On ...
5 years, 9 months ago (2015-03-12 19:36:08 UTC) #16
stapelberg
Thanks for the additional information. I’ve uploaded a new patch set which replaces the first ...
5 years, 9 months ago (2015-03-15 15:44:51 UTC) #17
Daniel Erat
i'm okay with this approach since it doesn't affect the dsf == 1.0 case, but ...
5 years, 9 months ago (2015-03-16 13:57:54 UTC) #18
stapelberg
Thanks for the review! PTAL. https://codereview.chromium.org/929733002/diff/20001/chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc File chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc (right): https://codereview.chromium.org/929733002/diff/20001/chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc#newcode125 chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc:125: { On 2015/03/16 13:57:53, ...
5 years, 9 months ago (2015-03-17 08:36:34 UTC) #19
oshima
https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_params_linux.cc File ui/gfx/font_render_params_linux.cc (right): https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_params_linux.cc#newcode217 ui/gfx/font_render_params_linux.cc:217: if (screen) { On 2015/03/17 08:36:34, stapelberg wrote: > ...
5 years, 9 months ago (2015-03-17 19:59:02 UTC) #20
stapelberg
https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_params_linux.cc File ui/gfx/font_render_params_linux.cc (right): https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_params_linux.cc#newcode217 ui/gfx/font_render_params_linux.cc:217: if (screen) { On 2015/03/17 19:59:02, oshima wrote: > ...
5 years, 9 months ago (2015-03-18 08:17:53 UTC) #21
oshima
https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_params_linux.cc File ui/gfx/font_render_params_linux.cc (right): https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_params_linux.cc#newcode217 ui/gfx/font_render_params_linux.cc:217: if (screen) { On 2015/03/18 08:17:53, stapelberg wrote: > ...
5 years, 9 months ago (2015-03-18 21:50:46 UTC) #22
stapelberg
https://codereview.chromium.org/929733002/diff/40001/chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc File chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc (right): https://codereview.chromium.org/929733002/diff/40001/chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc#newcode113 chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc:113: On 2015/03/18 21:50:46, oshima wrote: > is it possible ...
5 years, 9 months ago (2015-03-19 07:54:52 UTC) #23
oshima
On 2015/03/19 07:54:52, stapelberg wrote: > https://codereview.chromium.org/929733002/diff/40001/chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc > File chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc (right): > > https://codereview.chromium.org/929733002/diff/40001/chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc#newcode113 > ...
5 years, 9 months ago (2015-03-19 16:55:40 UTC) #24
Daniel Erat
https://codereview.chromium.org/929733002/diff/40001/ui/gfx/linux_font_delegate.h File ui/gfx/linux_font_delegate.h (right): https://codereview.chromium.org/929733002/diff/40001/ui/gfx/linux_font_delegate.h#newcode31 ui/gfx/linux_font_delegate.h:31: static LinuxFontDelegate* instance(); this can remain const, no? https://codereview.chromium.org/929733002/diff/40001/ui/gfx/platform_font_linux.cc ...
5 years, 9 months ago (2015-03-19 17:11:44 UTC) #25
stapelberg
https://codereview.chromium.org/929733002/diff/40001/ui/gfx/linux_font_delegate.h File ui/gfx/linux_font_delegate.h (right): https://codereview.chromium.org/929733002/diff/40001/ui/gfx/linux_font_delegate.h#newcode31 ui/gfx/linux_font_delegate.h:31: static LinuxFontDelegate* instance(); On 2015/03/19 17:11:44, Daniel Erat (OOO ...
5 years, 9 months ago (2015-03-19 17:20:56 UTC) #27
Daniel Erat
thanks! lgtm
5 years, 9 months ago (2015-03-19 18:06:06 UTC) #28
msw
lgtm with a nit. https://codereview.chromium.org/929733002/diff/60001/ui/gfx/font_render_params_linux.cc File ui/gfx/font_render_params_linux.cc (right): https://codereview.chromium.org/929733002/diff/60001/ui/gfx/font_render_params_linux.cc#newcode67 ui/gfx/font_render_params_linux.cc:67: bool IsBrowserTextSubpixelPositioningEnabled( nit: maybe inline ...
5 years, 9 months ago (2015-03-19 19:30:43 UTC) #29
sadrul
ui/views lgtm
5 years, 9 months ago (2015-03-19 19:32:39 UTC) #30
stapelberg
https://codereview.chromium.org/929733002/diff/60001/ui/gfx/font_render_params_linux.cc File ui/gfx/font_render_params_linux.cc (right): https://codereview.chromium.org/929733002/diff/60001/ui/gfx/font_render_params_linux.cc#newcode67 ui/gfx/font_render_params_linux.cc:67: bool IsBrowserTextSubpixelPositioningEnabled( On 2015/03/19 19:30:43, msw wrote: > nit: ...
5 years, 9 months ago (2015-03-19 19:39:25 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/929733002/80001
5 years, 9 months ago (2015-03-19 19:40:01 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/929733002/80001
5 years, 9 months ago (2015-03-19 19:40:44 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/929733002/100001
5 years, 9 months ago (2015-03-19 21:50:28 UTC) #39
commit-bot: I haz the power
Committed patchset #6 (id:100001)
5 years, 9 months ago (2015-03-19 22:40:52 UTC) #40
commit-bot: I haz the power
5 years, 9 months ago (2015-03-19 22:41:47 UTC) #41
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/aa051ee8d1a7ff725225d783d1a96663d5aedb08
Cr-Commit-Position: refs/heads/master@{#321457}

Powered by Google App Engine
This is Rietveld 408576698