[Webkit-unassigned] [Bug 24689] Upstream V8 bindings for Workers proxies.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Mar 19 12:42:32 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=24689
------- Comment #4 from dimich at chromium.org 2009-03-19 12:42 PDT -------
(In reply to comment #3)
> (From update of attachment 28742 [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)
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
More information about the webkit-unassigned
mailing list