[v8] Let ScriptController have more than one windowShell
Created attachment 45948 [details] Patch
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.
Attachment 45948 [details] was posted by a committer and has review+, assigning to Adam Barth for commit.
Thanks for the review. I'm entertained that almost all of your comments are things I copy/pasted from the JSC version. :)
Committed r52877: <http://trac.webkit.org/changeset/52877>
Committed r52882: <http://trac.webkit.org/changeset/52882>
I screwed up landing this. I'll try again when the world is greener.
The world is green! Finally!
Comment on attachment 45948 [details] Patch Clearing the r+ flag for now. I need to re-do an earlier patch.
If we want to solve this problem, we'll need to start over.