Add basic support for Latin1 strings to the API.

Add basic support for Latin1 strings to the API.

R=erik.corry@gmail.com
BUG=

[Erik Corry, 2012-08-10 11:28:51]

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?

[Yang, 2012-08-10 13:14:45]

Addressed most comments. I explained my opinion on
MakeExternal(ExternalLatin1StringResource*) below.  The short version: this way
the embedder can avoid using ExternalAsciiStringResource, relying on V8 to sort
things out. If we eventually decide that we need to support Latin1 natively, we
can simply reimplement MakeExternal to accept non-ASCII Latin1 string resources.

@Dan: since you are the target audience here, I would really appreciate your
opinion on this one as well.  Please tell me if there are more changes that you
need or changes that are not necessary (so that I can remove them).

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.
On 2012/08/10 11:28:51, Erik Corry wrote:
> Comment seems wrong as it can't return a UTF8 encoding.

Done.

http://codereview.chromium.org/10828229/diff/1/include/v8.h#newcode1259
include/v8.h:1259: int encoding = UTF8_ENCODING);
On 2012/08/10 11:28:51, Erik Corry wrote:
> 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.

I see your point here.  But then I also feel a certain obligation to maintain
backward-compatibility... I could set the default encoding to LATIN1_ENCODING |
STRICT_ASCII_HINT, but this may still break things for embedders that don't
update their code.

http://codereview.chromium.org/10828229/diff/1/include/v8.h#newcode1261
include/v8.h:1261: /** Allocates a new string from 16-bit character codes.*/
On 2012/08/10 11:28:51, Erik Corry wrote:
> Not your error, but:
> 16-bit character codes -> 16-bit UTF16 code units
> Here and in other places

Done.

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
On 2012/08/10 11:28:51, Erik Corry wrote:
> is called ->is called immediately
> ?

Done.

http://codereview.chromium.org/10828229/diff/1/include/v8.h#newcode1348
include/v8.h:1348: V8EXPORT bool MakeExternal(ExternalLatin1StringResource*
resource);
On 2012/08/10 11:28:51, Erik Corry wrote:
> Seems like this should be an ExternalASCIIStringResource.

It's actually intended to be like this.  I'm torn between offering a
MakeExternal for Latin1 string resources that refuses to externalize for
non-Latin1 strings (in which case the embedder has to resort to using two-byte
string resources if he *really* wants to externalize), or directly expose to the
embedder that V8 still uses ASCII strings internally, forcing him to use
ExternalAsciiStringResource here.

The former has the advantage that if we decide to support Latin1 natively in V8,
we can simply reimplement ExternalLatin1StringResource and the corresponding
MakeExternal.  It would then also accept non-ASCII Latin1 string resources when
externalizing.  This all would remain transparent to the embedder, i.e. he
doesn't have to change his code since it already assumes that V8 can handle
Latin1.

http://codereview.chromium.org/10828229/diff/1/include/v8.h#newcode4389
include/v8.h:4389: if ((GetExternalStringEncoding() & kStringEncodingMask) ==
On 2012/08/10 11:28:51, Erik Corry wrote:
> 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'.

Done.

http://codereview.chromium.org/10828229/diff/1/include/v8.h#newcode4407
include/v8.h:4407: case I::kExternalTwoByteRepresentationTag |
I::kExternalAsciiDataHintTag:
On 2012/08/10 11:28:51, Erik Corry wrote:
> Indentation looks wrong here.

Done.

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) {
On 2012/08/10 11:28:51, Erik Corry wrote:
> I don't see why we need this.

Explained in v8.h

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 =
On 2012/08/10 11:28:51, Erik Corry wrote:
> Inadvertent edit?

Yes.
Done.

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(
On 2012/08/10 11:28:51, Erik Corry wrote:
> Inadvertent edit?

Yup.
Done.

http://codereview.chromium.org/10828229/diff/1/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/10828229/diff/1/test/cctest/test-api.cc#newcod...
test/cctest/test-api.cc:989: CHECK(!latin1_a->MakeExternal(latin1_a_resource));
This would be the use case of MakeExternal(ExternalLatin1StringResource*).
Currently, we simply refuse to externalize non-ASCII strings if provided with an
external Latin1 string resource. If necessary, we could change that behavior to
accept those strings once V8 can deal with Latin1 strings internally.

With this, the embedder can get rid of usage of ExternalAsciiStringResource
entirely.

[Erik Corry, 2012-08-21 12:50:26]

LGTM