[webkit-reviews] review granted: [Bug 49401] Defer ScriptExecutionContext::Task's in Document when page loading is deferred : [Attachment 76562] rerun builtbot

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yong Li <yong.li.webkit at gmail.com>'s
request for review:
Bug 49401: Defer ScriptExecutionContext::Task's in Document when page loading
is deferred
https://bugs.webkit.org/show_bug.cgi?id=49401

Attachment 76562: rerun builtbot
https://bugs.webkit.org/attachment.cgi?id=76562&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=76562&action=review

Change looks OK, but this is a super-tricky area; I’d prefer it if we had more
tests.

> WebCore/dom/Document.cpp:4693
> +void Document::didReceiveTask(void* rawContext)

I used the name “untypedContext” for this when I recently wrote some code using
the same idiom. Your name is good too. Just wanted to share mine.

> WebCore/dom/Document.cpp:4710
> +	   document->m_pendingTasks.append(context->task.leakPtr());

The leakPtr here is not what you want. You want to call release here instead.
This only compiles because of LOOSE_OWN_PTR.

> WebCore/dom/Document.cpp:4728
> +    while (!m_pendingTasks.isEmpty()) {
> +	   OwnPtr<Task> task = m_pendingTasks[0].release();
> +	   m_pendingTasks.remove(0);
> +	   task->performTask(this);
> +    }

This function is an argument for using the Deque class. Deque has a takeFirst
function that is perfect for idioms like this.

> WebCore/loader/FrameLoader.cpp:260
> +    // This code is not logically part of load deferring, but we do not want
JS code executed beneath modal
> +    // windows or sheets, which is exactly when PageGroupLoadDeferrer is
used.
> +    // NOTE: if PageGroupLoadDeferrer is ever used for tasks other than
showing a modal window or sheet,

It no longer makes sense to mention PageGroupLoadDeferrer here. This comment is
now far enough away from PageGroupLoadDeferrer that people will not see the
comment.

The word “if” should be capitalized here at the start of a sentence.

> WebCore/loader/FrameLoader.cpp:265
> +    if (defers)
> +	   m_frame->document()->suspendScheduledTasks();
> +    else
> +	   m_frame->document()->resumeScheduledTasks();

What’s the guarantee that we won’t change the document of a frame while
suspended? If we don’t have that guarantee we could end up calling
suspendScheduledTasks on a document and never calling resumeScheduledTasks on
it.