Bug 160892 – [Cocoa] Distinguish between paint advances and base advances

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Distinguish between paint advances and base advances

[Myles C. Maxfield]

Created attachment 286153 [details]
WIP

[Myles C. Maxfield]

Created attachment 286655 [details]
Patch

[Myles C. Maxfield]

Created attachment 286692 [details]
WIP

[Myles C. Maxfield]

Created attachment 286813 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Myles C. Maxfield]

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

[Myles C. Maxfield]

Created attachment 287171 [details]
WIP

[Myles C. Maxfield]

Created attachment 287333 [details]
Needs to pass svg/ tests

[Myles C. Maxfield]

Created attachment 287344 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Myles C. Maxfield]

Created attachment 287428 [details]
Patch

[Anders Carlsson]

Comment on attachment 287428 [details]
Patch

WK2 parts look fine.

[Myles C. Maxfield]

Created attachment 287455 [details]
Patch

[Myles C. Maxfield]

PLT says there is no regression.

[Simon Fraser (smfr)]

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.

[Myles C. Maxfield]

Created attachment 287597 [details]
Patch

[WebKit Commit Bot]

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.

[Myles C. Maxfield]

Created attachment 287600 [details]
Patch

[Myles C. Maxfield]

(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

[Myles C. Maxfield]

Created attachment 287705 [details]
Patch

[Simon Fraser (smfr)]

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?

[Myles C. Maxfield]

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"

[Myles C. Maxfield]

Committed r205373: <http://trac.webkit.org/changeset/205373>

[Ryan Haddad]

Reverted r205373 for reason:

This change causes LayoutTest crashes under GuardMalloc

Committed r205382: <http://trac.webkit.org/changeset/205382>

[Myles C. Maxfield]

Committed r205396: <http://trac.webkit.org/changeset/205396>

[Myles C. Maxfield]

Updating tests in https://trac.webkit.org/changeset/205398

[Myles C. Maxfield]

This caused https://bugs.webkit.org/show_bug.cgi?id=165084