Bug 67302 – Add pass-throughs for NPObject/v8::Value marshalling to WebBindings

• Component: WebKit API
• Status: RESOLVED FIXED

[Greg Billock]

Expose a couple of methods to WebBindings to expose type translation from V8NPUtils. This makes complex type translation possible from the API client side.

[Greg Billock]

Created attachment 105796 [details]
Patch

[Darin Fisher (:fishd, Google)]

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

[Greg Billock]

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.

[Greg Billock]

Created attachment 105847 [details]
Patch

[Darin Fisher (:fishd, Google)]

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?

[Greg Billock]

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.

[Greg Billock]

Created attachment 105863 [details]
Patch

[WebKit Review Bot]

Comment on attachment 105863 [details]
Patch

Clearing flags on attachment: 105863

Committed r94277: <http://trac.webkit.org/changeset/94277>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.