|
|
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. |
DescriptionFix 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. #
Messages
Total messages: 41 (9 generated)
stapelberg@google.com changed reviewers: + estade@chromium.org, scottmg@chromium.org
based on the before and after screencaps, I think Chrome's already working as intended.
With all due respect, I totally disagree. The fonts are HUGE in Chrome without my change. No other application renders like that. What would be the point in overly large fonts?! On Tue, Feb 17, 2015 at 9:41 AM, <estade@chromium.org> wrote: > based on the before and after screencaps, I think Chrome's already working > as > intended. > > https://codereview.chromium.org/929733002/ > -- Best regards, Michael To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/02/17 17:54:00, chromium-reviews wrote: > With all due respect, I totally disagree. The fonts are HUGE in Chrome > without my change. No other application renders like that. What would be > the point in overly large fonts?! I think we do want the default font-size in the content to match the default font-size in the UI. (i.e. 'After this change' looks more correct). Can you see if we can instead fix the TODO in //ui/views/widget/desktop_aura/desktop_screen_x11.cc and use gtk-xft-dpi in GetDeviceScaleFactor()? Would that fix this too? Or can we instead set query.device_scale_factor correctly here in UpdateDefaultFont()?
On 2015/03/10 20:23:56, sadrul wrote: > On 2015/02/17 17:54:00, chromium-reviews wrote: > > With all due respect, I totally disagree. The fonts are HUGE in Chrome > > without my change. No other application renders like that. What would be > > the point in overly large fonts?! > > I think we do want the default font-size in the content to match the default > font-size in the UI. (i.e. 'After this change' looks more correct). > > Can you see if we can instead fix the TODO in > //ui/views/widget/desktop_aura/desktop_screen_x11.cc and use gtk-xft-dpi in > GetDeviceScaleFactor()? Would that fix this too? Or can we instead set > query.device_scale_factor correctly here in UpdateDefaultFont()? this seems like it's essentially reverting https://codereview.chromium.org/406493002, which was needed to fix http://crbug.com/375824. i'm worried that it'll make that bug regress for non-high-DPI users with DPIs other than 96. chrome's UI font code ought to match what gtk2 is doing here (i.e. chrome's UI font should match gtk2 apps' in both sizing and rendering settings).
On 2015/03/10 20:23:56, sadrul wrote: > On 2015/02/17 17:54:00, chromium-reviews wrote: > > With all due respect, I totally disagree. The fonts are HUGE in Chrome > > without my change. No other application renders like that. What would be > > the point in overly large fonts?! > > I think we do want the default font-size in the content to match the default > font-size in the UI. (i.e. 'After this change' looks more correct). > > Can you see if we can instead fix the TODO in > //ui/views/widget/desktop_aura/desktop_screen_x11.cc and use gtk-xft-dpi in > GetDeviceScaleFactor()? Would that fix this too? Or can we instead set > query.device_scale_factor correctly here in UpdateDefaultFont()? I looked at that TODO, but |scale| is already at 1.8 (so the resulting scaling factor is 2), which is correct for my case. So, no, that doesn’t fix anything as far as I understand it. Hardcoding query.device_scale_factor = 2; in line 1372 in chrome/browser/ui/libgtk2ui/gtk2_ui.cc doesn’t help either, if that’s what you had in mind with your second suggestion.
On 2015/03/10 20:32:43, Daniel Erat wrote: > On 2015/03/10 20:23:56, sadrul wrote: > > On 2015/02/17 17:54:00, chromium-reviews wrote: > > > With all due respect, I totally disagree. The fonts are HUGE in Chrome > > > without my change. No other application renders like that. What would be > > > the point in overly large fonts?! > > > > I think we do want the default font-size in the content to match the default > > font-size in the UI. (i.e. 'After this change' looks more correct). > > > > Can you see if we can instead fix the TODO in > > //ui/views/widget/desktop_aura/desktop_screen_x11.cc and use gtk-xft-dpi in > > GetDeviceScaleFactor()? Would that fix this too? Or can we instead set > > query.device_scale_factor correctly here in UpdateDefaultFont()? > > this seems like it's essentially reverting > https://codereview.chromium.org/406493002, which was needed to fix > http://crbug.com/375824. i'm worried that it'll make that bug regress for > non-high-DPI users with DPIs other than 96. chrome's UI font code ought to match > what gtk2 is doing here (i.e. chrome's UI font should match gtk2 apps' in both > sizing and rendering settings). Thanks for pointing that out. I’ve added a debug statement, and the current code (without my change) results in “Bitstream Vera Sans 8” on my machine. Now, what it actually renders is definitely _not_ equal to Bitstream Vera Sans 8 in other hidpi-aware applications. I’ve upoaded http://t.zekjur.net/2015-03-10-230746_468x263_scrot.png to illustrate the difference: my window manager is configured to draw window titles in Bitstream Vera Sans 8 as well, and it’s drastically different than what Chrome renders. I think my fix may not be correct in the sense that it fixes the wrong place in the code, but there clearly is a difference in font rendering that needs to be addressed :).
On 2015/03/10 21:59:21, stapelberg wrote: > On 2015/03/10 20:23:56, sadrul wrote: > > On 2015/02/17 17:54:00, chromium-reviews wrote: > > > With all due respect, I totally disagree. The fonts are HUGE in Chrome > > > without my change. No other application renders like that. What would be > > > the point in overly large fonts?! > > > > I think we do want the default font-size in the content to match the default > > font-size in the UI. (i.e. 'After this change' looks more correct). > > > > Can you see if we can instead fix the TODO in > > //ui/views/widget/desktop_aura/desktop_screen_x11.cc and use gtk-xft-dpi in > > GetDeviceScaleFactor()? Would that fix this too? Or can we instead set > > query.device_scale_factor correctly here in UpdateDefaultFont()? > > I looked at that TODO, but |scale| is already at 1.8 (so the resulting scaling > factor is 2), which is correct for my case. So, no, that doesn’t fix anything as > far as I understand it. > > Hardcoding query.device_scale_factor = 2; in line 1372 in > chrome/browser/ui/libgtk2ui/gtk2_ui.cc doesn’t help either, if that’s what you > had in mind with your second suggestion. Yeah, that's what I meant. Maybe we need to use PlatformFontLinux::device_scale_factor_ on Linux too, instead of only on ChromeOS? I think we need to look at the chromeos-specific code related to fonts in ui/gfx/ and see if we need/want them on Linux too.
sadrul@chromium.org changed reviewers: + derat@chromium.org
[ +derat@ ]
On 2015/03/10 22:12:36, sadrul wrote: > On 2015/03/10 21:59:21, stapelberg wrote: > > On 2015/03/10 20:23:56, sadrul wrote: > > > On 2015/02/17 17:54:00, chromium-reviews wrote: > > > > With all due respect, I totally disagree. The fonts are HUGE in Chrome > > > > without my change. No other application renders like that. What would be > > > > the point in overly large fonts?! > > > > > > I think we do want the default font-size in the content to match the default > > > font-size in the UI. (i.e. 'After this change' looks more correct). > > > > > > Can you see if we can instead fix the TODO in > > > //ui/views/widget/desktop_aura/desktop_screen_x11.cc and use gtk-xft-dpi in > > > GetDeviceScaleFactor()? Would that fix this too? Or can we instead set > > > query.device_scale_factor correctly here in UpdateDefaultFont()? > > > > I looked at that TODO, but |scale| is already at 1.8 (so the resulting scaling > > factor is 2), which is correct for my case. So, no, that doesn’t fix anything > as > > far as I understand it. > > > > Hardcoding query.device_scale_factor = 2; in line 1372 in > > chrome/browser/ui/libgtk2ui/gtk2_ui.cc doesn’t help either, if that’s what you > > had in mind with your second suggestion. > > Yeah, that's what I meant. > > Maybe we need to use PlatformFontLinux::device_scale_factor_ on Linux too, > instead of only on ChromeOS? I think we need to look at the chromeos-specific > code related to fonts in ui/gfx/ and see if we need/want them on Linux too. I’ve tried that before and it shows no change whatsoever. http://t.zekjur.net/chromeos_font.patch is the change to the code I did. Maybe I missed an occurence?
On 2015/03/10 22:27:53, stapelberg wrote: > On 2015/03/10 22:12:36, sadrul wrote: > > On 2015/03/10 21:59:21, stapelberg wrote: > > > On 2015/03/10 20:23:56, sadrul wrote: > > > > On 2015/02/17 17:54:00, chromium-reviews wrote: > > > > > With all due respect, I totally disagree. The fonts are HUGE in Chrome > > > > > without my change. No other application renders like that. What would be > > > > > the point in overly large fonts?! > > > > > > > > I think we do want the default font-size in the content to match the > default > > > > font-size in the UI. (i.e. 'After this change' looks more correct). > > > > > > > > Can you see if we can instead fix the TODO in > > > > //ui/views/widget/desktop_aura/desktop_screen_x11.cc and use gtk-xft-dpi > in > > > > GetDeviceScaleFactor()? Would that fix this too? Or can we instead set > > > > query.device_scale_factor correctly here in UpdateDefaultFont()? > > > > > > I looked at that TODO, but |scale| is already at 1.8 (so the resulting > scaling > > > factor is 2), which is correct for my case. So, no, that doesn’t fix > anything > > as > > > far as I understand it. > > > > > > Hardcoding query.device_scale_factor = 2; in line 1372 in > > > chrome/browser/ui/libgtk2ui/gtk2_ui.cc doesn’t help either, if that’s what > you > > > had in mind with your second suggestion. > > > > Yeah, that's what I meant. > > > > Maybe we need to use PlatformFontLinux::device_scale_factor_ on Linux too, > > instead of only on ChromeOS? I think we need to look at the chromeos-specific > > code related to fonts in ui/gfx/ and see if we need/want them on Linux too. > > I’ve tried that before and it shows no change whatsoever. > http://t.zekjur.net/chromeos_font.patch is the change to the code I did. Maybe I > missed an occurence? So, I’ve looked into this a bit more. src/ui/aura/window_tree_host.cc contains calls to compositor_->SetScaleAndSize which scale _the entire UI_. See e.g. http://t.zekjur.net/3x.png and http://t.zekjur.net/6x.png (note how the “New Tab” string is scaled up). Based on that, I think we should _not_ replicate the Pango font scaling logic and instead go ahead with my patch. I’m still not sure on the implications on the bug you’ve cited. What do you think?
derat@chromium.org changed reviewers: + oshima@chromium.org
On 2015/03/12 09:15:31, stapelberg wrote: > On 2015/03/10 22:27:53, stapelberg wrote: > > On 2015/03/10 22:12:36, sadrul wrote: > > > On 2015/03/10 21:59:21, stapelberg wrote: > > > > On 2015/03/10 20:23:56, sadrul wrote: > > > > > On 2015/02/17 17:54:00, chromium-reviews wrote: > > > > > > With all due respect, I totally disagree. The fonts are HUGE in Chrome > > > > > > without my change. No other application renders like that. What would > be > > > > > > the point in overly large fonts?! > > > > > > > > > > I think we do want the default font-size in the content to match the > > default > > > > > font-size in the UI. (i.e. 'After this change' looks more correct). > > > > > > > > > > Can you see if we can instead fix the TODO in > > > > > //ui/views/widget/desktop_aura/desktop_screen_x11.cc and use gtk-xft-dpi > > in > > > > > GetDeviceScaleFactor()? Would that fix this too? Or can we instead set > > > > > query.device_scale_factor correctly here in UpdateDefaultFont()? > > > > > > > > I looked at that TODO, but |scale| is already at 1.8 (so the resulting > > scaling > > > > factor is 2), which is correct for my case. So, no, that doesn’t fix > > anything > > > as > > > > far as I understand it. > > > > > > > > Hardcoding query.device_scale_factor = 2; in line 1372 in > > > > chrome/browser/ui/libgtk2ui/gtk2_ui.cc doesn’t help either, if that’s what > > you > > > > had in mind with your second suggestion. > > > > > > Yeah, that's what I meant. > > > > > > Maybe we need to use PlatformFontLinux::device_scale_factor_ on Linux too, > > > instead of only on ChromeOS? I think we need to look at the > chromeos-specific > > > code related to fonts in ui/gfx/ and see if we need/want them on Linux too. > > > > I’ve tried that before and it shows no change whatsoever. > > http://t.zekjur.net/chromeos_font.patch is the change to the code I did. Maybe > I > > missed an occurence? > > So, I’ve looked into this a bit more. src/ui/aura/window_tree_host.cc contains > calls to compositor_->SetScaleAndSize which scale _the entire UI_. See e.g. > http://t.zekjur.net/3x.png and http://t.zekjur.net/6x.png (note how the “New > Tab” string is scaled up). > > Based on that, I think we should _not_ replicate the Pango font scaling logic > and instead go ahead with my patch. > > I’m still not sure on the implications on the bug you’ve cited. > > What do you think? adding oshima because i'm not sure what the plans are for supporting high-DPI on linux in the rest of the UI. we certainly would want to render text at the correct final size rather than rendering it at a smaller size and letting the compositor scale it up, though.
On 2015/03/12 13:48:22, Daniel Erat wrote: > On 2015/03/12 09:15:31, stapelberg wrote: > > On 2015/03/10 22:27:53, stapelberg wrote: > > > On 2015/03/10 22:12:36, sadrul wrote: > > > > On 2015/03/10 21:59:21, stapelberg wrote: > > > > > On 2015/03/10 20:23:56, sadrul wrote: > > > > > > On 2015/02/17 17:54:00, chromium-reviews wrote: > > > > > > > With all due respect, I totally disagree. The fonts are HUGE in > Chrome > > > > > > > without my change. No other application renders like that. What > would > > be > > > > > > > the point in overly large fonts?! > > > > > > > > > > > > I think we do want the default font-size in the content to match the > > > default > > > > > > font-size in the UI. (i.e. 'After this change' looks more correct). > > > > > > > > > > > > Can you see if we can instead fix the TODO in > > > > > > //ui/views/widget/desktop_aura/desktop_screen_x11.cc and use > gtk-xft-dpi > > > in > > > > > > GetDeviceScaleFactor()? Would that fix this too? Or can we instead set > > > > > > query.device_scale_factor correctly here in UpdateDefaultFont()? > > > > > > > > > > I looked at that TODO, but |scale| is already at 1.8 (so the resulting > > > scaling > > > > > factor is 2), which is correct for my case. So, no, that doesn’t fix > > > anything > > > > as > > > > > far as I understand it. > > > > > > > > > > Hardcoding query.device_scale_factor = 2; in line 1372 in > > > > > chrome/browser/ui/libgtk2ui/gtk2_ui.cc doesn’t help either, if that’s > what > > > you > > > > > had in mind with your second suggestion. > > > > > > > > Yeah, that's what I meant. > > > > > > > > Maybe we need to use PlatformFontLinux::device_scale_factor_ on Linux too, > > > > instead of only on ChromeOS? I think we need to look at the > > chromeos-specific > > > > code related to fonts in ui/gfx/ and see if we need/want them on Linux > too. > > > > > > I’ve tried that before and it shows no change whatsoever. > > > http://t.zekjur.net/chromeos_font.patch is the change to the code I did. > Maybe > > I > > > missed an occurence? > > > > So, I’ve looked into this a bit more. src/ui/aura/window_tree_host.cc contains > > calls to compositor_->SetScaleAndSize which scale _the entire UI_. See e.g. > > http://t.zekjur.net/3x.png and http://t.zekjur.net/6x.png (note how the “New > > Tab” string is scaled up). > > > > Based on that, I think we should _not_ replicate the Pango font scaling logic > > and instead go ahead with my patch. > > > > I’m still not sure on the implications on the bug you’ve cited. > > > > What do you think? > > adding oshima because i'm not sure what the plans are for supporting high-DPI on > linux in the rest of the UI. we certainly would want to render text at the > correct final size rather than rendering it at a smaller size and letting the > compositor scale it up, though. In case there is misunderstanding. We do not scale texture for high DPI. We create texture at native pixel size, and draw font at native size. The drawing code uses DIP coordinates (1280x850 pixel), but canvas do know the scale factor so it draws fonts/graphics at the native resolution.
Thanks for the additional information. I’ve uploaded a new patch set which replaces the first patch set. With the new version, I actually get better rendering (the font is crisper), and I’m fairly convinced that the general approach is now the correct one :). I don’t see the commit description of my patch set show up anywhere here, so I’m pasting it: 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). PTAL.
i'm okay with this approach since it doesn't affect the dsf == 1.0 case, but oshima ought to review it. https://codereview.chromium.org/929733002/diff/20001/chrome/browser/ui/aura/c... File chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc (right): https://codereview.chromium.org/929733002/diff/20001/chrome/browser/ui/aura/c... chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc:125: { the code that creates LinuxUI in chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc looks like it's hidden behind this: #if defined(USE_X11) && !defined(OS_CHROMEOS) if (GetInitialDesktop() != chrome::HOST_DESKTOP_TYPE_ASH) { this is already behind !OS_CHROMEOS, but it seems like you should have the rest of it here. https://codereview.chromium.org/929733002/diff/20001/chrome/browser/ui/aura/c... chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc:128: views::LinuxUI *gtk2_ui = views::LinuxUI::instance(); nit: '*' goes to left of space https://codereview.chromium.org/929733002/diff/20001/chrome/browser/ui/aura/c... chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc:129: CHECK(gtk2_ui); i'd just inline the views::LinuxUI::instance() where you use it below to keep the code simpler; it doesn't look like this should ever be null. https://codereview.chromium.org/929733002/diff/20001/chrome/browser/ui/aura/c... chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc:131: CHECK(display.is_valid()); i don't see how it can ever be invalid. https://codereview.chromium.org/929733002/diff/20001/chrome/browser/ui/libgtk... File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right): https://codereview.chromium.org/929733002/diff/20001/chrome/browser/ui/libgtk... chrome/browser/ui/libgtk2ui/gtk2_ui.cc:700: gfx::FontRenderParams* params_out) { can't this still be const? it doesn't look like you made any changes to it. https://codereview.chromium.org/929733002/diff/20001/chrome/browser/ui/libgtk... File chrome/browser/ui/libgtk2ui/gtk2_ui.h (right): https://codereview.chromium.org/929733002/diff/20001/chrome/browser/ui/libgtk... chrome/browser/ui/libgtk2ui/gtk2_ui.h:28: typedef struct _PangoFontDescription PangoFontDescription; you might want to rebase or merge your change against tip-of-tree -- i think that this typedef has already been present for a few weeks. https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_param... File ui/gfx/font_render_params_linux.cc (right): https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_param... ui/gfx/font_render_params_linux.cc:69: return query.device_scale_factor > 1.0f; it looks like you're enabling subpixel positioning for desktop linux here. was that needed and intentional? if so, please mention in the changelist description that you're doing it. https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_param... ui/gfx/font_render_params_linux.cc:210: if (actual_query.device_scale_factor == 0) just move this test out of the #ifdefs: if (actual_query.device_scale_factor == 0) { #if defined(OS_CHROMEOS) actual_query.device_scale_factor = ...; #else actual_query.device_scale_factor = gfx::Screen::GetNativeScreen()-> GetPrimaryDisplay().device_scale_factor(); #endif } https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_param... ui/gfx/font_render_params_linux.cc:214: // path than on ChromeOS to figure out the device scale factor. nit: s/ChromeOS/Chrome OS/ https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_param... ui/gfx/font_render_params_linux.cc:217: if (screen) { does gfx::Screen::GetNativeScreen() ever return null? it doesn't look like it.
Thanks for the review! PTAL. https://codereview.chromium.org/929733002/diff/20001/chrome/browser/ui/aura/c... File chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc (right): https://codereview.chromium.org/929733002/diff/20001/chrome/browser/ui/aura/c... chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc:125: { On 2015/03/16 13:57:53, Daniel Erat (OOO to Thursday) wrote: > the code that creates LinuxUI in > chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc looks like it's > hidden behind this: > > #if defined(USE_X11) && !defined(OS_CHROMEOS) > if (GetInitialDesktop() != chrome::HOST_DESKTOP_TYPE_ASH) { > > this is already behind !OS_CHROMEOS, but it seems like you should have the rest > of it here. Done. https://codereview.chromium.org/929733002/diff/20001/chrome/browser/ui/aura/c... chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc:128: views::LinuxUI *gtk2_ui = views::LinuxUI::instance(); On 2015/03/16 13:57:53, Daniel Erat (OOO to Thursday) wrote: > nit: '*' goes to left of space Done. https://codereview.chromium.org/929733002/diff/20001/chrome/browser/ui/aura/c... chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc:129: CHECK(gtk2_ui); On 2015/03/16 13:57:53, Daniel Erat (OOO to Thursday) wrote: > i'd just inline the views::LinuxUI::instance() where you use it below to keep > the code simpler; it doesn't look like this should ever be null. Done. https://codereview.chromium.org/929733002/diff/20001/chrome/browser/ui/aura/c... chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc:131: CHECK(display.is_valid()); On 2015/03/16 13:57:53, Daniel Erat (OOO to Thursday) wrote: > i don't see how it can ever be invalid. Done. https://codereview.chromium.org/929733002/diff/20001/chrome/browser/ui/libgtk... File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right): https://codereview.chromium.org/929733002/diff/20001/chrome/browser/ui/libgtk... chrome/browser/ui/libgtk2ui/gtk2_ui.cc:700: gfx::FontRenderParams* params_out) { On 2015/03/16 13:57:53, Daniel Erat (OOO to Thursday) wrote: > can't this still be const? it doesn't look like you made any changes to it. You’re right. I forgot to change this back after refactoring. Done. https://codereview.chromium.org/929733002/diff/20001/chrome/browser/ui/libgtk... File chrome/browser/ui/libgtk2ui/gtk2_ui.h (right): https://codereview.chromium.org/929733002/diff/20001/chrome/browser/ui/libgtk... chrome/browser/ui/libgtk2ui/gtk2_ui.h:28: typedef struct _PangoFontDescription PangoFontDescription; On 2015/03/16 13:57:53, Daniel Erat (OOO to Thursday) wrote: > you might want to rebase or merge your change against tip-of-tree -- i think > that this typedef has already been present for a few weeks. Done. https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_param... File ui/gfx/font_render_params_linux.cc (right): https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_param... ui/gfx/font_render_params_linux.cc:69: return query.device_scale_factor > 1.0f; On 2015/03/16 13:57:53, Daniel Erat (OOO to Thursday) wrote: > it looks like you're enabling subpixel positioning for desktop linux here. was > that needed and intentional? if so, please mention in the changelist description > that you're doing it. Yeah, enabling this on HiDPi displays is what Chrome OS does, and the results indeed look nicer (at least on my screen and to me): http://t.zekjur.net/without-subpixel.png http://t.zekjur.net/with-subpixel.png https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_param... ui/gfx/font_render_params_linux.cc:210: if (actual_query.device_scale_factor == 0) On 2015/03/16 13:57:54, Daniel Erat (OOO to Thursday) wrote: > just move this test out of the #ifdefs: > > if (actual_query.device_scale_factor == 0) { > #if defined(OS_CHROMEOS) > actual_query.device_scale_factor = ...; > #else > actual_query.device_scale_factor = > gfx::Screen::GetNativeScreen()-> > GetPrimaryDisplay().device_scale_factor(); > #endif > } Done. https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_param... ui/gfx/font_render_params_linux.cc:214: // path than on ChromeOS to figure out the device scale factor. On 2015/03/16 13:57:54, Daniel Erat (OOO to Thursday) wrote: > nit: s/ChromeOS/Chrome OS/ Done. https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_param... ui/gfx/font_render_params_linux.cc:217: if (screen) { On 2015/03/16 13:57:54, Daniel Erat (OOO to Thursday) wrote: > does gfx::Screen::GetNativeScreen() ever return null? it doesn't look like it. Yes, it can. I’m getting the following segmentation fault when removing the check: #0 0x0000555556abe6bb in gfx::GetFontRenderParams(gfx::FontRenderParamsQuery const&, std::string*) () #1 0x000055555903b23a in libgtk2ui::Gtk2UI::UpdateDefaultFont(_PangoFontDescription const*) () #2 0x0000555559038b45 in libgtk2ui::Gtk2UI::LoadGtkValues() () #3 0x00005555590388dd in libgtk2ui::Gtk2UI::Initialize() () #4 0x00005555581df402 in ChromeBrowserMainExtraPartsAura::ToolkitInitialized() () #5 0x00005555562a50fa in ChromeBrowserMainParts::ToolkitInitialized() () #6 0x0000555558704548 in content::BrowserMainLoop::InitializeToolkit() () #7 0x0000555558524173 in content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) () #8 0x0000555558523ebd in content::BrowserMain(content::MainFunctionParams const&) () #9 0x0000555556521875 in content::ContentMainRunnerImpl::Run() () #10 0x0000555556520590 in content::ContentMain(content::ContentMainParams const&) () #11 0x0000555555fc62f8 in ChromeMain () #12 0x00007ffff1765fe0 in __libc_start_main () at /lib64/libc.so.6 #13 0x0000555555fc61af in _start ()
https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_param... File ui/gfx/font_render_params_linux.cc (right): https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_param... ui/gfx/font_render_params_linux.cc:217: if (screen) { On 2015/03/17 08:36:34, stapelberg wrote: > On 2015/03/16 13:57:54, Daniel Erat (OOO to Thursday) wrote: > > does gfx::Screen::GetNativeScreen() ever return null? it doesn't look like it. > > Yes, it can. I’m getting the following segmentation fault when removing the > check: > > #0 0x0000555556abe6bb in gfx::GetFontRenderParams(gfx::FontRenderParamsQuery > const&, std::string*) () > #1 0x000055555903b23a in > libgtk2ui::Gtk2UI::UpdateDefaultFont(_PangoFontDescription const*) () > #2 0x0000555559038b45 in libgtk2ui::Gtk2UI::LoadGtkValues() () > #3 0x00005555590388dd in libgtk2ui::Gtk2UI::Initialize() () > #4 0x00005555581df402 in ChromeBrowserMainExtraPartsAura::ToolkitInitialized() > () > #5 0x00005555562a50fa in ChromeBrowserMainParts::ToolkitInitialized() () > #6 0x0000555558704548 in content::BrowserMainLoop::InitializeToolkit() () > #7 0x0000555558524173 in > content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) > () > #8 0x0000555558523ebd in content::BrowserMain(content::MainFunctionParams > const&) () > #9 0x0000555556521875 in content::ContentMainRunnerImpl::Run() () > #10 0x0000555556520590 in content::ContentMain(content::ContentMainParams > const&) () > #11 0x0000555555fc62f8 in ChromeMain () > #12 0x00007ffff1765fe0 in __libc_start_main () at /lib64/libc.so.6 > #13 0x0000555555fc61af in _start () Doesn't this mean that this code inside block will never be executed?
https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_param... File ui/gfx/font_render_params_linux.cc (right): https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_param... ui/gfx/font_render_params_linux.cc:217: if (screen) { On 2015/03/17 19:59:02, oshima wrote: > On 2015/03/17 08:36:34, stapelberg wrote: > > On 2015/03/16 13:57:54, Daniel Erat (OOO to Thursday) wrote: > > > does gfx::Screen::GetNativeScreen() ever return null? it doesn't look like > it. > > > > Yes, it can. I’m getting the following segmentation fault when removing the > > check: > > > > #0 0x0000555556abe6bb in gfx::GetFontRenderParams(gfx::FontRenderParamsQuery > > const&, std::string*) () > > #1 0x000055555903b23a in > > libgtk2ui::Gtk2UI::UpdateDefaultFont(_PangoFontDescription const*) () > > #2 0x0000555559038b45 in libgtk2ui::Gtk2UI::LoadGtkValues() () > > #3 0x00005555590388dd in libgtk2ui::Gtk2UI::Initialize() () > > #4 0x00005555581df402 in > ChromeBrowserMainExtraPartsAura::ToolkitInitialized() > > () > > #5 0x00005555562a50fa in ChromeBrowserMainParts::ToolkitInitialized() () > > #6 0x0000555558704548 in content::BrowserMainLoop::InitializeToolkit() () > > #7 0x0000555558524173 in > > content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) > > () > > #8 0x0000555558523ebd in content::BrowserMain(content::MainFunctionParams > > const&) () > > #9 0x0000555556521875 in content::ContentMainRunnerImpl::Run() () > > #10 0x0000555556520590 in content::ContentMain(content::ContentMainParams > > const&) () > > #11 0x0000555555fc62f8 in ChromeMain () > > #12 0x00007ffff1765fe0 in __libc_start_main () at /lib64/libc.so.6 > > #13 0x0000555555fc61af in _start () > > Doesn't this mean that this code inside block will never be executed? No. When adding a LOG statement before line 218, I see that statement being executed plenty of times.
https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_param... File ui/gfx/font_render_params_linux.cc (right): https://codereview.chromium.org/929733002/diff/20001/ui/gfx/font_render_param... ui/gfx/font_render_params_linux.cc:217: if (screen) { On 2015/03/18 08:17:53, stapelberg wrote: > On 2015/03/17 19:59:02, oshima wrote: > > On 2015/03/17 08:36:34, stapelberg wrote: > > > On 2015/03/16 13:57:54, Daniel Erat (OOO to Thursday) wrote: > > > > does gfx::Screen::GetNativeScreen() ever return null? it doesn't look like > > it. > > > > > > Yes, it can. I’m getting the following segmentation fault when removing the > > > check: > > > > > > #0 0x0000555556abe6bb in > gfx::GetFontRenderParams(gfx::FontRenderParamsQuery > > > const&, std::string*) () > > > #1 0x000055555903b23a in > > > libgtk2ui::Gtk2UI::UpdateDefaultFont(_PangoFontDescription const*) () > > > #2 0x0000555559038b45 in libgtk2ui::Gtk2UI::LoadGtkValues() () > > > #3 0x00005555590388dd in libgtk2ui::Gtk2UI::Initialize() () > > > #4 0x00005555581df402 in > > ChromeBrowserMainExtraPartsAura::ToolkitInitialized() > > > () > > > #5 0x00005555562a50fa in ChromeBrowserMainParts::ToolkitInitialized() () > > > #6 0x0000555558704548 in content::BrowserMainLoop::InitializeToolkit() () > > > #7 0x0000555558524173 in > > > content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams > const&) > > > () > > > #8 0x0000555558523ebd in content::BrowserMain(content::MainFunctionParams > > > const&) () > > > #9 0x0000555556521875 in content::ContentMainRunnerImpl::Run() () > > > #10 0x0000555556520590 in content::ContentMain(content::ContentMainParams > > > const&) () > > > #11 0x0000555555fc62f8 in ChromeMain () > > > #12 0x00007ffff1765fe0 in __libc_start_main () at /lib64/libc.so.6 > > > #13 0x0000555555fc61af in _start () > > > > Doesn't this mean that this code inside block will never be executed? > > No. When adding a LOG statement before line 218, I see that statement being > executed plenty of times. Oh, sorry, I was searching for different method name. https://codereview.chromium.org/929733002/diff/40001/chrome/browser/ui/aura/c... File chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc (right): https://codereview.chromium.org/929733002/diff/40001/chrome/browser/ui/aura/c... chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc:113: is it possible to move the gfx::screen creation here and remove null check in GetFontRenderParams?
https://codereview.chromium.org/929733002/diff/40001/chrome/browser/ui/aura/c... File chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc (right): https://codereview.chromium.org/929733002/diff/40001/chrome/browser/ui/aura/c... chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc:113: On 2015/03/18 21:50:46, oshima wrote: > is it possible to move the gfx::screen creation here and remove null check in > GetFontRenderParams? No. If I do that, Chromium will segfault with the following backtrace: #0 0x00005555568f2f47 in ui::GetSupportedScaleFactor(float) () #1 0x0000555558ce4f7f in (anonymous namespace)::GetFallbackDisplayList() () #2 0x0000555558ce48b0 in views::DesktopScreenX11::DesktopScreenX11() () #3 0x0000555558ce5c66 in views::CreateDesktopScreen() () #4 0x00005555581df465 in ChromeBrowserMainExtraPartsAura::ToolkitInitialized() () #5 0x00005555562a50fa in ChromeBrowserMainParts::ToolkitInitialized() () #6 0x0000555558704568 in content::BrowserMainLoop::InitializeToolkit() () #7 0x0000555558524193 in content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) () #8 0x0000555558523edd in content::BrowserMain(content::MainFunctionParams const&) () #9 0x0000555556521875 in content::ContentMainRunnerImpl::Run() () #10 0x0000555556520590 in content::ContentMain(content::ContentMainParams const&) () #11 0x0000555555fc62f8 in ChromeMain () #12 0x00007ffff1765fe0 in __libc_start_main () at /lib64/libc.so.6 #13 0x0000555555fc61af in _start () I think SetSupportedScaleFactor (called from ResourceBundle::InitSharedInstance() has not yet been called at that point.
On 2015/03/19 07:54:52, stapelberg wrote: > https://codereview.chromium.org/929733002/diff/40001/chrome/browser/ui/aura/c... > File chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc (right): > > https://codereview.chromium.org/929733002/diff/40001/chrome/browser/ui/aura/c... > chrome/browser/ui/aura/chrome_browser_main_extra_parts_aura.cc:113: > On 2015/03/18 21:50:46, oshima wrote: > > is it possible to move the gfx::screen creation here and remove null check in > > GetFontRenderParams? > > No. If I do that, Chromium will segfault with the following backtrace: > > #0 0x00005555568f2f47 in ui::GetSupportedScaleFactor(float) () > #1 0x0000555558ce4f7f in (anonymous namespace)::GetFallbackDisplayList() () > #2 0x0000555558ce48b0 in views::DesktopScreenX11::DesktopScreenX11() () > #3 0x0000555558ce5c66 in views::CreateDesktopScreen() () > #4 0x00005555581df465 in ChromeBrowserMainExtraPartsAura::ToolkitInitialized() > () > #5 0x00005555562a50fa in ChromeBrowserMainParts::ToolkitInitialized() () > #6 0x0000555558704568 in content::BrowserMainLoop::InitializeToolkit() () > #7 0x0000555558524193 in > content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) > () > #8 0x0000555558523edd in content::BrowserMain(content::MainFunctionParams > const&) () > #9 0x0000555556521875 in content::ContentMainRunnerImpl::Run() () > #10 0x0000555556520590 in content::ContentMain(content::ContentMainParams > const&) () > #11 0x0000555555fc62f8 in ChromeMain () > #12 0x00007ffff1765fe0 in __libc_start_main () at /lib64/libc.so.6 > #13 0x0000555555fc61af in _start () > > I think SetSupportedScaleFactor (called from > ResourceBundle::InitSharedInstance() has not yet been called at that point. ok, lgtm my bits for now. I think the initialization order has to be fixed/improved, but that's not your fault. I'll look into it but you can move ahead with derat's review.
https://codereview.chromium.org/929733002/diff/40001/ui/gfx/linux_font_delega... File ui/gfx/linux_font_delegate.h (right): https://codereview.chromium.org/929733002/diff/40001/ui/gfx/linux_font_delega... 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_lin... File ui/gfx/platform_font_linux.cc (right): https://codereview.chromium.org/929733002/diff/40001/ui/gfx/platform_font_lin... ui/gfx/platform_font_linux.cc:94: LinuxFontDelegate* delegate = LinuxFontDelegate::instance(); you should be able to preserve the 'const' here
stapelberg@google.com changed reviewers: + msw@chromium.org
https://codereview.chromium.org/929733002/diff/40001/ui/gfx/linux_font_delega... File ui/gfx/linux_font_delegate.h (right): https://codereview.chromium.org/929733002/diff/40001/ui/gfx/linux_font_delega... ui/gfx/linux_font_delegate.h:31: static LinuxFontDelegate* instance(); On 2015/03/19 17:11:44, Daniel Erat (OOO to Thursday) wrote: > this can remain const, no? Done. https://codereview.chromium.org/929733002/diff/40001/ui/gfx/platform_font_lin... File ui/gfx/platform_font_linux.cc (right): https://codereview.chromium.org/929733002/diff/40001/ui/gfx/platform_font_lin... ui/gfx/platform_font_linux.cc:94: LinuxFontDelegate* delegate = LinuxFontDelegate::instance(); On 2015/03/19 17:11:44, Daniel Erat (OOO to Thursday) wrote: > you should be able to preserve the 'const' here Done.
thanks! lgtm
lgtm with a nit. https://codereview.chromium.org/929733002/diff/60001/ui/gfx/font_render_param... File ui/gfx/font_render_params_linux.cc (right): https://codereview.chromium.org/929733002/diff/60001/ui/gfx/font_render_param... ui/gfx/font_render_params_linux.cc:67: bool IsBrowserTextSubpixelPositioningEnabled( nit: maybe inline this now?
ui/views lgtm
https://codereview.chromium.org/929733002/diff/60001/ui/gfx/font_render_param... File ui/gfx/font_render_params_linux.cc (right): https://codereview.chromium.org/929733002/diff/60001/ui/gfx/font_render_param... ui/gfx/font_render_params_linux.cc:67: bool IsBrowserTextSubpixelPositioningEnabled( On 2015/03/19 19:30:43, msw wrote: > nit: maybe inline this now? Done.
The CQ bit was checked by stapelberg@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, derat@chromium.org, msw@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/929733002/#ps80001 (title: "Fix Pango font rendering with HiDPi displays on Linux.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/929733002/80001
The CQ bit was checked by stapelberg@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/929733002/80001
The CQ bit was checked by stapelberg@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org, oshima@chromium.org, msw@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/929733002/#ps100001 (title: "Fix Pango font rendering with HiDPi displays on Linux.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/929733002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/aa051ee8d1a7ff725225d783d1a96663d5aedb08 Cr-Commit-Position: refs/heads/master@{#321457} |