[webkit-reviews] review granted: [Bug 100235] [V8] Remove ScriptController::windowShell() : [Attachment 173219] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Dan Carney <dcarney at google.com>'s
request for review:
Bug 100235: [V8] Remove ScriptController::windowShell()
https://bugs.webkit.org/show_bug.cgi?id=100235

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=173219&action=review


> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3410
> -	   if (frame->script()->windowShell()->context().IsEmpty() &&
frame->script()->windowShell()->initializeIfNeeded()) {
> -	       // initializeIfNeeded may have created a wrapper for the object,
retry from the start.
> +	   if (frame->script()->initializeMainWorld()) {
> +	       // initializeMainWorld may have created a wrapper for the
object, retry from the start.

You need to run-bindings-tests --reset-results to avoid turning the bot red.

> Source/WebCore/bindings/v8/ScriptController.cpp:348
> -    return iter == m_isolatedWorlds.end() ? 0 : iter->value;
> +    return iter == m_isolatedWorlds.end() ? 0 :
iter->value->isContextInitialized() ? iter->value : 0;

I would probably have broken this down into multiple statements.  Nested
ternary operators can be slightly hard to read.

> Source/WebCore/bindings/v8/ScriptController.cpp:369
> +    if (!shell->isContextInitialized()) {
> +	   if (shell->initializeIfNeeded()) {

These lines can be combined.  Do we really need to call isContextInitialized? 
I would think that initializeIfNeeded would check isContextInitialized because
it says "if needed".

> Source/WebCore/bindings/v8/ScriptController.cpp:727
> +    if ((!m_windowShell->isContextInitialized() ||
!m_windowShell->isGlobalInitialized()) &&
m_frame->loader()->stateMachine()->creatingInitialEmptyDocument())

This line still seems strange, but I think we should worry about it later.