See summary.
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.