[webkit-reviews] review denied: [Bug 24152] Move the logic to do the unconfirmed message count and pending activity setting from WorkerMessagingProxy to Worker. : [Attachment 28016] Proposed Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 24152: Move the logic to do the unconfirmed message count and pending
activity setting from WorkerMessagingProxy to Worker.
https://bugs.webkit.org/show_bug.cgi?id=24152

Attachment 28016: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=28016&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+    bool hasPendingActivity = !m_askedToTerminate &&
(m_unconfirmedMessageCount || m_workerContextHadPendingActivity);
+    return hasPendingActivity || ActiveDOMObject::hasPendingActivity();

This variable name is not helpful. I suggest workerContextHasPendingActivity.

+	 bool m_askedToTerminate;

Why duplicate this member variable in Worker when it's already available in the
proxy?

-	 m_unconfirmedMessageCount = taskCount;
-	 m_workerThreadHadPendingActivity = true; // Worker initialization
means a pending activity.
+	 m_workerObject->reportPendingActivity(false, true); // Worker
initialization means a pending activity.

This patch makes workers that failed to load leak. Previously,
m_unconfirmedMessageCount was only set to non-zero after creating the thread,
so messages posted to a worker that never spawned a thread didn't count as
activity. Consider the following test:

var worker = new Worker("does-not-exist.js");
worker.postMessage(...);
worker = null;