Summary: | Add fallback fonts to video captions stylesheet. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Per Arne Vollan <pvollan> | ||||||||
Component: | Media | Assignee: | Per Arne Vollan <pvollan> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bfulgham, commit-queue, eric.carlson, jonlee, mmaxfield, ryanhaddad | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Per Arne Vollan
2017-04-04 22:28:59 PDT
Created attachment 306253 [details]
Patch
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 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. (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. Committed <https://trac.webkit.org/changeset/215175/webkit>. (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 (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 Reverted r215175 for reason: This change caused a flaky crash in existing media tests. Committed r215185: <http://trac.webkit.org/changeset/215185> 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 Created attachment 306797 [details]
Patch
Created attachment 306813 [details]
Patch
(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. Did you figure out why the flakiness happened? (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 on attachment 306813 [details] Patch Clearing flags on attachment: 306813 Committed r215461: <http://trac.webkit.org/changeset/215461> All reviewed patches have been landed. Closing bug. |