[Webkit-unassigned] [Bug 96637] Remove V8DOMWindowShell::getEntered

[bugzilla-daemon at webkit.org]

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


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #163847|review?                     |review+, commit-queue-
               Flag|                            |




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

This looks great!  A few very minor comments below.

I suspect we'll eventually to unify this data with V8PerContextData, but currently the lifetimes are a bit different.  This is great for now though.

> Source/WebCore/bindings/v8/ScopedPersistent.h:86
> +    void clearWeak(void* parameter, v8::WeakReferenceCallback callback)
> +    {
> +        ASSERT(!m_handle.IsEmpty());
> +        m_handle.MakeWeak(parameter, callback);
> +        m_handle.Clear();
> +    }

Rather than encouraging this tricky lifetime semantics by adding this feature to ScopedPersistent, it might be better if we provide a way to leak the handle.  For example:

v8::Persistent<T> leakHandle()
{
    v8::Persistent<T> handle = m_handle;
    m_handel.Clear();
    return handle;
}

Then the caller can write:

m_whatever.leakHandle().MakeWeak(...)

and then the caller will know that it is responsible for disposing the handle because the word "leak" appears at the call site.

> Source/WebCore/bindings/v8/ScriptController.cpp:398
> +            delete isolatedWorldShell;

Normally I would complain about raw calls to delete, but I know you're going to make these into OwnPtrs shortly.  :)

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:179
> -V8DOMWindowShell* V8DOMWindowShell::enteredIsolatedWorldContext()
> +V8DOMWindowShell::IsolatedContextData* V8DOMWindowShell::enteredIsolatedContextData(v8::Local<v8::Context> context)

enteredIsolatedContextData -> toIsolatedContextData

The word "entered" in these functions refers to the V8 API concept of the "entered" v8::Context.  For example, in v8::Context::GetEntered().  Here, you're asking the caller to supply the context.  They might supply the entered context or they might supply whatever context they want.

nit: Local -> Handle.  We try to use the more general type whenever it doesn't mater.  I see that you're mirroring setIsolatedWorldField, but that should also use Handle rather than Local.

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:360
> +        m_isolatedContextData = IsolatedContextData::create(m_world.get(), m_perContextData.release(), m_isolatedWorldShellSecurityOrigin.get());

Can't you skip these get() calls?

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:622
> +        m_isolatedContextData->setSecurityOrigin(m_isolatedWorldShellSecurityOrigin.get());

ditto

> Source/WebCore/bindings/v8/V8DOMWindowShell.h:71
> +    private:

This line should have a blank line before it.

> Source/WebCore/bindings/v8/V8DOMWindowShell.h:73
> +        IsolatedContextData(PassRefPtr<DOMWrapperWorld> world, PassOwnPtr<V8PerContextData> perContextData,
> +                PassRefPtr<SecurityOrigin> securityOrigin)

This can all go on one line.

> Source/WebCore/bindings/v8/V8DOMWindowShell.h:76
> +            , m_securityOrigin(securityOrigin) { }

These { } should each be on a separate line and have a blank line after them.

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