Bug 24689 – Upstream V8 bindings for Workers proxies.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Dmitry Titov]

Adding V8 bindings for Workers proxies, converting the coding style to WebKit as much as possible.

[Dmitry Titov]

Created attachment 28741 [details]
Proposed patch

[Dmitry Titov]

Created attachment 28742 [details]
Updated patch.

Fixed ChangeLog, Copyright and couple of style issues noted by David Levin.

[Dimitri Glazkov (Google)]

Comment on attachment 28742 [details]
Updated patch.



> +#include "v8Binding.h"
V8Binding

> +#include "v8Proxy.h"
V8Proxy

> +#include "v8_index.h"
Ideally, we should create V8Index.h that forwards to v8_index, just like I did for V8Proxy and friends. But this include isn't needed, right? V8Proxy includes v8_index.

> +    v8::Handle<v8::String> implicit_proto_string = v8::String::New("__proto__");

implicitProtoString?
this method needs to be scrubbed for local var naming.

> +    v8::TryCatch try_catch;
tryCatch.

> +v8::Handle<v8::Value> WorkerContextExecutionProxy::ToV8Object(V8ClassIndex::V8WrapperType type, void* impl)

toV8Object

> +    if (!impl)
> +        return v8::Null();
> +
> +    if (type == V8ClassIndex::WORKERCONTEXT)
> +        return WorkerContextToV8Object(static_cast<WorkerContext*>(impl));
> +
> +    // Non DOM node
> +    v8::Persistent<v8::Object> result = GetDOMObjectMap().get(impl);
Perhaps it would be good to add a domObjectMap() helper to V8Proxy, which for now wraps around GetDOMObjectMap?

