Add an optional source length field to the Extension constructor.

Add an optional source length field to the Extension constructor.

The length field allows the caller to pass in the address of a non-null-
terminated string. This removes the implicit requirement that the
Extension's source be null-terminated, and makes it possible for callers
to send in addresses of embedded (and possibly not null-terminated)
resource strings, thereby saving a certain amount of resident memory per
extension.

TEST=added two unit tests to test-api.cc
BUG=95147
Committed: http://code.google.com/p/v8/source/detail?r=9365

[miket_OOO, 2011-09-14 20:04:54]

Kindly review. Thank you.

[Aaron Boodman, 2011-09-14 20:33:18]

Cool, I hadn't thought about having the ESR be a member of v8::Extension that
way.

http://codereview.chromium.org/7889046/diff/1/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/7889046/diff/1/include/v8.h#newcode1171
include/v8.h:1171: V8EXPORT const
ExternalAsciiStringResource*GetExternalAsciiStringResource()
Nit: Put the space back after the *.

http://codereview.chromium.org/7889046/diff/1/include/v8.h#newcode2456
include/v8.h:2456: ExternalAsciiStringResourceImpl(const char *data, size_t
length)
In chrome, the * goes next to the type.

http://codereview.chromium.org/7889046/diff/1/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/7889046/diff/1/src/api.cc#newcode507
src/api.cc:507: source_length_(source_length >= 0 ?
Nested ternary operators are hard to read and this is a bit long for an
initialization list. how about just doing the strlen() bit in the constructor
body.

[Aaron Boodman, 2011-09-14 20:39:32]

Oh, and I am still kinda worried that v8 is going to try and delete the ESR
somewhere. Are you sure its destructor doesn't get called (except when the
v8::Extension is deleted?)?

[miket_OOO, 2011-09-14 20:47:04]

On 2011/09/14 20:39:32, Aaron Boodman wrote:
> Oh, and I am still kinda worried that v8 is going to try and delete the ESR
> somewhere. Are you sure its destructor doesn't get called (except when the
> v8::Extension is deleted?)?

No, I am not too confident about that. While I'm waiting for Vyacheslav's
comments, I'll see whether I can get a better answer.

[miket_OOO, 2011-09-14 20:47:16]

http://codereview.chromium.org/7889046/diff/1/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/7889046/diff/1/include/v8.h#newcode1171
include/v8.h:1171: V8EXPORT const
ExternalAsciiStringResource*GetExternalAsciiStringResource()
On 2011/09/14 20:33:18, Aaron Boodman wrote:
> Nit: Put the space back after the *.

Whoops! Thanks.

http://codereview.chromium.org/7889046/diff/1/include/v8.h#newcode2456
include/v8.h:2456: ExternalAsciiStringResourceImpl(const char *data, size_t
length)
On 2011/09/14 20:33:18, Aaron Boodman wrote:
> In chrome, the * goes next to the type.

I'm trying to untrain myself. Fixed; thanks.

http://codereview.chromium.org/7889046/diff/1/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/7889046/diff/1/src/api.cc#newcode507
src/api.cc:507: source_length_(source_length >= 0 ?
On 2011/09/14 20:33:18, Aaron Boodman wrote:
> Nested ternary operators are hard to read and this is a bit long for an
> initialization list. how about just doing the strlen() bit in the constructor
> body.

You're right; it works great as-is but it's fragile enough to break if you look
at it funny. I'll fix it, and in the process return the source_length/source
arguments to a more natural order in the declaration.

[miket_OOO, 2011-09-14 23:00:58]

> > Nested ternary operators are hard to read and this is a bit long for an
> > initialization list. how about just doing the strlen() bit in the
constructor
> > body.
> 
> You're right; it works great as-is but it's fragile enough [snip]

I remembered why this is the way it is. The ExternalStringResourceBase class has
made the assignment operator private, which means that I must initialize source_
in the initialization list. I'm open to other readability improvements, but
unless you know a trick in C++ that I don't (entirely possible), I think we're
stuck as long as assignment is disabled.

[Vyacheslav Egorov (Chromium), 2011-09-15 10:24:47]

redirecting to Florian

[fschneider, 2011-09-19 07:58:57]

http://codereview.chromium.org/7889046/diff/1/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/7889046/diff/1/include/v8.h#newcode2455
include/v8.h:2455: ExternalAsciiStringResourceImpl() : data_(0), length_(0) {};
No ; at the end?

http://codereview.chromium.org/7889046/diff/1/include/v8.h#newcode2457
include/v8.h:2457: : data_(data), length_(length) {};
; at the end should not be necessary

http://codereview.chromium.org/7889046/diff/1/include/v8.h#newcode2460
include/v8.h:2460: private:
Newline before private:

http://codereview.chromium.org/7889046/diff/1/include/v8.h#newcode2476
include/v8.h:2476: int source_length = -1);
I wish there was a way to reduce the number of optional paramters in a row. E.g.
does the source parameter have to be optional?

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

http://codereview.chromium.org/7889046/diff/1/test/cctest/test-api.cc#newcode...
test/cctest/test-api.cc:4298: for (int source_len =
kEmbeddedExtensionSourceValidLen;
Why does this loop go from kEmbeddedExtensionSourceValidLen to
kEmbeddedExtensionSourceValidLen? Shouldn't it start from 0?

[miket_OOO, 2011-09-19 17:39:20]

Made suggested changes. Re-ran unit tests. Please re-review.

http://codereview.chromium.org/7889046/diff/1/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/7889046/diff/1/include/v8.h#newcode2455
include/v8.h:2455: ExternalAsciiStringResourceImpl() : data_(0), length_(0) {};
On 2011/09/19 07:58:57, fschneider wrote:
> No ; at the end?

Fixed (et seq.). Thanks. Still a bit rusty on C++ :(.

http://codereview.chromium.org/7889046/diff/1/include/v8.h#newcode2460
include/v8.h:2460: private:
On 2011/09/19 07:58:57, fschneider wrote:
> Newline before private:

Fixed.

http://codereview.chromium.org/7889046/diff/1/include/v8.h#newcode2476
include/v8.h:2476: int source_length = -1);
On 2011/09/19 07:58:57, fschneider wrote:
> I wish there was a way to reduce the number of optional paramters in a row.
E.g.
> does the source parameter have to be optional?

Agreed. I'd appreciate your opinion on altering vs. extending the existing v8
API. I am taking the conservative route.

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

http://codereview.chromium.org/7889046/diff/1/test/cctest/test-api.cc#newcode...
test/cctest/test-api.cc:4298: for (int source_len =
kEmbeddedExtensionSourceValidLen;
On 2011/09/19 07:58:57, fschneider wrote:
> Why does this loop go from kEmbeddedExtensionSourceValidLen to
> kEmbeddedExtensionSourceValidLen? Shouldn't it start from 0?

Thank you for catching this. I have changed the range to [-1, +1] (relative to
the constant). I was doing some debugging and neglected to return to the
original range before uploading. Now fixed.

[fschneider, 2011-09-20 10:40:56]

LGTM.

[miket_OOO, 2011-09-20 17:19:16]

On 2011/09/20 10:40:56, fschneider wrote:
> LGTM.

Thanks, Florian. Will you please submit? (I'm also pressing the commit button on
the CR page, but I'm not sure whether that does anything in the v8 project.)