Add script streaming API

Add script streaming API.

Blink will use this API to stream script data into V8 as the scripts
load. During loading, V8 can already parse the scripts. They will be then
compiled and executed when the loading is complete.

BUG=
R=jochen@chromium.org, rossberg@chromium.org, svenpanne@chromium.org, yangguo@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=23865

[jochen (gone - plz use gerrit), 2014-08-13 14:24:46]

https://codereview.chromium.org/366153002/diff/600001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/366153002/diff/600001/include/v8.h#newcode968
include/v8.h:968: virtual unsigned GetSomeData(const char** src, unsigned
position) = 0;
GetMoreData?

it's a bit odd to pass TWO_BYTE data around as char* btw

what's the meaning of position?

why is v8 creating the background thread, and not the embedder? maybe
StartStreamingScript should return some object that has a method
"StartOnThread()". the embedder then creates a thread and calls that method on
that thread.

https://codereview.chromium.org/366153002/diff/600001/include/v8.h#newcode1137
include/v8.h:1137: V8_INLINE StreamedSource(ExternalSourceStream*
source_stream);
explicit

https://codereview.chromium.org/366153002/diff/600001/include/v8.h#newcode1141
include/v8.h:1141: // caller. The CachedData object is alive as long as the
Source object is
you mean StreamedSource object?

[marja, 2014-08-14 11:29:22]

Thanks for comments (mostly done expect one counter-argument).

https://codereview.chromium.org/366153002/diff/600001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/366153002/diff/600001/include/v8.h#newcode968
include/v8.h:968: virtual unsigned GetSomeData(const char** src, unsigned
position) = 0;
On 2014/08/13 14:24:45, jochen (slow - soon OOO) wrote:
> GetMoreData?
> 
> it's a bit odd to pass TWO_BYTE data around as char* btw
> 
> what's the meaning of position?
> 
> why is v8 creating the background thread, and not the embedder? maybe
> StartStreamingScript should return some object that has a method
> "StartOnThread()". the embedder then creates a thread and calls that method on
> that thread.

1) This API (the name GetSomeData and passing the position) was adopted from
SharedBuffer. But GetMoreData without position works too (the embedder can keep
track of where we are), so changed this to GetMoreData without position.

2) It's not odd to pass raw data as char*. Note that V8 is responsible of
decoding it, and at this point it's just a byte stream. Other places  pass raw
byte data (which is possibly UTF-16) as char* too, for example the threaded HTML
parser: BackgroundHTMLParser::appendRawBytesFromParserThread

3) Moved the threading to Blink. This now returns a ScriptStreamingTask which
has a method Run(). (StartOnThread() would be implying that it just launches
something and returns, whereas Run() can really block and wait for data.)

https://codereview.chromium.org/366153002/diff/600001/include/v8.h#newcode1137
include/v8.h:1137: V8_INLINE StreamedSource(ExternalSourceStream*
source_stream);
On 2014/08/13 14:24:45, jochen (slow - soon OOO) wrote:
> explicit

Done.

https://codereview.chromium.org/366153002/diff/600001/include/v8.h#newcode1141
include/v8.h:1141: // caller. The CachedData object is alive as long as the
Source object is
On 2014/08/13 14:24:45, jochen (slow - soon OOO) wrote:
> you mean StreamedSource object?

Done.

[marja, 2014-08-29 13:10:51]

marja@chromium.org changed reviewers:
+ svenpanne@chromium.org

[Sven Panne, 2014-09-01 09:07:46]

https://codereview.chromium.org/366153002/diff/900001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/366153002/diff/900001/include/v8.h#newcode1111
include/v8.h:1111: virtual unsigned GetMoreData(const char** src) = 0;
I think size_t is more appropriate than unsigned.

https://codereview.chromium.org/366153002/diff/900001/include/v8.h#newcode1118
include/v8.h:1118: void SetEncoding(Encoding e) { encoding = e; }
What's the rationale behind this two-stage initialization of the encoding?
Naively, I would assume a single constructor with the encoding and "encoding"
being const. If there is a good reason for the current design, it should be
explained in a comment.

