[webkit-reviews] review denied: [Bug 62060] Need to enable font cache purging in MemoryPressureHandler : [Attachment 95976] Patch for review

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 62060: Need to enable font cache purging in MemoryPressureHandler
https://bugs.webkit.org/show_bug.cgi?id=62060

Attachment 95976: Patch for review
https://bugs.webkit.org/attachment.cgi?id=95976&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.