Bug 26332 - Upstream V8Helpers
Summary: Upstream V8Helpers
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: David Levin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-11 14:48 PDT by Nate Chapin
Modified: 2009-06-12 11:03 PDT (History)
1 user (show)

See Also:


Attachments
patch (5.46 KB, patch)
2009-06-11 14:53 PDT, Nate Chapin
levin: review-
Details | Formatted Diff | Diff
patch2 (5.55 KB, patch)
2009-06-11 16:31 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
patch3 (5.49 KB, patch)
2009-06-11 16:35 PDT, Nate Chapin
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2009-06-11 14:48:39 PDT
See summary.
Comment 1 Nate Chapin 2009-06-11 14:53:53 PDT
Created attachment 31173 [details]
patch
Comment 2 David Levin 2009-06-11 15:26:43 PDT
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?
Comment 3 Nate Chapin 2009-06-11 16:31:44 PDT
Created attachment 31176 [details]
patch2
Comment 4 Nate Chapin 2009-06-11 16:34:03 PDT
(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.
Comment 5 Nate Chapin 2009-06-11 16:35:25 PDT
Created attachment 31177 [details]
patch3

Missed a couple items, I think everything's fixed now.
Comment 6 David Levin 2009-06-11 16:40:44 PDT
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::
Comment 7 David Levin 2009-06-11 16:42:41 PDT
Assign to levin for landing.
Comment 8 David Levin 2009-06-12 11:03:53 PDT
Committed as http://trac.webkit.org/changeset/44626.