[webkit-reviews] review denied: [Bug 24299] V8's NP bindings should be in WebKit : [Attachment 28193] Patch v1

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Mar 3 15:23:44 PST 2009

Darin Fisher (:fishd, Google) <darin at chromium.org> has denied Brett Wilson
(Google) <brettw at chromium.org>'s request for review:
Bug 24299: V8's NP bindings should be in WebKit

Attachment 28193: Patch v1

------- Additional Comments from Darin Fisher (:fishd, Google)
<darin at chromium.org>
looks good overall.  just a pile of style issues.

>+++ b/WebCore/ChangeLog
>+	  * bindings/v8/np_v8object.cpp: Added.
>+	  (AllocV8NPObject):
>+	  (FreeV8NPObject):
>+	  (listFromVariantArgs):
>+	  (NPIdentifierToV8Identifier):

nit: for newly added files, please exclude the methods that they define
in the ChangeLog.

>+++ b/WebCore/bindings/v8/np_v8object.cpp

maybe the file names should be more webkit style?  NPV8Object.cpp?

>@@ -0,0 +1,496 @@
>+ * Copyright (C) 2004, 2006 Apple Computer, Inc.  All rights reserved.
>+ * Copyright (C) 2007 Google, Inc.  All rights reserved.

Should this be 2007-2009 Google?

>+#include "v8_custom.h"
>+#include "v8_helpers.h"
>+#include "v8_np_utils.h"
>+#include "v8_proxy.h"

we should probably include the new form of these headers: V8Custom.h,
V8Proxy.h, etc.

i figure the others should be renamed too.