> +    if (result.IsEmpty()) {
> +        v8::Local<v8::Object> v8obj = instantiateV8Object(type, type, impl);
probably just object in this context.

> +    v8::Handle<v8::Object> result = instantiateV8Object(type, V8ClassIndex::EVENT, event);
can I recommend renaming instantiateV8Object to toV8? It's a convention I've seen in JSC (toJS), and it's less verbose.

> +    m_listeners.push_back(listener.get());

push_back?! This smells like std::list -- and it is!! We should use Vector here.
> +#include "v8.h"
<v8.h>

> +#include "v8_index.h"
Really need V8Index.h, then.

> +        // Returns a local handle of the context.
> +        v8::Local<v8::Context> GetContext() { return v8::Local<v8::Context>::New(m_context); }

context();

> +
> +        // Returns the dom constructor function for the given node type.
> +        v8::Local<v8::Function> GetConstructor(V8ClassIndex::V8WrapperType);

constructor();

> +
> +        // Finds or creates an event listener;
> +        PassRefPtr<V8EventListener> FindOrCreateEventListener(v8::Local<v8::Value> listener, bool isInline, bool findOnly);

findOrCreateEventListener

> +
> +        // Removes an event listener;
> +        void RemoveEventListener(V8EventListener*);

removeEventListener

> +
> +        // Track the event so that we can detach it from the JS wrapper when a worker
> +        // terminates. This is needed because we need to be able to dispose these
> +        // events and releases references to their event targets: WorkerContext.
> +        void TrackEvent(Event*);

trackEvent

> +        // Returns the JS wrapper of object.
> +        static v8::Handle<v8::Value> ToV8Object(V8ClassIndex::V8WrapperType type, void* impl);
> +        static v8::Handle<v8::Value> EventToV8Object(Event* event);
> +        static v8::Handle<v8::Value> EventTargetToV8Object(EventTarget* target);
> +        static v8::Handle<v8::Value> WorkerContextToV8Object(WorkerContext* wc);

camelCase for all these.

> +        typedef std::list<V8EventListener*> EventListenerList;

Vector<V8EventListener*>, and typedefs typically go on top of all decls.

> diff --git a/WebCore/bindings/v8/WorkerScriptController.cpp b/WebCore/bindings/v8/WorkerScriptController.cpp
> +#include "v8.h"
<v8.h>

[Dmitry Titov]

(In reply to comment #3)
> (From update of attachment 28742 [details] [review])
> > +#include "v8Binding.h"
> V8Binding
> > +#include "v8Proxy.h"
> V8Proxy

Done.

> > +#include "v8_index.h"
> Ideally, we should create V8Index.h that forwards to v8_index, just like I did
> for V8Proxy and friends. But this include isn't needed, right? V8Proxy includes
> v8_index.

Removed the include. Not needed indeed.

> > +    v8::Handle<v8::String> implicit_proto_string = v8::String::New("__proto__");
> 
> implicitProtoString?
> this method needs to be scrubbed for local var naming.

Scrubbed. Scrubbed other methods in this file too.

> > +    v8::TryCatch try_catch;
> tryCatch.

Done.

> > +v8::Handle<v8::Value> WorkerContextExecutionProxy::ToV8Object(V8ClassIndex::V8WrapperType type, void* impl)
> 
> toV8Object

Can not do now, see comment at the bottom.

> > +    v8::Persistent<v8::Object> result = GetDOMObjectMap().get(impl);
> Perhaps it would be good to add a domObjectMap() helper to V8Proxy, which for
> now wraps around GetDOMObjectMap?

Done. 

> > +        v8::Local<v8::Object> v8obj = instantiateV8Object(type, type, impl);
> probably just object in this context.

Changed to object.

> > +    v8::Handle<v8::Object> result = instantiateV8Object(type, V8ClassIndex::EVENT, event);
> can I recommend renaming instantiateV8Object to toV8? It's a convention I've
> seen in JSC (toJS), and it's less verbose.

Done.

> > +    m_listeners.push_back(listener.get());
> push_back?! This smells like std::list -- and it is!! We should use Vector
> here.

Replaced with Vector.

> > +#include "v8.h"
> <v8.h>

Done.

> > +#include "v8_index.h"
> Really need V8Index.h, then.

Created V8Index.h, added to the patch.

> > +        v8::Local<v8::Context> GetContext() { return v8::Local<v8::Context>::New(m_context); }
> 
> context();

Can not do, see below.

> > +        v8::Local<v8::Function> GetConstructor(V8ClassIndex::V8WrapperType);
> 
> constructor();

Can not do, see below.

> 
> > +        PassRefPtr<V8EventListener> FindOrCreateEventListener(v8::Local<v8::Value> listener, bool isInline, bool findOnly);
> 
> findOrCreateEventListener

Can not do, see below.

> > +        void RemoveEventListener(V8EventListener*);
> 
> removeEventListener

Can not do, see below.

> > +        void TrackEvent(Event*);
> 
> trackEvent

Done.

> > +        // Returns the JS wrapper of object.
> > +        static v8::Handle<v8::Value> ToV8Object(V8ClassIndex::V8WrapperType type, void* impl);
> > +        static v8::Handle<v8::Value> EventToV8Object(Event* event);
> > +        static v8::Handle<v8::Value> EventTargetToV8Object(EventTarget* target);
> > +        static v8::Handle<v8::Value> WorkerContextToV8Object(WorkerContext* wc);
> 
> camelCase for all these.

Can not do, see below.

> > +        typedef std::list<V8EventListener*> EventListenerList;
> Vector<V8EventListener*>, and typedefs typically go on top of all decls.

Converted to Vector and don't need a typedef anymore (because Vector is iterated by index and not by iterator, for which the typedef was used before)

> > +#include "v8.h"
> <v8.h>

Done.
--------------------------

There are several methods that this class implements that can not be camelCased right now - because the V8 code-generating script still generates them capitalized. The easiest way to change that would be to change the naming when the code-generating script will be upstreamed - at this point, all the files to be changed will be in one place (WebKit.org)

[Dmitry Titov]

Created attachment 28758 [details]
Updated patch.

Updated per Dimitry's comments ^^^.

[Dimitri Glazkov (Google)]

Comment on attachment 28758 [details]
Updated patch.

We're very, very close! One more round of bathing, as eseidel used to say, and we're home.

> +    v8::HandleScope scope;
> +    v8::Persistent<v8::Object> wrapper = GetDOMObjectMap().get(m_workerContext);
> +    if (!wrapper.IsEmpty())
> +        V8Proxy::SetDOMWrapper(wrapper, V8ClassIndex::INVALID_CLASS_INDEX, NULL);
> +    GetDOMObjectMap().forget(m_workerContext);

domObjectMap
> +
> +    // Insert the object instance as the prototype of the shadow object.
> +    v8::Handle<v8::Object> v8_global = m_context->Global();
> +    v8_global->Set(implicitProtoString, jsWorkerContext);
> +}

global or globalObject

> +    // Non DOM node
> +    v8::Persistent<v8::Object> result = GetDOMObjectMap().get(impl);

domObjectMap

> +    v8::Handle<v8::Object> wrapper = GetDOMObjectMap().get(event);

domObjectMap

> +    if (GetDOMObjectMap().contains(event)) {
> +        GetDOMObjectMap().forget(event);

getDOMObjectMap

[Dmitry Titov]

Created attachment 28768 [details]
Asymptotically approaching the cleanness.

Ny bad, somehow I missed the domObjectMap() change, I thought I did it but it's not in the patch :-)
This one should be closer.

[Dimitri Glazkov (Google)]

Comment on attachment 28768 [details]
Asymptotically approaching the cleanness.

ship it.

[Dmitry Titov]

Landed: http://trac.webkit.org/changeset/41862

[Dmitry Titov]

While files were finally scrubbed for WebKit style, couple of errors was introduced. Now that they are merged into Chromium and compiled, need to update the upstream copy. Patch forthcoming.

[Dmitry Titov]

Created attachment 28892 [details]
Fixes after merge into Chromium.

[Dimitri Glazkov (Google)]

Comment on attachment 28892 [details]
Fixes after merge into Chromium.

Boy, do I feel stoopid for not catching "= ;". :-\

[Dmitry Titov]

Landed: http://trac.webkit.org/changeset/41941