Bug 137550 – Remove second-to-last client of WebFontCache

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Remove second-to-last client of WebFontCache

[Myles C. Maxfield]

Created attachment 239520 [details]
Patch

[WebKit Commit Bot]

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.

[Myles C. Maxfield]

Created attachment 239521 [details]
Patch

[Darin Adler]

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?

[Myles C. Maxfield]

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().

[WebKit Commit Bot]

Comment on attachment 239521 [details]
Patch

Clearing flags on attachment: 239521

Committed r174517: <http://trac.webkit.org/changeset/174517>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.