Bug 67302 - Add pass-throughs for NPObject/v8::Value marshalling to WebBindings
Summary: Add pass-throughs for NPObject/v8::Value marshalling to WebBindings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-31 11:01 PDT by Greg Billock
Modified: 2011-08-31 23:59 PDT (History)
2 users (show)

See Also:


Attachments
Patch (4.36 KB, patch)
2011-08-31 11:05 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (4.27 KB, patch)
2011-08-31 15:55 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (4.22 KB, patch)
2011-08-31 17:02 PDT, Greg Billock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg Billock 2011-08-31 11:01:52 PDT
Expose a couple of methods to WebBindings to expose type translation from V8NPUtils. This makes complex type translation possible from the API client side.
Comment 1 Greg Billock 2011-08-31 11:05:05 PDT
Created attachment 105796 [details]
Patch
Comment 2 Darin Fisher (:fishd, Google) 2011-08-31 13:49:36 PDT
Comment on attachment 105796 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105796&action=review

> Source/WebKit/chromium/public/WebBindings.h:169
> +    WEBKIT_EXPORT static v8::Handle<v8::Value> convertNPVariantToV8Object(const NPVariant*);

This returns a v8::Value, not a v8::Object.  Maybe the name should be convertNPVariantToV8Value?  or, maybe we could even use short-cuts like:

  toV8Value
  toNPVariant
Comment 3 Greg Billock 2011-08-31 15:37:51 PDT
Comment on attachment 105796 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105796&action=review

>> Source/WebKit/chromium/public/WebBindings.h:169
>> +    WEBKIT_EXPORT static v8::Handle<v8::Value> convertNPVariantToV8Object(const NPVariant*);
> 
> This returns a v8::Value, not a v8::Object.  Maybe the name should be convertNPVariantToV8Value?  or, maybe we could even use short-cuts like:
> 
>   toV8Value
>   toNPVariant

I'm happy to switch to that. I was basically copying the V8NPUtils names exactly to make the wrapping very clear. It looked like other WebBindings and Web* APIs tended to do that, although not universally.
Comment 4 Greg Billock 2011-08-31 15:55:51 PDT
Created attachment 105847 [details]
Patch
Comment 5 Darin Fisher (:fishd, Google) 2011-08-31 16:47:02 PDT
Comment on attachment 105847 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105847&action=review

> Source/WebKit/chromium/public/WebBindings.h:167
> +    // Conversion utilities to/from V8 native objects and NPObject wrappers.

nit: s/NPObject/NPVariant/

> Source/WebKit/chromium/src/WebBindings.cpp:339
> +    WebCore::convertV8ObjectToNPVariant(object, root, result);

nit: note the 'using namespace WebCore' at the top of this file.  you can drop the WebCore:: prefixes.

> Source/WebKit/chromium/src/WebBindings.cpp:346
> +        if (object->_class != WebCore::npScriptObjectClass)

ditto

> Source/WebKit/chromium/src/WebBindings.cpp:353
> +    return WebCore::convertNPVariantToV8Object(variant, static_cast<NPObject*>(0));

is the static_cast necessary?  can you just get by passing 0 or nullptr?
Comment 6 Greg Billock 2011-08-31 17:01:13 PDT
Comment on attachment 105847 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105847&action=review

>> Source/WebKit/chromium/public/WebBindings.h:167
>> +    // Conversion utilities to/from V8 native objects and NPObject wrappers.
> 
> nit: s/NPObject/NPVariant/

Done.

>> Source/WebKit/chromium/src/WebBindings.cpp:339
>> +    WebCore::convertV8ObjectToNPVariant(object, root, result);
> 
> nit: note the 'using namespace WebCore' at the top of this file.  you can drop the WebCore:: prefixes.

Done.

>> Source/WebKit/chromium/src/WebBindings.cpp:346
>> +        if (object->_class != WebCore::npScriptObjectClass)
> 
> ditto

Done.

>> Source/WebKit/chromium/src/WebBindings.cpp:353
>> +    return WebCore::convertNPVariantToV8Object(variant, static_cast<NPObject*>(0));
> 
> is the static_cast necessary?  can you just get by passing 0 or nullptr?

Yeah, that works. Changed.
Comment 7 Greg Billock 2011-08-31 17:02:07 PDT
Created attachment 105863 [details]
Patch
Comment 8 WebKit Review Bot 2011-08-31 23:59:08 PDT
Comment on attachment 105863 [details]
Patch

Clearing flags on attachment: 105863

Committed r94277: <http://trac.webkit.org/changeset/94277>
Comment 9 WebKit Review Bot 2011-08-31 23:59:12 PDT
All reviewed patches have been landed.  Closing bug.