Bug 34513 - SharedWorkerScriptLoader should not be an ActiveDOMObject
Summary: SharedWorkerScriptLoader should not be an ActiveDOMObject
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Andrew Wilson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-02 22:33 PST by Andrew Wilson
Modified: 2010-02-03 12:14 PST (History)
4 users (show)

See Also:


Attachments
Patch (10.49 KB, patch)
2010-02-02 22:52 PST, Andrew Wilson
no flags Details | Formatted Diff | Diff
Fixed style warning (10.44 KB, patch)
2010-02-02 23:02 PST, Andrew Wilson
no flags Details | Formatted Diff | Diff
Patch (10.47 KB, patch)
2010-02-03 11:22 PST, Andrew Wilson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Wilson 2010-02-02 22:33:18 PST
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).
Comment 1 Andrew Wilson 2010-02-02 22:52:55 PST
Created attachment 47995 [details]
Patch
Comment 2 WebKit Review Bot 2010-02-02 23:00:37 PST
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.
Comment 3 Andrew Wilson 2010-02-02 23:02:49 PST
Created attachment 47996 [details]
Fixed style warning
Comment 4 Alexey Proskuryakov 2010-02-03 08:53:51 PST
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.
Comment 5 Andrew Wilson 2010-02-03 11:22:27 PST
Created attachment 48056 [details]
Patch
Comment 6 Andrew Wilson 2010-02-03 11:23:14 PST
Thanks, that was all good feedback. I've updated the patch accordingly.
Comment 7 Alexey Proskuryakov 2010-02-03 11:33:14 PST
Comment on attachment 48056 [details]
Patch

r=me
Comment 8 WebKit Commit Bot 2010-02-03 12:14:43 PST
Comment on attachment 48056 [details]
Patch

Clearing flags on attachment: 48056

Committed r54292: <http://trac.webkit.org/changeset/54292>
Comment 9 WebKit Commit Bot 2010-02-03 12:14:51 PST
All reviewed patches have been landed.  Closing bug.