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

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

Remove the LOGFONT lfQuality check in computePaintTextFlags and instead
base the textFlags entierly on the system ClearType settings (and
whether a font is a web font). Skia will then use this information to
pick the appropriate type of font smoothing to apply (if any).

BUG=249099
R=bungeman@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163507

[eae, 2013-12-09 04:27:59]

This change is based on my understanding of how the flag is used after we
discussed it a couple of weeks ago. Please do verify that my assumptions are
correct.

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

lgtm

https://codereview.chromium.org/99333013/diff/80001/Source/platform/fonts/win...
File Source/platform/fonts/win/FontPlatformDataWin.cpp (right):

https://codereview.chromium.org/99333013/diff/80001/Source/platform/fonts/win...
Source/platform/fonts/win/FontPlatformDataWin.cpp:88: static uint32_t
getSystemTextFlags()
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).

[eae, 2013-12-09 22:21:20]

On 2013/12/09 15:29:58, bungeman2 wrote:
> lgtm
> 
>
https://codereview.chromium.org/99333013/diff/80001/Source/platform/fonts/win...
> File Source/platform/fonts/win/FontPlatformDataWin.cpp (right):
> 
>
https://codereview.chromium.org/99333013/diff/80001/Source/platform/fonts/win...
> Source/platform/fonts/win/FontPlatformDataWin.cpp:88: static uint32_t
> getSystemTextFlags()
> 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).

My plan is to use this setting for both for now and then move us over to the DW
specific setting further down the road. How does that sound to you? This also
seems to mirror what FF is doing.

[commit-bot: I haz the power, 2013-12-09 22:22:24]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[bungeman-chromium, 2013-12-09 22:24:25]

On 2013/12/09 22:21:20, eae wrote:
> On 2013/12/09 15:29:58, bungeman2 wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/99333013/diff/80001/Source/platform/fonts/win...
> > File Source/platform/fonts/win/FontPlatformDataWin.cpp (right):
> > 
> >
>
https://codereview.chromium.org/99333013/diff/80001/Source/platform/fonts/win...
> > Source/platform/fonts/win/FontPlatformDataWin.cpp:88: static uint32_t
> > getSystemTextFlags()
> > 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).
> 
> My plan is to use this setting for both for now and then move us over to the
DW
> specific setting further down the road. How does that sound to you? This also
> seems to mirror what FF is doing.

Sounds fine to me.

[eae, 2013-12-09 23:01:40]

Need r+ from Blink committer.

[tasak, 2013-12-10 05:26:31]

lgtm

[eae, 2013-12-10 05:35:59]

Thanks!

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-12-10 05:36:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eae@chromium.org/99333013/80001

[commit-bot: I haz the power, 2013-12-10 07:10:36]

Change committed as 163507

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

Some initial comments speculating as to why this may have caused a slowdown.

https://codereview.chromium.org/99333013/diff/80001/Source/platform/fonts/win...
File Source/platform/fonts/win/FontPlatformDataWin.cpp (left):

https://codereview.chromium.org/99333013/diff/80001/Source/platform/fonts/win...
Source/platform/fonts/win/FontPlatformDataWin.cpp:192: , m_paintTextFlags(0)
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.

https://codereview.chromium.org/99333013/diff/80001/Source/platform/fonts/win...
Source/platform/fonts/win/FontPlatformDataWin.cpp:282: m_paintTextFlags =
computePaintTextFlags(logFont);
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.)

https://codereview.chromium.org/99333013/diff/80001/Source/platform/fonts/win...
File Source/platform/fonts/win/FontPlatformDataWin.cpp (right):

https://codereview.chromium.org/99333013/diff/80001/Source/platform/fonts/win...
Source/platform/fonts/win/FontPlatformDataWin.cpp:125: if
(isWebFont(fontFamilyName()) && !isRunningLayoutTest())
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.

https://codereview.chromium.org/99333013/diff/80001/Source/platform/fonts/win...
Source/platform/fonts/win/FontPlatformDataWin.cpp:300: // FIXME: This returns
the requested name, perhaps a better solution would be to
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).

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

https://codereview.chromium.org/99333013/diff/80001/Source/platform/fonts/win...
File Source/platform/fonts/win/FontPlatformDataWin.cpp (right):

https://codereview.chromium.org/99333013/diff/80001/Source/platform/fonts/win...
Source/platform/fonts/win/FontPlatformDataWin.cpp:125: if
(isWebFont(fontFamilyName()) && !isRunningLayoutTest())
On 2014/02/10 20:59:57, bungeman2 wrote:
> 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.

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.