Bug 229643

Summary: CSSFontFaceSet.clear() should not clear CSS-connected members
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, macpherson, menard, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 229850    
Attachments:
Description Flags
Patch
none
Patch darin: review+

Description Myles C. Maxfield 2021-08-27 23:54:26 PDT
CSSFontFaceSet.clear() should not clear CSS-connected members
Comment 1 Myles C. Maxfield 2021-08-27 23:55:32 PDT
Created attachment 436709 [details]
Patch
Comment 2 Myles C. Maxfield 2021-08-27 23:59:35 PDT
Created attachment 436710 [details]
Patch
Comment 3 Simon Fraser (smfr) 2021-08-30 16:11:45 PDT
Comment on attachment 436710 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Test: fast/text/FontFaceSet-clear-css-connected.html

Can this be a WPT?
Comment 4 Myles C. Maxfield 2021-08-31 02:19:14 PDT
Comment on attachment 436710 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        Test: fast/text/FontFaceSet-clear-css-connected.html
> 
> Can this be a WPT?

https://github.com/web-platform-tests/wpt/pull/30254
Comment 5 Darin Adler 2021-08-31 11:12:06 PDT
Comment on attachment 436710 [details]
Patch

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

> Source/WebCore/css/FontFaceSet.cpp:137
> +        m_backing->remove(m_backing.get()[facesPartitionIndex]);

Is this high-performance enough? Might be worth thinking that through.
Comment 6 Myles C. Maxfield 2021-09-01 00:44:54 PDT
Comment on attachment 436710 [details]
Patch

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

>> Source/WebCore/css/FontFaceSet.cpp:137
>> +        m_backing->remove(m_backing.get()[facesPartitionIndex]);
> 
> Is this high-performance enough? Might be worth thinking that through.

I've never actually seen any website ever call this function, so I think sharing code and guaranteeing that all the necessary work in remove() runs is probably more valuable than squeezing more performance out of this function.
Comment 7 Myles C. Maxfield 2021-09-01 00:45:13 PDT
Comment on attachment 436710 [details]
Patch

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

>>> Source/WebCore/css/FontFaceSet.cpp:137
>>> +        m_backing->remove(m_backing.get()[facesPartitionIndex]);
>> 
>> Is this high-performance enough? Might be worth thinking that through.
> 
> I've never actually seen any website ever call this function, so I think sharing code and guaranteeing that all the necessary work in remove() runs is probably more valuable than squeezing more performance out of this function.

I guess I can at least iterate backwards, though.
Comment 8 Myles C. Maxfield 2021-09-01 01:59:56 PDT
Committed r281842 (241175@main): <https://commits.webkit.org/241175@main>
Comment 9 Radar WebKit Bug Importer 2021-09-01 02:00:20 PDT
<rdar://problem/82617907>