Don't check lfQuality in LOGFONT as it has no effect on rendering

Source/platform/fonts/win/FontPlatformDataWin.cpp

 /*
  * Copyright (C) 2006, 2007 Apple Computer, Inc.
  * Copyright (c) 2006, 2007, 2008, 2009, 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
@@ skipping 67 matching lines @@
     SkASSERT(!(textFlags & ~textFlagsMask));
     uint32_t flags = paint->getFlags();
     flags &= ~textFlagsMask;
     flags |= textFlags;
     paint->setFlags(flags);
 }
 
 // Lookup the current system settings for font smoothing.
 // We cache these values for performance, but if the browser has a way to be
 // notified when these change, we could re-query them at that time.
-static uint32_t getDefaultGDITextFlags()
+static uint32_t getSystemTextFlags()

[bungeman-chromium, 2013/12/09 15:29:58]

I have no real issue with this renaming, but note that DirectWrite actually uses
a different set of settings (which can actually vary per monitor/display).

 {
     static bool gInited;
     static uint32_t gFlags;
     if (!gInited) {
         BOOL enabled;
         gFlags = 0;
         if (SystemParametersInfo(SPI_GETFONTSMOOTHING, 0, &enabled, 0) && enabled) {
             gFlags |= SkPaint::kAntiAlias_Flag;
 
             UINT smoothType;
             if (SystemParametersInfo(SPI_GETFONTSMOOTHINGTYPE, 0, &smoothType, 0)) {
                 if (FE_FONTSMOOTHINGCLEARTYPE == smoothType)
                     gFlags |= SkPaint::kLCDRenderText_Flag;
             }
         }
         gInited = true;
     }
     return gFlags;
 }
 
-static bool isWebFont(const LOGFONT& lf)
+static bool isWebFont(const String& familyName)
 {
-    // web-fonts have artifical names constructed to always be
+    // Web-fonts have artifical names constructed to always be:
     // 1. 24 characters, followed by a '\0'
     // 2. the last two characters are '=='
-    return '=' == lf.lfFaceName[22] && '=' == lf.lfFaceName[23] && '\0' == lf.lfFaceName[24];
+    return familyName.length() == 24
+        && '=' == familyName[22] && '=' == familyName[23];
 }
 
-static int computePaintTextFlags(const LOGFONT& lf)
+int FontPlatformData::paintTextFlags() const
 {
-    int textFlags = 0;
-    switch (lf.lfQuality) {
-    case NONANTIALIASED_QUALITY:
-        textFlags = 0;
-        break;
-    case ANTIALIASED_QUALITY:
-        textFlags = SkPaint::kAntiAlias_Flag;
-        break;
-    case CLEARTYPE_QUALITY:
-        textFlags = (SkPaint::kAntiAlias_Flag | SkPaint::kLCDRenderText_Flag);
-        break;
-    default:
-        textFlags = getDefaultGDITextFlags();
-        break;
-    }
+    int textFlags = getSystemTextFlags();
 
-    // only allow features that SystemParametersInfo allows
-    textFlags &= getDefaultGDITextFlags();
-
-    /*
-     *  FontPlatformData(...) will read our logfont, and try to honor the the lfQuality
-     *  setting (computing the corresponding SkPaint flags for AA and LCD). However, it
-     *  will limit the quality based on its query of SPI_GETFONTSMOOTHING. This could mean
-     *  we end up drawing the text in BW, even though our lfQuality requested antialiasing.
-     *
-     *  Many web-fonts are so poorly hinted that they are terrible to read when drawn in BW.
-     *  In these cases, we have decided to FORCE these fonts to be drawn with at least grayscale AA,
-     *  even when the System (getDefaultGDITextFlags) tells us to draw only in BW.
-     */
-    if (isWebFont(lf) && !isRunningLayoutTest())
+    // Many web-fonts are so poorly hinted that they are terrible to read when drawn in BW.
+    // In these cases, we have decided to FORCE these fonts to be drawn with at least grayscale AA,
+    // even when the System (getSystemTextFlags) tells us to draw only in BW.
+    if (isWebFont(fontFamilyName()) && !isRunningLayoutTest())

[bungeman-chromium, 2014/02/10 20:59:57]

Hopefully getting the family name here doesn't take too long. It seems like
before we were going off the logfont data which was already in hand as opposed
to going off and finding the name of the font.

