[Webkit-unassigned] [Bug 26857] [Chromium] Upstream V8Binding

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=26857


levin at chromium.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #32159|review?                     |review-
               Flag|                            |




------- Comment #6 from levin at chromium.org  2009-07-01 22:46 PDT -------
(From update of attachment 32159)
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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.