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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1270723002: Improve CSS Style matching spec compliance (Closed)

Created:
5 years, 4 months ago by drott
Modified:
5 years, 4 months ago
CC:
blink-reviews, krit, pdr+graphicswatchlist_chromium.org, drott+blinkwatch_chromium.org, blink-reviews-css, dshwang, jbroman, Justin Novosad, danakj, dglazkov+blink, Rik, apavlov+blink_chromium.org, darktears, f(malita), Stephen Chennney, rwlbuis
Base URL:
git@github.com:drott/blink-crosswalk.git@reimplementMatching
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Improve CSS Style matching spec compliance Bring our CSS font style matching closer to the relevant section of the spec. [1] We previously had issues on matching correctly on font-stretch and on font-style. These are addressed in this patch by reimplementing the search for the correct font candidate out of the list of available @font-face's. [1] https://drafts.csswg.org/css-fonts/#font-style-matching BUG=514751, 513670 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200895

Patch Set 1 #

Patch Set 2 : Matching reimplemented, Tests passing \o/ #

Patch Set 3 : TODOs with bugs, redundant file removed, test expectations fix #

Total comments: 4

Patch Set 4 : Header, brackets removed, attempt at fixing windows synthetic oblique test failure #

Patch Set 5 : Fix for oblique on Windows, rebaselines requested #

Patch Set 6 : Android build fix #

Patch Set 7 : Run test on Windows, Android build fix attempt #2 #

Patch Set 8 : Android build: incl MathExtras, cast #

Patch Set 9 : stdlib instead of MathExtras #

Total comments: 2

