[Webkit-unassigned] [Bug 95735] Removed V8IsolatedContext

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=95735


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #162013|review?                     |review-
               Flag|                            |




--- Comment #2 from Adam Barth <abarth at webkit.org>  2012-09-04 11:26:01 PST ---
(From update of attachment 162013)
View in context: https://bugs.webkit.org/attachment.cgi?id=162013&action=review

This is a big step forward.  Below are a bunch of minor comments.  The only one that's really important is the question about line 58 of WorldContextHandle.cpp.  I'd like to see another iteration of this patch, if that ok with you.

> Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:98
> +    if (worldId == mainWorldId) {
> +        ASSERT_NOT_REACHED();
> +        return mainThreadNormalWorld();
> +    }

Why not leave this as an ASSERT?

> Source/WebCore/bindings/v8/ScriptController.cpp:329
> +    if (UNLIKELY(worldId == DOMWrapperWorld::mainWorldId)) {
> +        ASSERT_NOT_REACHED();
> +        return m_windowShell;
> +    }

Why not just ASSERT this?

> Source/WebCore/bindings/v8/ScriptController.cpp:352
> +    if (UNLIKELY(!world)) {
> +        ASSERT_NOT_REACHED();
> +        return 0;
> +    }

This should just be an ASSERT.

> Source/WebCore/bindings/v8/ScriptController.cpp:376
> +    // Except in the test runner, worldID should be non 0 as it conflicts with the mainWorldId.
> +    if (UNLIKELY(!worldID))
> +        worldID = DOMWrapperWorld::uninitializedWorldId;

Should we change the test runner so that the testing environment is more like the production environment?

> Source/WebCore/bindings/v8/ScriptController.h:200
> +    typedef HashMap<int, RefPtr<V8DOMWindowShell> > IsolatedWorldMap;
> +    typedef HashMap<int, RefPtr<SecurityOrigin> > IsolatedWorldSecurityOriginMap;

I wonder if we should move the SecurityOrigin pointer into V8DOMWindowShell.  We can do that in a separate patch though.

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:234
> +    if (UNLIKELY(m_context.get().IsWeak())) {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }

ASSERT

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:253
> +            if (m_world->isMainWorld()) {

Do we ever call this function with weak and !m_world->isMainWorld() ?

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:525
> +    if (!m_world->isMainWorld()) {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }

ASSERT

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:560
> +    if (!m_world->isMainWorld()) {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }

ASSERT

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:564
> +    // FIXME: This shouldn't be possible anymore.
> +    if (!m_frame->document())
> +        return;

Is this a bad merge?  I thought I deleted this code from trunk.

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:594
> +    if (!m_world->isMainWorld()) {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }

ASSERT

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:612
> +    if (!m_world->isMainWorld()) {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }

ASSERT

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:633
> +    if (!m_world->isMainWorld()) {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }

ASSERT

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:646
> +    if (m_world->isMainWorld()) {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }

ASSERT

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:651
> +        InspectorInstrumentation::didCreateIsolatedContext(m_frame, scriptState, securityOrigin.get());

Why does setIsolatedWorldSecurityOrigin call didCreateIsolatedContext ?  I would assume that would be the responsibility of the create function, not of a function that just sets the security origin.

> Source/WebCore/bindings/v8/V8DOMWindowShell.h:132
> +    v8::Persistent<v8::Context> createNewContext(v8::Handle<v8::Object> global);

createNewContext -> createContext

Why does this function return a v8::Persistent<v8::Context> ?  It seems like the result could just be stored in m_context.

> Source/WebCore/bindings/v8/V8DOMWindowShell.h:133
> +    bool installDOMWindow(v8::Handle<v8::Context>, DOMWindow*);

Now that this function isn't static, it doesn't need to take a v8::Handle<v8::Context> parameter.  It can just use m_context.

> Source/WebCore/bindings/v8/V8DOMWindowShell.h:146
> +    RefPtr<SecurityOrigin> m_isolatedWorldShellSecurityOrigin;

So, we keep a SecurityOrigin pointer here and in ScriptController?  Do we need to have both of them?  Do they play different roles?

> Source/WebCore/bindings/v8/WorldContextHandle.cpp:53
> +        ASSERT_NOT_REACHED();

Please just use ASSERT rather than trying to handle these impossible cases.

> Source/WebCore/bindings/v8/WorldContextHandle.cpp:58
> +    m_context = SharedPersistent<v8::Context>::create(v8::Persistent<v8::Context>::New(context));

Here is the v8::Persistent<v8::Context>::Dispose() that balances this New() ?

> Source/WebCore/bindings/v8/WorldContextHandle.cpp:70
> +    if (m_context->get().IsEmpty()) {
> +        ASSERT_NOT_REACHED();
> +        // FIXME: This is problematic, but an empty context is worse.
>          return script->mainWorldContext();
> +    }

Please just ASSERT

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.