[webkit-reviews] review denied: [Bug 49401] Defer ScriptExecutionContext::Task's in Document when page loading is deferred : [Attachment 76407] finally found the warning that fails build

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 76407: finally found the warning that fails build
https://bugs.webkit.org/attachment.cgi?id=76407&action=review

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

> WebCore/dom/Document.cpp:600
> +    for (size_t i = 0; i < m_pendingTasks.size(); ++i)
> +	   delete m_pendingTasks[i];

There’s a function for this called deleteAllValues.

But more importantly, Vector<OwnPtr> works just fine, so we should use it here
rather than ll those leakPtr calls.

> WebCore/dom/Document.cpp:4696
> -static void performTask(void* ctx)
> +void Document::didReceiveTask(void* ctx)

Lets use a word here instead of the “ctx” abbreviation. Maybe contextPtr? Yes,
that uses an abbrevation too, so I may be a hypocrite.

> WebCore/dom/Document.cpp:4700
> +    OwnPtr<PerformTaskContext>
context(reinterpret_cast<PerformTaskContext*>(ctx));

This should be using adoptPtr and should use static_cast rather than
reinterpret_cast.

> WebCore/dom/Document.cpp:4728
> +	   OwnPtr<Task> task(m_pendingTasks[0]);

This should use adoptPtr, but would actually use release if it was a
Vector<OwnPtr>.

Since we use this by adding to the end and removing from the start, this should
be a Deque rather than a Vector. If it was a Deque we could probably use the
takeFirst function rather than writing it like this.

> WebCore/dom/Document.cpp:4738
> +    suspendActiveDOMObjects(ActiveDOMObject::WillShowDialog);
> +    asyncScriptRunner()->suspend();
> +    m_pendingTasksTimer.stop();

Perhaps suspendActiveDOMObjects could be made private now?

> WebCore/dom/Document.cpp:4746
> +    resumeActiveDOMObjects();

Perhaps resumeActiveDOMObjects could be made private now?

> WebCore/dom/Document.h:1067
> +    virtual void willDeferLoading();
> +    virtual void didResumeLoading();

Why are these functions virtual?

It would be better if these names paired up a bit better.

> WebCore/page/PageGroupLoadDeferrer.cpp:50
> -		   for (Frame* frame = otherPage->mainFrame(); frame; frame =
frame->tree()->traverseNext()) {
> -		      
frame->document()->suspendActiveDOMObjects(ActiveDOMObject::WillShowDialog);
> -		       frame->document()->asyncScriptRunner()->suspend();
> -		   }
> +		   for (Frame* frame = otherPage->mainFrame(); frame; frame =
frame->tree()->traverseNext())
> +		       frame->document()->willDeferLoading();

Each frame already gets a call about deferring loading in response to the
Page::setDefersLoading call. Specifically, FrameLoader::setDefersLoading. Could
we put the responsibility for calling these functions there? Is there some
other call site that calls setDefersLoading but does not want this additional
work to be done?

I find the willDefer and didResume names a little confusing and this is
existing code is strangely disconnected from the main setDefersLoading feature.