Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(60)

Issue 1014953002: Restore GDI text size rounding. (Closed)

Created:
5 years, 9 months ago by bungeman-skia
Modified:
5 years, 9 months ago
Reviewers:
reed1, mtklein, eae, scottmg
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

Restore GDI text size rounding. Add call to SkScalarRoundToScalar(). The old code calculated the scale from the text size, but now the text size is calculated from the scale (which is arguably the right way to think about it). However, the old code always rounded the final resulting text size, while the new code does not. In the 'no hinting' case, the text size is already rounded to an integer (so that the rest of the matrix is minimized). In the 'hinted' case, the entire scale has been removed from the matrix, so the scale value is the 'real' residual size. The old code rounded this size, and the new code should as well. BUG=464784 Committed: https://skia.googlesource.com/skia/+/6f94076da504a9e292c7f6173b039d2692d47c51

Patch Set 1 #

Total comments: 2

Patch Set 2 : Add comments. #

Patch Set 3 : More comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+15 lines, -3 lines) Patch
M src/ports/SkFontHost_win.cpp View 1 2 2 chunks +15 lines, -3 lines 0 comments Download

Messages

Total messages: 21 (6 generated)
bungeman-skia
This is a fix for https://codereview.chromium.org/748883005/ .
5 years, 9 months ago (2015-03-17 21:27:59 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1014953002/1
5 years, 9 months ago (2015-03-17 21:29:31 UTC) #4
commit-bot: I haz the power
Note for Reviewers: The CQ is waiting for an approval. If you believe that the ...
5 years, 9 months ago (2015-03-17 21:29:33 UTC) #5
bungeman-skia
Testing effect on Blink with https://crrev.com/1011283002 .
5 years, 9 months ago (2015-03-17 22:52:13 UTC) #6
bungeman-skia
On 2015/03/17 22:52:13, bungeman1 wrote: > Testing effect on Blink with https://crrev.com/1011283002 . As expected, ...
5 years, 9 months ago (2015-03-17 23:00:25 UTC) #7
scottmg
Confirmed fixes the gmail case as I was testing it. Thanks! That's sad it's not ...
5 years, 9 months ago (2015-03-17 23:31:07 UTC) #8
scottmg
+eae for fyi on blink layout tests. In gmail it's "Arial 80%" == 12.8px that ...
5 years, 9 months ago (2015-03-17 23:33:10 UTC) #10
commit-bot: I haz the power
No LGTM from a valid reviewer yet. Please ask for an LGTM from a full ...
5 years, 9 months ago (2015-03-18 03:29:34 UTC) #12
reed1
https://codereview.chromium.org/1014953002/diff/1/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): https://codereview.chromium.org/1014953002/diff/1/src/ports/SkFontHost_win.cpp#newcode636 src/ports/SkFontHost_win.cpp:636: SkScalar gdiTextSize = SkScalarRoundToScalar(scale.fY); // Comment here, to document ...
5 years, 9 months ago (2015-03-18 13:57:08 UTC) #13
bungeman-skia
How about a comment like this? https://codereview.chromium.org/1014953002/diff/1/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): https://codereview.chromium.org/1014953002/diff/1/src/ports/SkFontHost_win.cpp#newcode636 src/ports/SkFontHost_win.cpp:636: SkScalar gdiTextSize = ...
5 years, 9 months ago (2015-03-18 14:22:14 UTC) #14
reed1
lgtm w/ request for comment on zero-check for size
5 years, 9 months ago (2015-03-18 14:56:50 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1014953002/40001
5 years, 9 months ago (2015-03-18 15:04:07 UTC) #18
bungeman-skia
On 2015/03/18 14:56:50, reed1 wrote: > lgtm w/ request for comment on zero-check for size ...
5 years, 9 months ago (2015-03-18 15:04:10 UTC) #19
commit-bot: I haz the power
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/6f94076da504a9e292c7f6173b039d2692d47c51
5 years, 9 months ago (2015-03-18 15:25:47 UTC) #20
bungeman-skia
5 years, 9 months ago (2015-03-20 15:52:00 UTC) #21
Message was sent while issue was closed.
On 2015/03/17 23:33:10, scottmg wrote:
> +eae for fyi on blink layout tests. In gmail it's "Arial 80%" == 12.8px that
> regressed. Somewhat frightening that Blink layout tests didn't notice. Are
there
> just no GDI tests any more? Or we just happened not to hit a fractional
rounding
> case?

So it turns out there are bots on the Blink waterfall which test GDI (the XP
bots), but there aren't any available as trybots. So at least there is some
post-hoc coverage. I'm going to go and try to rebaseline those now. Turns out
there are ~300 tests which change. I don't remember doing an rebaseline when
this broke, however.

Powered by Google App Engine
This is Rietveld 408576698