[bungeman-skia, 2014/02/10 23:16:15]

Yep, this is it. Doing the work of actually digging out the name each time we
create the paint (basically every measure and draw for each bit of text) is
taking up all of the time. I ran

python tools/perf/run_benchmark -v --browser=release rasterize_and_record.top_25

locally at tip of tree with and without this 'if' (and the line below) commented
out. When commented out, the record_time is cut in half.

         textFlags |= SkPaint::kAntiAlias_Flag;
     return textFlags;
 }
 
 #if !USE(HARFBUZZ)
-PassRefPtr<SkTypeface> CreateTypefaceFromHFont(HFONT hfont, int* size, int* paintTextFlags)
+PassRefPtr<SkTypeface> CreateTypefaceFromHFont(HFONT hfont, int* size)
 {
     LOGFONT info;
     GetObject(hfont, sizeof(info), &info);
     if (size) {
         int height = info.lfHeight;
         if (height < 0)
             height = -height;
         *size = height;
     }
-    if (paintTextFlags)
-        *paintTextFlags = computePaintTextFlags(info);
     return adoptRef(SkCreateTypefaceFromLOGFONT(info));
 }
 #endif
 
 FontPlatformData::FontPlatformData(WTF::HashTableDeletedValueType)
     : m_textSize(-1)
     , m_fakeBold(false)
     , m_fakeItalic(false)
     , m_orientation(Horizontal)
     , m_typeface(adoptRef(SkTypeface::RefDefault()))
-    , m_paintTextFlags(0)
     , m_isHashTableDeletedValue(true)
     , m_useSubpixelPositioning(false)
 {
 #if !USE(HARFBUZZ)
     m_font = 0;
     m_scriptCache = 0;
 #endif
 }
 
 FontPlatformData::FontPlatformData()
     : m_textSize(0)
     , m_fakeBold(false)
     , m_fakeItalic(false)
     , m_orientation(Horizontal)
     , m_typeface(adoptRef(SkTypeface::RefDefault()))
-    , m_paintTextFlags(0)

[bungeman-chromium, 2014/02/10 20:59:57]

It seems like the paintTextFlags were often 0 (monochrome) in this older code.
This isn't true in the new code, not sure if that affects anything.

     , m_isHashTableDeletedValue(false)
     , m_useSubpixelPositioning(false)
 {
 #if !USE(HARFBUZZ)
     m_font = 0;
     m_scriptCache = 0;
 #endif
 }
 
 #if ENABLE(GDI_FONTS_ON_WINDOWS) && !USE(HARFBUZZ)
 FontPlatformData::FontPlatformData(HFONT font, float size, FontOrientation orientation)
     : m_font(RefCountedHFONT::create(font))
     , m_textSize(size)
     , m_fakeBold(false)
     , m_fakeItalic(false)
     , m_orientation(orientation)
     , m_scriptCache(0)
-    , m_typeface(CreateTypefaceFromHFont(font, 0, &m_paintTextFlags))
+    , m_typeface(CreateTypefaceFromHFont(font, 0))
     , m_isHashTableDeletedValue(false)
     , m_useSubpixelPositioning(false)
 {
 }
 #endif
 
 // FIXME: this constructor is needed for SVG fonts but doesn't seem to do much
 FontPlatformData::FontPlatformData(float size, bool bold, bool oblique)
     : m_textSize(size)
     , m_fakeBold(false)
     , m_fakeItalic(false)
     , m_orientation(Horizontal)
     , m_typeface(adoptRef(SkTypeface::RefDefault()))
-    , m_paintTextFlags(0)
     , m_isHashTableDeletedValue(false)
     , m_useSubpixelPositioning(false)
 {
 #if !USE(HARFBUZZ)
     m_font = 0;
     m_scriptCache = 0;
 #endif
 }
 
 FontPlatformData::FontPlatformData(const FontPlatformData& data)
     : m_textSize(data.m_textSize)
     , m_fakeBold(data.m_fakeBold)
     , m_fakeItalic(data.m_fakeItalic)
     , m_orientation(data.m_orientation)
     , m_typeface(data.m_typeface)
