Bug 62060

Summary: Need to enable font cache purging in MemoryPressureHandler
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: WebCore Misc.Assignee: Michael Saboff <msaboff>
Status: RESOLVED FIXED    
Severity: Normal CC: ggaren
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch for review
ggaren: review-
Updated patch with changes in response to reviewer's comments ggaren: review+

Michael Saboff
Reported 2011-06-03 15:29:17 PDT
This was split out from https://bugs.webkit.org/show_bug.cgi?id=61875. Need to add call to fontCache()->purgeInactiveFontData(); in MemoryPressureHandler::respondToMemoryPressure(). There is another change discovered to add as well, prevent the ::install() method from multiple invocation.
Attachments
Patch for review (3.49 KB, patch)
2011-06-03 15:52 PDT, Michael Saboff 		ggaren: review-
DetailsFormatted DiffDiff
Updated patch with changes in response to reviewer's comments (5.37 KB, patch)
2011-06-06 16:05 PDT, Michael Saboff 		ggaren: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Michael Saboff
Comment 1 2011-06-03 15:52:43 PDT
Created attachment 95976 [details] Patch for review
Geoffrey Garen
Comment 2 2011-06-06 14:16:53 PDT
Comment on attachment 95976 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=95976&action=review r- because I don't think it's good to have two tests for the same thing. > Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:67 > + if (m_installed) > + return; > + > if (!_cache_event_source) { > dispatch_async(dispatch_get_main_queue(), ^{ > _cache_event_source = dispatch_source_create(DISPATCH_SOURCE_TYPE_VM, 0, DISPATCH_VM_PRESSURE, dispatch_get_main_queue()); There is an existing "I've been installed" test: "if (!_cache_event_source)". You're adding a second: "if (m_installed)". I don't think it's good to have two tests for the same thing. Why isn't the current test sufficient? Is it because initialization is asynchronous, so multiple calls to install() can occur before initialization? If so, you can avoid two tests for the same thing either by removing the initialization delay, or by keeping the delay and adding a null check for _cache_event_source inside the delayed code. BTW, an early return after the _cache_event_source test would reduce the indentation level of this code, making it more readable.
Geoffrey Garen
Comment 3 2011-06-06 14:30:03 PDT
...another option is to have install() initialize the memoryPressureHandler singleton, and have the singleton's constructor do all the dispatch registration.
Michael Saboff
Comment 4 2011-06-06 16:05:17 PDT
Created attachment 96145 [details] Updated patch with changes in response to reviewer's comments
Geoffrey Garen
Comment 5 2011-06-07 11:12:44 PDT
Comment on attachment 96145 [details] Updated patch with changes in response to reviewer's comments r=me
Michael Saboff
Comment 6 2011-06-07 13:49:19 PDT
Committed r88261: <http://trac.webkit.org/changeset/88261>
Note You need to log in before you can comment on or make changes to this bug.