<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Memory leaks when autoLoadImages is off"
href="https://bugs.webkit.org/show_bug.cgi?id=154452">bug 154452</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 #272033 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Memory leaks when autoLoadImages is off"
href="https://bugs.webkit.org/show_bug.cgi?id=154452#c11">Comment # 11</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Memory leaks when autoLoadImages is off"
href="https://bugs.webkit.org/show_bug.cgi?id=154452">bug 154452</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=272033&action=diff" name="attach_272033" title="Patch">attachment 272033</a> <a href="attachment.cgi?id=272033&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=272033&action=review">https://bugs.webkit.org/attachment.cgi?id=272033&action=review</a>
Thanks for tackling this.
By checking autoLoadImages() directly, I think this creates code that is quite fragile, relying on effects that are far away from the code making the decision. We should seek a revised solution that doesn’t require ImageLoader to separately second guess decisions that CachedResourceLoader will make about doing image loads.
We also need to make a regression test for this or explain why creating one was impossible. A subtle bug like this needs a test even more than a more straightforard one.
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:94
> + , m_postponeLoadImage(false)</span >
We generally try to name boolean data members with predicates that fit the phrase "loader <xxx>".
In particular we try to avoid verb phrases, since booleans are not themselves actions. This would lead to a name like "is postponing image load", which would be m_isPostponingImageLoad.
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:238
> + // Remember if we're (auto) loading the image right away. See updatedHasPendingEvent().</span >
This comment is confusing to anyone who doesn’t already know what the code should do. I suggest a different comment.
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:239
> + m_postponeLoadImage = !document.cachedResourceLoader().autoLoadImages();</span >
Checking autoLoadImages seems likely to give an incorrect result for data URLs. Can we change this code so it relies on on the result of CachedResourceLoader::shouldPerformImageLoad or CachedResourceLoader::shouldDeferImageLoad or something along those lines? If not, then this code is going to give a bad result.
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:373
> + // <a class="bz_bug_link
bz_status_UNCONFIRMED "
title="UNCONFIRMED - Memory leak in CachedImage"
href="show_bug.cgi?id=17469">https://bugs.webkit.org/show_bug.cgi?id=17469</a> (maybe)
> + // <a class="bz_bug_link
bz_status_RESOLVED bz_closed"
title="RESOLVED FIXED - Memory leak for a protected element having pending events in ImageLoader"
href="show_bug.cgi?id=146538">https://bugs.webkit.org/show_bug.cgi?id=146538</a>
> + // <a href="https://bugreports.qt.io/browse/QTBUG-38857">https://bugreports.qt.io/browse/QTBUG-38857</a>
> + // <a href="https://github.com/ariya/phantomjs/issues/12903">https://github.com/ariya/phantomjs/issues/12903</a></span >
This list of URLs is not helpful. I don’t think we need to mention even one of these, since we normally rely on the change log for that, but if we absolutely feel the need to mention a specific bug report, we should point to just one of these, the one from the WebKit project, not from other projects, and it, in turn, can point to all the others.</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>