[webkit-reviews] review denied: [Bug 26566] [V8] Upstream NPV8Object and V8NPUtils : [Attachment 31765] address eric's comments

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.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 31765: address eric's comments
https://bugs.webkit.org/attachment.cgi?id=31765&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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?