Bug 229644

Summary: document.fonts.size needs to update style so it doesn't return stale values
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, kangil.han, 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, 229911    
Attachments:
Description Flags
Patch
none
Patch
none
Patch darin: review+

Myles C. Maxfield
Reported 2021-08-28 00:27:48 PDT
document.fonts.size needs to update style so it doesn't return stale values
Attachments
Patch (6.86 KB, patch)
2021-08-28 00:28 PDT, Myles C. Maxfield 		no flags
DetailsFormatted DiffDiff
Patch (6.82 KB, patch)
2021-08-31 02:52 PDT, Myles C. Maxfield 		no flags
DetailsFormatted DiffDiff
Patch (6.88 KB, patch)
2021-08-31 12:05 PDT, Myles C. Maxfield 		darin: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Myles C. Maxfield
Comment 1 2021-08-28 00:28:41 PDT
Created attachment 436711 [details] Patch
Simon Fraser (smfr)
Comment 2 2021-08-30 16:13:46 PDT
Comment on attachment 436711 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436711&action=review > Source/WebCore/css/FontFaceSet.cpp:115 > + return m_backing->faceCountAfterUpdatingStyle(); Can a style update destroy |this|?
Myles C. Maxfield
Comment 3 2021-08-31 02:52:30 PDT
Created attachment 436862 [details] Patch
Myles C. Maxfield
Comment 4 2021-08-31 12:05:39 PDT
Created attachment 436923 [details] Patch
Darin Adler
Comment 5 2021-08-31 14:02:34 PDT
Comment on attachment 436923 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436923&action=review > Source/WebCore/css/CSSFontFaceSet.cpp:108 > + auto protect = makeRef(const_cast<CSSFontFaceSet&>(*this)); Self-protecting is not the correct pattern for our reference counting. Instead callers should be protecting the pointers of objects before they call functions. Please talk to Ryosuke about this if you don’t agree. Also, we don’t need makeRef for this any more; if we wanted a "protector" we could write: Ref protect = const_cast<CSSFontFaceSet&>(*this); But probably won’t be doing that. > Source/WebCore/css/CSSFontFaceSet.cpp:110 > + m_owningFontSelector->fontStyleUpdateNeeded(); Names here don’t make sense to me. This function’s name says "after updating style" which sounds asynchronous, but the name of the function we are calling says "update needed", which sounds like a request to schedule something in the future, asynchronously or lazily. > Source/WebCore/css/CSSFontSelector.h:92 > + void fontStyleUpdateNeeded(); Should rename this so it’s clear it’s a synchronous operation, not a passive notification that triggers something lazy in the future (as mentioned above). > Source/WebCore/css/FontFaceSet.cpp:115 > + return m_backing->faceCountAfterUpdatingStyle(); I find this function confusing. When would you call the "bad face count" function that doesn’t update style? Names really don’t make that clear. > Source/WebCore/dom/Document.cpp:7447 > + updateStyleIfNeeded(); // FIXME: This is unnecessary. Instead, the actual accessors in the FontFaceSet need to update style. Seems like a good direction.
Myles C. Maxfield
Comment 6 2021-09-01 02:38:22 PDT
Committed r281845 (241177@main): <https://commits.webkit.org/241177@main>
Radar WebKit Bug Importer
Comment 7 2021-09-01 02:39:18 PDT
<rdar://problem/82618851>
Note You need to log in before you can comment on or make changes to this bug.