[webkit-reviews] review granted: [Bug 53123] Crashes loading pages when cancelling subresource loads through WebKit : [Attachment 80143] [PATCH] Take 2

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 53123: Crashes loading pages when cancelling subresource loads through
WebKit
https://bugs.webkit.org/show_bug.cgi?id=53123

Attachment 80143: [PATCH] Take 2
https://bugs.webkit.org/attachment.cgi?id=80143&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=80143&action=review

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:83
> +    , m_loadDonePendingActionTimer(this,
&CachedResourceLoader::loadDonePendingActionTimerFired)

i would remove the word "Pending" from these names. It doesn't add much.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:532
> +
> +    if (request) {
> +	   checkForPendingPreloads();
> +	   resourceLoadScheduler()->servePendingRequests();
> +    } else {
> +	   // If the request passed to this function is null, loadDone finished
synchronously from when
> +	   // the load was started, so we want to kick off our next set of
loads (via checkForPendingPreloads
> +	   // and servePendingRequests) asynchronously.
> +	   m_loadDonePendingActionTimer.startOneShot(0);
> +    }

This would read better with early return and the usual case on the left:

if (!request) {
   ....
   m_loadDonePendingActionTimer.startOneShot(0)
   return;
}
...

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:540
> +void
CachedResourceLoader::loadDonePendingActionTimerFired(Timer<CachedResourceLoade
r>*)
> +{
>      checkForPendingPreloads();
>      resourceLoadScheduler()->servePendingRequests();
>  }

You could put these calls to a function (performPostLoadActions() or something)
and call that from both loadDone() and here.