[webkit-reviews] review denied: [Bug 24689] Upstream V8 bindings for Workers proxies. : [Attachment 28742] Updated patch.

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Dmitry Titov
<dimich at chromium.org>'s request for review:
Bug 24689: Upstream V8 bindings for Workers proxies.
https://bugs.webkit.org/show_bug.cgi?id=24689

Attachment 28742: Updated patch.
https://bugs.webkit.org/attachment.cgi?id=28742&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>


> +#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>