This change feels like it is adding more than we need to support WebKit. I would prefer just to add whatever we need to make it work.
http://codereview.chromium.org/10828229/diff/1/include/v8.h File include/v8.h (right): http://codereview.chromium.org/10828229/diff/1/include/v8.h#newcode1233 include/v8.h:1233: * and possibly a hint on whether the content is ASCII. Comment seems wrong as it can't return a UTF8 encoding. http://codereview.chromium.org/10828229/diff/1/include/v8.h#newcode1259 include/v8.h:1259: int encoding = UTF8_ENCODING); No point in making the second argument optional. There's no way to call this with only 1 argument because in that case the encoding is UTF8 and you should have given a length. Perhaps reorder the arguments? Here and other places in this file. http://codereview.chromium.org/10828229/diff/1/include/v8.h#newcode1261 include/v8.h:1261: /** Allocates a new string from 16-bit character codes.*/ Not your error, but: 16-bit character codes -> 16-bit UTF16 code units Here and in other places http://codereview.chromium.org/10828229/diff/1/include/v8.h#newcode1329 include/v8.h:1329: * string object on the V8 heap and the Dispose method is called on the is called ->is called immediately ? http://codereview.chromium.org/10828229/diff/1/include/v8.h#newcode1348 include/v8.h:1348: V8EXPORT bool MakeExternal(ExternalLatin1StringResource* resource); Seems like this should be an ExternalASCIIStringResource. http://codereview.chromium.org/10828229/diff/1/include/v8.h#newcode4389 include/v8.h:4389: if ((GetExternalStringEncoding() & kStringEncodingMask) == This calls GetExternalStringEncoding which gets one of 4 answers, then we throw some information away so that there are only 2 answers and do an 'if' on it. I think it would be faster and simpler to just get the representation of the string directly and test that in the 'if'. http://codereview.chromium.org/10828229/diff/1/include/v8.h#newcode4407 include/v8.h:4407: case I::kExternalTwoByteRepresentationTag | I::kExternalAsciiDataHintTag: Indentation looks wrong here. http://codereview.chromium.org/10828229/diff/1/src/api.cc File src/api.cc (right): http://codereview.chromium.org/10828229/diff/1/src/api.cc#newcode4912 src/api.cc:4912: bool v8::String::MakeExternal(ExternalLatin1StringResource* resource) { I don't see why we need this. http://codereview.chromium.org/10828229/diff/1/src/factory.cc File src/factory.cc (right): http://codereview.chromium.org/10828229/diff/1/src/factory.cc#newcode734 src/factory.cc:734: Handle<String> error_string = Inadvertent edit? http://codereview.chromium.org/10828229/diff/1/src/parser.cc File src/parser.cc (right): http://codereview.chromium.org/10828229/diff/1/src/parser.cc#newcode1828 src/parser.cc:1828: isolate()->factory()->NewStringFromUtf8( Inadvertent edit? http://codereview.chromium.org/10828229/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
