Summary: | [Chromium] Upstream V8Binding | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nate Chapin <japhet> | ||||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | ||||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Nate Chapin
2009-06-30 13:42:22 PDT
Created attachment 32090 [details]
patch
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 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.
Created attachment 32130 [details]
patch2
Created attachment 32159 [details]
patch3
Forgot to edit CodeGeneratorV8.pm upstream in previous patches.
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. Created attachment 32198 [details]
patch4
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. Created attachment 32212 [details]
patch5
Rolled back in r45574 because it was going to be breaking the Chromium canary for too long, will resubmit tomorrow. Re-landed at r45603. |