Introduce extras export object.

Introduce extras export object.

BUG=

Committed: https://crrev.com/9a1490ad6ffb7100c4b35581b2787ac1c08695d8
Cr-Commit-Position: refs/heads/master@{#28499}

[Yang, 2015-05-18 14:01:08]

yangguo@chromium.org changed reviewers:
+ domenic@chromium.org, jochen@chromium.org

[Yang, 2015-05-18 14:01:09]

This replaces https://codereview.chromium.org/1128113006/.

[domenic, 2015-05-18 16:43:11]

https://codereview.chromium.org/1140333003/diff/40001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1140333003/diff/40001/include/v8.h#newcode6379
include/v8.h:6379: MaybeLocal<Object> GetExtrasExportsObject();
FYI I wasn't sure whether to use MaybeLocal or Local. MaybeLocal is new and cool
but other methods of this class all return Locals.

https://codereview.chromium.org/1140333003/diff/40001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1140333003/diff/40001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1930: JSObject::NormalizeProperties(builtin_exports,
CLEAR_INOBJECT_PROPERTIES, 2,
copypasta: builtin_exports should be extras_exports

[domenic, 2015-05-18 17:41:39]

https://codereview.chromium.org/1140333003/diff/40001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1140333003/diff/40001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1472: return CompileNative(isolate, name, source_code,
arraysize(args), args);
I think removing the access to the buildin_exports_object prevents extras code
from using InternalPackedArray.

[Yang, 2015-05-18 18:30:56]

Will upload a new patch set tomorrow morning.

https://codereview.chromium.org/1140333003/diff/40001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1140333003/diff/40001/include/v8.h#newcode6379
include/v8.h:6379: MaybeLocal<Object> GetExtrasExportsObject();
On 2015/05/18 16:43:11, domenic wrote:
> FYI I wasn't sure whether to use MaybeLocal or Local. MaybeLocal is new and
cool
> but other methods of this class all return Locals.

You are right. At first I had a version where we could have undefined as extras
object, if there are no extra scripts, in which case we would return an empty
handle. Then I changed my mind, but forgot this part. I will change this and
update the comment.

https://codereview.chromium.org/1140333003/diff/40001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1140333003/diff/40001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1472: return CompileNative(isolate, name, source_code,
arraysize(args), args);
On 2015/05/18 17:41:39, domenic wrote:
> I think removing the access to the buildin_exports_object prevents extras code
> from using InternalPackedArray.

This is correct and intentional. I want to have 
- a "shared" container for all normal natives that are included in the snapshot.
After running the internal scripts, this container will be thrown away.
- a "builtin exports" container to export and keep stuff required for
experimental natives, since those are not part of the snapshot, and need to stay
alive after creating the snapshot.
- a "extras exports" container for sharing things between C++ and JS for extra
scripts
- an "api exports" container to export things from normal natives to extra
natives, for example InternalPackedArrays.

The latter is not implemented yet, and I'm not entirely sure whether extra
exports and api exports should be the same object. It would be simpler if they
are, but the api exports can die after the extra scripts have been executed,
where as the extra exports need to stay alive for its purpose.

https://codereview.chromium.org/1140333003/diff/40001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1930: JSObject::NormalizeProperties(builtin_exports,
CLEAR_INOBJECT_PROPERTIES, 2,
On 2015/05/18 16:43:11, domenic wrote:
> copypasta: builtin_exports should be extras_exports

Yup. Thanks for catching this. Will fix.

[domenic, 2015-05-18 18:33:45]

On 2015/05/18 at 18:30:56, yangguo wrote:

> The latter is not implemented yet, and I'm not entirely sure whether extra
exports and api exports should be the same object. It would be simpler if they
are, but the api exports can die after the extra scripts have been executed,
where as the extra exports need to stay alive for its purpose.

Hmm OK. This would be a blocker for streams, so sooner is better.

[Yang, 2015-05-19 06:15:13]

On 2015/05/18 18:33:45, domenic wrote:
> On 2015/05/18 at 18:30:56, yangguo wrote:
> 
> > The latter is not implemented yet, and I'm not entirely sure whether extra
> exports and api exports should be the same object. It would be simpler if they
> are, but the api exports can die after the extra scripts have been executed,
> where as the extra exports need to stay alive for its purpose.
> 
> Hmm OK. This would be a blocker for streams, so sooner is better.

I uploaded a new patch set. I'll introduce the api exports object in a follow-up
CL.

[domenic, 2015-05-19 15:38:10]

lgtm

LGTM

https://codereview.chromium.org/1140333003/diff/60001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1140333003/diff/60001/include/v8.h#newcode6377
include/v8.h:6377: * If there are no extra native scripts, return an emtpy
handle.
Is this line still accurate?

[jochen (gone - plz use gerrit), 2015-05-19 15:49:39]

lgtm

[Yang, 2015-05-20 07:00:22]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2015-05-20 07:00:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
domenic@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1140333003/#ps80001
(title: "fixed comments")

[commit-bot: I haz the power, 2015-05-20 07:00:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1140333003/80001

[commit-bot: I haz the power, 2015-05-20 07:38:27]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-20 07:38:40]

Patchset 5 (id:??) landed as
https://crrev.com/9a1490ad6ffb7100c4b35581b2787ac1c08695d8
Cr-Commit-Position: refs/heads/master@{#28499}