Bug 170495 - Add fallback fonts to video captions stylesheet.
Summary: Add fallback fonts to video captions stylesheet.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media Elements (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-04 22:28 PDT by Per Arne Vollan
Modified: 2017-04-19 22:52 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.89 KB, patch)
2017-04-04 22:34 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (4.26 KB, patch)
2017-04-11 03:57 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (4.38 KB, patch)
2017-04-11 07:06 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2017-04-04 22:28:59 PDT
The kCTFontCascadeListAttribute key is used to obtain the cascade list for a font reference.
Comment 1 Per Arne Vollan 2017-04-04 22:34:50 PDT
Created attachment 306253 [details]
Patch
Comment 2 Eric Carlson 2017-04-05 10:42:18 PDT
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
Comment 3 Myles C. Maxfield 2017-04-07 16:45:48 PDT
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.
Comment 4 Per Arne Vollan 2017-04-10 02:55:33 PDT
(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.
Comment 5 Per Arne Vollan 2017-04-10 03:03:35 PDT
Committed <https://trac.webkit.org/changeset/215175/webkit>.
Comment 6 Ryan Haddad 2017-04-10 10:17:25 PDT
(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
Comment 7 Ryan Haddad 2017-04-10 10:26:24 PDT
(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
Comment 8 Ryan Haddad 2017-04-10 10:28:06 PDT
Reverted r215175 for reason:

This change caused a flaky crash in existing media tests.

Committed r215185: <http://trac.webkit.org/changeset/215185>
Comment 9 Myles C. Maxfield 2017-04-10 13:58:31 PDT
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
Comment 10 Per Arne Vollan 2017-04-11 03:57:13 PDT
Created attachment 306797 [details]
Patch
Comment 11 Per Arne Vollan 2017-04-11 07:06:15 PDT
Created attachment 306813 [details]
Patch
Comment 12 Per Arne Vollan 2017-04-11 07:07:34 PDT
(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.
Comment 13 Myles C. Maxfield 2017-04-11 17:19:33 PDT
Did you figure out why the flakiness happened?
Comment 14 Per Arne Vollan 2017-04-12 00:25:57 PDT
(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.
Comment 15 WebKit Commit Bot 2017-04-18 09:15:35 PDT
Comment on attachment 306813 [details]
Patch

Clearing flags on attachment: 306813

Committed r215461: <http://trac.webkit.org/changeset/215461>
Comment 16 WebKit Commit Bot 2017-04-18 09:15:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Per Arne Vollan 2017-04-19 22:52:59 PDT
rdar://problem/31725856