Bug 24689 - Upstream V8 bindings for Workers proxies.
Summary: Upstream V8 bindings for Workers proxies.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-18 18:11 PDT by Dmitry Titov
Modified: 2009-03-30 16:33 PDT (History)
1 user (show)

See Also:


Attachments
Proposed patch (25.84 KB, patch)
2009-03-18 18:13 PDT, Dmitry Titov
no flags Details | Formatted Diff | Diff
Updated patch. (24.16 KB, patch)
2009-03-18 18:40 PDT, Dmitry Titov
dglazkov: review-
Details | Formatted Diff | Diff
Updated patch. (26.57 KB, patch)
2009-03-19 12:43 PDT, Dmitry Titov
dglazkov: review-
Details | Formatted Diff | Diff
Asymptotically approaching the cleanness. (27.23 KB, patch)
2009-03-19 16:04 PDT, Dmitry Titov
dglazkov: review+
Details | Formatted Diff | Diff
Fixes after merge into Chromium. (2.23 KB, patch)
2009-03-24 09:53 PDT, Dmitry Titov
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Titov 2009-03-18 18:11:06 PDT
Adding V8 bindings for Workers proxies, converting the coding style to WebKit as much as possible.
Comment 1 Dmitry Titov 2009-03-18 18:13:43 PDT
Created attachment 28741 [details]
Proposed patch
Comment 2 Dmitry Titov 2009-03-18 18:40:56 PDT
Created attachment 28742 [details]
Updated patch.

Fixed ChangeLog, Copyright and couple of style issues noted by David Levin.
Comment 3 Dimitri Glazkov (Google) 2009-03-19 10:25:31 PDT
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>
Comment 4 Dmitry Titov 2009-03-19 12:42:31 PDT
(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)
Comment 5 Dmitry Titov 2009-03-19 12:43:36 PDT
Created attachment 28758 [details]
Updated patch.

Updated per Dimitry's comments ^^^.
Comment 6 Dimitri Glazkov (Google) 2009-03-19 13:49:08 PDT
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
Comment 7 Dmitry Titov 2009-03-19 16:04:42 PDT
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.
Comment 8 Dimitri Glazkov (Google) 2009-03-19 19:37:34 PDT
Comment on attachment 28768 [details]
Asymptotically approaching the cleanness.

ship it.
Comment 9 Dmitry Titov 2009-03-20 10:37:21 PDT
Landed: http://trac.webkit.org/changeset/41862
Comment 10 Dmitry Titov 2009-03-24 09:49:59 PDT
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.
Comment 11 Dmitry Titov 2009-03-24 09:53:35 PDT
Created attachment 28892 [details]
Fixes after merge into Chromium.
Comment 12 Dimitri Glazkov (Google) 2009-03-24 10:04:45 PDT
Comment on attachment 28892 [details]
Fixes after merge into Chromium.

Boy, do I feel stoopid for not catching "= ;". :-\
Comment 13 Dmitry Titov 2009-03-30 16:33:51 PDT
Landed: http://trac.webkit.org/changeset/41941