[rename] Rename internal field to embedder field.

[rename] Rename internal field to embedder field.

This CL renames all occurrences of "internal field" to "embedder field"
to prevent confusion. As it turns out, these fields are not internal to
V8, but are actually embedder provided fields that should not be mucked
with by the internal implementation of V8.

Note that WASM does use these fields, and it should not.

BUG=v8:6058
Review-Url: https://codereview.chromium.org/2741683004
Cr-Commit-Position: refs/heads/master@{#43900}
Committed: https://chromium.googlesource.com/v8/v8/+/72e539360ed644c905b0a67b02b3d63b18a282a7

[titzer, 2017-03-10 12:27:26]

The CQ bit was checked by titzer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 12:27:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 12:30:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 12:30:46]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/23875)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/23965)

[titzer, 2017-03-10 13:14:45]

The CQ bit was checked by titzer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 13:14:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 13:21:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 13:21:22]

Dry run: Try jobs failed on following builders:
  v8_linux_chromium_gn_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)

[titzer, 2017-03-10 13:28:50]

The CQ bit was checked by titzer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 13:29:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 13:36:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 13:36:26]

Dry run: Try jobs failed on following builders:
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)

[titzer, 2017-03-13 09:04:37]

titzer@chromium.org changed reviewers:
+ jochen@chromium.org, mlippautz@chromium.org, vogelheim@chromium.org,
yangguo@chromium.org

[titzer, 2017-03-13 09:04:38]

PTAL

[Yang, 2017-03-13 09:15:08]

On 2017/03/13 09:04:38, titzer wrote:
> PTAL

src/snapshot lgtm.

[Yang, 2017-03-13 09:15:26]

On 2017/03/13 09:15:08, Yang wrote:
> On 2017/03/13 09:04:38, titzer wrote:
> > PTAL
> 
> src/snapshot lgtm.

src/debug lgtm too.

[jochen (gone - plz use gerrit), 2017-03-13 09:24:35]

lgtm

[Michael Lippautz, 2017-03-13 09:26:30]

On 2017/03/13 09:24:35, jochen wrote:
> lgtm

lgtm (mostly rubber)

[vogelheim, 2017-03-13 09:33:01]

lgtm

Nice! Two things, though:

1, Some of the bots have compiler failures, so I guess there's a thing missing,
somewhere.

2, Not sure if the CL description is elaborate enough for the 'V8 API Changes'
doc. (
https://docs.google.com/document/d/1g8JFi8T_oAE_7uAri7Njtig7fKaPDfotU6huOa1al...
)

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

https://codereview.chromium.org/2741683004/diff/40001/include/v8-experimental...
include/v8-experimental.h:33: // TODO(v8): deprecate this in favor of
LoadEmbedderField
Maybe; TODO(vogelheim)

(I need to either clean-up and land this work, or remove it entirely. Should
anything in here should be my TODO, probably.)

[titzer, 2017-03-13 12:45:02]

The CQ bit was checked by titzer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 12:45:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 12:54:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 12:54:24]

Dry run: Try jobs failed on following builders:
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[titzer, 2017-03-13 13:04:08]

The CQ bit was checked by titzer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 13:04:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 13:09:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 13:09:08]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)

[titzer, 2017-03-13 13:21:31]

The CQ bit was checked by titzer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 13:21:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 13:30:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 13:30:47]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[adamk, 2017-03-13 22:01:09]

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

[adamk, 2017-03-13 22:01:10]

I can understand confusion within V8, but it's not clear to me why this
confusion extends to the API. Are you sure this much API churn is worthwhile for
a renaming? As someone who used to use the API before I knew much about V8 (from
WebKit/Blink bindings), I can say that I never found this use of "internal
field" confusing in the API.

[titzer, 2017-03-14 08:52:27]

On 2017/03/13 22:01:10, adamk wrote:
> I can understand confusion within V8, but it's not clear to me why this
> confusion extends to the API. Are you sure this much API churn is worthwhile
for
> a renaming? As someone who used to use the API before I knew much about V8
(from
> WebKit/Blink bindings), I can say that I never found this use of "internal
> field" confusing in the API.

I worried about that too, as that churn has a cost (and I'm already paying
trying to figure out what the linker errors are, ha!).
I considered only renaming them internally, but that would end up with two
different names for the same thing, which would probably be even more confusing.
The API churn hopefully is temporary versus the lasting (small) cost of the
confusion.

[jochen (gone - plz use gerrit), 2017-03-14 13:12:42]

actually, I think Adam's point is valid. You want to help V8 developers to avoid
confusion - external embedders shouldn't have to pay for this

[titzer, 2017-03-14 13:29:46]

On 2017/03/14 13:12:42, jochen wrote:
> actually, I think Adam's point is valid. You want to help V8 developers to
avoid
> confusion - external embedders shouldn't have to pay for this

Would you prefer that I rename them internally and leave the API unchanged? I
can do that as well.

[jochen (gone - plz use gerrit), 2017-03-14 13:31:56]

yeah, that sounds like the right compromise

[ulan, 2017-03-14 13:35:43]

On 2017/03/14 13:12:42, jochen wrote:
> actually, I think Adam's point is valid. You want to help V8 developers to
avoid
> confusion - external embedders shouldn't have to pay for this
+1, for the embedders both names are OK so they will see this change as
unnecessary churn, not getting any value from it.

[titzer, 2017-03-14 13:39:37]

