[webkit-reviews] review granted: [Bug 185284] ResourceLoader::cancel() shouldn't synchronously fire load event on document : [Attachment 341743] Fixes the bug

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 185284: ResourceLoader::cancel() shouldn't synchronously fire load event on
document
https://bugs.webkit.org/show_bug.cgi?id=185284

Attachment 341743: Fixes the bug

https://bugs.webkit.org/attachment.cgi?id=341743&action=review




--- Comment #16 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 341743
  --> https://bugs.webkit.org/attachment.cgi?id=341743
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=341743&action=review

> Source/WebCore/loader/FrameLoader.cpp:799
> +    if (type == LoadCompletionType::Finish)
> +	   checkLoadComplete();
> +    else
> +	   scheduleCheckLoadComplete();

Could we in principle always do these asynchronously?

> Source/WebCore/loader/FrameLoader.cpp:896
> -void FrameLoader::checkTimerFired()
> +void FrameLoader::checkCompletenessNow()

I'd slightly prefer keeping the timerFired function and just calling the new
function from that. Canonical naming makes it easier to find what happens when
a timer fires.

> Source/WebCore/loader/FrameLoader.cpp:1782
> +    if (m_checkTimer.isActive()) {

You could early return instead.

> Source/WebCore/loader/FrameLoader.cpp:2417
> -	       DocumentLoader* dl = m_documentLoader.get();	       
> -	       if (!dl || (dl->isLoadingInAPISense() && !dl->isStopping()))
> +	       DocumentLoader* dl = m_documentLoader.get();
> +	       if (!dl || (dl->isLoadingInAPISense() && !dl->isStopping() &&
!m_checkingLoadCompleteForDetachment))
>		   return;

The awkwardly named local ('dl') seem unnecessary. Also this would read better
as two ifs, !m_documentLoader and the rest.

> Source/WebCore/loader/FrameLoader.h:141
> +    void stopAllLoadersAndCheckcompleteness();

Capitalization: stopAllLoadersAndCheckCompleteness

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:1302
> +void CachedResourceLoader::loadDone(LoadCompletionType type, bool
shouldPerformPostLoadActions)

I wonder if shouldPerformPostLoadActions could be merged into
LoadCompletionType? It is set when m_state is CancelledWhileInitializing which
sounds like a load completion type...

Could we at least ASSERT(shouldPerformPostLoadActions || type ==
LoadCompletionType::Cancel)?