[webkit-reviews] review granted: [Bug 96637] Remove V8DOMWindowShell::getEntered : [Attachment 163847] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Dan Carney <dcarney at google.com>'s
request for review:
Bug 96637: Remove V8DOMWindowShell::getEntered
https://bugs.webkit.org/show_bug.cgi?id=96637

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.