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#newcode989
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.
http://codereview.chromium.org/10828229/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
