[webkit-reviews] review denied: [Bug 26332] Upstream V8Helpers : [Attachment 31173] patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jun 11 15:26:43 PDT 2009
David Levin <levin at chromium.org> has denied Nate Chapin <japhet at google.com>'s
request for review:
Bug 26332: Upstream V8Helpers
https://bugs.webkit.org/show_bug.cgi?id=26332
Attachment 31173: patch
https://bugs.webkit.org/attachment.cgi?id=31173&action=review
------- Additional Comments from David Levin <levin at chromium.org>
Just a few issues to clear up so r- for now.
> Index: WebCore/bindings/v8/V8Helpers.cpp
> +#define max max
> +#define min min
What is this about? (Why are they needed?)
> +using WebCore::V8Custom;
Do we have a V8Custom namespace?
> +void WrapNPObject(v8::Handle<v8::Object> object, NPObject* npObject)
WrapNPObject casing. Should be wrapNPObject
> +v8::Local<v8::Context> getV8Context(NPP npp, NPObject* npObject)
toV8Context would be more in keeping with WebKit naming (dglazkov helped me out
here).
> +WebCore::V8Proxy* GetV8Proxy(NPObject* npObject)
s/GetV8Proxy/toV8Proxy/
> Index: WebCore/bindings/v8/V8Helpers.h
> +// Retrieves the V8 Context from the NP context pr obj (at most 1 may be
NULL).
I can't understand this comment at all.
> +
> +// Get V8Proxy object from an NPObject.
This comment doesn't seem to add anything at all (except a line of text that
may get out of sync with the code).
> +#endif // V8Helpers_h
One space before trailing comments.
Why aren't the functions declared in the WebCore namespace like the rest of the
functions in these headers?
More information about the webkit-reviews
mailing list