[webkit-reviews] review denied: [Bug 26258] Finish upstreaming V8Custom : [Attachment 31101] patch2

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Nate Chapin <japhet at google.com>'s
request for review:
Bug 26258: Finish upstreaming V8Custom
https://bugs.webkit.org/show_bug.cgi?id=26258

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

------- Additional Comments from David Levin <levin at chromium.org>
This is a lot better.  Just a few last changes and we'll both be done with this
one :)


> Index: WebCore/bindings/v8/custom/V8CustomBinding.cpp
> +ACCESSOR_GETTER(DocumentImplementation)
> +{
> +    ASSERT(info.Holder()->InternalFieldCount() >=
kDocumentMinimumInternalFieldCount);
> +    // Check if the internal field already contains a wrapper.
> +    v8::Local<v8::Value> implementation =
info.Holder()->GetInternalField(kDocumentImplementationIndex);
> +    if (!implementation->IsUndefined())
> +	   return implementation;
> +    // Generate a wrapper.
> +    Document* document =
V8Proxy::DOMWrapperToNative<Document>(info.Holder());
> +    v8::Handle<v8::Value> wrapper =
V8Proxy::DOMImplementationToV8Object(document->implementation());
> +    // Store the wrapper in the internal field.
> +    info.Holder()->SetInternalField(kDocumentImplementationIndex, wrapper);
> +
This code feels very dense.  It would look better with returns before each
comment line.



Right before NAMED_ACCESS_CHECK(History) there is an extra blank line.


> +// static
I would just delete this comment.

> +Frame* V8Custom::GetTargetFrame(v8::Local<v8::Object> host,
v8::Local<v8::Value> data)
> +{
> +    Frame* target = 0;
> +    switch (V8ClassIndex::FromInt(data->Int32Value())) {
> +    case V8ClassIndex::DOMWINDOW: {
> +	   v8::Handle<v8::Value> window =
V8Proxy::LookupDOMWrapper(V8ClassIndex::DOMWINDOW, host);
> +	   if (window.IsEmpty())
> +	       return target;
> +
> +	   DOMWindow* targetWin =
V8Proxy::ToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, window);
s/targetWin/targetWindow/ (avoid abbreviations).


> +#if ENABLE(SVG)
> +V8ClassIndex::V8WrapperType V8Custom::DowncastSVGPathSeg(void* path_seg)
 path_seg bad casing -- use camelCase.


> Index: WebCore/bindings/v8/custom/V8CustomBinding.h
> ===================================================================
> +
> +#define ACCESSOR_GETTER(NAME) \
> +v8::Handle<v8::Value> V8Custom::v8##NAME##AccessorGetter( \
> +    v8::Local<v8::String> name, const v8::AccessorInfo& info)

These defines that aren't indented are confusing to read.

Here's two alternatives:

#define ACCESSOR_GETTER(NAME) \
    v8::Handle<v8::Value> V8Custom::v8##NAME##AccessorGetter( \
	v8::Local<v8::String> name, const v8::AccessorInfo& info)

#define ACCESSOR_GETTER(NAME) \
    v8::Handle<v8::Value>
V8Custom::v8##NAME##AccessorGetter(v8::Local<v8::String> name, const
v8::AccessorInfo& info)


> +    class V8Custom {
> +    public:
> +
Typically this is no blank line after public:, private:, etc.
> +	   // Constants.


> +	   static const int kMessagePortInternalFieldCount =
kDefaultWrapperInternalFieldCount + 2;
> +
> +    #if ENABLE(WORKERS)
Don't indent the preprocessor directives.

> +	   static const int kStyleSheetOwnerNodeIndex =
kDefaultWrapperInternalFieldCount + 0;
> +	   static const int kStyleSheetInternalFieldCount =
kDefaultWrapperInternalFieldCount + 1;
> +
> +    #define DECLARE_PROPERTY_ACCESSOR_GETTER(NAME) \
Don't indent the preprocessor directives.

> +
> +    #define DECLARE_PROPERTY_ACCESSOR(NAME)
DECLARE_PROPERTY_ACCESSOR_GETTER(NAME); DECLARE_PROPERTY_ACCESSOR_SETTER(NAME);


I'd omit the trailing ";"

> +	   DECLARE_PROPERTY_ACCESSOR_GETTER(DOMWindowCrypto);
> +	   // Customized getter&setter of DOMWindow.location

s/&/and/  This comment doesn't event seem correct as it is just a setter.

These comments appear to be inconsistently done and don't seem to provide any
value.	(They don't answer Why? just What? and the code already answer What?
just fine.)

A blank line to separate the different objects seems sufficient (and delete the
comments).



> +	   DECLARE_CALLBACK(NodeAppendChild);
> +
> +	   // Custom implementation is Navigator properties.
The sentence doesn't seem grammatically correct.

> +	   // We actually only need this because WebKit has
> +	   // navigator.appVersion as custom. Our version just
> +	   // passes through.
I like this comment as it answers: Why?
> +	   DECLARE_PROPERTY_ACCESSOR(NavigatorAppVersion);

> +	   #if ENABLE(DATABASE)
Don't indent preprocessor directives.

> +
> +    private:
> +	   static v8::Handle<v8::Value> WindowSetTimeoutImpl(const
v8::Arguments& args, bool singleShot);
Remove the param name "args" since it doesn't add any information.
 


> Index: WebCore/bindings/v8/custom/V8WebKitPointConstructor.cpp
> ===================================================================
> --- WebCore/bindings/v8/custom/V8WebKitPointConstructor.cpp	(revision 0)
> +++ WebCore/bindings/v8/custom/V8WebKitPointConstructor.cpp	(revision 0)
> @@ -0,0 +1,46 @@
> +/*
> + * Copyright (C) 2007,2008,2009 Google Inc. All rights reserved.
Spaces after commas.