On 2017/03/14 13:35:43, ulan wrote:
> On 2017/03/14 13:12:42, jochen wrote:
> > actually, I think Adam's point is valid. You want to help V8 developers to
> avoid
> > confusion - external embedders shouldn't have to pay for this
> +1, for the embedders both names are OK so they will see this change as
> unnecessary churn, not getting any value from it.

Agreed, I will do that, as it is actually easier anyway.

[titzer, 2017-03-14 15:47:05]

The CQ bit was checked by titzer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 15:47:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 16:12:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 16:12:22]

Dry run: This issue passed the CQ dry run.

[titzer, 2017-03-14 17:17:43]

On 2017/03/14 13:39:37, titzer wrote:
> On 2017/03/14 13:35:43, ulan wrote:
> > On 2017/03/14 13:12:42, jochen wrote:
> > > actually, I think Adam's point is valid. You want to help V8 developers to
> > avoid
> > > confusion - external embedders shouldn't have to pay for this
> > +1, for the embedders both names are OK so they will see this change as
> > unnecessary churn, not getting any value from it.
> 
> Agreed, I will do that, as it is actually easier anyway.

PTAL, I've undone all API changes other than an addition here or there to make
the internal/embedder naming issues easier.

[vogelheim, 2017-03-15 09:43:51]

lgtm - Looks even better now! :)

https://codereview.chromium.org/2741683004/diff/120001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2741683004/diff/120001/src/objects.h#newcode1...
src/objects.h:10441: // [object]: the [[IteratedObject]] inobject property.
Thanks for the comment fix! I hadn't noticed that in the earlier round. :)

[jochen (gone - plz use gerrit), 2017-03-15 15:51:00]

why do you still change include/v8.h?

[titzer, 2017-03-15 15:54:43]

On 2017/03/15 15:51:00, jochen wrote:
> why do you still change include/v8.h?

It defines a couple of enums and other types that are also used internally in
V8.
So to avoid the V8 code using both terms, I added a small number of
s/internal/embedder/ named things in order to make V8 code uniform if possible.

That makes this CL the widest possible proliferation of the name "embedder
field" without adding or deprecating any methods in the API. There's also a
comment or two explaining that we use "embedder field" in the V8 code but
"internal field" in the API.

[adamk, 2017-03-16 16:36:11]

lgtm from my end, thanks

[titzer, 2017-03-16 17:58:24]

The CQ bit was checked by titzer@chromium.org

[titzer, 2017-03-16 17:58:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlippautz@chromium.org, jochen@chromium.org, yangguo@chromium.org,
adamk@chromium.org, vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2741683004/#ps160001
(title: "[rename] Rename internal field to embedder field.")

[commit-bot: I haz the power, 2017-03-16 17:58:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 18:05:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 18:05:01]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/36902)

[titzer, 2017-03-16 19:17:13]

Description was changed from

==========
[rename] Rename internal field to embedder field.

This CL renames all occurrences of "internal field" to "embedder field"
to prevent confusion. As it turns out, these fields are not internal to
V8, but are actually embedder provided fields that should not be mucked
with by the internal implementation of V8.

Note that WASM does use these fields, and it should not.

BUG=v8:6058
==========

to

==========
[rename] Rename internal field to embedder field.

This CL renames all occurrences of "internal field" to "embedder field"
to prevent confusion. As it turns out, these fields are not internal to
V8, but are actually embedder provided fields that should not be mucked
with by the internal implementation of V8.

Note that WASM does use these fields, and it should not.

BUG=v8:6058
==========

[titzer, 2017-03-16 19:17:13]

titzer@chromium.org changed reviewers:
+ verwaest@chromium.org

[titzer, 2017-03-16 19:17:27]

On 2017/03/16 18:05:01, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_presubmit on master.tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/36902)

+verwaest for crankshaft

[Toon Verwaest, 2017-03-17 13:23:01]

lgtm

[titzer, 2017-03-17 13:23:27]

The CQ bit was checked by titzer@chromium.org

[commit-bot: I haz the power, 2017-03-17 13:23:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 13:26:06]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1489757007802030,
"parent_rev": "22372f5aa8af7ddb48c8151272c71e939f9e6e68", "commit_rev":
"72e539360ed644c905b0a67b02b3d63b18a282a7"}

[commit-bot: I haz the power, 2017-03-17 13:26:12]

Description was changed from

==========
[rename] Rename internal field to embedder field.

This CL renames all occurrences of "internal field" to "embedder field"
to prevent confusion. As it turns out, these fields are not internal to
V8, but are actually embedder provided fields that should not be mucked
with by the internal implementation of V8.

Note that WASM does use these fields, and it should not.

BUG=v8:6058
==========

to

==========
[rename] Rename internal field to embedder field.

This CL renames all occurrences of "internal field" to "embedder field"
to prevent confusion. As it turns out, these fields are not internal to
V8, but are actually embedder provided fields that should not be mucked
with by the internal implementation of V8.

Note that WASM does use these fields, and it should not.

BUG=v8:6058

Review-Url: https://codereview.chromium.org/2741683004
Cr-Commit-Position: refs/heads/master@{#43900}
Committed:
https://chromium.googlesource.com/v8/v8/+/72e539360ed644c905b0a67b02b3d63b18a...
==========

[commit-bot: I haz the power, 2017-03-17 13:26:14]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/v8/v8/+/72e539360ed644c905b0a67b02b3d63b18a...