Bug 170495 – Add fallback fonts to video captions stylesheet.

• Component: Media
• Status: RESOLVED FIXED

[Per Arne Vollan]

The kCTFontCascadeListAttribute key is used to obtain the cascade list for a font reference.

[Per Arne Vollan]

Created attachment 306253 [details]
Patch

[Eric Carlson]

Comment on attachment 306253 [details]
Patch

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

This looks OK to me, but someone that understands more about fonts (Myles...) should review.

> Source/WebCore/ChangeLog:10
> +        I have not added a test, since CaptionUserPreferences::testingMode() returns true whe running tests,

Nit: whe -> when

[Myles C. Maxfield]

Comment on attachment 306253 [details]
Patch

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

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:87
> +#define kCTFontCascadeListAttribute getkCTFontCascadeListAttribute()

Why are we soft linking CoreText? It's available on all platforms which also have MACaptionAppearanceCopyFontDescriptorForStyle().

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:437
> +    auto cascadeList = adoptCF(CTFontDescriptorCopyAttribute(font.get(), kCTFontCascadeListAttribute));

Put the static_cast<CFArrayRef>() here rather than on line 440.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:441
> +        auto fontCascadeName = adoptCF(CTFontDescriptorCopyAttribute(fontCascade, kCTFontNameAttribute));

Ditto.

[Per Arne Vollan]

(In reply to Myles C. Maxfield from comment #3)
> Comment on attachment 306253 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306253&action=review
> 
> > Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:87
> > +#define kCTFontCascadeListAttribute getkCTFontCascadeListAttribute()
> 
> Why are we soft linking CoreText? It's available on all platforms which also
> have MACaptionAppearanceCopyFontDescriptorForStyle().
> 

I added this to fix the Windows build.

> > Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:437
> > +    auto cascadeList = adoptCF(CTFontDescriptorCopyAttribute(font.get(), kCTFontCascadeListAttribute));
> 
> Put the static_cast<CFArrayRef>() here rather than on line 440.
> 
> > Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:441
> > +        auto fontCascadeName = adoptCF(CTFontDescriptorCopyAttribute(fontCascade, kCTFontNameAttribute));
> 
> Ditto.

Thanks for reviewing! I will update the patch before landing.

[Per Arne Vollan]

Committed <https://trac.webkit.org/changeset/215175/webkit>.

[Ryan Haddad]

(In reply to Per Arne Vollan from comment #5)
> Committed <https://trac.webkit.org/changeset/215175/webkit>.

This change introduced a flaky crash seen with LayoutTest media/modern-media-controls/tracks-support/tracks-support-auto-text-track.html

https://build.webkit.org/results/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/r215180%20(579)/results.html

[Ryan Haddad]

(In reply to Ryan Haddad from comment #6)
> (In reply to Per Arne Vollan from comment #5)
> > Committed <https://trac.webkit.org/changeset/215175/webkit>.
> 
> This change introduced a flaky crash seen with LayoutTest
> media/modern-media-controls/tracks-support/tracks-support-auto-text-track.
> html
> 
> https://build.webkit.org/results/
> Apple%20El%20Capitan%20Release%20WK2%20(Tests)/r215180%20(579)/results.html

Another example seen here with fast/performance/performance-now-crash-on-navigated-window.html. The crashlog is actually attributed to fast/media/window-oncuechange.html

https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r215181%20(433)/results.html

[Ryan Haddad]

Reverted r215175 for reason:

This change caused a flaky crash in existing media tests.

Committed r215185: <http://trac.webkit.org/changeset/215185>

[Myles C. Maxfield]

Comment on attachment 306253 [details]
Patch

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

>>> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:87
>>> +#define kCTFontCascadeListAttribute getkCTFontCascadeListAttribute()
>> 
>> Why are we soft linking CoreText? It's available on all platforms which also have MACaptionAppearanceCopyFontDescriptorForStyle().
> 
> I added this to fix the Windows build.

You shouldn't need soft linking to do this. We use Core Text stuff elsewhere in our Windows port. You should remove this soft linking stuff (including the part you didn't write) and instead put the declarations in Source/WebCore/platform/spi/win/CoreTextSPIWin.h

[Per Arne Vollan]

Created attachment 306797 [details]
Patch

[Per Arne Vollan]

Created attachment 306813 [details]
Patch

[Per Arne Vollan]

(In reply to Myles C. Maxfield from comment #9)
> Comment on attachment 306253 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306253&action=review
> 
> >>> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:87
> >>> +#define kCTFontCascadeListAttribute getkCTFontCascadeListAttribute()
> >> 
> >> Why are we soft linking CoreText? It's available on all platforms which also have MACaptionAppearanceCopyFontDescriptorForStyle().
> > 
> > I added this to fix the Windows build.
> 
> You shouldn't need soft linking to do this. We use Core Text stuff elsewhere
> in our Windows port. You should remove this soft linking stuff (including
> the part you didn't write) and instead put the declarations in
> Source/WebCore/platform/spi/win/CoreTextSPIWin.h

Thanks! I have removed the soft linking, and added null pointer checks.

[Myles C. Maxfield]

Did you figure out why the flakiness happened?

[Per Arne Vollan]

(In reply to Myles C. Maxfield from comment #13)
> Did you figure out why the flakiness happened?

Yes, the problem was that the call CTFontDescriptorCopyAttribute(font.get(), kCTFontCascadeListAttribute) returned null in some cases. I added null pointer checks in the latest patch.

[WebKit Commit Bot]

Comment on attachment 306813 [details]
Patch

Clearing flags on attachment: 306813

Committed r215461: <http://trac.webkit.org/changeset/215461>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Per Arne Vollan]

rdar://problem/31725856