Bug 62060 - Need to enable font cache purging in MemoryPressureHandler
Summary: Need to enable font cache purging in MemoryPressureHandler
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-03 15:29 PDT by Michael Saboff
Modified: 2011-06-07 13:49 PDT (History)
1 user (show)

See Also:


Attachments
Patch for review (3.49 KB, patch)
2011-06-03 15:52 PDT, Michael Saboff
ggaren: review-
Details | Formatted Diff | Diff
Updated patch with changes in response to reviewer's comments (5.37 KB, patch)
2011-06-06 16:05 PDT, Michael Saboff
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 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.
Comment 1 Michael Saboff 2011-06-03 15:52:43 PDT
Created attachment 95976 [details]
Patch for review
Comment 2 Geoffrey Garen 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.
Comment 3 Geoffrey Garen 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.
Comment 4 Michael Saboff 2011-06-06 16:05:17 PDT
Created attachment 96145 [details]
Updated patch with changes in response to reviewer's comments
Comment 5 Geoffrey Garen 2011-06-07 11:12:44 PDT
Comment on attachment 96145 [details]
Updated patch with changes in response to reviewer's comments

r=me
Comment 6 Michael Saboff 2011-06-07 13:49:19 PDT
Committed r88261: <http://trac.webkit.org/changeset/88261>