[webkit-reviews] review denied: [Bug 26566] [V8] Upstream NPV8Object and V8NPUtils : [Attachment 31585] First try at upstream the files

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Albert J. Wong <ajwong at chromium.org>'s
request for review:
Bug 26566: [V8] Upstream NPV8Object and V8NPUtils
https://bugs.webkit.org/show_bug.cgi?id=26566

Attachment 31585: First try at upstream the files
https://bugs.webkit.org/attachment.cgi?id=31585&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
Style:
 68 static v8::Handle<v8::Value>* listFromVariantArgs(const NPVariant*
arguments, uint32_t argumentCount, NPObject *owner)

again:
 72	    const NPVariant *arg = &arguments[index];

again:
 83	    return v8::String::New(static_cast<const char
*>(identifier->value.string));

Is this safe?
 85	char buf[32];
 86	sprintf(buf, "%d", identifier->value.number);
 87	return v8::String::New(buf);
shouldn't that be snprintf?

Can we say local variable?
 102	    
object->GetInternalField(V8Custom::kDOMWrapperTypeIndex)->IsNumber() &&
 103	    
object->GetInternalField(V8Custom::kDOMWrapperTypeIndex)->Uint32Value() ==
V8ClassIndex::NPOBJECT) {

Early return would have been cleaner here:
     if (npobj->_class == npScriptObjectClass) {
 125	     V8NPObject* object = reinterpret_cast<V8NPObject*>(npobj);
 126

Indent should be 4 spaces:
 140	     if (methodName == NPN_GetStringIdentifier("eval")) {
 141	       if (argumentCount != 1)
 142		   return false;
 143	       if (arguments[0].type != NPVariantType_String)
 144		   return false;
 145	       return NPN_Evaluate(npp, npobj,
const_cast<NPString*>(&arguments[0].value.stringValue), result);
 146	     }

The comment here is useless:
 159	     ASSERT(proxy);  // Must not be NULL.

listFromVariantArgs should have create in its name.  This is not clear usage at
all:
 166	     v8::Handle<v8::Value>* argv = listFromVariantArgs(arguments,
argumentCount, npobj);
 167	     v8::Local<v8::Value> resultObj = proxy->CallFunction(func,
object->v8Object, argumentCount, argv);
 168	     delete[] argv;

There is also now PassOwnPtr which could be used there.

Early return would again be clearer:
 193	 if (npobj->_class == npScriptObjectClass) {
 194	     V8NPObject* object = reinterpret_cast<V8NPObject*>(npobj);

Style:
 346	 V8NPObject *object = reinterpret_cast<V8NPObject*>(npobj);

Again, ownership is unclear here:
 500		 // Create list of arguments to pass to v8.
 501		 v8::Handle<v8::Value>* argv = listFromVariantArgs(arguments,
argumentCount, npobj);
 502		 resultObj = proxy->NewInstance(ctor, argumentCount, argv);
 503		 delete[] argv;

Useless comment:
36 namespace WebCore {
 37	class DOMWindow;
 38 } // namespace WebCore

npp is not needed as an arg name here:
 58 NPObject* npCreateV8ScriptObject(NPP npp, v8::Handle<v8::Object>,
WebCore::DOMWindow*);

Is this ownership really correct?
 67	    v8::String::Utf8Value utf8(object);
 68	    char* utf8_chars = strdup(*utf8);
 69	    STRINGN_TO_NPVARIANT(utf8_chars, utf8.length(), *result);

How do we know that the strdup is matched correctly with a free() call?

Wouldn't a switch work better here?
 84	if (type == NPVariantType_Int32)
 85	    return v8::Integer::New(NPVARIANT_TO_INT32(*variant));
 86	if (type == NPVariantType_Double)
 87	    return v8::Number::New(NPVARIANT_TO_DOUBLE(*variant));

Isn't there a safer WriteAscii call which puts the buffer length check closer
to the actual buffer write?
112	int bufLen = str->Length() + 1;
 113	 if (bufLen <= kStackBufSize) {
 114	     // Use local stack buffer to avoid heap allocations for small
strings. Here we should only use the stack space for
 115	     // stack_buf when it's used, not when we use the heap.
 116	     char stackBuf[kStackBufSize];
 117	     str->WriteAscii(stackBuf);
 118	     return NPN_GetStringIdentifier(stackBuf);
 119	 }

r- for the possible buffer overrun.

I find this code exteremely hard to read.  Maybe V8 is just super-verbose, or
maybe NPAPI is super ugly, or maybe both.  Maybe there is something we could do
to make this all legible?