[webkit-reviews] review denied: [Bug 26857] [Chromium] Upstream V8Binding : [Attachment 32159] patch3

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jul 1 22:46:40 PDT 2009


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

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

------- Additional Comments from David Levin <levin at chromium.org>
A few things to address.


> Index: WebCore/bindings/v8/V8Binding.cpp
> +class WebCoreStringResource: public v8::String::ExternalStringResource {

Add a space after WebCoreStringResource.

> +public:
> +    explicit WebCoreStringResource(const String& string)
> +	   : m_impl(string.impl()) { }

The { } should go on separate lines at this point.

> +
> +    virtual ~WebCoreStringResource() {}

Add space in {}


> +String v8StringToWebCoreString(v8::Handle<v8::String> v8String, bool
externalize)
> +{
> +    WebCoreStringResource* stringResource =
static_cast<WebCoreStringResource*>(v8String->GetExternalStringResource());
> +    if (stringResource)
> +	   return stringResource->webcoreString();
> +
> +    int length = v8String->Length();
> +    if (length == 0) {

Comparison to 0.


> +String v8ValueToWebCoreString(v8::Handle<v8::Value> object)
> +{
> +    if (object->IsString()) {
> +	   v8::Handle<v8::String> v8String =
v8::Handle<v8::String>::Cast(object);
> +	   String webCoreString = v8StringToWebCoreString(v8String, true);
> +	   return webCoreString;

This seems overly verbose plus it may inhibit return value optimization.

I'd suggest just doing:
	return v8StringToWebCoreString(v8String, true);



> +    } else if (object->IsInt32()) {
> +	   int value = object->Int32Value();
> +	   // Most numbers used are <= 100. Even if they aren't used there's
very little in using the space.
> +	   const int kLowNumbers = 100;
> +	   static AtomicString lowNumbers[kLowNumbers + 1];

Consider using DEFINE_STATIC_LOCAL



> Property changes on: WebCore/bindings/v8/V8Binding.cpp
> ___________________________________________________________________
> Added: svn:executable
>    + *

This should be fixed.



> Index: WebCore/bindings/v8/V8Binding.h
> +#include "config.h"

Don't include config.h in header files.


More information about the webkit-reviews mailing list