<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:mrobinson@webkit.org" title="Martin Robinson <mrobinson@webkit.org>"> <span class="fn">Martin Robinson</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - MemoryPressureHandler doesn't work if cgroups aren't present in Linux"
href="https://bugs.webkit.org/show_bug.cgi?id=155255">bug 155255</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #273468 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - MemoryPressureHandler doesn't work if cgroups aren't present in Linux"
href="https://bugs.webkit.org/show_bug.cgi?id=155255#c4">Comment # 4</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - MemoryPressureHandler doesn't work if cgroups aren't present in Linux"
href="https://bugs.webkit.org/show_bug.cgi?id=155255">bug 155255</a>
from <span class="vcard"><a class="email" href="mailto:mrobinson@webkit.org" title="Martin Robinson <mrobinson@webkit.org>"> <span class="fn">Martin Robinson</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=273468&action=diff" name="attach_273468" title="Patch">attachment 273468</a> <a href="attachment.cgi?id=273468&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=273468&action=review">https://bugs.webkit.org/attachment.cgi?id=273468&action=review</a>
Looks good, but I think it needs a little bit of cleanup. Thanks!
<span class="quote">> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:54
> +static const unsigned s_pollTimeSec = 1;</span >
We try to avoid abbreviated variable names in WebKit. How about s_pollingIntervalInSeconds?
<span class="quote">> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:56
> +static const size_t s_memCriticalLimit = 100 * KB * KB; // 100 MB
> +static const size_t s_memNonCriticalLimit = 300 * KB * KB; // 300 MB</span >
memory isn't abbreviated below, so it probably shouldn't be abbreviated here.
<span class="quote">> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:118
> + size_t memFree = static_cast<size_t>(-1); // bytes</span >
I think this comment could be expanded. What does "bytes" signify here. Comments in WebKit should be complete sentences with periods. If you are trying to say that this value is stored in bytes, a better method would simply be to call this variable "memoryFreeInBytes."
<span class="quote">> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:135
> + bool critical = memFree < s_memCriticalLimit;</span >
Maybe better to call this underMemoryPressure or something similar.
<span class="quote">> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:210
> + } while (false);</span >
Probably better to use a helper function here rather than do { } while.
<span class="quote">> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:225
> + do {
> + m_threadID = createThread(pollMemoryPressure, this, "WebCore: MemoryPressureHandler");
> + if (!m_threadID) {
> + logErrorAndCloseFDs("Failed to create a thread for MemoryPressureHandler");
> + break;
> + }
> +
> + LOG(MemoryPressure, "Vmstat memory pressure handler installed.");
> + vmstatPressureHandlerOk = true;
> + } while (false);</span >
Probably better to use a helper function here rather than do { } while.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>