<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Do not release OpenGL resource immediately when leaving accelerated compositing mode"
href="https://bugs.webkit.org/show_bug.cgi?id=167544">bug 167544</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 #300011 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Do not release OpenGL resource immediately when leaving accelerated compositing mode"
href="https://bugs.webkit.org/show_bug.cgi?id=167544#c3">Comment # 3</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Do not release OpenGL resource immediately when leaving accelerated compositing mode"
href="https://bugs.webkit.org/show_bug.cgi?id=167544">bug 167544</a>
from <span class="vcard"><a class="email" href="mailto:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=300011&action=diff" name="attach_300011" title="Patch">attachment 300011</a> <a href="attachment.cgi?id=300011&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=300011&action=review">https://bugs.webkit.org/attachment.cgi?id=300011&action=review</a>
Nice!
Now I know all this recent work to bring back on-demand accelerated composting is going to have a dramatic memory reduction benefit, but these changes seem risky and I don't think we should backport them to 2.14 now that we're nearly to the end of the release cycle. My recommendation is to let it bake in trunk at this point. Not just this commit, but the whole set. It's your call, of course.
<span class="quote">> Source/WebKit2/ChangeLog:24
> + starting a timer of 5 secons to discard it if not reused.</span >
seconds
<span class="quote">> Source/WebKit2/WebProcess/WebPage/AcceleratedDrawingArea.cpp:382
> + m_previousLayerTreeHost = WTFMove(m_layerTreeHost);</span >
If something ever goes wrong, better to have a null pointer dereference than a use after free, so I think you should set m_layerTreeHost to nullptr here, even if it's not strictly necessary. And it looks like it *is* necessary, since you have checks in several places to only use m_previousLayerTreeHost if m_layerTreeHost is null. If it's really not needed, please explain why.
<span class="quote">> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:229
> + m_isDiscardable = discardable;
> + if (m_isDiscardable) {
> + m_discardableSyncActions = OptionSet<DiscardableSyncActions>();
> + return;
> + }</span >
Personally, I would split the rest of this function below this point into a new function named ThreadedCoordinatedLayerTreeHost::applyDiscardableSyncActions.
<span class="quote">> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.h:111
> + enum class DiscardableSyncActions { UpdateSize = 1 << 1, UpdateViewport = 1 << 2, UpdateScale = 1 << 3, UpdateBackground = 1 << 4 };</span >
I would also prefer not to write enums on one line like this.</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>