Bug 145569 – SoftBank Emoji are not transformed by shaping when in a run of their own

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

SoftBank Emoji don't render using our complex text codepath

[Myles C. Maxfield]

Created attachment 254105 [details]
Patch

[Myles C. Maxfield]

Created attachment 254106 [details]
Patch

[Enrica Casucci]

Comment on attachment 254106 [details]
Patch

The title of the bug is inconsistent with the implementation. I believe you want to use the complex path. You can write a test case or simply add this character to one of the existing tests. The layout information should be enough to show that this is actually drawing correctly. Please add the test before submitting and fix the bug title.

[Myles C. Maxfield]

Created attachment 254115 [details]
Patch

[Myles C. Maxfield]

Created attachment 254116 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 254116 [details]
Patch for landing

Rejecting attachment 254116 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 254116, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/6241245944872960

[Myles C. Maxfield]

Created attachment 254138 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 254138 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254138&action=review

> Source/WebCore/platform/graphics/WidthIterator.cpp:196
> +        || (glyphBufferSize > lastGlyphCount + 1 && m_run.length() > 1 && (m_typesettingFeatures & (Kerning | Ligatures)));

This is not correct. We need to update lastGlyphCount in this case.

[Myles C. Maxfield]

Comment on attachment 254138 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254138&action=review

> Source/WebCore/platform/graphics/WidthIterator.cpp:258
> +            if (shouldApplyFontTransforms(glyphBuffer->size(), lastGlyphCount, previousCharacter)) {

This is going to crash a lot.

[Myles C. Maxfield]

Created attachment 254141 [details]
Patch

[Build Bot]

Comment on attachment 254141 [details]
Patch

Attachment 254141 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6148784962666496

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 254143 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 254141 [details]
Patch

Attachment 254141 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6050187143282688

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 254147 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Myles C. Maxfield]

Comment on attachment 254141 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254141&action=review

> Source/WebCore/platform/graphics/WidthIterator.cpp:357
> +        if (shouldApplyFontTransforms(glyphBuffer, lastGlyphCount, previousCharacter) && glyphBuffer && FontCascade::treatAsSpace(character)) {

This screwed everything up :(

[Myles C. Maxfield]

Created attachment 254198 [details]
Patch

[Dean Jackson]

Comment on attachment 254198 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254198&action=review

> Source/WebCore/platform/graphics/WidthIterator.cpp:199
> +    if (m_run.length() <= 1 || !(m_typesettingFeatures & (Kerning | Ligatures)))

I probably forget this, but you didn't mean < 1 here right?

[Darin Adler]

Comment on attachment 254198 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254198&action=review

> Source/WTF/wtf/unicode/CharacterNames.h:95
> +const UChar softBankEmojiStart = 0xE001;
> +const UChar softBankEmojiEnd = 0xE537;

This file is kept in alphabetical order, so these are misplaced.

This file currently only includes names that are taken directly from the Unicode specification and names of actual characters, not range boundaries. So I’m not sure these constants belong in this file and even if we do put them in this file I suggest we put them in a separate paragraph.

I think this would probably be better as an inline function isSoftBankEmoji rather than two constants.

> Source/WebCore/platform/graphics/WidthIterator.h:93
> +    // The returned optional represents whether or not we should apply font transforms, and the returned bool represents whether or not
> +    // we should force shaping to occur (even if guards in applyFontTransforms() say it's unnecessary)

That is super confusing. Can we return a struct with named booleans or an enum rather than this strange tri-state thing?

[Dean Jackson]

Comment on attachment 254198 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254198&action=review

>> Source/WebCore/platform/graphics/WidthIterator.cpp:199
>> +    if (m_run.length() <= 1 || !(m_typesettingFeatures & (Kerning | Ligatures)))
> 
> I probably forget this, but you didn't mean < 1 here right?

After chatting, we think it maybe should be > 1

[Myles C. Maxfield]

<rdar://problem/20671711>

[Myles C. Maxfield]

Committed r185175: <http://trac.webkit.org/changeset/185175>

[Myles C. Maxfield]

Committed r185176: <http://trac.webkit.org/changeset/185176>