Bug 33243 - [v8] Let ScriptController have more than one windowShell
Summary: [v8] Let ScriptController have more than one windowShell
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adam Barth
Depends on:
Reported: 2010-01-05 17:28 PST by Adam Barth
Modified: 2011-10-13 15:36 PDT (History)
2 users (show)

See Also:

Patch (17.74 KB, patch)
2010-01-05 17:30 PST, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2010-01-05 17:28:29 PST
[v8] Let ScriptController have more than one windowShell
Comment 1 Adam Barth 2010-01-05 17:30:53 PST
Created attachment 45948 [details]
Comment 2 Eric Seidel (no email) 2010-01-06 09:14:57 PST
Comment on attachment 45948 [details]

ShellMap seems like a slightly lame name.  WindowShellMap might be better.  or WorldToShellMap?

 142         // JSC triggers a GC here, but we haven't historically.

Do we know why JSC does?  Maybe to make leak detection easier? (which wouldn't be a good reason, but it's all I can think of atm)

I might have put this:
// Force the mainWindowShell to exist.
 237     windowShell(mainThreadNormalWorld());
in a function called "ensureMainWindowShell()" (then you wouldn't need a comment).

Oh.  Why not just call mainWorldWindowShell()?

C++ is really lame about constructs like these:
for (ShellMap::iterator iter = m_windowShells.begin(); iter != m_windowShells.end(); ++iter)
 486         iter->second->clearForNavigation();
I guess the best way would be to use a MACRO to save the copy/paste iteration code.  WE could use function pointers, but those are complicated.  I think for now the copy/paste might be OK, but it's still lame. :(  Especially since we have 4 copy/pastes of that code!

Does the JS version destroy all shells in:
464495 void ScriptController::destroyWindowShell()
Seems that method is poorly named if that's the case.

I'm not sure why:
 67     V8DOMWindowShell* windowShell(DOMWrapperWorld* world)
 72     V8DOMWindowShell* existingWindowShell(DOMWrapperWorld* world) const
would be in the header like that.

I wonder if "if (iter)" would function the same as "iter != m_windowShells.end())"

"world" is superfluous:
 197     V8DOMWindowShell* initScript(DOMWrapperWorld* world);

 48     // For some reason JSDOMWindowShell uses the DOMWindow as the first arg...
Sam Weinig would know why.

This seems OK.  See comments above.
Comment 3 Eric Seidel (no email) 2010-01-06 09:31:54 PST
Attachment 45948 [details] was posted by a committer and has review+, assigning to Adam Barth for commit.
Comment 4 Adam Barth 2010-01-06 12:00:17 PST
Thanks for the review.  I'm entertained that almost all of your comments are things I copy/pasted from the JSC version.  :)
Comment 5 Adam Barth 2010-01-06 16:16:11 PST
Committed r52877: <http://trac.webkit.org/changeset/52877>
Comment 6 Adam Barth 2010-01-06 16:47:14 PST
Committed r52882: <http://trac.webkit.org/changeset/52882>
Comment 7 Adam Barth 2010-01-06 16:47:53 PST
I screwed up landing this.  I'll try again when the world is greener.
Comment 8 Eric Seidel (no email) 2010-01-06 23:44:45 PST
The world is green!  Finally!
Comment 9 Adam Barth 2010-01-10 23:05:07 PST
Comment on attachment 45948 [details]

Clearing the r+ flag for now.  I need to re-do an earlier patch.
Comment 10 Adam Barth 2011-10-13 15:36:52 PDT
If we want to solve this problem, we'll need to start over.