Bug 145336 – [iOS] Arabic ligatures are broken in Google Maps

• Component: Text
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[iOS] Arabic ligatures are broken in Google Maps

[Myles C. Maxfield]

Created attachment 253632 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/20689607>

[Build Bot]

Comment on attachment 253632 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

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

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

[Build Bot]

Comment on attachment 253632 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 253637 [details]
Archive of layout-test-results from ews101 for mac-mavericks

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

[Ryosuke Niwa]

Comment on attachment 253632 [details]
Patch

Looks like it fails to build on Mac?

[Darin Adler]

Comment on attachment 253632 [details]
Patch

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

Lots of tests failing, so I am not comfortable saying review+, but otherwise I would have.

> Source/WebCore/platform/graphics/mac/FontCustomPlatformData.cpp:36
> +    return FontPlatformData(CTFontCreateWithFontDescriptor(m_fontDescriptor.get(), size, nullptr), size, bold, italic, orientation, widthVariant);

Needs to say adoptCF(xxx).get() to avoid leaking the CTFont.

> Source/WebCore/platform/graphics/mac/FontCustomPlatformData.h:40
> +    explicit FontCustomPlatformData(CTFontDescriptorRef fontDescriptor)

Not sure this really needs to be explicit.

[Myles C. Maxfield]

Comment on attachment 253632 [details]
Patch

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

>> Source/WebCore/platform/graphics/mac/FontCustomPlatformData.cpp:36
>> +    return FontPlatformData(CTFontCreateWithFontDescriptor(m_fontDescriptor.get(), size, nullptr), size, bold, italic, orientation, widthVariant);
> 
> Needs to say adoptCF(xxx).get() to avoid leaking the CTFont.

Man, I get this wrong every time :(((((

[Myles C. Maxfield]

Crashes on Mavericks caused by <rdar://problem/16980736>. There's no workaround, so I'll need to gate this change on the version of the OS.

[Myles C. Maxfield]

Created attachment 253771 [details]
Patch

[Darin Adler]

Comment on attachment 253771 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/FontCustomPlatformData.h:34
> +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED <= 80000) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED <= 101000)

Reminder of something we talked about in person:

This could either be < instead of <= or list an older version, because the bug we are working around does *not* exist in iOS 8 and OS X 10.10, but only in older ones. Something you were planning to verify.

[Myles C. Maxfield]

Committed r184899: <http://trac.webkit.org/changeset/184899>

[Alexey Proskuryakov]

This broke platform/mac/fast/ruby/ruby-expansion-cjk-2.html on Yosemite.

[Darin Adler]

(In reply to comment #14)
> This broke platform/mac/fast/ruby/ruby-expansion-cjk-2.html on Yosemite.

Best resolution for that is likely to not use the new code path on Yosemite, only on even newer OS X.

[Myles C. Maxfield]

(In reply to comment #14)
> This broke platform/mac/fast/ruby/ruby-expansion-cjk-2.html on Yosemite.

Yes, I saw. The troubling thing is that the test shouldn't be sensitive to WK1 vs WK2, which means that I don't understand something. I think it's valuable for me to figure out what's going on rather than just take the old code path on yosemite.

[Daniel Bates]

After talking with Myles Maxfield on IRC today (05/27), we chose to mark the test platform/mac/fast/ruby/ruby-expansion-cjk-2.html as failing on Yosemite.

I also removed references to tests fast/ruby/ruby-expansion-cjk*.html from the TestExpectation files of other ports since these files were moved to platform/mac in <http://trac.webkit.org/changeset/184899> and hence are considered Mac-specific.

Committed these changes in <https://trac.webkit.org/changeset/184914>.

[Alexey Proskuryakov]

As discussed in bug 143378, I think that moving tests to platform/mac is not the best thing to do. Keeping track of expectations and expected results becomes very difficult, as we get paths like platform/mac/platform/mac-wk2/...

[Darin Adler]

(In reply to comment #18)
> As discussed in bug 143378, I think that moving tests to platform/mac is not
> the best thing to do. Keeping track of expectations and expected results
> becomes very difficult, as we get paths like
> platform/mac/platform/mac-wk2/...

Is that going to be true for the long term? What’s the best way to handle Mac-specific tests?

[Daniel Bates]

Committed updated test expectations in <http://trac.webkit.org/changeset/184915> per IRC discussion with Alexey Proskuryakov.

[Alexey Proskuryakov]

> Is that going to be true for the long term? What’s the best way to handle Mac-specific tests?

I think that the best way is to have subdirectories named after specific technologies (e.g. LayoutTests/fast/text/coretext), and to skip those directories on other platforms. When that doesn't work well, we could have directories like LayoutTests/fast/text/mac, although that would always beg the question of whether the tests run on iOS.

I want to move all tests from LayoutTests/platform/ and make run-webkit-tests not look for tests there, but I don't know when that would materialize.