font-optical-sizing applies the wrong variation value
Created attachment 368815 [details] Needs version check
Attachment 368815 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:17: 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 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
<rdar://problem/50152854>
I probably need to make a font to test this
We also might want to consider whether the font is system ui or not
Comment on attachment 368815 [details] Needs version check Attachment 368815 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12064977 New failing tests: fast/text/justify-ideograph-complex.html
Created attachment 368820 [details] Archive of layout-test-results from ews100 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 368815 [details] Needs version check Attachment 368815 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12065111 New failing tests: fast/text/justify-ideograph-complex.html
Created attachment 368823 [details] Archive of layout-test-results from ews105 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 368815 [details] Needs version check Attachment 368815 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12065220 New failing tests: fast/text/justify-ideograph-complex.html
Created attachment 368826 [details] Archive of layout-test-results from ews117 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 368815 [details] Needs version check Attachment 368815 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12065267 New failing tests: platform/ios/ios/fast/text/opticalFontWithWeight.html platform/ios/ios/fast/text/opticalFontWithTextStyle.html fast/forms/auto-fill-button/input-strong-password-auto-fill-button.html
Created attachment 368827 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.4
Comment on attachment 368815 [details] Needs version check Attachment 368815 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12067490 New failing tests: security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.html legacy-animation-engine/fast/layers/no-clipping-overflow-hidden-hardware-acceleration.html
Created attachment 368845 [details] Archive of layout-test-results from ews213 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews213 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 370156 [details] WIP
Created attachment 370173 [details] Patch
Comment on attachment 370173 [details] Patch Attachment 370173 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12221141 New failing tests: fast/text/variations/case-axis-names.html fast/text/variations/font-selection-properties.html fast/text/variations/gx-width.html fast/text/variations/font-face-clamp.html fast/text/justify-ideograph-complex.html fast/text/optical-sizing-trak-2.html
Created attachment 370176 [details] Archive of layout-test-results from ews103 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 370173 [details] Patch Attachment 370173 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12221189 New failing tests: fast/text/variations/case-axis-names.html fast/text/variations/font-selection-properties.html fast/text/variations/gx-width.html fast/text/variations/font-face-clamp.html fast/text/justify-ideograph-complex.html fast/text/optical-sizing-trak-2.html
Created attachment 370178 [details] Archive of layout-test-results from ews104 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 370173 [details] Patch Attachment 370173 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12221231 New failing tests: fast/text/variations/case-axis-names.html fast/text/variations/font-selection-properties.html fast/text/variations/gx-width.html fast/text/variations/font-face-clamp.html fast/text/justify-ideograph-complex.html fast/text/optical-sizing-trak-2.html
Created attachment 370179 [details] Archive of layout-test-results from ews115 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 370173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370173&action=review > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:513 > + case 'trak': Ok as is. Wow. Wow. Wow. A multi character constant. I have never run across this in practice until now... its impl defined. Clever. Very clever. I actually don't know which I prefer the cleverness or using the k-type constant. Kinda leaning towards the later if I were to ever touch this function for source compatibility. Likely never will or could be changed though :/ could in theory be made safe for all compilers though :/ > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:514 > + foundTrak = true; Ok as is. Missing a break. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:529 > + enum class TrackingType { Ok as is. Missing a width. In my opinion, looks prettier in one line. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:532 > + Manual Ok as-is. Missing comma for future proofing. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:555 > const auto& variations = fontDescription.variationSettings(); ^^^^ remove the unused_param for size above this line. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:558 > + FontType fontType(originalFont); Ok as is. Uniform initializer syntax is prettier in my opinion. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:560 > + bool forceOpticalSizingOn = fontOpticalSizing == FontOpticalSizing::Enabled && (fontType.variationType == FontType::VariationType::TrueTypeGX && defaultValues.contains({{'o', 'p', 's', 'z'}})); Ok as is. Parens not needed. All you have are &&s > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:563 > + if (!originalFont || (!features.size() && variations.isEmpty() && textRenderingMode == TextRenderingMode::AutoTextRendering && variantSettings.isAllNormal() && fontOpticalSizing == FontOpticalSizing::Enabled && !forceOpticalSizingOn Ok as is. No, features.isEmpty? I would add one in my opinion. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:564 > && (!fontFaceFeatures || !fontFaceFeatures->size()) && (!fontFaceVariantSettings || fontFaceVariantSettings->isAllNormal()))) Ok as is. Again, no isEmpty()? > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:565 > return originalFont; ^^^^ in my opinion if condition could be simplified and made more readable by extracting into local vars.
View in context: https://bugs.webkit.org/attachment.cgi?id=370173&action=review > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:513 > + case 'trak': Ok as is. Wow. Wow. Wow. A multi character constant. I have never run across this in practice until now... its impl defined. Clever. Very clever. I actually don't know which I prefer the cleverness or using the k-type constant. Kinda leaning towards the later if I were to ever touch this function for source compatibility. Likely never will or could be changed though :/ could in theory be made safe for all compilers though :/ > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:514 > + foundTrak = true; Ok as is. Missing a break. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:529 > + enum class TrackingType { Ok as is. Missing a width. In my opinion, looks prettier in one line. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:532 > + Manual Ok as-is. Missing comma for future proofing. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:555 > const auto& variations = fontDescription.variationSettings(); ^^^^ remove the unused_param for size above this line. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:558 > + FontType fontType(originalFont); Ok as is. Uniform initializer syntax is prettier in my opinion. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:560 > + bool forceOpticalSizingOn = fontOpticalSizing == FontOpticalSizing::Enabled && (fontType.variationType == FontType::VariationType::TrueTypeGX && defaultValues.contains({{'o', 'p', 's', 'z'}})); Ok as is. Parens not needed. All you have are &&s > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:563 > + if (!originalFont || (!features.size() && variations.isEmpty() && textRenderingMode == TextRenderingMode::AutoTextRendering && variantSettings.isAllNormal() && fontOpticalSizing == FontOpticalSizing::Enabled && !forceOpticalSizingOn Ok as is. No, features.isEmpty? I would add one in my opinion. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:564 > && (!fontFaceFeatures || !fontFaceFeatures->size()) && (!fontFaceVariantSettings || fontFaceVariantSettings->isAllNormal()))) Ok as is. Again, no isEmpty()? > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:565 > return originalFont; ^^^^ in my opinion if condition could be simplified and made more readable by extracting into local vars.
Looks like the new tests are all failing on High Sierra.
Created attachment 370255 [details] Patch
Comment on attachment 370255 [details] Patch Attachment 370255 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12237507 New failing tests: fast/text/optical-sizing-units-2.html fast/text/optical-sizing-trak-2.html
Created attachment 370262 [details] Archive of layout-test-results from ews215 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews215 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 370255 [details] Patch Attachment 370255 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12237448 New failing tests: fast/text/justify-ideograph-complex.html fast/text/optical-sizing-trak-2.html
Created attachment 370263 [details] Archive of layout-test-results from ews105 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 370255 [details] Patch Attachment 370255 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12237569 New failing tests: fast/text/justify-ideograph-complex.html fast/text/optical-sizing-trak-2.html
Created attachment 370266 [details] Archive of layout-test-results from ews117 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 370255 [details] Patch Attachment 370255 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12238574 New failing tests: fast/text/justify-ideograph-complex.html fast/text/optical-sizing-trak-2.html
Created attachment 370269 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 370281 [details] Patch
Created attachment 370282 [details] Patch
Comment on attachment 370282 [details] Patch Attachment 370282 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12240975 New failing tests: webaudio/silent-audio-interrupted-in-background.html
Created attachment 370291 [details] Archive of layout-test-results from ews114 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-highsierra Platform: Mac OS X 10.13.6
Test failure is unrelated.
Comment on attachment 370282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370282&action=review > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:557 > + if (!originalFont || (features.isEmpty() && !forceVariations && variations.isEmpty() && textRenderingMode == TextRenderingMode::AutoTextRendering && variantSettings.isAllNormal() && fontOpticalSizing == FontOpticalSizing::Enabled && !forceOpticalSizingOn > + && (!fontFaceFeatures || fontFaceFeatures->isEmpty()) && (!fontFaceVariantSettings || fontFaceVariantSettings->isAllNormal()))) The test would read better with some sensibly named helper booleans, or as a lambda function.
Comment on attachment 370282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370282&action=review > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:527 > + enum class TrackingType : uint8_t { None, Automatic, Manual, }; > + TrackingType trackingType { TrackingType::None }; We have this new code to compute trackingType but I don't see trackingType actually being used anywhere? Can you just remove the code and simplify the patch?
Committed r245598: <https://trac.webkit.org/changeset/245598>
Reverted r245598 for reason: Breaks watchOS build. Committed r245647: <https://trac.webkit.org/changeset/245647>
Created attachment 370470 [details] Patch for committing
This latest patch includes the fix from https://bugs.webkit.org/show_bug.cgi?id=198099
*** Bug 198099 has been marked as a duplicate of this bug. ***
Committed r245672: <https://trac.webkit.org/changeset/245672>