[webkit-reviews] review denied: [Bug 83093] [V8] Add a per context data store and use that for caching boiler plates as well as constructor functions : [Attachment 135460] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Erik Arvidsson <arv at chromium.org>'s
request for review:
Bug 83093: [V8] Add a per context data store and use that for caching boiler
plates as well as constructor functions
https://bugs.webkit.org/show_bug.cgi?id=83093

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=135460&action=review


I'm being a bit more picky about naming than usual because the V8 bindings are
a style disaster.  I'm hoping that through this sort of work we can improve the
style and patterns in the code so that future folks working on this code will
have good examples to work from.

One thing that's important is that v8::Persistent<T>::New must always be
balanced with v8::Persistent<T>::Dispose.  They're like new and delete in C++. 
It's lame that V8 invite us to manually call new and delete.  We have
SharedPersistent and OwnHandle (maybe should be called OwnPersistent?) to help
us here.  They're not widely used, but it would be good to use these rather
than calling New and Dispose manually when convenient.

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:47
> +	       wrapper.Clear();

I don't think there's any need to call Clear() here.  That just sets the local
variable to zero.

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:63
> +bool V8BindingPerContextData::installHiddenObjectPrototype()

install probably isn't the right verb for this function anymore now that we're
storing the object prototype ourselves (rather than "installing" it into a
hidden variable in the context.

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:79
> +    m_objectPrototype = v8::Persistent<v8::Value>::New(objectPrototype);

Where is the Dispose that balances this New?  Should we use
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/v8/OwnHandle.h to
make sure we don't leak?

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:88
> +    v8::Local<v8::Function> function = getConstructor(type);

Is there a better verb we can use than "get" for getConstructor?

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:97
> +v8::Local<v8::Function>
V8BindingPerContextData::getConstructorSlowCase(WrapperTypeInfo* type)

Another "get".

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:121
> +    // Hotmail fix, see comments above.

Is this comment still needed?  I would remove it and rely on our test coverage
to explain to us why this is needed.

> Source/WebCore/bindings/v8/V8BindingPerContextData.h:53
> +    // WARNING: Call |installHiddenObjectPrototype| only on fresh contexts!

This comment is out of date.  Rather than having this comment, perhaps the
function should just ASSERT(m_objectPrototype.isEmpty()) ?

> Source/WebCore/bindings/v8/V8BindingPerContextData.h:65
> +    v8::Local<v8::Function> getConstructor(WrapperTypeInfo* type)

Ok, so I would call this function constructorForType

> Source/WebCore/bindings/v8/V8BindingPerContextData.h:70
> +	   return getConstructorSlowCase(type);

getConstructorSlowCase => constructorForTypeSlowCase ?

> Source/WebCore/bindings/v8/V8BindingPerContextData.h:75
> +    V8BindingPerContextData(v8::Handle<v8::Context> context)
> +	 : m_context(context) { }

Please mark one-argument constructors "explicit".  Also, these { } should be on
their own lines.

> Source/WebCore/bindings/v8/V8BindingPerContextData.h:90
> +    v8::Persistent<v8::Context> m_context;

Where are the New() and Dispose() calls for this persistent handle?  Should we
use
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/v8/SharedPersisten
t.h to make sure the context doesn't get Disposed out from under us?

> Source/WebCore/bindings/v8/V8DOMWindowShell.h:83
> +    V8BindingPerContextData* perContextData() { return
m_perContextData.get(); }

I was hoping we could get this from a v8::Context itself rather than hanging it
off the window shell.  This might be a good intermediate state though.

> Source/WebCore/bindings/v8/V8DOMWrapper.cpp:107
> +    if (UNLIKELY(!!(isolatedContext = V8IsolatedContext::getEntered())))
> +	   perContextData = isolatedContext->perContextData();
> +    else
> +	   perContextData =
V8Proxy::retrieve(frame)->windowShell()->perContextData();

Wouldn't this be better if we could get the perContextData directly from the
v8::Context?

> Source/WebCore/bindings/v8/V8DOMWrapper.h:152
> +	   static V8BindingPerContextData* getPerContextData(V8Proxy*);
> +#if ENABLE(WORKERS)
> +	   static V8BindingPerContextData* getPerContextData(WorkerContext*);
> +#endif

Can we drop the "get" prefix?

> Source/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp:155
> +    if (!m_perContextData->installHiddenObjectPrototype()) {

Why isn't this work done by the constructor?  It seems like everyone calls it
immediately.  I guess they'd all need to check for failure...  Maybe this
function should just be called init().