LGTM with a few nits and one real comment.

Thanks!


-- Vitaly


http://codereview.chromium.org/8536042/diff/10001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

http://codereview.chromium.org/8536042/diff/10001/src/bootstrapper.cc#newcode214
src/bootstrapper.cc:214: enum ExtensionTraversalState {
nit: Insert a blank line.

http://codereview.chromium.org/8536042/diff/10001/src/bootstrapper.cc#newcode226
src/bootstrapper.cc:226: };
nit: DISALLOW_COPY_AND_ASSIGN.

http://codereview.chromium.org/8536042/diff/10001/src/bootstrapper.cc#newcode1955
src/bootstrapper.cc:1955: return v8::internal::ComputeIntegerHash(
Oh, actually there's already ComputePointerHash in utils.h :)

http://codereview.chromium.org/8536042/diff/10001/src/bootstrapper.cc#newcode1969
src/bootstrapper.cc:1969: : map_(MatchRegisteredExtensions,
ExtensionStatesAllocator(), 8)
... and does the default allocator (if you omit args 2 and 3 here) work?

http://codereview.chromium.org/8536042/diff/10001/src/bootstrapper.cc#newcode1983
src/bootstrapper.cc:1983: Genesis::ExtensionTraversalState state) {
nit: Formatting.

http://codereview.chromium.org/8536042/diff/10001/src/bootstrapper.cc#newcode2041
src/bootstrapper.cc:2041: bool
Genesis::InstallExtension(v8::RegisteredExtension* current,
ExtensionStates* extension_states) {
nit: Formatting.

http://codereview.chromium.org/8536042/diff/10001/src/isolate.h
File src/isolate.h (right):

http://codereview.chromium.org/8536042/diff/10001/src/isolate.h#newcode898
src/isolate.h:898: void extension_installed() {
Something UpperCase here, e.g. NotifyExtensionInstalled().

http://codereview.chromium.org/8536042/diff/10001/src/isolate.h#newcode1165
src/isolate.h:1165: bool has_installed_extensions_;
Is this initialized somewhere?

http://codereview.chromium.org/8536042/

--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev

Reply via email to