Bug 26857 - [Chromium] Upstream V8Binding
Summary: [Chromium] Upstream V8Binding
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-30 13:42 PDT by Nate Chapin
Modified: 2009-07-07 14:09 PDT (History)
0 users

See Also:


Attachments
patch (28.66 KB, patch)
2009-06-30 13:51 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
patch2 (31.07 KB, patch)
2009-07-01 11:12 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
patch3 (33.44 KB, patch)
2009-07-01 16:44 PDT, Nate Chapin
levin: review-
Details | Formatted Diff | Diff
patch4 (33.21 KB, patch)
2009-07-02 14:27 PDT, Nate Chapin
levin: review-
Details | Formatted Diff | Diff
patch5 (33.18 KB, patch)
2009-07-02 17:00 PDT, Nate Chapin
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2009-06-30 13:42:22 PDT
See summary.
Comment 1 Nate Chapin 2009-06-30 13:51:25 PDT
Created attachment 32090 [details]
patch
Comment 2 Eric Seidel (no email) 2009-06-30 14:41:57 PDT
Comment on attachment 32090 [details]
patch

Seems strange that valueToStringWithNullOrUndefinedCheck is the odd-man out here.  All the rest are toWebCoreString() instead of valueTo*
Comment 3 Dimitri Glazkov (Google) 2009-06-30 19:45:31 PDT
Comment on attachment 32090 [details]
patch

Clearing the flag, Nate's working on a bigger/better naming convention. Oh, and it's also faster. Because obviously fastness is important.
Comment 4 Nate Chapin 2009-07-01 11:12:10 PDT
Created attachment 32130 [details]
patch2
Comment 5 Nate Chapin 2009-07-01 16:44:47 PDT
Created attachment 32159 [details]
patch3

Forgot to edit CodeGeneratorV8.pm upstream in previous patches.
Comment 6 David Levin 2009-07-01 22:46:40 PDT
Comment on attachment 32159 [details]
patch3

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.
Comment 7 Nate Chapin 2009-07-02 14:27:18 PDT
Created attachment 32198 [details]
patch4
Comment 8 David Levin 2009-07-02 15:16:25 PDT
Comment on attachment 32198 [details]
patch4

A few last things to address:

> Index: WebCore/bindings/v8/V8Binding.cpp
> +class WebCoreStringResource : public v8::String::ExternalStringResource {
> +public:
> +    explicit WebCoreStringResource(const String& string)
> +        : m_impl(string.impl()) { }

The { } should go on separate lines (since you have initializers).


> +String v8StringToWebCoreString(v8::Handle<v8::String> v8String, bool externalize)
> +{
...
> +
> +    int length = v8String->Length();
> +    if (length) {

if (!length)



> +String v8ValueToWebCoreString(v8::Handle<v8::Value> object)
> +{
...
> +        return webCoreString;
> +    } else {

This else should go away since there is a "return" finishing the previous block.
Comment 9 Nate Chapin 2009-07-02 17:00:56 PDT
Created attachment 32212 [details]
patch5
Comment 10 Nate Chapin 2009-07-06 16:21:01 PDT
Rolled back in r45574 because it was going to be breaking the Chromium canary for too long, will resubmit tomorrow.
Comment 11 Nate Chapin 2009-07-07 13:09:34 PDT
Re-landed at r45603.