| Summary: | [Cocoa] Distinguish between paint advances and base advances | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||||||||||||
| Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | buildbot, commit-queue, dino, jonlee, rniwa, ryanhaddad, simon.fraser, thorton | ||||||||||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||||||||||||||||
| Bug Blocks: | 138213 | ||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||
|
Description
Myles C. Maxfield
2016-08-15 23:10:08 PDT
Created attachment 286153 [details]
WIP
Created attachment 286655 [details]
Patch
Created attachment 286692 [details]
WIP
Created attachment 286813 [details]
Patch
Comment on attachment 286813 [details] Patch Attachment 286813 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1931286 Number of test failures exceeded the failure limit. Created attachment 286819 [details]
Archive of layout-test-results from ews101 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 286813 [details] Patch Attachment 286813 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1931657 Number of test failures exceeded the failure limit. Created attachment 286827 [details]
Archive of layout-test-results from ews116 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 286813 [details] Patch Attachment 286813 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1937220 Number of test failures exceeded the failure limit. Created attachment 286931 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Tests to investigate: svg/ +css2.1/t051202-c26-psudo-nest-00-c.html expected actual diff pretty diff images diff (4.59%) text image+text pass history +css2.1/t1508-c527-font-00-b.html expected actual diff pretty diff images diff (0.65%) text image+text pass history +fast/inline/absolute-positioned-inline-in-centred-block.html expected actual diff pretty diff images diff (0.18%) text image+text pass history +fast/text/complex-first-glyph-with-initial-advance.html reference images diff (0.08%) image pass history +fast/text/complex-initial-advance.html reference images diff (0.02%) image pass history +fast/text/space-width.html reference images diff (1%) image pass history +http/tests/contentextensions/make-https.html expected actual diff pretty diff text pass history +imported/blink/fast/text/international/vertical-positioning-with-combining-marks.html reference images diff (0.06%) image pass history +platform/mac/fast/text/international/Geeza-Pro-vertical-metrics-adjustment.html expected actual diff pretty diff images diff (1.12%) text image+text pass history Created attachment 287171 [details]
WIP
Created attachment 287333 [details]
Needs to pass svg/ tests
Created attachment 287344 [details]
Patch
Comment on attachment 287344 [details] Patch Attachment 287344 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1971753 New failing tests: fast/inline/absolute-positioned-inline-in-centred-block.html css2.1/t051202-c26-psudo-nest-00-c.html css2.1/t1508-c527-font-00-b.html Created attachment 287381 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 287344 [details] Patch Attachment 287344 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1973411 New failing tests: fast/inline/absolute-positioned-inline-in-centred-block.html css2.1/t051202-c26-psudo-nest-00-c.html css2.1/t1508-c527-font-00-b.html Created attachment 287396 [details]
Archive of layout-test-results from ews103 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 287344 [details] Patch Attachment 287344 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1974059 New failing tests: fast/inline/absolute-positioned-inline-in-centred-block.html css2.1/t051202-c26-psudo-nest-00-c.html css2.1/t1508-c527-font-00-b.html Created attachment 287415 [details]
Archive of layout-test-results from ews115 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 287428 [details]
Patch
Comment on attachment 287428 [details]
Patch
WK2 parts look fine.
Created attachment 287455 [details]
Patch
PLT says there is no regression. Comment on attachment 287455 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287455&action=review Please break this patch into: RenderText& changes int -> unsigned changes The rest. > Source/WebCore/ChangeLog:10 > + for a glyph advance to negative in the horizontal direction, and have large advances to be negative? > Source/WebCore/ChangeLog:29 > + This patch also exposes an SPI function in WebKit 2 which allows us to trigger this > + new measurement strategy at will. Because complex text measurement occurs deep "trigger at will" sounds weird. Maybe "switch at runtime" > Source/WebCore/platform/graphics/mac/ComplexTextController.h:159 > bool m_mayUseNaturalWritingDirection; > bool m_forTextEmphasis; Why don't these have initializers? > Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:134 > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101100) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED < 90000) > + // If the compiler isn't smart enough to optimize out the call to CTRunGetBaseAdvancesAndOrigins(), this will fail to link on > + // old OSes. This function isn't inlined, so the compiler may not be able to see through this call to know that the argument > + // is false. Therefore, we can just force it to false to make the linker succeed. > + ASSERT(!useLayoutSpecificAdvances); > + useLayoutSpecificAdvances = false; > +#endif I think it would be better to soft-link the symbol. Created attachment 287597 [details]
Patch
Attachment 287597 [details] did not pass style-queue:
ERROR: Source/WebCore/ChangeLog:31: 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 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 287600 [details]
Patch
(In reply to comment #25) > Comment on attachment 287455 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=287455&action=review > > Please break this patch into: > RenderText& changes > int -> unsigned changes > The rest. https://bugs.webkit.org/show_bug.cgi?id=161473 Created attachment 287705 [details]
Patch
Comment on attachment 287705 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287705&action=review > Source/WebCore/ChangeLog:16 > + In many text engines, glyph shaping actually can be split into two phases: adjusting How about in WebKit? > Source/WebCore/ChangeLog:28 > + No new tests just yet, because I have to create a font which exersizes the exersizes > Source/WebCore/platform/graphics/mac/ComplexTextController.h:99 > + const CGSize* baseAdvances() const { return m_baseAdvances; } I would like a comment here to explain what a "base advance" is. > Source/WebCore/platform/graphics/mac/ComplexTextController.h:151 > bool m_mayUseNaturalWritingDirection; > bool m_forTextEmphasis; Why don't these get initialized? > Source/WebCore/platform/graphics/mac/ComplexTextController.h:182 > int m_end; > > - CGFloat m_totalWidth; > + CGFloat m_totalWidth { 0 }; > > - float m_runWidthSoFar; > - unsigned m_numGlyphsSoFar; > - size_t m_currentRun; > - unsigned m_glyphInCurrentRun; > - unsigned m_characterInCurrentGlyph; > - float m_finalRoundingWidth; > - float m_expansion; > - float m_expansionPerOpportunity; > - float m_leadingExpansion; > + float m_runWidthSoFar { 0 }; > + unsigned m_numGlyphsSoFar { 0 }; > + unsigned m_currentRun { 0 }; > + unsigned m_glyphInCurrentRun { 0 }; > + unsigned m_characterInCurrentGlyph { 0 }; > + float m_expansion { 0 }; > + float m_expansionPerOpportunity { 0 }; > + float m_leadingExpansion { 0 }; > > HashSet<const Font*>* m_fallbackFonts; I'm sure we could improve packing here. > Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:138 > + CTRunGetBaseAdvancesAndOrigins(ctRun, CFRangeMake(0, 0), m_baseAdvancesVector.data(), m_glyphOrigins.data()); Does CFRangeMake(0, 0) have special meaning? Comment on attachment 287705 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287705&action=review >> Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:138 >> + CTRunGetBaseAdvancesAndOrigins(ctRun, CFRangeMake(0, 0), m_baseAdvancesVector.data(), m_glyphOrigins.data()); > > Does CFRangeMake(0, 0) have special meaning? it means "the whole thing" Committed r205373: <http://trac.webkit.org/changeset/205373> Reverted r205373 for reason: This change causes LayoutTest crashes under GuardMalloc Committed r205382: <http://trac.webkit.org/changeset/205382> Committed r205396: <http://trac.webkit.org/changeset/205396> Updating tests in https://trac.webkit.org/changeset/205398 This caused https://bugs.webkit.org/show_bug.cgi?id=165084 |