Fix Pango font rendering with HiDPi displays on Linux.

ui/gfx/font_render_params_linux.cc

Index: ui/gfx/font_render_params_linux.cc
diff --git a/ui/gfx/font_render_params_linux.cc b/ui/gfx/font_render_params_linux.cc
index 5698f0d76cd08ffb85d42092918fdc5ed7051d38..c1dfb1b1c323f27b768e6ebf552c8670b7e39897 100644
--- a/ui/gfx/font_render_params_linux.cc
+++ b/ui/gfx/font_render_params_linux.cc
@@ -16,8 +16,10 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/synchronization/lock.h"
+#include "ui/gfx/display.h"
 #include "ui/gfx/font.h"
 #include "ui/gfx/linux_font_delegate.h"
+#include "ui/gfx/screen.h"
 #include "ui/gfx/switches.h"
 
 namespace gfx {
@@ -64,11 +66,7 @@ base::LazyInstance<SynchronizedCache>::Leaky g_synchronized_cache =
 
 bool IsBrowserTextSubpixelPositioningEnabled(
     const FontRenderParamsQuery& query) {
-#if defined(OS_CHROMEOS)
   return query.device_scale_factor > 1.0f;

[Daniel Erat, 2015/03/16 13:57:53]

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.

[stapelberg, 2015/03/17 08:36:34]

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

-#else
-  return false;
-#endif
 }
 
 // Converts Fontconfig FC_HINT_STYLE to FontRenderParams::Hinting.
@@ -211,6 +209,16 @@ FontRenderParams GetFontRenderParams(const FontRenderParamsQuery& query,
 #if defined(OS_CHROMEOS)
   if (actual_query.device_scale_factor == 0)

[Daniel Erat, 2015/03/16 13:57:54]

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
}

[stapelberg, 2015/03/17 08:36:34]

Done.

     actual_query.device_scale_factor = device_scale_factor_for_internal_display;
+#else
+  // Linux does not support per-display DPI, so we use a slightly simpler code
+  // path than on ChromeOS to figure out the device scale factor.

[Daniel Erat, 2015/03/16 13:57:54]

nit: s/ChromeOS/Chrome OS/

[stapelberg, 2015/03/17 08:36:33]

Done.

+  if (actual_query.device_scale_factor == 0) {
+    gfx::Screen* screen = gfx::Screen::GetNativeScreen();
+    if (screen) {

[Daniel Erat, 2015/03/16 13:57:54]

does gfx::Screen::GetNativeScreen() ever return null? it doesn't look like it.

[stapelberg, 2015/03/17 08:36:34]

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 ()

[oshima, 2015/03/17 19:59:02]

Doesn't this mean that this code inside block will never be executed?

[stapelberg, 2015/03/18 08:17:53]

No. When adding a LOG statement before line 218, I see that statement being
executed plenty of times.

[oshima, 2015/03/18 21:50:45]

Oh, sorry, I was searching for different method name.

+      gfx::Display display = screen->GetPrimaryDisplay();
+      actual_query.device_scale_factor = display.device_scale_factor();
+    }
+  }
 #endif
   const uint32 hash = HashFontRenderParamsQuery(actual_query);
   SynchronizedCache* synchronized_cache = g_synchronized_cache.Pointer();