>+	  // Special case the "eval" method.
>+	  if (methodName == NPN_GetStringIdentifier("eval")) {
>+	    if (argCount != 1)
>+		return false;

nit: indentation

>+	  // TODO: fix variable naming


>+// TODO: Fix it same as NPN_Invoke (HandleScope and such)

nit: only one newline between functions?

>+	  // Convert the result back to an NPVariant.
>+	  ConvertV8ObjectToNPVariant(resultObj, npobj, result);

nit: should be convertV8ObjectToNPVariant.

>+bool NPN_EvaluateHelper(NPP npp, bool popupsAllowed, NPObject* npobj,
NPString* npscript, NPVariant *result)
>+    VOID_TO_NPVARIANT(*result);
>+    if (!npobj)
>+	  return false;
>+    if (npobj->_class != NPScriptObjectClass)

perhaps this global variable should be named npScriptObjectClass to better
match webkit style?

>+    v8::Handle<v8::Context> context = GetV8Context(npp, npobj);


>+    WebCore::V8Proxy* proxy = GetV8Proxy(npobj);


>+    // TODO(mbelshe) - verify that setting to undefined is right.


>+	  // TODO(fqian): http://b/issue?id=1210340: Use a v8::Object::Keys()
>+	  // when it exists, instead of evaluating javascript.
>+	  // TODO(mpcomplete): figure out how to cache this helper function.


>+	  // Run a helper function that collects the properties on the object
>+	  // an array.
>+	  const char kEnumeratorCode[] =


>+++ b/WebCore/bindings/v8/np_v8object.h

ditto re file naming

>+#include "third_party/npapi/bindings/npruntime.h"

this include path is bad.  webkit convention is to use <bindings/npruntime.h>

>+NPObject* NPN_CreateScriptObject(NPP npp, v8::Handle<v8::Object>,
>+NPObject* NPN_CreateNoScriptObject();

may be a bit bad for these to be prefixed by NPN_ since that is the
"namespace" of NPAPI-defined functions.

>+++ b/WebCore/bindings/v8/npruntime.cpp
>+#include "config.h"
>+#include <map>
>+#include <set>
>+#include <string>

not supposed to be using STL container classes.

>+#include <v8.h>
>+#include "bindings/npruntime.h"

nit: webkit style is to put the header being implemented as the first header
after the config.h

>+#include "ChromiumBridge.h"
>+#include "np_v8object.h"
>+#include "npruntime_priv.h"
>+#include "v8_npobject.h"
>+#include <wtf/Assertions.h>
>+using namespace v8;
>+// TODO: Consider removing locks if we're singlethreaded already.


>+// The static initializer here should work okay, but we want to avoid
>+// static initialization in general.
>+// Commenting out the locks to avoid dependencies on chrome for now.
>+// Need a platform abstraction which we can use.
>+// static Lock StringIdentifierMapLock;

the comments about locks here is irrelevant and should be deleted.
there is only one webkit thread where this code is ever used.

>+typedef std::map<const StringKey, PrivateIdentifier*> StringIdentifierMap;

convert to HashMap?

>+static StringIdentifierMap* getStringIdentifierMap() {

bracket should be on next line

>+// TODO: Consider removing locks if we're singlethreaded already.
>+// static Lock IntIdentifierMapLock;

ditto for removing this bogus lock talk.

>+typedef std::map<int, PrivateIdentifier*> IntIdentifierMap;


>+	  // AutoLock safeLock(StringIdentifierMapLock);


>+void NPN_GetStringIdentifiers(const NPUTF8** names, int32_t nameCount,
>+				NPIdentifier* identifiers) {
>+    ASSERT(names);
>+    ASSERT(identifiers);
>+    if (names && identifiers)
>+	  for (int i = 0; i < nameCount; i++)
>+	      identifiers[i] = NPN_GetStringIdentifier(names[i]);

webkit style guide says to use {}'s when the body of an "if" takes more
than one line.

>+    // AutoLock safeLock(IntIdentifierMapLock);

delete all of these

>+bool NPN_IdentifierIsString(NPIdentifier identifier) {
>+NPUTF8 *NPN_UTF8FromIdentifier(NPIdentifier identifier) {
>+int32_t NPN_IntFromIdentifier(NPIdentifier identifier) {

bracket on next line.

>+static const char* kCounterNPObjects = "NPObjects";


>+// The registry is designed for quick lookup of NPObjects.
>+// JS needs to be able to quickly lookup a given NPObject to determine
>+// if it is alive or not.
>+// The browser needs to be able to quickly lookup all NPObjects which are
>+// "owned" by an object.
>+// The g_live_objects is a hash table of all live objects to their owner
>+// objects.  Presence in this table is used primarily to determine if
>+// objects are live or not.
>+// The g_root_objects is a hash table of root objects to a set of
>+// objects that should be deactivated in sync with the root.	A
>+// root is defined as a top-level owner object.  This is used on
>+// Frame teardown to deactivate all objects associated
>+// with a particular plugin.
>+typedef std::set<NPObject*> NPObjectSet;
>+typedef std::map<NPObject*, NPObject*> NPObjectMap;
>+typedef std::map<NPObject*, NPObjectSet*> NPRootObjectMap;

can these use HashSet and HashMap?

>+// A map of live NPObjects with pointers to their Roots.
>+NPObjectMap g_live_objects;


>+// A map of the root objects and the list of NPObjects
>+// associated with that object.
>+NPRootObjectMap g_root_objects;


>+	if (parent) {
>+	  owner = parent;
>+	}
>+	ASSERT(g_root_objects.find(obj) == g_root_objects.end());
>+	if (g_root_objects.find(owner) != g_root_objects.end())
>+	  (g_root_objects[owner])->insert(obj);
>+    }


>+void _NPN_UnregisterObject(NPObject* obj) {
>+    if (g_live_objects.find(obj) != g_live_objects.end())
>+	owner = g_live_objects.find(obj)->second;


>+	      // Remove the JS references to the object.
>+	      ForgetV8ObjectForNPObject(sub_object);


>+++ b/WebCore/bindings/v8/npruntime_priv.h

shouldn't the copyright notice in this file mention google?

>+#include "third_party/npapi/bindings/npruntime.h"


>+++ b/WebCore/bindings/v8/v8_np_utils.cpp
>@@ -0,0 +1,124 @@
>+// Copyright (c) 2008, Google Inc.

and 2009?

>+#include "DOMWindow.h"
>+#include "Frame.h"
>+#include "PlatformString.h"
>+#undef LOG

is this #undef really necessary?  maybe it is left over cruft?

>+#include "npruntime_priv.h"
>+#include "np_v8object.h"
>+#include "v8_npobject.h"
>+#include "v8_proxy.h"

#include "V8Proxy.h"

>+	  char* utf8_chars = strdup(*utf8);


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


>+    const int kStackBufSize = 100;


>+++ b/WebCore/bindings/v8/v8_np_utils.h

>+#include "third_party/npapi/bindings/npruntime.h"


More information about the webkit-reviews mailing list