[webkit-reviews] review denied: [Bug 34513] SharedWorkerScriptLoader should not be an ActiveDOMObject : [Attachment 47996] Fixed style warning

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Andrew Wilson
<atwilson at chromium.org>'s request for review:
Bug 34513: SharedWorkerScriptLoader should not be an ActiveDOMObject
https://bugs.webkit.org/show_bug.cgi?id=34513

Attachment 47996: Fixed style warning
https://bugs.webkit.org/attachment.cgi?id=47996&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+    // FIXME: This method is not guaranteed to be invoked if we are loading
from WorkerContext (see the comment in WorkerScriptLoaderClient).
+    // We need to address this before supporting nested workers.

Instead of calling it "the comment", I'd say "comment for
WorkerScriptLoaderClient::notifyFinished()".

-    virtual void contextDestroyed();
+    static void contextDetached(ScriptExecutionContext*);

Unlike contextDestroyed(), contextDetached() is called to achieve a specific
effect. I'd consider calling it something like stopAllLoadersForContext().

+    const ScriptExecutionContext* parentContext() { return
m_worker->scriptExecutionContext(); }

Is "parent context" appropriate terminology here? In cross-platform code, I'd
ask to rename it to workerContext(), but maybe in Chromium parts, this is
already used elsewhere.

+    bool m_loadPending;

Doesn't "pending" mean "about to start", at least in WebCore parlance? This
looks more like "m_loading" to me.

None of my comments necessarily qualify for r-, the patch looks good. Marking
r- for you to consider making the changes, but feel free to mark for review
again if you like the current version more than any variations.