Bug 24299 – V8's NP bindings should be in WebKit

• Component: Platform
• Status: RESOLVED FIXED

[Brett Wilson (Google)]

We need to upstream V8's NP* bindings into the WebKit tree

[Brett Wilson (Google)]

Created attachment 28193 [details]
Patch v1

[Darin Fisher (:fishd, Google)]

Comment on attachment 28193 [details]
Patch v1

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 -> FIXME


>+
>+
>+// 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);

getV8Context?


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

getV8Proxy


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

TODO -> FIXME


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

FIXME


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

enumeratorCode


>+++ 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>, WebCore::DOMWindow*);
>+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.

FIXME

>+// 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;

HashMap?


>+        // AutoLock safeLock(StringIdentifierMapLock);

delete


>+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";

counterNPObjects


>+// 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;

liveObjects


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

rootObjects


>+      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);
>+    }

indentation


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

indentation


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

forgetV8ObjectForNPObject


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

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


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

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);

utf8Chars


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

getStringIdentifier


>+{
>+    const int kStackBufSize = 100;

stackBufSize


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

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

bindings/npruntime.h

[Darin Fisher (:fishd, Google)]

I'm pretty sure Nate resolved this a long time ago.