[Webkit-unassigned] [Bug 95735] Removed V8IsolatedContext

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Sep 10 09:53:44 PDT 2012


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


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #163077|review?                     |review+
               Flag|                            |




--- Comment #14 from Adam Barth <abarth at webkit.org>  2012-09-10 09:54:02 PST ---
(From update of attachment 163077)
View in context: https://bugs.webkit.org/attachment.cgi?id=163077&action=review

There's some more polishing that we could do on this patch, but I'd like to get this landed.  We can iterate after landing it.

> 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".

> Source/WebCore/bindings/v8/ScriptController.cpp:470
> -    v8::Handle<v8::Context> v8Context = ScriptController::mainWorldContext(frame);
> +    v8::Handle<v8::Context> v8Context = mainWorldContext(frame);

I'd change the type of this handle to Local to document that it is a local handle.

> Source/WebCore/bindings/v8/ScriptController.h:72
> +    // FIXME: Replace existingWindowShell with existingWindowShellInternal see comment in V8DOMWindowShell::initializeIfNeeded.
> +    ScriptController* existingWindowShell(DOMWrapperWorld*) { return this; }

Why does existingWindowShell return a ScriptController* rather than a V8DOMWindowShell* ?

> 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.

> 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.

> 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.

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

reinterpret_cast -> static_cast

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

reinterpret_cast -> static_cast

> Source/WebCore/bindings/v8/V8DOMWindowShell.h:108
> +        // This is a temporary performance optimization. Essentially,
> +        // GetHiddenValue is too slow for this code path. We need to get the
> +        // V8 team to add a real property to v8::Context for isolated worlds.
> +        // Until then, we optimize the common case of not having any isolated
> +        // worlds at all.

I'd get rid of this comment.  It's not really temporary!

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


More information about the webkit-unassigned mailing list