[webkit-reviews] review granted: [Bug 33243] [v8] Let ScriptController have more than one windowShell : [Attachment 45948] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 33243: [v8] Let ScriptController have more than one windowShell
https://bugs.webkit.org/show_bug.cgi?id=33243

Attachment 45948: Patch
https://bugs.webkit.org/attachment.cgi?id=45948&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.