Remove second-to-last client of WebFontCache
Created attachment 239520 [details] Patch
Attachment 239520 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 239521 [details] Patch
Comment on attachment 239521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239521&action=review > Source/WebKit/mac/WebView/WebHTMLView.mm:4897 > // Find the font the same way the rendering code would later if it encountered this CSS. This whole “find font” sequence, in the end, is just used to get a font name. I think we should just refactor into a helper that returns an NSString rather than having this tricky dance here in the middle of a function that’s already doing so many other things. > Source/WebKit/mac/WebView/WebHTMLView.mm:4902 > + FontCachePurgePreventer purgePreventer; Why is this needed? This seems like a really tricky class to use right if this is really needed for a simple case like this. Can purging really happen in the tiny window before we call platformData().font() and then fontName?
Comment on attachment 239521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239521&action=review >> Source/WebKit/mac/WebView/WebHTMLView.mm:4897 >> // Find the font the same way the rendering code would later if it encountered this CSS. > > This whole “find font” sequence, in the end, is just used to get a font name. I think we should just refactor into a helper that returns an NSString rather than having this tricky dance here in the middle of a function that’s already doing so many other things. I agree with you, however, I think that doing this kind of refactor would cut through many levels of function calls, including splitting WebFontCache which is going away soon. I think that such a refactoring would ultimately introduce more complexity than this patch adds. >> Source/WebKit/mac/WebView/WebHTMLView.mm:4902 >> + FontCachePurgePreventer purgePreventer; > > Why is this needed? This seems like a really tricky class to use right if this is really needed for a simple case like this. Can purging really happen in the tiny window before we call platformData().font() and then fontName? There is an assert in getCachedFontData().
Comment on attachment 239521 [details] Patch Clearing flags on attachment: 239521 Committed r174517: <http://trac.webkit.org/changeset/174517>
All reviewed patches have been landed. Closing bug.
Comment on attachment 239521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239521&action=review >>> Source/WebKit/mac/WebView/WebHTMLView.mm:4897 >>> // Find the font the same way the rendering code would later if it encountered this CSS. >> >> This whole “find font” sequence, in the end, is just used to get a font name. I think we should just refactor into a helper that returns an NSString rather than having this tricky dance here in the middle of a function that’s already doing so many other things. > > I agree with you, however, I think that doing this kind of refactor would cut through many levels of function calls, including splitting WebFontCache which is going away soon. I think that such a refactoring would ultimately introduce more complexity than this patch adds. I think you misunderstood. I just meant a helper function right here in this source file.