| Summary: | Expand font-weight and font-stretch to take any number | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||||||||||||||
| Component: | Text | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | buildbot, commit-queue, dino, hyatt, jonlee, mmaxfield, rniwa, simon.fraser, thorton, webkit-bug-importer | ||||||||||||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||||||||||||||||||
| Bug Blocks: | 162815 | ||||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||||
|
Description
Myles C. Maxfield
2017-02-26 21:09:51 PST
Created attachment 303420 [details]
WIP
*** Bug 168890 has been marked as a duplicate of this bug. *** Created attachment 303443 [details]
WIP
Created attachment 303445 [details]
WIP
Created attachment 303471 [details]
WIP
Created attachment 303477 [details]
WIP
Attachment 303477 [details] did not pass style-queue:
ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 50 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 303482 [details]
WIP
Created attachment 303494 [details]
Patch
Created attachment 303495 [details]
Patch
Created attachment 303503 [details]
Patch
Comment on attachment 303503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303503&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2571 > + else if (weight == FontSelectionValue(100) > + || weight == FontSelectionValue(200) > + || weight == FontSelectionValue(300) > + || weight == FontSelectionValue(400) > + || weight == FontSelectionValue(500) > + || weight == FontSelectionValue(600) > + || weight == FontSelectionValue(700) > + || weight == FontSelectionValue(800) > + || weight == FontSelectionValue(900)) Maybe have a isWeightABuiltInValue or something. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2594 > + if (stretch == FontSelectionValue(50)) > + computedFont->stretch = CSSValuePool::singleton().createIdentifierValue(CSSValueUltraCondensed); > + else if (stretch == FontSelectionValue(62.5f)) > + computedFont->stretch = CSSValuePool::singleton().createIdentifierValue(CSSValueExtraCondensed); > + else if (stretch == FontSelectionValue(75)) > + computedFont->stretch = CSSValuePool::singleton().createIdentifierValue(CSSValueCondensed); > + else if (stretch == FontSelectionValue(87.5f)) > + computedFont->stretch = CSSValuePool::singleton().createIdentifierValue(CSSValueSemiCondensed); > + else if (stretch == normalStretchValue()) > + computedFont->stretch = CSSValuePool::singleton().createIdentifierValue(CSSValueNormal); > + else if (stretch == FontSelectionValue(112.5f)) > + computedFont->stretch = CSSValuePool::singleton().createIdentifierValue(CSSValueSemiExpanded); > + else if (stretch == FontSelectionValue(125)) > + computedFont->stretch = CSSValuePool::singleton().createIdentifierValue(CSSValueExpanded); > + else if (stretch == FontSelectionValue(150)) > + computedFont->stretch = CSSValuePool::singleton().createIdentifierValue(CSSValueExtraExpanded); > + else if (stretch == FontSelectionValue(200)) > + computedFont->stretch = CSSValuePool::singleton().createIdentifierValue(CSSValueUltraExpanded); It would be much nicer to see the magic numbers in a data table that maps float to identifier. In fact, maybe you should group all of the font-related magic numbers into one place (its own header or something). > Source/WebCore/css/CSSFontFace.cpp:152 > + case CSSValueBold: > + case CSSValueBolder: > + result = boldWeightValue(); Odd that both map to the same number? > Source/WebCore/css/CSSFontFace.cpp:155 > + result = FontSelectionValue(200); More magic numbers. > Source/WebCore/css/CSSFontFace.cpp:230 > case CSSValueUltraCondensed: > - return FontSelectionValue(50); > + result = FontSelectionValue(50); > + break; > case CSSValueExtraCondensed: > - return FontSelectionValue(62.5f); > + result = FontSelectionValue(62.5f); > + break; > case CSSValueCondensed: > - return FontSelectionValue(75); > + result = FontSelectionValue(75); > + break; > case CSSValueSemiCondensed: > - return FontSelectionValue(87.5f); > + result = FontSelectionValue(87.5f); > + break; > case CSSValueNormal: > - return FontSelectionValue(100); > + result = normalStretchValue(); > + break; > case CSSValueSemiExpanded: > - return FontSelectionValue(112.5f); > + result = FontSelectionValue(112.5f); > + break; > case CSSValueExpanded: > - return FontSelectionValue(125); > + result = FontSelectionValue(125); > + break; > case CSSValueExtraExpanded: > - return FontSelectionValue(150); > + result = FontSelectionValue(150); > + break; > case CSSValueUltraExpanded: > - return FontSelectionValue(200); > + result = FontSelectionValue(200); > + break; More fodder for a data table. > Source/WebCore/css/CSSFontFace.cpp:258 > +static std::optional<FontSelectionRange> calculateStyleRange(CSSValue& value) "style" is a bit confusing in this context. Not CSS-type style, or RenderStyle, right? Is this just about sloping? > Source/WebCore/css/CSSFontFaceSet.cpp:314 > + case CSSValueBold: > + case CSSValueBolder: > + return boldWeightValue(); > + case CSSValueNormal: > + return normalWeightValue(); > + case CSSValueLighter: > + return FontSelectionValue(200); Seen these before. > Source/WebCore/css/CSSFontFaceSet.cpp:357 > + case CSSValueUltraCondensed: > + return FontSelectionValue(50); > + case CSSValueExtraCondensed: > + return FontSelectionValue(62.5f); > + case CSSValueCondensed: > + return FontSelectionValue(75); > + case CSSValueSemiCondensed: > + return FontSelectionValue(87.5f); > + case CSSValueNormal: > + return normalStretchValue(); > + case CSSValueSemiExpanded: > + return FontSelectionValue(112.5f); > + case CSSValueExpanded: > + return FontSelectionValue(125); > + case CSSValueExtraExpanded: > + return FontSelectionValue(150); > + case CSSValueUltraExpanded: > + return FontSelectionValue(200); More data table fodder. > Source/WebCore/css/CSSFontFaceSet.cpp:386 > + case CSSValueNormal: > + return FontSelectionValue(); > + case CSSValueItalic: > + case CSSValueOblique: > + return italicThreshold(); Seen this before. > Source/WebCore/css/StyleBuilderConverter.h:1179 > + case CSSValueNormal: > + return FontSelectionValue(400); > + case CSSValueBold: > + return FontSelectionValue(700); > + case CSSValueBolder: More fodder for the data table. > Source/WebCore/css/StyleBuilderConverter.h:1249 > + case CSSValueNormal: > + return FontSelectionValue(); > + case CSSValueOblique: > + case CSSValueItalic: > + return FontSelectionValue(20); Looks sharable. > Source/WebCore/platform/graphics/FontDescription.cpp:117 > + if (weight < FontSelectionValue(350)) > + return FontSelectionValue(400); > + if (weight < FontSelectionValue(550)) > + return FontSelectionValue(700); > + if (weight < FontSelectionValue(900)) > + return FontSelectionValue(900); > + return weight; Also this. > Source/WebCore/platform/graphics/FontSelectionAlgorithm.h:201 > static inline FontSelectionValue boldThreshold() > { > static NeverDestroyed<FontSelectionValue> result = FontSelectionValue(600); > return result.get(); > } > > +static inline FontSelectionValue boldWeightValue() > +{ > + static NeverDestroyed<FontSelectionValue> result = FontSelectionValue(700); > + return result.get(); > +} > + > +static inline FontSelectionValue normalWeightValue() > +{ > + static NeverDestroyed<FontSelectionValue> result = FontSelectionValue(400); > + return result.get(); > +} > + > +static inline bool isFontWeightBold(FontSelectionValue fontWeight) > +{ > + return fontWeight >= boldThreshold(); > +} > + > static inline FontSelectionValue weightSearchThreshold() > { > static NeverDestroyed<FontSelectionValue> result = FontSelectionValue(500); > return result.get(); > } > > +static inline FontSelectionValue normalStretchValue() > +{ > + static NeverDestroyed<FontSelectionValue> result = FontSelectionValue(100); > + return result.get(); Could come from a data table. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:712 > + if (weight < -0.6) > + return FontSelectionValue(100); > + if (weight < -0.365) > + return FontSelectionValue(200); > + if (weight < -0.115) > + return FontSelectionValue(300); > + if (weight < 0.130) > + return FontSelectionValue(400); > + if (weight < 0.235) > + return FontSelectionValue(500); > + if (weight < 0.350) > + return FontSelectionValue(600); > + if (weight < 0.500) > + return FontSelectionValue(700); > + if (weight < 0.700) > + return FontSelectionValue(800); Also this. > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:182 > + if (weight < FontSelectionValue(150)) > return FC_WEIGHT_THIN; > - case FontWeight200: > + if (weight < FontSelectionValue(250)) > return FC_WEIGHT_ULTRALIGHT; > - case FontWeight300: > + if (weight < FontSelectionValue(350)) > return FC_WEIGHT_LIGHT; > - case FontWeight400: > + if (weight < FontSelectionValue(450)) > return FC_WEIGHT_REGULAR; > - case FontWeight500: > + if (weight < FontSelectionValue(550)) > return FC_WEIGHT_MEDIUM; > - case FontWeight600: > + if (weight < FontSelectionValue(650)) > return FC_WEIGHT_SEMIBOLD; > - case FontWeight700: > + if (weight < FontSelectionValue(750)) > return FC_WEIGHT_BOLD; > - case FontWeight800: > + if (weight < FontSelectionValue(850)) Maybe this too. > Source/WebCore/platform/graphics/ios/FontCacheIOS.mm:91 > - if (weight > FontWeight300) { > + if (weight >= FontSelectionValue(350)) { > if (bold) > fontType = kCTFontUIFontEmphasizedSystem; > - } else if (weight > FontWeight200) > + } else if (weight >= FontSelectionValue(250)) > fontType = static_cast<CTFontUIFontType>(kCTFontUIFontSystemLight); > - else if (weight > FontWeight100) > + else if (weight >= FontSelectionValue(150)) > fontType = static_cast<CTFontUIFontType>(kCTFontUIFontSystemThin); And this. > Source/WebCore/platform/graphics/ios/FontCacheIOS.mm:118 > + float ctWeight = kCTFontWeightRegular; > + if (weight < FontSelectionValue(150)) > + ctWeight = kCTFontWeightUltraLight; > + else if (weight < FontSelectionValue(250)) > + ctWeight = kCTFontWeightThin; > + else if (weight < FontSelectionValue(350)) > + ctWeight = kCTFontWeightLight; > + else if (weight < FontSelectionValue(450)) > + ctWeight = kCTFontWeightRegular; > + else if (weight < FontSelectionValue(550)) > + ctWeight = kCTFontWeightMedium; > + else if (weight < FontSelectionValue(650)) > + ctWeight = kCTFontWeightSemibold; > + else if (weight < FontSelectionValue(750)) > + ctWeight = kCTFontWeightBold; > + else if (weight < FontSelectionValue(850)) > + ctWeight = kCTFontWeightHeavy; Def. this. > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:73 > + if (fontWeight < FontSelectionValue(150)) > + return NSFontWeightUltraLight; > + if (fontWeight < FontSelectionValue(250)) > + return NSFontWeightThin; > + if (fontWeight < FontSelectionValue(350)) > + return NSFontWeightLight; > + if (fontWeight < FontSelectionValue(450)) > + return NSFontWeightRegular; > + if (fontWeight < FontSelectionValue(550)) > + return NSFontWeightMedium; > + if (fontWeight < FontSelectionValue(650)) > + return NSFontWeightSemibold; > + if (fontWeight < FontSelectionValue(750)) > + return NSFontWeightBold; > + if (fontWeight < FontSelectionValue(850)) > + return NSFontWeightHeavy; > + return NSFontWeightBlack; So many magic numbers. > Source/WebCore/platform/graphics/win/FontCacheWin.cpp:419 > + if (fontWeight < FontSelectionValue(150)) > + return FW_THIN; > + if (fontWeight < FontSelectionValue(150)) > + return FW_EXTRALIGHT; > + if (fontWeight < FontSelectionValue(150)) > + return FW_LIGHT; > + if (fontWeight < FontSelectionValue(150)) > + return FW_NORMAL; > + if (fontWeight < FontSelectionValue(150)) > + return FW_MEDIUM; > + if (fontWeight < FontSelectionValue(150)) > + return FW_SEMIBOLD; > + if (fontWeight < FontSelectionValue(150)) > + return FW_BOLD; > + if (fontWeight < FontSelectionValue(150)) > + return FW_EXTRABOLD; > + return FW_HEAVY; Gah. > Source/WebCore/platform/graphics/win/FontCacheWin.cpp:581 > + case FW_THIN: > + weight = FontSelectionValue(100); > + break; > + case FW_EXTRALIGHT: > + weight = FontSelectionValue(200); > + break; > + case FW_LIGHT: > + weight = FontSelectionValue(300); > + break; > + case FW_NORMAL: > + weight = FontSelectionValue(400); > + break; > + case FW_MEDIUM: > + weight = FontSelectionValue(500); > + break; > + case FW_SEMIBOLD: > + weight = FontSelectionValue(600); > + break; > + case FW_BOLD: > + weight = FontSelectionValue(700); > + break; > + case FW_EXTRABOLD: > + weight = FontSelectionValue(800); > + break; > + default: > + weight = FontSelectionValue(900); > + break; Oh my > Source/WebCore/rendering/RenderThemeMac.mm:382 > + static const FontSelectionValue fontWeights[] = { > + FontSelectionValue(100), > + FontSelectionValue(100), > + FontSelectionValue(200), > + FontSelectionValue(300), > + FontSelectionValue(400), > + FontSelectionValue(500), > + FontSelectionValue(600), > + FontSelectionValue(600), > + FontSelectionValue(700), > + FontSelectionValue(800), > + FontSelectionValue(800), > + FontSelectionValue(900), > + FontSelectionValue(900), > + FontSelectionValue(900) Roll this into some shared data table? Created attachment 303525 [details]
WIP
Comment on attachment 303503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303503&action=review >> Source/WebCore/css/CSSFontFace.cpp:152 >> + result = boldWeightValue(); > > Odd that both map to the same number? That's how it's supposed to work :/ Created attachment 303531 [details]
Patch
Comment on attachment 303531 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303531&action=review > Source/WebCore/css/CSSFontFace.cpp:127 > +static std::optional<FontSelectionRange> calculateWeightRange(CSSValue& value) You never return nullopt so why the option return value? > Source/WebCore/css/CSSFontFace.cpp:209 > +static std::optional<FontSelectionRange> calculateItalicRange(CSSValue& value) Ditto. > Source/WebCore/css/parser/CSSPropertyParser.cpp:879 > if ((weight % 100) || weight < 100 || weight > 900) Maybe these numbers should go into the header. Created attachment 303534 [details]
Patch for committing
Created attachment 303535 [details]
Patch for committing
Comment on attachment 303531 [details] Patch Attachment 303531 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3253850 Number of test failures exceeded the failure limit. Created attachment 303538 [details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 303531 [details] Patch Attachment 303531 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3253863 Number of test failures exceeded the failure limit. Created attachment 303539 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 303531 [details] Patch Attachment 303531 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3253819 Number of test failures exceeded the failure limit. Created attachment 303540 [details]
Archive of layout-test-results from ews117 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 303531 [details] Patch Attachment 303531 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3253867 Number of test failures exceeded the failure limit. Created attachment 303542 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 303535 [details] Patch for committing Attachment 303535 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3253912 Number of test failures exceeded the failure limit. Created attachment 303544 [details]
Archive of layout-test-results from ews112 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 303546 [details]
Patch for committing
Comment on attachment 303546 [details] Patch for committing Clearing flags on attachment: 303546 Committed r213464: <http://trac.webkit.org/changeset/213464> |