Bug 34513 – SharedWorkerScriptLoader should not be an ActiveDOMObject

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Andrew Wilson]

SharedWorkerScriptLoader should not be an ActiveDOMObject as it does not have a JS wrapper.

Additionally, the current use of the API is causing a failed assertion in certain cases in the chromium implementation (http://crbug.com/33662).

We need to change the implementation to not derive from ActiveDOMObject, and to use a different mechanism for shutting down pending script loads (waiting until contextDestroyed() is invoked is too late).

[Andrew Wilson]

Created attachment 47995 [details]
Patch

[WebKit Review Bot]

Attachment 47995 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/SharedWorkerRepository.cpp:116:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebKit/chromium/src/SharedWorkerRepository.cpp:132:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andrew Wilson]

Created attachment 47996 [details]
Fixed style warning

[Alexey Proskuryakov]

Comment on attachment 47996 [details]
Fixed style warning

+    // 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.

[Andrew Wilson]

Created attachment 48056 [details]
Patch

[Andrew Wilson]

Thanks, that was all good feedback. I've updated the patch accordingly.

[Alexey Proskuryakov]

Comment on attachment 48056 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 48056 [details]
Patch

Clearing flags on attachment: 48056

Committed r54292: <http://trac.webkit.org/changeset/54292>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.