Issue 1921903002: Add oblique as a slant.

bungeman-skia

Description was changed from ========== Add oblique as a slant. ========== to ========== Add oblique ...

4 years, 8 months ago (2016-04-25 22:18:20 UTC) #1

bungeman-skia

The CQ bit was checked by bungeman@google.com to run a CQ dry run

4 years, 8 months ago (2016-04-25 22:18:37 UTC) #2

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1921903002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921903002/1

4 years, 8 months ago (2016-04-25 22:18:47 UTC) #3

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 8 months ago (2016-04-25 22:21:17 UTC) #4

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-Debug-Trybot/builds/8110) Build-Win-MSVC-x86_64-Debug-Trybot on ...

4 years, 8 months ago (2016-04-25 22:21:18 UTC) #5

bungeman-skia

The CQ bit was checked by bungeman@google.com to run a CQ dry run

4 years, 8 months ago (2016-04-25 22:40:53 UTC) #6

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1921903002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921903002/20001

4 years, 8 months ago (2016-04-25 22:41:02 UTC) #7

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 8 months ago (2016-04-25 22:44:48 UTC) #8

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-Debug-Trybot/builds/8111)

4 years, 8 months ago (2016-04-25 22:44:49 UTC) #9

bungeman-skia

Description was changed from ========== Add oblique as a slant. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1921903002 ========== to ========== ...

4 years, 8 months ago (2016-04-25 23:06:18 UTC) #10

bungeman-skia

The CQ bit was checked by bungeman@google.com to run a CQ dry run

4 years, 8 months ago (2016-04-26 14:24:14 UTC) #11

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1921903002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921903002/20001

4 years, 8 months ago (2016-04-26 14:24:20 UTC) #12

bungeman-skia

Description was changed from ========== Add oblique as a slant. Some fonts have italic and ...

4 years, 8 months ago (2016-04-26 17:34:36 UTC) #14

bungeman-skia

bungeman@google.com changed reviewers: + reed@google.com

4 years, 8 months ago (2016-04-26 19:01:51 UTC) #15

bungeman-skia

The CQ bit was checked by bungeman@google.com to run a CQ dry run

4 years, 8 months ago (2016-04-26 19:10:37 UTC) #17

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1921903002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921903002/40001

4 years, 8 months ago (2016-04-26 19:10:45 UTC) #18

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 8 months ago (2016-04-26 19:38:48 UTC) #20

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 8 months ago (2016-04-26 19:38:49 UTC) #21

reed1

lgtm with question https://codereview.chromium.org/1921903002/diff/40001/src/core/SkFontMgr.cpp File src/core/SkFontMgr.cpp (right): https://codereview.chromium.org/1921903002/diff/40001/src/core/SkFontMgr.cpp#newcode243 src/core/SkFontMgr.cpp:243: // Style takes priority over all ...

4 years, 8 months ago (2016-04-26 20:36:10 UTC) #22

bungeman-skia

https://codereview.chromium.org/1921903002/diff/40001/src/core/SkFontMgr.cpp File src/core/SkFontMgr.cpp (right): https://codereview.chromium.org/1921903002/diff/40001/src/core/SkFontMgr.cpp#newcode243 src/core/SkFontMgr.cpp:243: // Style takes priority over all valid weights. On ...

4 years, 7 months ago (2016-04-27 15:37:20 UTC) #23

https://codereview.chromium.org/1921903002/diff/40001/src/core/SkFontMgr.cpp
File src/core/SkFontMgr.cpp (right):

https://codereview.chromium.org/1921903002/diff/40001/src/core/SkFontMgr.cpp#...
src/core/SkFontMgr.cpp:243: // Style takes priority over all valid weights.
On 2016/04/26 20:36:09, reed1 wrote:
> s/Style/Slant ?

Eck, yeah. So CSS uses the terms stretch, style, weight where we use width,
slant, and weight. I'll update the comments above each of the sections here to
make that clearer.

https://codereview.chromium.org/1921903002/diff/40001/src/core/SkFontMgr.cpp#...
src/core/SkFontMgr.cpp:245: case SkFontStyle::kUpright_Slant: {
On 2016/04/26 20:36:09, reed1 wrote:
> Sure seems like a table or other calc might be clearer. (not sure how to do
that
> tho).

I originally wrote this as a small 3x3 score table, and have gone back to that.
I wrote the 'switch' code on the basis that it would be easy for the compromised
renderer process to spew garbage SkFontStyle data and read outside this static
score table in the browser process if we didn't do additional runtime checks.
However, I have now convinced myself that SkFontStyles are all being created
through the constructor (which pins the values to legal ones) and also that any
code which tries to create SkFontStyles by casting bits deserves the pain they
will eventually receive. Also, it turns out we're squeezing the slant into 8
bits, so an attacker could only read up to 2^16 bytes of static data anyway,
even if some code were insecure.

I've added a bunch of assert here to state the preconditions of this working so
that if anything changes in the future we'll know about it.

> 
> Why don't we give the same (low) score to italic and oblique, if we're trying
to
> match upright?
> 
> I get the preference for the "other" slant if we're requesting one, and always
> giving upright the lowest score.

Because that's the way it's spec'ed in the CSS3 font matching algorithm. When
requesting CSS3 'normal' (what we call upright) and there is no 'normal' then
'oblique' is preferred to 'italic'. Obviously we don't expect this to happen
very often, but we might as well implement the specification.

bungeman-skia

The CQ bit was checked by bungeman@google.com to run a CQ dry run

4 years, 7 months ago (2016-04-27 15:37:25 UTC) #24

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1921903002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921903002/80001

4 years, 7 months ago (2016-04-27 15:37:30 UTC) #25

bungeman-skia

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the ...

4 years, 7 months ago (2016-04-27 17:19:41 UTC) #28

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1921903002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921903002/80001

4 years, 7 months ago (2016-04-27 17:19:48 UTC) #29

commit-bot: I haz the power

Description was changed from ========== Add oblique as a slant. Some fonts have italic and ...

4 years, 7 months ago (2016-04-27 17:21:07 UTC) #30

commit-bot: I haz the power

4 years, 7 months ago (2016-04-27 17:21:08 UTC) #31

Message was sent while issue was closed.

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/b4bb7d825566042ed64697be49457dbac060e6c4

Issue 1921903002: Add oblique as a slant. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Fix Windows. #

Patch Set 3 : Rebase and add test. #

Patch Set 4 : Address comments. #

Patch Set 5 : static_assert what can be. #

Messages