Fix Pango font rendering with HiDPi displays on Linux.

ui/gfx/font_render_params_linux.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/gfx/font_render_params.h"
 
 #include <fontconfig/fontconfig.h>
 
 #include "base/command_line.h"
 #include "base/containers/mru_cache.h"
 #include "base/hash.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
 #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 {
 
 namespace {
 
 #if defined(OS_CHROMEOS)
 // A device scale factor for an internal display (if any)
 // that is used to determine if subpixel positioning should be used.
 float device_scale_factor_for_internal_display = 1.0f;
@@ skipping 26 matching lines @@
 
   base::Lock lock;
   Cache cache;
 };
 
 base::LazyInstance<SynchronizedCache>::Leaky g_synchronized_cache =
     LAZY_INSTANCE_INITIALIZER;
 
 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.
 FontRenderParams::Hinting ConvertFontconfigHintStyle(int hint_style) {
   switch (hint_style) {
     case FC_HINT_SLIGHT: return FontRenderParams::HINTING_SLIGHT;
     case FC_HINT_MEDIUM: return FontRenderParams::HINTING_MEDIUM;
     case FC_HINT_FULL:   return FontRenderParams::HINTING_FULL;
     default:             return FontRenderParams::HINTING_NONE;
   }
@@ skipping 120 matching lines @@
       query.point_size, query.style, JoinString(query.families, ',').c_str(),
       query.device_scale_factor));
 }
 
 }  // namespace
 
 FontRenderParams GetFontRenderParams(const FontRenderParamsQuery& query,
                                      std::string* family_out) {
   FontRenderParamsQuery actual_query(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();
 
   {
     // Try to find a cached result so Fontconfig doesn't need to be queried.
     base::AutoLock lock(synchronized_cache->lock);
     Cache::const_iterator it = synchronized_cache->cache.Get(hash);
     if (it != synchronized_cache->cache.end()) {
       DVLOG(1) << "Returning cached params for " << hash;
@@ skipping 60 matching lines @@
 float GetFontRenderParamsDeviceScaleFactor() {
   return device_scale_factor_for_internal_display;
 }
 
 void SetFontRenderParamsDeviceScaleFactor(float device_scale_factor) {
   device_scale_factor_for_internal_display = device_scale_factor;
 }
 #endif
 
 }  // namespace gfx