Bug 26566 – [Chromium] Upstream NPV8Object and V8NPUtils

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Albert J. Wong]

Upstreaming 
  V8NPUtils.h
  V8NPUtils.cpp
  NPV8Object.h
  NPV8Object.cpp

I didn't quite know how to handle the NPN_* functions.  It didn't seem right to rename them.  Let me know if/how they should be changed.

Also, there's a use if v8::Undefined() that I think I'm supposed to do something with, but not sure what.

[Albert J. Wong]

Created attachment 31585 [details]
First try at upstream the files

[Albert J. Wong]

Here is the related chromium change.

http://codereview.chromium.org/141022

[Eric Seidel (no email)]

Comment on attachment 31585 [details]
First try at upstream the files

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?

[Albert J. Wong]

Created attachment 31765 [details]
address eric's comments

I tried to address all of Eric's comments.

I couldn't use a PassOwnPtr however because the delete is an array delete.  Silly C++ and its asymetric deletes. :-/

I also made a few attempts at cleaning up the code organization + factoring out common parts, but ultimately failed at all of it.  There's too much stuff going where error handling and object construction are interwoven. :(

[David Levin]

Comment on attachment 31765 [details]
address eric's comments

A few more items to fix up.  Almost there.


> diff --git a/WebCore/bindings/v8/NPV8Object.cpp b/WebCore/bindings/v8/NPV8Object.cpp
> +/*
> + * Copyright (C) 2004, 2006 Apple Computer, Inc.  All rights reserved.
> + * Copyright (C) 2007-2009 Google, Inc.  All rights reserved.

Uses commas instead of range.  2007, 2008, 2009

> +#include <stdio.h>

> +#include "NPV8Object.h"
This one should be here.

> +
> +#include <v8.h>
> +

> +#include "ChromiumBridge.h"
> +#include "DOMWindow.h"
> +#include "Frame.h"
> +#include "PlatformString.h"
> +#include "ScriptController.h"
> +#include "V8CustomBinding.h"
> +#include "V8Helpers.h"
> +#include "V8NPUtils.h"
> +#include "V8Proxy.h"
> +#include "bindings/npruntime.h"
> +#include "npruntime_priv.h"

Sort these.

> +// FIXME(mbelshe): Comments on why use malloc and free.

FIXME's never really have a name.  (Typically because one can tell who added them due to revision history, but I know that isn't true here.  Still I'm not sure how much value the name has at this point.  There has been plenty of time for folks to address these.)

End result: remove the name.

> +    char buffer[32];
> +    snprintf(buffer, sizoef(buffer), "%d", identifier->value.number);

typo: "sizoef"

> +NPObject* npCreateV8ScriptObject(NPP npp, v8::Handle<v8::Object> object, WebCore::DOMWindow* root)
> +{
> +    v8::Local<v8::Value> typeIndex = object->GetInternalField(V8Custom::kDOMWrapperTypeIndex);
> +    // Check to see if this object is already wrapped.
> +    if (object->InternalFieldCount() == V8Custom::kNPObjectInternalFieldCount &&
> +        typeIndex->IsNumber() &&
> +        typeIndex->Uint32Value() == V8ClassIndex::NPOBJECT) {

&& go at the beginning of lines (or even better make it one line).

> +    v8::Handle<v8::Value>* argv = createValueListFromVariantArgs(arguments, argumentCount, npObject);
> +    v8::Local<v8::Value> resultObject = proxy->CallFunction(function, v8NpObject->v8Object, argumentCount, argv);
> +    delete[] argv;
It could use a OwnArrayPtr here but for such a small scope, it feels like overkill.

> +        // Create list of arguments to pass to v8.
> +        v8::Handle<v8::Value>* argv = createValueListFromVariantArgs(arguments, argumentCount, npObject);
> +        resultObject = proxy->CallFunction(function, functionObject, argumentCount, argv);
> +        delete[] argv;
Same thing about OwnArrayPtr.  Just for you to consider.

> +    V8NPObject *object = reinterpret_cast<V8NPObject*>(npObject);
incorrect * placement. (search for this because it occurs several times for "V8NPObject *").

> +    v8::Handle<v8::Object> obj(object->v8Object);
> +    // FIXME(mbelshe): Verify that setting to undefined is right.

remove name in fixme.

> +        // FIXME(fqian): http://b/issue?id=1210340: Use a v8::Object::Keys() method when it exists, instead of evaluating javascript.
> +
> +        // FIXME(mpcomplete): Figure out how to cache this helper function.  Run a helper function that collects the properties

remove name in fixme.


> diff --git a/WebCore/bindings/v8/NPV8Object.h b/WebCore/bindings/v8/NPV8Object.h

> + * Copyright (C) 2006-2009 Google Inc. All rights reserved.
expand range to comma separated years.

> diff --git a/WebCore/bindings/v8/V8NPUtils.cpp b/WebCore/bindings/v8/V8NPUtils.cpp

> + * Copyright (C) 2008-2009 Google Inc. All rights reserved.

expand range to comma separated years.


> +v8::Handle<v8::Value> convertNPVariantToV8Object(const NPVariant* variant, NPObject* npobject)
> +{
> +    NPVariantType type = variant->type;
> +
> +    switch (type) {
> +    case NPVariantType_Int32:
> +        return v8::Integer::New(NPVARIANT_TO_INT32(*variant));
> +    case  NPVariantType_Double:
> +        return v8::Number::New(NPVARIANT_TO_DOUBLE(*variant));
> +    case  NPVariantType_Bool:
> +        return NPVARIANT_TO_BOOLEAN(*variant) ? v8::True() : v8::False();
> +    case  NPVariantType_Null:
> +        return v8::Null();
> +    case  NPVariantType_Void:
> +        return v8::Undefined();
> +    case  NPVariantType_String: {
> +        NPString src = NPVARIANT_TO_STRING(*variant);
> +        return v8::String::New(src.UTF8Characters, src.UTF8Length);
> +    }
> +    case  NPVariantType_Object: {


The two spaces after "case" is odd.

> +
> +// Helper function to create an NPN String Identifier from a v8 string.
> +NPIdentifier getStringIdentifier(v8::Handle<v8::String> str)
str -> string

> +    const int kStackBufSize = 100;

Buf -> buffer

> +    int bufLen = str->Length() + 1;

bufLen -> bufferLength

> +    if (bufLen <= kStackBufSize) {
> +        // Use local stack buffer to avoid heap allocations for small strings. Here we should only use the stack space for
> +        // stack_buf when it's used, not when we use the heap.

"stack_buf" may need to be updated to new name.

> +        //
> +        // WriteAscii is guaranteed to generate a null-terminated string because bufLen is constructed to be one greater
> +        // than the string length.
> +        char stackBuf[kStackBufSize];
Buf -> Buffer


> diff --git a/WebCore/bindings/v8/V8NPUtils.h b/WebCore/bindings/v8/V8NPUtils.h

> @@ -0,0 +1,25 @@
> +// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.

This copyright needs to be replaced.  You can use the one from WebCore/loader/ThreadableLoader.h as good boilerplate.

> +
> +#include <v8.h>
> +#include "third_party/npapi/bindings/npruntime.h"
> +
> +namespace WebCore {
> +    class Frame;
> +}

"Frame" isn't used below. Can we get rid of this?

[Albert J. Wong]

Created attachment 31807 [details]
address Dave's comments. :)

This time, I verified that my compile command actually compiled the changes.  Yay!.

[Albert J. Wong]

Created attachment 31810 [details]
Remove some of the more obvious comments, and fix * association.

[David Levin]

Assign to levin for landing.

[David Levin]

Committed as http://trac.webkit.org/changeset/45189.