Patch Set 10 : Fix TestExpectations #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+1755 lines, -118 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 1 chunk +10 lines, -0 lines 0 comments Download
A LayoutTests/css3/fonts/font-style-matching.html View 1 1 chunk +95 lines, -0 lines 0 comments Download
A LayoutTests/css3/fonts/font-style-matching-expected.txt View 1 1 chunk +1147 lines, -0 lines 0 comments Download
A LayoutTests/css3/fonts/resources/fonts/CSSMatchingTest_condensed_italic_100.ttf View Binary file 0 comments Download
A LayoutTests/css3/fonts/resources/fonts/CSSMatchingTest_condensed_italic_900.ttf View Binary file 0 comments Download
A LayoutTests/css3/fonts/resources/fonts/CSSMatchingTest_condensed_normal_100.ttf View Binary file 0 comments Download
A LayoutTests/css3/fonts/resources/fonts/CSSMatchingTest_condensed_normal_900.ttf View Binary file 0 comments Download
A LayoutTests/css3/fonts/resources/fonts/CSSMatchingTest_expanded_italic_100.ttf View Binary file 0 comments Download
A LayoutTests/css3/fonts/resources/fonts/CSSMatchingTest_expanded_italic_900.ttf View Binary file 0 comments Download
A LayoutTests/css3/fonts/resources/fonts/CSSMatchingTest_expanded_normal_100.ttf View Binary file 0 comments Download
A LayoutTests/css3/fonts/resources/fonts/CSSMatchingTest_expanded_normal_900.ttf View Binary file 0 comments Download
A LayoutTests/css3/fonts/resources/style-matching-test.js View 1 2 1 chunk +259 lines, -0 lines 0 comments Download
M LayoutTests/fast/css/font-face-multiple-faces.html View 1 2 2 chunks +0 lines, -11 lines 0 comments Download
M LayoutTests/fast/css/font-face-multiple-faces-expected.html View 1 2 1 chunk +0 lines, -5 lines 0 comments Download
M Source/core/core.gypi View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 1 comment Download
M Source/core/css/CSSFontFaceSource.cpp View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/CSSPrimitiveValueMappings.h View 1 2 3 2 chunks +4 lines, -1 line 0 comments Download
M Source/core/css/FontFace.cpp View 1 4 chunks +40 lines, -3 lines 0 comments Download
M Source/core/css/FontFaceCache.cpp View 1 2 3 4 chunks +4 lines, -79 lines 0 comments Download
A Source/core/css/FontStyleMatcher.h View 1 2 1 chunk +26 lines, -0 lines 0 comments Download
A Source/core/css/FontStyleMatcher.cpp View 1 2 3 4 5 6 7 8 1 chunk +141 lines, -0 lines 0 comments Download
M Source/platform/fonts/FontDescription.h View 1 1 chunk +1 line, -1 line 0 comments Download
M Source/platform/fonts/FontTraits.h View 1 2 4 chunks +17 lines, -14 lines 0 comments Download
M Source/platform/fonts/skia/FontCacheSkia.cpp View 1 2 3 4 3 chunks +3 lines, -3 lines 0 comments Download
M Source/platform/fonts/win/FontCacheSkiaWin.cpp View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M Source/web/PopupMenuImpl.cpp View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 43 (17 generated)
drott
5 years, 4 months ago (2015-08-04 14:34:22 UTC) #2
eae
LGTM https://codereview.chromium.org/1270723002/diff/40001/Source/core/css/FontStyleMatcher.cpp File Source/core/css/FontStyleMatcher.cpp (right): https://codereview.chromium.org/1270723002/diff/40001/Source/core/css/FontStyleMatcher.cpp#newcode1 Source/core/css/FontStyleMatcher.cpp:1: // Copyright 2015 The Chromium Authors. All rights ...
5 years, 4 months ago (2015-08-04 18:23:28 UTC) #3
dglazkov
More accurate title would be nice, too :)
5 years, 4 months ago (2015-08-04 18:26:53 UTC) #4
Kunihiko Sakamoto
LGTM https://codereview.chromium.org/1270723002/diff/40001/LayoutTests/css3/fonts/resources/style-matching-test.js File LayoutTests/css3/fonts/resources/style-matching-test.js (right): https://codereview.chromium.org/1270723002/diff/40001/LayoutTests/css3/fonts/resources/style-matching-test.js#newcode145 LayoutTests/css3/fonts/resources/style-matching-test.js:145: // fonts.ready event fires too early, used fonts ...
5 years, 4 months ago (2015-08-05 10:36:36 UTC) #6
drott
Thanks for your review, Kunihiko. > https://codereview.chromium.org/1270723002/diff/40001/LayoutTests/css3/fonts/resources/style-matching-test.js > File LayoutTests/css3/fonts/resources/style-matching-test.js (right): > > https://codereview.chromium.org/1270723002/diff/40001/LayoutTests/css3/fonts/resources/style-matching-test.js#newcode145 > ...
5 years, 4 months ago (2015-08-18 14:52:30 UTC) #7
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1270723002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1270723002/100001
5 years, 4 months ago (2015-08-18 15:26:39 UTC) #9
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile_dbg/builds/48936)
5 years, 4 months ago (2015-08-18 15:44:15 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/1270723002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1270723002/140001
5 years, 4 months ago (2015-08-19 08:32:16 UTC) #13
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_dbg_recipe/builds/108831)
5 years, 4 months ago (2015-08-19 08:55:55 UTC) #15
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1270723002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1270723002/160001
5 years, 4 months ago (2015-08-19 09:08:01 UTC) #18
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_dbg_recipe/builds/108843)
5 years, 4 months ago (2015-08-19 09:33:23 UTC) #20
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1270723002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1270723002/180001
5 years, 4 months ago (2015-08-19 09:54:11 UTC) #22
drott
eae@, ksakamoto@ - could you take a final look: the latest updates to this CL ...
5 years, 4 months ago (2015-08-19 10:57:10 UTC) #23
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 4 months ago (2015-08-19 12:55:52 UTC) #25
eae
LGTM, thanks for the update!
5 years, 4 months ago (2015-08-19 16:47:00 UTC) #26
Kunihiko Sakamoto
LGTM!
5 years, 4 months ago (2015-08-20 01:15:34 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1270723002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1270723002/180001
5 years, 4 months ago (2015-08-20 06:32:43 UTC) #29
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/90485)
5 years, 4 months ago (2015-08-20 06:42:13 UTC) #31
drott
philipj@opera.com, tkent@, jochen@ - could you review the changes in PopupMenuImpl.cpp? Thanks.
5 years, 4 months ago (2015-08-20 08:19:26 UTC) #33
tkent
lgtm https://codereview.chromium.org/1270723002/diff/180001/Source/web/PopupMenuImpl.cpp File Source/web/PopupMenuImpl.cpp (right): https://codereview.chromium.org/1270723002/diff/180001/Source/web/PopupMenuImpl.cpp#newcode71 Source/web/PopupMenuImpl.cpp:71: // TODO crbug.com/516675 Add stretch to serialization should ...
5 years, 4 months ago (2015-08-20 08:22:33 UTC) #34
tkent
https://codereview.chromium.org/1270723002/diff/180001/LayoutTests/TestExpectations File LayoutTests/TestExpectations (right): https://codereview.chromium.org/1270723002/diff/180001/LayoutTests/TestExpectations#newcode46 LayoutTests/TestExpectations:46: crbug.com/521860 fast/forms/submit-change-fragment.html [ Pass Timeout ] Looks to add ...
5 years, 4 months ago (2015-08-20 08:23:39 UTC) #35
drott
> Looks to add unrelated tests. Rebase failure? Yes, thanks very much for spotting this. ...
5 years, 4 months ago (2015-08-20 08:46:38 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1270723002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1270723002/200001
5 years, 4 months ago (2015-08-20 08:46:55 UTC) #39
commit-bot: I haz the power
Committed patchset #10 (id:200001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200895
5 years, 4 months ago (2015-08-20 10:26:58 UTC) #40
brucedawson
https://codereview.chromium.org/1270723002/diff/200001/Source/core/core.gypi File Source/core/core.gypi (right): https://codereview.chromium.org/1270723002/diff/200001/Source/core/core.gypi#newcode1137 Source/core/core.gypi:1137: 'css/FontStyleMatcher.h' There is a missing comma at the end ...
5 years, 4 months ago (2015-08-24 17:23:32 UTC) #42
drott
5 years, 4 months ago (2015-08-25 08:03:08 UTC) #43
Message was sent while issue was closed. 
> There is a missing comma at the end of this line. 

Thanks for spotting this, fix in https://codereview.chromium.org/1305943004/

Powered by Google App Engine
This is Rietveld 408576698