Bug 33243 – [v8] Let ScriptController have more than one windowShell

• Status: RESOLVED
• Resolution: LATER
• Component: WebCore JavaScript

[Adam Barth]

[v8] Let ScriptController have more than one windowShell

[Adam Barth]

Created attachment 45948 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 45948 [details]
Patch

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)
or
 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.

[Eric Seidel (no email)]

Attachment 45948 [details] was posted by a committer and has review+, assigning to Adam Barth for commit.

[Adam Barth]

Thanks for the review.  I'm entertained that almost all of your comments are things I copy/pasted from the JSC version.  :)

[Adam Barth]

Committed r52877: <http://trac.webkit.org/changeset/52877>

[Adam Barth]

Committed r52882: <http://trac.webkit.org/changeset/52882>

[Adam Barth]

I screwed up landing this.  I'll try again when the world is greener.

[Eric Seidel (no email)]

The world is green!  Finally!

[Adam Barth]

Comment on attachment 45948 [details]
Patch

Clearing the r+ flag for now.  I need to re-do an earlier patch.

[Adam Barth]

If we want to solve this problem, we'll need to start over.