[Webkit-unassigned] [Bug 95735] Removed V8IsolatedContext

[bugzilla-daemon at webkit.org]

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





--- Comment #17 from Dan Carney <dcarney at google.com>  2012-09-10 10:33:47 PST ---
(From update of attachment 163077)
View in context: https://bugs.webkit.org/attachment.cgi?id=163077&action=review

>> Source/WebCore/bindings/v8/ScriptController.cpp:379
>> +        V8DOMWindowShell* isolatedWorldShell = getOrCreateIsolatedWorldContext(worldID, extensionGroup);
> 
> nit: getOrCreateIsolatedWorldContext -> ensureIsolatedWindowShell(worldID, extensionGroup) ?
> 
> We typically use the word "ensure" to mean "get or create".

ok

>> Source/WebCore/bindings/v8/ScriptController.h:72
>> +    ScriptController* existingWindowShell(DOMWrapperWorld*) { return this; }
> 
> Why does existingWindowShell return a ScriptController* rather than a V8DOMWindowShell* ?

I'll fix that after this patch lands.  There's a weird reliance on existingWindowShell never returning a main world shell.  I think it only caused issues with one test.

>> Source/WebCore/bindings/v8/ScriptController.h:201
>> +    typedef HashMap<int, V8DOMWindowShell*> IsolatedWorldMap;
> 
> I see.  We're still using the V8 garbage collector to manage the lifetime of the V8DOMWindowShell in the case of isolated worlds...  Interesting.  I'm not sure that's what we want in the long term, but I think its ok for now.

I tried to minimize change.  For all but the worlds that are created for one call, I believe I can switch to having the lifecycle identical to that of the main window shell.

>> Source/WebCore/bindings/v8/SharedPersistent.h:47
>> +            return adoptRef(new SharedPersistent<T>(v8::Persistent<v8::Context>::New(value)));
> 
> Technically this should be v8::Persistent<T>::New since we don't know that T is v8::Context.

yep.  copy and paste failure.

>> Source/WebCore/bindings/v8/SharedPersistent.h:49
>> +        ~SharedPersistent()
> 
> I thought we were just going to have this class hold a ScopedPersistent rather than a v8::Persistent.  That way we'd get the calls to New and Dispose for free.

This class will be deleted shortly, but I'll change it.

>> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:181
>> +    return reinterpret_cast<V8DOMWindowShell*>(toInnerGlobalObject(v8::Context::GetEntered())->GetPointerFromInternalField(V8DOMWindow::enteredIsolatedWorldIndex));
> 
> reinterpret_cast -> static_cast

ok

>> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:219
>> +    delete reinterpret_cast<V8DOMWindowShell*>(parameter);
> 
> reinterpret_cast -> static_cast

ok

>> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:237
>> +        m_context.get().MakeWeak(this, isolatedContextWeakCallback);
> 
> We should probably 0 out m_frame at this point so we don't have a dangling reference to the Frame.

ok

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