Bug 137550 - Remove second-to-last client of WebFontCache
Summary: Remove second-to-last client of WebFontCache
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-08 23:48 PDT by Myles C. Maxfield
Modified: 2014-10-09 14:35 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.46 KB, patch)
2014-10-08 23:55 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (5.52 KB, patch)
2014-10-08 23:59 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2014-10-08 23:48:46 PDT
Remove second-to-last client of WebFontCache
Comment 1 Myles C. Maxfield 2014-10-08 23:55:21 PDT
Created attachment 239520 [details]
Patch
Comment 2 WebKit Commit Bot 2014-10-08 23:57:07 PDT
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.
Comment 3 Myles C. Maxfield 2014-10-08 23:59:25 PDT
Created attachment 239521 [details]
Patch
Comment 4 Darin Adler 2014-10-09 09:32:40 PDT
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 5 Myles C. Maxfield 2014-10-09 11:39:08 PDT
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 6 WebKit Commit Bot 2014-10-09 12:16:08 PDT
Comment on attachment 239521 [details]
Patch

Clearing flags on attachment: 239521

Committed r174517: <http://trac.webkit.org/changeset/174517>
Comment 7 WebKit Commit Bot 2014-10-09 12:16:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Darin Adler 2014-10-09 14:35:03 PDT
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.