[webkit-reviews] review denied: [Bug 26623] [Chromium] Upstream V8Proxy : [Attachment 31815] patch4

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Nate Chapin <japhet at google.com>'s
request for review:
Bug 26623: [Chromium] Upstream V8Proxy
https://bugs.webkit.org/show_bug.cgi?id=26623

Attachment 31815: patch4
https://bugs.webkit.org/attachment.cgi?id=31815&action=review

------- Additional Comments from David Levin <levin at chromium.org>
I think we can finish with just one more round.  A few specific things to clean
up below.



> Index: WebCore/ChangeLog
> +2009-06-24  Nate Chapin  <japhet at chromium.org>

I think this changelog need to be updated due to the new changes.


> Index: WebCore/bindings/v8/V8Proxy.h

very minor nit: General comment, WebKit uses one space after periods in
comments. If you do a search and replace of ".	" with ". ", that would fix
this up.

> +    class CSSStyleDeclaration;
> +    class ClientRectList;
> +    class DOMImplementation;
> +    class Element;
> +    class Event;
> +    class EventListener;
> +    class Frame;
> +    class HTMLCollection;
> +    class HTMLOptionsCollection;
> +    class HTMLElement;
> +    class HTMLDocument;
> +    class MediaList;
> +    class NamedNodeMap;
> +    class Node;
> +    class NodeList;
> +    class Screen;
> +    class String;
> +    class StyleSheet;
> +    class SVGElement;
> +    class DOMWindow;
> +    class Document;
> +    class EventTarget;
> +    class Event;
> +    class EventListener;
> +    class Navigator;
> +    class MimeType;
> +    class MimeTypeArray;
> +    class Plugin;
> +    class PluginArray;
> +    class StyleSheetList;
> +    class CSSValue;
> +    class CSSRule;
> +    class CSSRuleList;
> +    class CSSValueList;
> +    class NodeFilter;
> +    class ScriptExecutionContext;

These should be sorted (to avoid duplicates) for example Event is duplicated
and a few others.


> + #if ENABLE(SVG)
> +    class SVGElementInstance;
> + #endif

Extra space before #'s here.

> +    class V8EventListener;
> +    class V8ObjectEventListener;

Put with the rest of the forward declarations.

> +    void logInfo(Frame* frame, const String& msg, const String& url);

Remove param name "frame".
s/msg/message/

> +    class GlobalHandleInfo {
> +    public:
> +	   GlobalHandleInfo(void* host, GlobalHandleType type) : host_(host),
type_(type) { }
> +	   void* host_;

Use m_host

> +	   GlobalHandleType type_;
Use m_type

> +    };
> +
> +#endif  // NDEBUG

single space before end of line comments.


> +	    enum ErrorType {
> +		RANGE_ERROR,
> +		REFERENCE_ERROR,
> +		SYNTAX_ERROR,
> +		TYPE_ERROR,
> +		GENERAL_ERROR
> +	    };

"Enum members should user InterCaps with an initial capital letter."
RangeError, etc.


> +	   void setEventHandlerLineNumber(int lineNumber) { m_handlerLineno =
lineNumber; }
> +	   void finishedWithEvent(Event* event) { }

Remove "event".


