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

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted  review:
Bug 100235: [V8] Remove ScriptController::windowShell()
https://bugs.webkit.org/show_bug.cgi?id=100235

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

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


This looks great!  I'm not sure what's going on with the test failure, but you
might want to investigate that before landing.

> Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:70
> +    for (WorldMap::iterator i = isolatedWorlds.begin(), e =
isolatedWorlds.end(); i != e; ++i)

style nit: we usually use "it" rather than "i" for iterators.  Also, there's no
reason to store "e" in a variable.  You can just call end() each time in the
loop condition.

> Source/WebCore/bindings/v8/ScriptController.cpp:492
> +    v8::Handle<v8::Context> v8Context = m_windowShell->context();
>      v8Context->AllowCodeGenerationFromStrings(true);

I would combine these lines.

> Source/WebCore/bindings/v8/ScriptController.h:176
>      // Dummy method to avoid a bunch of ifdef's in WebCore.

Is this comment still accurate?