Reviewers: jochen, Sven Panne,
Message:
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-task.h
File src/background-parsing-task.h (right):
https://codereview.chromium.org/366153002/diff/900001/src/background-parsing-task.h#newcode23
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#newcode23016
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#newcode23017
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#newcode23021
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 :(
Description:
Add script streaming API - DRAFT - NOT FOR COMMITTING.
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=
Please review this at https://codereview.chromium.org/366153002/
SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge
Affected files (+679, -90 lines):
M BUILD.gn
M include/v8.h
M src/api.cc
A src/background-parsing-task.h
A src/background-parsing-task.cc
M src/compiler.h
M src/compiler.cc
M src/counters.h
M src/parser.h
M src/parser.cc
M src/preparser.h
M src/scanner-character-streams.h
M src/scanner-character-streams.cc
M test/cctest/test-api.cc
M test/cctest/test-parsing.cc
M tools/gyp/v8.gyp
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