-    , m_paintTextFlags(data.m_paintTextFlags)
     , m_isHashTableDeletedValue(false)
     , m_useSubpixelPositioning(data.m_useSubpixelPositioning)
 {
 #if !USE(HARFBUZZ)
     m_font = data.m_font;
     m_scriptCache = 0;
 #endif
 }
 
 FontPlatformData::FontPlatformData(const FontPlatformData& data, float textSize)
     : m_textSize(textSize)
     , m_fakeBold(data.m_fakeBold)
     , m_fakeItalic(data.m_fakeItalic)
     , m_orientation(data.m_orientation)
     , m_typeface(data.m_typeface)
-    , m_paintTextFlags(data.m_paintTextFlags)
     , m_isHashTableDeletedValue(false)
     , m_useSubpixelPositioning(data.m_useSubpixelPositioning)
 {
 #if !USE(HARFBUZZ)
     m_font = data.m_font;
     m_scriptCache = 0;
 #endif
 }
 
 FontPlatformData::FontPlatformData(PassRefPtr<SkTypeface> tf, const char* family,
     float textSize, bool fakeBold, bool fakeItalic, FontOrientation orientation,
     bool useSubpixelPositioning)
     : m_textSize(textSize)
     , m_fakeBold(fakeBold)
     , m_fakeItalic(fakeItalic)
     , m_orientation(orientation)
     , m_typeface(tf)
     , m_isHashTableDeletedValue(false)
     , m_useSubpixelPositioning(useSubpixelPositioning)
 {
     // FIXME: This can be removed together with m_font once the last few
     // uses of hfont() has been eliminated.
     LOGFONT logFont;
     SkLOGFONTFromTypeface(m_typeface.get(), &logFont);
     logFont.lfHeight = -textSize;
-    m_paintTextFlags = computePaintTextFlags(logFont);

[bungeman-chromium, 2014/02/10 20:59:57]

Note that here 'logFont' would always be CLEARTYPE_QUALITY since that's the
canonical quality. The typeface itself doesn't care how it would be rendered,
that's up to a paint.

Note that after this line is removed, the previous three lines may be moved into
the #if below, since 'logFont' is no longer being used here. (I suppose that
means the FIXME can be fixed.)

 
 #if !USE(HARFBUZZ)
     HFONT hFont = CreateFontIndirect(&logFont);
     m_font = hFont ? RefCountedHFONT::create(hFont) : 0;
     m_scriptCache = 0;
 #endif
 }
 
 FontPlatformData& FontPlatformData::operator=(const FontPlatformData& data)
 {
     if (this != &data) {
         m_textSize = data.m_textSize;
         m_fakeBold = data.m_fakeBold;
         m_fakeItalic = data.m_fakeItalic;
         m_orientation = data.m_orientation;
         m_typeface = data.m_typeface;
-        m_paintTextFlags = data.m_paintTextFlags;
 
 #if !USE(HARFBUZZ)
         m_font = data.m_font;
         // The following fields will get re-computed if necessary.
         ScriptFreeCache(&m_scriptCache);
         m_scriptCache = 0;
         m_scriptFontProperties.clear();
 #endif
     }
     return *this;
@@ skipping 12 matching lines @@
 #if ENABLE(GDI_FONTS_ON_WINDOWS)
     HWndDC dc(0);
     HGDIOBJ oldFont = static_cast<HFONT>(SelectObject(dc, hfont()));
     WCHAR name[LF_FACESIZE];
     unsigned resultLength = GetTextFace(dc, LF_FACESIZE, name);
     if (resultLength > 0)
         resultLength--; // ignore the null terminator
     SelectObject(dc, oldFont);
     return String(name, resultLength);
 #else
     // FIXME: This returns the requested name, perhaps a better solution would be to

[bungeman-chromium, 2014/02/10 20:59:57]

Actually, this FIXME isn't true anymore on GDI. Under the hood
LogFontTypeface::getFamilyName does basically the same as was done above (call
GetTextFace on an HFONT).

     // return the list of names provided by SkTypeface::createFamilyNameIterator.
     ASSERT(typeface());
     SkString familyName;
     typeface()->getFamilyName(&familyName);
     return String::fromUTF8(familyName.c_str());
 #endif
 }
 
 bool FontPlatformData::isFixedPitch() const
 {
@@ skipping 87 matching lines @@
 #endif
 
 #ifndef NDEBUG
 String FontPlatformData::description() const
 {
     return String();
 }
 #endif
 
 }