[webkit-reviews] review denied: [Bug 56898] [chromium] WebBindings::getRangeImpl() should NULL check its NPObject argument : [Attachment 86579] Patch - remove linux warning

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied noel gordon <noel.gordon at gmail.com>'s
request for review:
Bug 56898: [chromium] WebBindings::getRangeImpl() should NULL check its
NPObject argument
https://bugs.webkit.org/show_bug.cgi?id=56898

Attachment 86579: Patch - remove linux warning
https://bugs.webkit.org/attachment.cgi?id=86579&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=86579&action=review

omg, the style in this file is so inconsistent.

> Source/WebKit/chromium/public/WebBindings.h:52
> +    WEBKIT_API static bool construct(NPP, NPObject*, const NPVariant* args,
uint32_t count, NPVariant* result);

Why change this? argCount seems more clear to me.

> Source/WebKit/chromium/public/WebBindings.h:76
> +    WEBKIT_API static void getStringIdentifiers(const NPUTF8** names,
int32_t count, NPIdentifier*);

ditto

> Source/WebKit/chromium/src/WebBindings.cpp:207
> +    if (!object || (object->_class != npScriptObjectClass))
> +	   return false;

Since this is a code change could we do the style change first and then a code
change?

> Source/WebKit/chromium/src/WebBindings.cpp:227
> +    V8NPObject* v8npobject = reinterpret_cast<V8NPObject*>(object);

To match the class name, this maybe should be v8NPObject.

> Source/WebKit/chromium/src/WebBindings.cpp:228
> +    v8::Handle<v8::Object> v8object(v8npobject->v8Object);

Ditto. This should probably be v8Object.