> +	   // Create a V8 wrapper for a C pointer
> +	   static v8::Handle<v8::Value> wrapCPointer(void* cptr)
> +	   {
> +	       // Represent void* as int
> +	       int addr = reinterpret_cast<int>(cptr);
> +	       ASSERT(!(addr & 0x01));	// the address must be aligned.
Single space before end of line comment.

"T"he address...


> +	   // Take C pointer out of a v8 wrapper

Add "."

> +#ifndef NDEBUG
> +	   // Checks if a v8 value can be a DOM wrapper

Add "."


> +	   template<typename T>
> +	   static v8::Handle<v8::Value>
convertToV8Object(V8ClassIndex::V8WrapperType type, PassRefPtr<T> imp)
> +	   {
> +	     return convertToV8Object(type, imp.get());

Fix indentation.

> +	   // Wrap and unwrap JS event listeners
Add "."

> +	   // Wrap JS node filter in C++

Add "."

> +#ifndef NDEBUG
> +	   // For debugging and leak detection purpose
Add "."


> +	   // Check whether a V8 value is a DOM Event wrapper
Add "."

> +	   // Take C pointer out of a v8 wrapper
Add "."

> +	   // The first parameter, desc_type, specifies the function descriptor

> +	   // used to create JS object. The second parameter, cptr_type,
specifies
> +	   // the type of third parameter, impl, for type casting.
> +	   // For example, a HTML element has HTMLELEMENT desc_type, but always

> +	   // use NODE as cptr_type. JS wrapper stores cptr_type and impl as
> +	   // internal fields.
> +	   static v8::Local<v8::Object>
instantiateV8Object(V8ClassIndex::V8WrapperType, V8ClassIndex::V8WrapperType,
void*);

Rework the comment to remove parameters names since they aren't in the decl
anymore.

> +	   static v8::Local<v8::Context> utilityContext()
> +	   {
> +	     if (m_utilityContext.IsEmpty())
> +		 createUtilityContext();
> +	     return v8::Local<v8::Context>::New(m_utilityContext);
> +	   }
Indentation is off.


> +    template <int tag, typename T>
> +    v8::Handle<v8::Value> V8Proxy::constructDOMObject(const v8::Arguments&
args)
> +    {
> +	   if (!args.IsConstructCall()) {
> +	     V8Proxy::throwError(V8Proxy::TYPE_ERROR, "DOM object constructor
cannot be called as a function.");
> +	     return v8::Undefined();
Fix indentation and switch to the new "throwError".



> Index: WebCore/bindings/v8/V8Proxy.cpp

> +#include <algorithm>
> +#include <utility>
> +#include <v8.h>
> +#include <v8-debug.h>
> +#include <wtf/Assertions.h>
> +#include <wtf/StdLibExtras.h>
> +#include <wtf/UnusedParam.h>

Typically these come after the "" includes.


> +// Static utility context.

Seems obvious I'd remove the comment.

> +v8::Persistent<v8::Context> V8Proxy::m_utilityContext;

> +void batchConfigureAttributes(v8::Handle<v8::ObjectTemplate> instance,
v8::Handle<v8::ObjectTemplate> proto, const BatchedAttribute* attributes,
size_t attributeCount)
> +void batchConfigureConstants(v8::Handle<v8::FunctionTemplate>
functionDescriptor, v8::Handle<v8::ObjectTemplate> proto, const
BatchedConstant* constants, size_t constantCount)

If these functions are only used within this file, make them "static".


> +v8::Handle<v8::Value>
V8Proxy::convertSVGObjectWithContextToV8Object(V8ClassIndex::V8WrapperType
type, void* object)
> +{
> +    if (!object)
> +	   return v8::Null();
> +
> +    v8::Persistent<v8::Object> result =
getDOMSVGObjectWithContextMap().get(object);
> +    if (!result.IsEmpty())
> +	   return result;
> +
> +    // Special case: SVGPathSegs need to be downcast to their real type
> +    if (type == V8ClassIndex::SVGPATHSEG)
> +	   type = V8Custom::DowncastSVGPathSeg(object);
> +
> +    v8::Local<v8::Object> v8Object = instantiateV8Object(type, type,
object);
> +    if (!v8Object.IsEmpty()) {
> +	 result = v8::Persistent<v8::Object>::New(v8Object);
Fix indentation starting here.



> +    for (size_t i = 0; i < grouper.size(); ) {
> +	   // Seek to the next key (or the end of the list).
> +	   size_t nextKeyIndex = grouper.size();
> +	   for (size_t j = i; j < grouper.size(); ++j) {
> +	       if (grouper[i].first != grouper[j].first) {
> +		 nextKeyIndex = j;
> +		 break;
Fix indentation.



> +typedef HashMap<int, v8::FunctionTemplate*> FunctionTemplateMap;
> +
> +bool AllowAllocation::m_current = false;
> +
> +// JavaScriptConsoleMessages encapsulate everything needed to
> +// log messages originating from JavaScript to the Chrome console.
> +class JavaScriptConsoleMessage {
> +public:
> +    JavaScriptConsoleMessage(const String& string, const String& sourceID,
unsigned lineNumber)
> +	   : m_string(string)
> +	   , m_sourceID(sourceID)
> +	   , m_lineNumber(lineNumber) { }
Put the { } on seperate lines below.


> +static void handleConsoleMessage(v8::Handle<v8::Message> message,
v8::Handle<v8::Value> data)
> +{
...
> +    bool useURL = (resourceName.IsEmpty() || !resourceName->IsString());
Remove unnecessary parens.

> +    String resourceNameString = (useURL) ? frame->document()->url() :
ToWebCoreString(resourceName);

Remove unnecessary parens.

> +enum DelayReporting {
> +    REPORT_LATER,
> +    REPORT_NOW
> +};

"Enum members should user InterCaps with an initial capital letter."
  ReportLater
  ReportNow


> +
> +static void reportUnsafeAccessTo(Frame* target, DelayReporting delay)
> +{
> +    ASSERT(target);
> +    Document* targetDocument = target->document();
> +    if (!targetDocument)
> +	   return;
> +
> +    Frame* source = V8Proxy::retrieveFrameForEnteredContext();
> +    if (!source || !source->document())
> +	   return;  // Ignore error if the source document is gone.
Single space before end of line comment.

> +    String str = String::format("Unsafe JavaScript attempt to access frame "

> +	   "with URL %s from frame with URL %s. "
> +	   "Domains, protocols and ports must match.\n",
> +	   targetDocument->url().string().utf8().data(),
> +	   sourceDocument->url().string().utf8().data());

When indenting within a parenthesized expression, indent to the (.

> +
> +    // Build a console message with fake source ID and line number.
> +    const String kSourceID = "";
> +    const int kLineNumber = 1;
> +    JavaScriptConsoleMessage message(str, kSourceID, kLineNumber);
> +
> +    if (delay == REPORT_NOW) {
> +	   // NOTE(tc): Apple prints the message in the target page, but it
seems like

I think (tc) is an alias, so we can remove that, and it should probably say
"Safari" instead of Apple.



> +bool V8Proxy::handleOutOfMemory()
> +{
...
> +    if (proxy) {
> +	   // Clean m_context, and event handlers.
> +	   proxy->clearForClose();
> +	   // Destroy the global object.
> +	   proxy->destroyGlobal();

"Destroy the global object." seems too obvious to have as a comment but leave a
blank line to separate from the other commented item.


> +	   // Isolate exceptions that occur when executing the code.  These
> +	   // exceptions should not interfere with javascript code we might
> +	   // evaluate from C++ when returning from here
Add "."

> +
> +	   // Evaluating the JavaScript could cause the frame to be
deallocated,
> +	   // so we start the keep alive timer here.
> +	   // Frame::keepAlive method adds the ref count of the frame and sets
a
> +	   // timer to decrease the ref count. It assumes that the current
JavaScript
> +	   // execution finishs before firing the timer.
s/finishs/finishes/

> +	   // See issue 1218756 and 914430.
s/issue/issues/  These are internal bug numbers so it is odd to keep them here
but maybe they are good for reference.	Ask around and see what people think
should be done with this.

> +v8::Local<v8::Value> V8Proxy::callFunction(v8::Handle<v8::Function>
function, v8::Handle<v8::Object> receiver, int argc, v8::Handle<v8::Value>
args[])
> +{
...
> +	   // Evaluating the JavaScript could cause the frame to be
deallocated,
> +	   // so we start the keep alive timer here.
> +	   // Frame::keepAlive method adds the ref count of the frame and sets
a
> +	   // timer to decrease the ref count. It assumes that the current
JavaScript
> +	   // execution finishs before firing the timer.
> +	   // See issue 1218756 and 914430.

Same code comment as before with the same issues.


> +v8::Local<v8::Value> V8Proxy::newInstance(v8::Handle<v8::Function>
constructor, int argc, v8::Handle<v8::Value> args[])
> +{
...
> +	   // See comment in V8Proxy::callFunction.

I'm happy the comment isn't here again, but maybe we can reduce the two copies
to one using a technique like this :)


> +
> +v8::Persistent<v8::FunctionTemplate>
V8Proxy::getTemplate(V8ClassIndex::V8WrapperType type)
> +{
> +    v8::Persistent<v8::FunctionTemplate>* cacheCell =
V8ClassIndex::GetCache(type);
> +    if (!(*cacheCell).IsEmpty())

The more typical way to write this would be: 
    if (!cacheCell->IsEmpty())

> +
> +    // not found

How about "Not in the cache."


> +void V8Proxy::clearForNavigation()
> +{
> +    // disconnect all event listeners
> +    disconnectEventListeners();

Remove comment.

> +void V8Proxy::setSecurityToken()
> +{
> +    Document* document = m_frame->document();
> +    // Setup security origin and security token
Add "."


> +v8::Persistent<v8::Context> V8Proxy::createNewContext(v8::Handle<v8::Object>
global)
> +{

> +    const char** extensionNames = new const char*[m_extensions.size()];

Make this a OwnArrayPtr (and get rid of the "delete[] extensionNames").

> +void V8Proxy::initContextIfNeeded()
> +{
...
> +    DEFINE_STATIC_LOCAL(bool, isV8Initialized, (false));

You really only need to use "DEFINE_STATIC_LOCAL" when you have a class
(something with a destructor).	For bool's, int's etc., just define them like
usual.
There may have been other instances of this, so just take this comment to apply
to all of them.

> +
> +    // Store the first global object created so we can reuse it.
> +    if (m_global.IsEmpty()) {
> +	   m_global = v8::Persistent<v8::Object>::New(v8Context->Global());
> +	   // Bail out if allocation of the first global objects fails.
> +	   if (m_global.IsEmpty()) {
> +	     disposeContextHandles();
> +	     return;

Fix indentation.