https://codereview.chromium.org/366153002/diff/900001/include/v8.h#newcode1134
include/v8.h:1134: ~StreamedSource();
If we don't allow subclassing, we should mark this class as final. No clue
whether we want subclassing, though... :-)

https://codereview.chromium.org/366153002/diff/900001/include/v8.h#newcode1208
include/v8.h:1208: static ScriptStreamingTask* StartStreamingScript(
It might be nice to move this function and the one below to Isolate, but this is
not crucial. Nevertheless, "static" functions are a code smell most of the time.

https://codereview.chromium.org/366153002/diff/900001/src/background-parsing-...
File src/background-parsing-task.h (right):

https://codereview.chromium.org/366153002/diff/900001/src/background-parsing-...
src/background-parsing-task.h:23: isolate_(isolate) {}
Why do we remember the Isolate? Just passing the hash seed would result in less
coupling.

https://codereview.chromium.org/366153002/diff/900001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/366153002/diff/900001/src/compiler.cc#newcode862
src/compiler.cc:862: // produced
Funny spacing.

https://codereview.chromium.org/366153002/diff/900001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/366153002/diff/900001/test/cctest/test-api.cc...
test/cctest/test-api.cc:23016: unsigned len = strlen(chunks_[index_]);
size_t

https://codereview.chromium.org/366153002/diff/900001/test/cctest/test-api.cc...
test/cctest/test-api.cc:23017: // snprintf will NULL-terminate, so reserve space
for the NULL too. However,
Nit: It zero-terminates, not NULL-terminates. :-) And even that is not
guaranteed. And some VisualStudio versions don't have it.

https://codereview.chromium.org/366153002/diff/900001/test/cctest/test-api.cc...
test/cctest/test-api.cc:23021: snprintf(copy, len + 1, "%s", chunks_[index_]);
Wouldn't memcpy simplify things here and below?

[marja, 2014-09-01 11:58:17]

Thanks for having a look!

I gather that this looks generally reasonable (the only non-trivially fixable
comment was about SetEncoding and I have changed the API), so next I'll 1)
develop tests and 2) fix the UTF-8 handling 3) land refactorings separately so
this gets smaller.

No action needed from anybody else at this point; I'll ping this CL again when
I'm done.

https://codereview.chromium.org/366153002/diff/900001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/366153002/diff/900001/include/v8.h#newcode1111
include/v8.h:1111: virtual unsigned GetMoreData(const char** src) = 0;
On 2014/09/01 09:07:46, Sven Panne wrote:
> I think size_t is more appropriate than unsigned.

Done.

https://codereview.chromium.org/366153002/diff/900001/include/v8.h#newcode1118
include/v8.h:1118: void SetEncoding(Encoding e) { encoding = e; }
On 2014/09/01 09:07:46, Sven Panne wrote:
> What's the rationale behind this two-stage initialization of the encoding?
> Naively, I would assume a single constructor with the encoding and "encoding"
> being const. If there is a good reason for the current design, it should be
> explained in a comment.

The original purpose was to allow the embedder pass the encoding when the first
block of data has arrived (i.e., not require it until it's absolutely needed).
However, the current Blink side does *not* do encoding detection from data, so I
simplified this: encoding is now passed as ctor param.

If we really want to do encoding detection on the Blink side, Blink can either
delay constructing ExternalSourceStream, or this can change.

https://codereview.chromium.org/366153002/diff/900001/include/v8.h#newcode1134
include/v8.h:1134: ~StreamedSource();
On 2014/09/01 09:07:46, Sven Panne wrote:
> If we don't allow subclassing, we should mark this class as final. No clue
> whether we want subclassing, though... :-)

