See summary.
Created attachment 31173 [details] patch
Comment on attachment 31173 [details] patch 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?
Created attachment 31176 [details] patch2
(In reply to comment #2) > (From update of attachment 31173 [details] [review]) > 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?) No idea, don't think it's necessary. Removed. > > > > +using WebCore::V8Custom; > Do we have a V8Custom namespace? V8Custom lives in V8CustomBinding.{h,cpp}. However, this appears to be unnecessary, so I removed it. > > > +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. Ditto. > > > + > > +// 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? > Everything else fixed.
Created attachment 31177 [details] patch3 Missed a couple items, I think everything's fixed now.
Comment on attachment 31177 [details] patch3 Looks good. I'll do the following on landing. > + // Get V8Proxy object from an NPObject. Let's delete this comment as it doesn't seem to add anything. > + WebCore::V8Proxy* toV8Proxy(NPObject*); Remove WebCore::
Assign to levin for landing.
Committed as http://trac.webkit.org/changeset/44626.