Summary: | Remove second-to-last client of WebFontCache | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||
Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cdumez, commit-queue, dino, jonlee, simon.fraser, thorton | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Myles C. Maxfield
2014-10-08 23:48:46 PDT
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. |