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
https://bugs.webkit.org/show_bug.cgi?id=24299
Attachment 28193: Patch v1
https://bugs.webkit.org/attachment.cgi?id=28193&action=review
------- 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 -> 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