Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(28)

Issue 10857030: Add basic support for Latin1 to the API. (Closed)

Created:
8 years, 4 months ago by Yang
Modified:
8 years, 3 months ago
Reviewers:
Erik Corry
CC:
v8-dev
Visibility:
Public.

Description

Add basic support for Latin1 to the API. BUG= Committed: https://code.google.com/p/v8/source/detail?r=12430

Patch Set 1 #

Patch Set 2 : removed MakeExternal(ExternalLatin1StringResource*) and added counters. #

Total comments: 28

Patch Set 3 : addressed comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+726 lines, -124 lines) Patch
M include/v8.h View 1 2 11 chunks +155 lines, -34 lines 0 comments Download
M src/api.cc View 1 2 19 chunks +176 lines, -61 lines 0 comments Download
M src/factory.h View 1 chunk +7 lines, -1 line 0 comments Download
M src/factory.cc View 1 chunk +15 lines, -2 lines 0 comments Download
M src/heap.h View 1 chunk +9 lines, -1 line 0 comments Download
M src/heap.cc View 1 4 chunks +44 lines, -21 lines 0 comments Download
M src/heap-inl.h View 1 2 2 chunks +26 lines, -3 lines 0 comments Download
M src/objects.h View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M src/v8-counters.h View 1 1 chunk +4 lines, -0 lines 0 comments Download
M test/cctest/test-api.cc View 1 2 1 chunk +286 lines, -1 line 0 comments Download

Messages

Total messages: 6 (0 generated)
Yang
Started a new CL because I accidentally rebased and messed up the diff. I also ...
8 years, 4 months ago (2012-08-16 09:44:22 UTC) #1
Yang
On 2012/08/16 09:44:22, Yang wrote: > Started a new CL because I accidentally rebased and ...
8 years, 4 months ago (2012-08-16 09:44:52 UTC) #2
Yang
On 2012/08/16 09:44:52, Yang wrote: > On 2012/08/16 09:44:22, Yang wrote: > > Started a ...
8 years, 4 months ago (2012-08-21 07:37:20 UTC) #3
Erik Corry
http://codereview.chromium.org/10857030/diff/2001/include/v8.h File include/v8.h (right): http://codereview.chromium.org/10857030/diff/2001/include/v8.h#newcode1075 include/v8.h:1075: UTF8_ENCODING = 1, The names are not UTF8 or ...
8 years, 4 months ago (2012-08-21 12:21:34 UTC) #4
Erik Corry
lgtm
8 years, 4 months ago (2012-08-21 12:58:49 UTC) #5
Yang
8 years, 4 months ago (2012-08-21 13:06:20 UTC) #6
Addressed comments.

http://codereview.chromium.org/10857030/diff/2001/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/10857030/diff/2001/include/v8.h#newcode1075
include/v8.h:1075: UTF8_ENCODING     = 1,
On 2012/08/21 12:21:34, Erik Corry wrote:
> The names are not UTF8 or UTF16, but rather UTF-8 and UTF-16.  These should
> therefore be called UTF_8_ENCODING and UTF_16_ENCODING.  Also this needs
fixing
> several places in the comments.

Done.

http://codereview.chromium.org/10857030/diff/2001/include/v8.h#newcode1079
include/v8.h:1079: STRICT_ASCII_HINT = 1 << 16,
On 2012/08/21 12:21:34, Erik Corry wrote:
> I think STRICT_ASCII_HINT should just be called ASCII_HINT

Done.

http://codereview.chromium.org/10857030/diff/2001/include/v8.h#newcode1100
include/v8.h:1100: // Latin1 characters.
On 2012/08/21 12:21:34, Erik Corry wrote:
> This one doesn't support PRESERVE_ASCII_NULL, or rather it is always on.  That
> should be noted.  May also apply to WriteUtf8.

Done.

http://codereview.chromium.org/10857030/diff/2001/include/v8.h#newcode1211
include/v8.h:1211: * strings are converted to ASCII or two-byte string depending
on whether
On 2012/08/21 12:21:34, Erik Corry wrote:
> string -> strings,

Done.

http://codereview.chromium.org/10857030/diff/2001/include/v8.h#newcode1232
include/v8.h:1232: * If the string is external, return the its encoding (Latin1
or UTF16)
On 2012/08/21 12:21:34, Erik Corry wrote:
> the its -> its

Done.

http://codereview.chromium.org/10857030/diff/2001/include/v8.h#newcode1248
include/v8.h:1248: * Allocates a new string from either UTF-8-, Latin1-encoded
data.
On 2012/08/21 12:21:34, Erik Corry wrote:
> "-," should be "or "

Done.

http://codereview.chromium.org/10857030/diff/2001/include/v8.h#newcode1249
include/v8.h:1249: * The second parameter 'length' gives the buffer length.  If
the data is
On 2012/08/21 12:21:34, Erik Corry wrote:
> is UTF-8 encoded -> may contain zero bytes

Done.

http://codereview.chromium.org/10857030/diff/2001/include/v8.h#newcode1254
include/v8.h:1254: * whether the string contains ASCII characters.  In case of
Latin1, the
On 2012/08/21 12:21:34, Erik Corry wrote:
> In case of -> In the case of

Done.

http://codereview.chromium.org/10857030/diff/2001/include/v8.h#newcode1259
include/v8.h:1259: int encoding = UTF8_ENCODING);
On 2012/08/21 12:21:34, Erik Corry wrote:
> Surely this should be a StringEncoding and not an int.

The encoding here may additionally contain NOT_ASCII_HINT or ASCII_HINT,
therefore I can't simply use an enum, since e.g. (ASCII_HINT | UTF-8-ENCODING)
is not a valid element of the enum.

http://codereview.chromium.org/10857030/diff/2001/include/v8.h#newcode1328
include/v8.h:1328: * If the data contains non-ASCII character, the string is
created as new
On 2012/08/21 12:21:34, Erik Corry wrote:
> contains -> contains a
> as new -> as a new

Done.

http://codereview.chromium.org/10857030/diff/2001/include/v8.h#newcode1330
include/v8.h:1330: * resource immediately.  This is because V8 is currently
unable to handle
On 2012/08/21 12:21:34, Erik Corry wrote:
> is currently unable -> is unable

Done.

http://codereview.chromium.org/10857030/diff/2001/src/heap-inl.h
File src/heap-inl.h (right):

http://codereview.chromium.org/10857030/diff/2001/src/heap-inl.h#newcode91
src/heap-inl.h:91: // Assert that the strict ascii hint is correct.
On 2012/08/21 12:21:34, Erik Corry wrote:
> ascii -> ASCII
> here and below

Done.

http://codereview.chromium.org/10857030/diff/2001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/10857030/diff/2001/src/objects.h#newcode7097
src/objects.h:7097: STRICT_ASCII = 1,
On 2012/08/21 12:21:34, Erik Corry wrote:
> Again, the STRICT_ seems wrong.

Done.

http://codereview.chromium.org/10857030/diff/2001/src/v8-counters.h
File src/v8-counters.h (right):

http://codereview.chromium.org/10857030/diff/2001/src/v8-counters.h#newcode256
src/v8-counters.h:256: SC(string_length_ascii, V8.StringLengthAScii)            
          \
On 2012/08/21 12:21:34, Erik Corry wrote:
> AScii is a strange way to capitalize.  I would accept Ascii or ASCII.

This was a typo.  Done.

Powered by Google App Engine
This is Rietveld 408576698