Added V8_FINAL (subclassing this won't make sense; it has no virtual funcs).

Btw, none of the other classes in the v8.h have V8_FINAL defined. (But the macro
is accessible, since this includes v8stdint.h which includes v8config.h.)

https://codereview.chromium.org/366153002/diff/900001/include/v8.h#newcode1208
include/v8.h:1208: static ScriptStreamingTask* StartStreamingScript(
On 2014/09/01 09:07:46, Sven Panne wrote:
> It might be nice to move this function and the one below to Isolate, but this
is
> not crucial. Nevertheless, "static" functions are a code smell most of the
time.

The other compile functions are below, as static functions of ScriptCompiler, so
I think these make sense here, given the current state of the rest of the API.

I'm not convinced that putting everything to Isolate smells less. At least now
the functions are grouped nicely (functions related to compiling scripts are in
ScriptCompiler).

Ofc if the Compile funcs were to move away from ScriptCompiler, these funcs
would move with them at that point.

https://codereview.chromium.org/366153002/diff/900001/src/background-parsing-...
File src/background-parsing-task.h (right):

https://codereview.chromium.org/366153002/diff/900001/src/background-parsing-...
src/background-parsing-task.h:23: isolate_(isolate) {}
On 2014/09/01 09:07:46, Sven Panne wrote:
> Why do we remember the Isolate? Just passing the hash seed would result in
less
> coupling.

Done.

https://codereview.chromium.org/366153002/diff/900001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/366153002/diff/900001/src/compiler.cc#newcode862
src/compiler.cc:862: // produced
On 2014/09/01 09:07:46, Sven Panne wrote:
> Funny spacing.

Done.

https://codereview.chromium.org/366153002/diff/900001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/366153002/diff/900001/test/cctest/test-api.cc...
test/cctest/test-api.cc:23016: unsigned len = strlen(chunks_[index_]);
On 2014/09/01 09:07:46, Sven Panne wrote:
> size_t

Done.

https://codereview.chromium.org/366153002/diff/900001/test/cctest/test-api.cc...
test/cctest/test-api.cc:23017: // snprintf will NULL-terminate, so reserve space
for the NULL too. However,
On 2014/09/01 09:07:46, Sven Panne wrote:
> Nit: It zero-terminates, not NULL-terminates. :-) And even that is not
> guaranteed. And some VisualStudio versions don't have it.

Done.

https://codereview.chromium.org/366153002/diff/900001/test/cctest/test-api.cc...
test/cctest/test-api.cc:23021: snprintf(copy, len + 1, "%s", chunks_[index_]);
On 2014/09/01 09:07:46, Sven Panne wrote:
> Wouldn't memcpy simplify things here and below?

Yes, but presubmit whines about it :(

[jochen (gone - plz use gerrit), 2014-09-01 13:16:32]

https://codereview.chromium.org/366153002/diff/900001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/366153002/diff/900001/test/cctest/test-api.cc...
test/cctest/test-api.cc:23021: snprintf(copy, len + 1, "%s", chunks_[index_]);
On 2014/09/01 11:58:17, marja wrote:
> On 2014/09/01 09:07:46, Sven Panne wrote:
> > Wouldn't memcpy simplify things here and below?
> 
> Yes, but presubmit whines about it :(

why not strncpy?

[marja, 2014-09-01 13:23:25]

https://codereview.chromium.org/366153002/diff/900001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/366153002/diff/900001/test/cctest/test-api.cc...
test/cctest/test-api.cc:23021: snprintf(copy, len + 1, "%s", chunks_[index_]);
Argh, this discussion was confused.

So I originally had *strcpy* (not memcpy), but the whine was:

/media/ssdvol/v8-bleeding2/test/cctest/test-api.cc:23021:  Almost always,
snprintf is better than strcpy  [runtime/printf] [4]

(Curiously, it didn't suggest strncpy, so I thought snprintf is the way.)

However, memcpy is the right function here, since I don't want to mess with
0-termination.

[marja, 2014-09-02 12:43:36]

https://codereview.chromium.org/366153002/diff/900001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/366153002/diff/900001/include/v8.h#newcode1134
include/v8.h:1134: ~StreamedSource();
Meanwhile, things seem to progress in bleeding edge. There's no V8_FINAL any
more; now V8 is using FINAL like chromium, and FINAL is not defined by any of
the include files of include/v8.h.

So I'm removing V8_FINAL; no other class in the API defined it anyway, so would
be a bit obscure to add it to this class only...

[marja, 2014-09-04 13:45:00]

svenpanne, jochen, ptal.

Okay, now this is ready for review for realz. There are even tests.

Please review as follows:
svenpanne: v8.h
jochen: everything


Re: previous discussion, I changed const char* to const uint8_t* in the API,
since that seems to be the data type for "bunch of raw bytes" on the V8 side
(Blink uses const char* but this seems to be the V8 way - see for example
CachedData).

[marja, 2014-09-04 14:03:15]

On 2014/09/04 13:45:00, marja wrote:
> svenpanne, jochen, ptal.
> 
> Okay, now this is ready for review for realz. There are even tests.
> 
> Please review as follows:
> svenpanne: v8.h
> jochen: everything
> 
> 
> Re: previous discussion, I changed const char* to const uint8_t* in the API,
> since that seems to be the data type for "bunch of raw bytes" on the V8 side
> (Blink uses const char* but this seems to be the V8 way - see for example
> CachedData).

(And changing the subject of the e-mail thread now too just for general
feeling-better-about-this-CL purposes.)

[Sven Panne, 2014-09-05 08:07:43]

API LGTM

[jochen (gone - plz use gerrit), 2014-09-08 11:27:07]

is there a way for the embedder to cancel parsing?

https://codereview.chromium.org/366153002/diff/1120001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/366153002/diff/1120001/include/v8.h#newcode1102
include/v8.h:1102: ExternalSourceStream(Encoding encoding) : encoding(encoding)
{}
explicit

https://codereview.chromium.org/366153002/diff/1120001/include/v8.h#newcode1116
include/v8.h:1116: virtual size_t GetMoreData(const uint8_t** src) = 0;
that's kinda odd, a pure virtual interface shouldn't have a ctor

https://codereview.chromium.org/366153002/diff/1120001/include/v8.h#newcode1120
include/v8.h:1120: const Encoding encoding;
or friends, or data members

it seems like all this class does is provide a single callback, no?

https://codereview.chromium.org/366153002/diff/1120001/include/v8.h#newcode1141
include/v8.h:1141: friend class internal::BackgroundParsingTask;
can we do without friends here?

https://codereview.chromium.org/366153002/diff/1120001/include/v8.h#newcode1147
include/v8.h:1147: ExternalSourceStream* source_stream;
members should end in _

https://codereview.chromium.org/366153002/diff/1120001/src/background-parsing...
File src/background-parsing-task.h (right):

https://codereview.chromium.org/366153002/diff/1120001/src/background-parsing...
src/background-parsing-task.h:21: class StreamingData {
if it's just a container, this should be a struct

https://codereview.chromium.org/366153002/diff/1120001/src/background-parsing...
src/background-parsing-task.h:23: StreamingData(i::CompilationInfo* i, uint32_t
h, UnicodeCache* u, bool a)
info, hash_seed, etc..

https://codereview.chromium.org/366153002/diff/1120001/src/background-parsing...
src/background-parsing-task.h:30: UnicodeCache* unicode_cache;  // Owned.
how can this be owned, if the data is copied around?

[marja, 2014-09-08 16:14:44]

marja@chromium.org changed reviewers:
+ yangguo@chromium.org

[marja, 2014-09-08 16:14:47]

jochen: thanks for having a look!

yangguo: jochen said you'd be a good person for reviewing the UTF-8 handling
part, so, could you review scanner-character-streams.* ?

https://codereview.chromium.org/366153002/diff/1120001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/366153002/diff/1120001/include/v8.h#newcode1102
include/v8.h:1102: ExternalSourceStream(Encoding encoding) : encoding(encoding)
{}
On 2014/09/08 11:27:06, jochen wrote:
> explicit

(This was removed)

https://codereview.chromium.org/366153002/diff/1120001/include/v8.h#newcode1116
include/v8.h:1116: virtual size_t GetMoreData(const uint8_t** src) = 0;
On 2014/09/08 11:27:06, jochen wrote:
> that's kinda odd, a pure virtual interface shouldn't have a ctor

Done.

https://codereview.chromium.org/366153002/diff/1120001/include/v8.h#newcode1120
include/v8.h:1120: const Encoding encoding;
On 2014/09/08 11:27:06, jochen wrote:
> or friends, or data members
> 
> it seems like all this class does is provide a single callback, no?

Done.

https://codereview.chromium.org/366153002/diff/1120001/include/v8.h#newcode1141
include/v8.h:1141: friend class internal::BackgroundParsingTask;
On 2014/09/08 11:27:06, jochen wrote:
> can we do without friends here?

Added a private implementation and an accessor to it, so the friends are gone.

https://codereview.chromium.org/366153002/diff/1120001/include/v8.h#newcode1147
include/v8.h:1147: ExternalSourceStream* source_stream;
On 2014/09/08 11:27:06, jochen wrote:
> members should end in _

These are now in an internal struct, so not ending in _.

https://codereview.chromium.org/366153002/diff/1120001/src/background-parsing...
File src/background-parsing-task.h (right):

https://codereview.chromium.org/366153002/diff/1120001/src/background-parsing...
src/background-parsing-task.h:21: class StreamingData {
On 2014/09/08 11:27:06, jochen wrote:
> if it's just a container, this should be a struct

Done.

https://codereview.chromium.org/366153002/diff/1120001/src/background-parsing...
src/background-parsing-task.h:23: StreamingData(i::CompilationInfo* i, uint32_t
h, UnicodeCache* u, bool a)
On 2014/09/08 11:27:06, jochen wrote:
> info, hash_seed, etc..

This code was deleted.

https://codereview.chromium.org/366153002/diff/1120001/src/background-parsing...
src/background-parsing-task.h:30: UnicodeCache* unicode_cache;  // Owned.
On 2014/09/08 11:27:06, jochen wrote:
> how can this be owned, if the data is copied around?

Prevented copying (which was non-prevented by omission).

[marja, 2014-09-08 16:15:47]

On 2014/09/08 11:27:07, jochen wrote:
> is there a way for the embedder to cancel parsing?

Yes; I clarified this in the comments:

* If the embedder wants to cancel the streaming, they should make the next
* GetMoreData call return 0. V8 will interpret it as end of data (and most
* probably, parsing will fail). The streaming task will return as soon as
* V8 has parsed the data it received so far.

[marja, 2014-09-08 16:34:08]

> yangguo: jochen said you'd be a good person for reviewing the UTF-8 handling
> part, so, could you review scanner-character-streams.* ?

I also meant to mention that there are tests in test-api.cc which test these
parts.

[jochen (gone - plz use gerrit), 2014-09-09 08:06:21]

lgtm

https://codereview.chromium.org/366153002/diff/1160001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/366153002/diff/1160001/include/v8.h#newcode1096
include/v8.h:1096: class V8_EXPORT ExternalSourceStream {
nit. no need to V8 EXPORT - everything here is inline

https://codereview.chromium.org/366153002/diff/1160001/src/background-parsing...
File src/background-parsing-task.cc (right):

https://codereview.chromium.org/366153002/diff/1160001/src/background-parsing...
src/background-parsing-task.cc:12: static const int
kBackgroundParserThreadStackSize = 64 * KB;
should this be FLAG_stack_size

https://codereview.chromium.org/366153002/diff/1160001/src/background-parsing...
src/background-parsing-task.cc:74: }
nit // namespace internal

also, empty line above this one

https://codereview.chromium.org/366153002/diff/1160001/src/background-parsing...
src/background-parsing-task.cc:75: }  // namespace v8::internal
// namespace v8

https://codereview.chromium.org/366153002/diff/1160001/src/background-parsing...
File src/background-parsing-task.h (right):

https://codereview.chromium.org/366153002/diff/1160001/src/background-parsing...
src/background-parsing-task.h:43: CompilationInfo* info;
nit. you could use SmartPointer<CompilationInfo> so you don't need to delete all
owned fields in the dtor (you won't need one at all)

[marja, 2014-09-09 11:46:30]

Thanks for review!

https://codereview.chromium.org/366153002/diff/1160001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/366153002/diff/1160001/include/v8.h#newcode1096
include/v8.h:1096: class V8_EXPORT ExternalSourceStream {
On 2014/09/09 08:06:20, jochen wrote:
> nit. no need to V8 EXPORT - everything here is inline

Done.

https://codereview.chromium.org/366153002/diff/1160001/src/background-parsing...
File src/background-parsing-task.cc (right):

https://codereview.chromium.org/366153002/diff/1160001/src/background-parsing...
src/background-parsing-task.cc:12: static const int
kBackgroundParserThreadStackSize = 64 * KB;
On 2014/09/09 08:06:20, jochen wrote:
> should this be FLAG_stack_size

Done.

https://codereview.chromium.org/366153002/diff/1160001/src/background-parsing...
src/background-parsing-task.cc:74: }
On 2014/09/09 08:06:20, jochen wrote:
> nit // namespace internal
> 
> also, empty line above this one

git cl format removes the empty line if I put one there.

https://codereview.chromium.org/366153002/diff/1160001/src/background-parsing...
src/background-parsing-task.cc:74: }
On 2014/09/09 08:06:20, jochen wrote:
> nit // namespace internal
> 
> also, empty line above this one

Counter nit: this seems pretty customary:

$ git grep "// namespace v8::internal" | wc -l
691
$ git grep "// namespace internal" | wc -l
96

So not changing this; also, there's no risk of misunderstanding it.

https://codereview.chromium.org/366153002/diff/1160001/src/background-parsing...
File src/background-parsing-task.h (right):

https://codereview.chromium.org/366153002/diff/1160001/src/background-parsing...
src/background-parsing-task.h:43: CompilationInfo* info;
On 2014/09/09 08:06:20, jochen wrote:
> nit. you could use SmartPointer<CompilationInfo> so you don't need to delete
all
> owned fields in the dtor (you won't need one at all)

Done.

[Yang, 2014-09-09 14:42:20]

On 2014/09/09 11:46:30, marja wrote:
> Thanks for review!
> 
> https://codereview.chromium.org/366153002/diff/1160001/include/v8.h
> File include/v8.h (right):
> 
>
https://codereview.chromium.org/366153002/diff/1160001/include/v8.h#newcode1096
> include/v8.h:1096: class V8_EXPORT ExternalSourceStream {
> On 2014/09/09 08:06:20, jochen wrote:
> > nit. no need to V8 EXPORT - everything here is inline
> 
> Done.
> 
>
https://codereview.chromium.org/366153002/diff/1160001/src/background-parsing...
> File src/background-parsing-task.cc (right):
> 
>
https://codereview.chromium.org/366153002/diff/1160001/src/background-parsing...
> src/background-parsing-task.cc:12: static const int
> kBackgroundParserThreadStackSize = 64 * KB;
> On 2014/09/09 08:06:20, jochen wrote:
> > should this be FLAG_stack_size
> 
> Done.
> 
>
https://codereview.chromium.org/366153002/diff/1160001/src/background-parsing...
> src/background-parsing-task.cc:74: }
> On 2014/09/09 08:06:20, jochen wrote:
> > nit // namespace internal
> > 
> > also, empty line above this one
> 
> git cl format removes the empty line if I put one there.
> 
>
https://codereview.chromium.org/366153002/diff/1160001/src/background-parsing...
> src/background-parsing-task.cc:74: }
> On 2014/09/09 08:06:20, jochen wrote:
> > nit // namespace internal
> > 
> > also, empty line above this one
> 
> Counter nit: this seems pretty customary:
> 
> $ git grep "// namespace v8::internal" | wc -l
> 691
> $ git grep "// namespace internal" | wc -l
> 96
> 
> So not changing this; also, there's no risk of misunderstanding it.
> 
>
https://codereview.chromium.org/366153002/diff/1160001/src/background-parsing...
> File src/background-parsing-task.h (right):
> 
>
https://codereview.chromium.org/366153002/diff/1160001/src/background-parsing...
> src/background-parsing-task.h:43: CompilationInfo* info;
> On 2014/09/09 08:06:20, jochen wrote:
> > nit. you could use SmartPointer<CompilationInfo> so you don't need to delete
> all
> > owned fields in the dtor (you won't need one at all)
> 
> Done.

lgtm.

[marja, 2014-09-09 14:53:54]

rossberg, fyi, I'm adding the streaming parsing API here, in case you want to
have a look. (If you don't have time to have a look before I land this, comments
are still welcome and I can do follow-up changes. The streaming code path is
pretty isolated.)

[rossberg, 2014-09-10 13:39:26]

rossberg@chromium.org changed reviewers:
+ rossberg@chromium.org

[rossberg, 2014-09-10 13:39:28]

From a superficial look, LGTM. Just one comment

https://codereview.chromium.org/366153002/diff/1180001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/366153002/diff/1180001/include/v8.h#newcode1129
include/v8.h:1129: enum Encoding { ONE_BYTE, TWO_BYTE, UTF8 };
Not specific to this CL, but I wonder if we shouldn't eventually change these
names to ASCII and UCS2, for more clarity...

https://codereview.chromium.org/366153002/diff/1180001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/366153002/diff/1180001/src/parser.h#newcode672
src/parser.h:672: Scope** ad_hoc_eval_scope);
Why is this called ad_hoc_eval_scope everywhere, and not just eval_scope?

[Yang, 2014-09-10 13:48:58]

https://codereview.chromium.org/366153002/diff/1180001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/366153002/diff/1180001/include/v8.h#newcode1129
include/v8.h:1129: enum Encoding { ONE_BYTE, TWO_BYTE, UTF8 };
On 2014/09/10 13:39:28, rossberg wrote:
> Not specific to this CL, but I wonder if we shouldn't eventually change these
> names to ASCII and UCS2, for more clarity...

Rather Latin1 instead of ASCII.

[marja, 2014-09-10 13:53:09]

(thanks for having a look; now only commenting on the encoding names, will fix
the other naming comment later)

https://codereview.chromium.org/366153002/diff/1180001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/366153002/diff/1180001/include/v8.h#newcode1129
include/v8.h:1129: enum Encoding { ONE_BYTE, TWO_BYTE, UTF8 };
On 2014/09/10 13:48:58, Yang wrote:
> On 2014/09/10 13:39:28, rossberg wrote:
> > Not specific to this CL, but I wonder if we shouldn't eventually change
these
> > names to ASCII and UCS2, for more clarity...
> 
> Rather Latin1 instead of ASCII.

Yep, not ASCII, since it actually is Latin1, not ascii. However, the V8 term for
it seems to be "one byte" - it's used everywhere.

(ASCII was leftover from the days where it was actually ASCII - yangguo@ is just
fixing it right now.)

Why "UCS2" though? Isn't UTF-16 a much more well known term? Do you mean it's
actually not UTF-16?

[rossberg, 2014-09-10 14:19:27]

On 2014/09/10 13:53:09, marja wrote:
> On 2014/09/10 13:48:58, Yang wrote:
> > On 2014/09/10 13:39:28, rossberg wrote:
> > > Not specific to this CL, but I wonder if we shouldn't eventually change
> these
> > > names to ASCII and UCS2, for more clarity...
> > 
> > Rather Latin1 instead of ASCII.
> 
> Yep, not ASCII, since it actually is Latin1, not ascii. However, the V8 term
for
> it seems to be "one byte" - it's used everywhere.

Yes, makes sense.

> Why "UCS2" though? Isn't UTF-16 a much more well known term? Do you mean it's
> actually not UTF-16?

It's not the same thing -- which is exactly why I think we should be more
explicit. I don't actually understand which way we currently interpret
"TWO_BYTE" input. (JavaScript used to assume UCS-2, but with ES6 that's getting
kind of reinterpreted as UTF-16, at least in some places.)

[marja, 2014-09-10 14:22:12]

https://codereview.chromium.org/366153002/diff/1180001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/366153002/diff/1180001/src/parser.h#newcode672
src/parser.h:672: Scope** ad_hoc_eval_scope);
On 2014/09/10 13:39:28, rossberg wrote:
> Why is this called ad_hoc_eval_scope everywhere, and not just eval_scope?

Renamed to eval_scope.

It was called ad hoc because of this comment:

          // TODO(mstarzinger): Global strict eval calls, need their own scope
          // as specified in ES5 10.4.2(3). The correct fix would be to always
          // add this scope in DoParseProgram(), but that requires adaptations
          // all over the code base, so we go with a quick-fix for now.
          // In the same manner, we have to patch the parsing mode.

[marja, 2014-09-11 11:06:48]

Committed patchset #62 (id:1260001) manually as 23865 (presubmit successful).

[marja, 2014-09-11 12:02:49]

On 2014/09/11 11:06:48, marja wrote:
> Committed patchset #62 (id:1260001) manually as 23865 (presubmit successful).

FYI, I had to revert this; the special characters were too much for the Windows
compiler.

A reincarnated version is here: https://codereview.chromium.org/566553002/