Hide WasmModule.origin field behind readable accessors.

Hide WasmModule.origin field behind readable accessors.

Besides adding accessors get_origin() and set_origin(), it creates easier test
accessors is_wasm() and is_asm_js().

This allows the possibility of caching boolean flags for is_wasm() and
is_asm_js() without having to change any code except for the files containing
the class definition for WasmModule.

BUG=v8:6152
R=bbudge@chromium.org,mtrofin@chromium.org

Review-Url: https://codereview.chromium.org/2771803005
Cr-Commit-Position: refs/heads/master@{#44130}
Committed: https://chromium.googlesource.com/v8/v8/+/98ed1f9ca97882c2566646c67cdfabaa71d7ac5c

[Karl, 2017-03-24 21:39:49]

Description was changed from

==========
Hide WasmModule.origin field behind readable accessors.

Besides adding accessors get_origin() and set_origin(), it creates easier test
accessors is_wasm() and is_asm_js().

This allows the possibility of caching boolean flags for is_wasm() and
is_asm_js() without having to change any code except for the files containing
the class definition for WasmModule.

BUG=v8:6152
==========

to

==========
Hide WasmModule.origin field behind readable accessors.

Besides adding accessors get_origin() and set_origin(), it creates easier test
accessors is_wasm() and is_asm_js().

This allows the possibility of caching boolean flags for is_wasm() and
is_asm_js() without having to change any code except for the files containing
the class definition for WasmModule.

BUG= https://bugs.chromium.org/p/v8/issues/detail?id=6152
R=bbudge@chromium.org,mtrofin@chromium.org
==========

[Karl, 2017-03-24 21:41:11]

kschimpf@google.com changed reviewers:
+ bbudge@chromium.org, mtrofin@chromium.org

[Karl, 2017-03-24 21:41:24]

kschimpf@google.com changed reviewers:
+ kschimpf@google.com

[bradnelson, 2017-03-24 21:54:13]

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

[bradnelson, 2017-03-24 21:54:13]

lgtm

[Karl, 2017-03-24 22:01:09]

The CQ bit was checked by kschimpf@google.com

[commit-bot: I haz the power, 2017-03-24 22:01:17]

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-24 22:03:31]

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

[commit-bot: I haz the power, 2017-03-24 22:03:32]

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...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

[Karl, 2017-03-24 22:14:49]

Description was changed from

==========
Hide WasmModule.origin field behind readable accessors.

Besides adding accessors get_origin() and set_origin(), it creates easier test
accessors is_wasm() and is_asm_js().

This allows the possibility of caching boolean flags for is_wasm() and
is_asm_js() without having to change any code except for the files containing
the class definition for WasmModule.

BUG= https://bugs.chromium.org/p/v8/issues/detail?id=6152
R=bbudge@chromium.org,mtrofin@chromium.org
==========

to

==========
Hide WasmModule.origin field behind readable accessors.

Besides adding accessors get_origin() and set_origin(), it creates easier test
accessors is_wasm() and is_asm_js().

This allows the possibility of caching boolean flags for is_wasm() and
is_asm_js() without having to change any code except for the files containing
the class definition for WasmModule.

BUG= v8:6152
R=bbudge@chromium.org,mtrofin@chromium.org
==========

[Karl, 2017-03-24 22:15:25]

The CQ bit was checked by kschimpf@google.com

[Karl, 2017-03-24 22:15:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@chromium.org
Link to the patchset: https://codereview.chromium.org/2771803005/#ps20001
(title: "Fix merge conflict with master.")

[commit-bot: I haz the power, 2017-03-24 22:15:31]

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-24 22:18:03]

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

[commit-bot: I haz the power, 2017-03-24 22:18:04]

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/37644)

[Karl, 2017-03-24 22:23:59]

The CQ bit was checked by kschimpf@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 22:24:01]

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

[Mircea Trofin, 2017-03-24 22:25:15]

https://codereview.chromium.org/2771803005/diff/20001/src/wasm/wasm-module.h
File src/wasm/wasm-module.h (right):

https://codereview.chromium.org/2771803005/diff/20001/src/wasm/wasm-module.h#...
src/wasm/wasm-module.h:192: ModuleOrigin origin = kWasmOrigin;  // origin of the
module
the naming style in V8 for fields would be "origin_". If we rename it, might as
well remove the ambiguity with the browser notion of origin. Perhaps we should
name the field "type_" or "kind_"?

https://codereview.chromium.org/2771803005/diff/20001/src/wasm/wasm-objects.cc
File src/wasm/wasm-objects.cc (right):

https://codereview.chromium.org/2771803005/diff/20001/src/wasm/wasm-objects.c...
src/wasm/wasm-objects.cc:227: if (origin == ModuleOrigin::kWasmOrigin) {
why not use the is_wasm() / is_asm_js() APIs here, too?

[Karl, 2017-03-24 22:29:11]

Need approval from Mircea (for src/compiler directory).

Comments below are things I had to fix when I tried to commit the CL.

https://codereview.chromium.org/2771803005/diff/40001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2771803005/diff/40001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2859: DCHECK_EQ(wasm::kWasmOrigin,
module_->module->get_origin());
Fixed debug check here.

https://codereview.chromium.org/2771803005/diff/40001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:3970: !module_env_->module->is_wasm()));
Fixed merge with master error here.

https://codereview.chromium.org/2771803005/diff/40001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2771803005/diff/40001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1323: module_->is_asm_js() ||
memory_->has_guard_region());
Fixed debug build here.

[Karl, 2017-03-24 22:41:40]

https://codereview.chromium.org/2771803005/diff/20001/src/wasm/wasm-module.h
File src/wasm/wasm-module.h (right):

https://codereview.chromium.org/2771803005/diff/20001/src/wasm/wasm-module.h#...
src/wasm/wasm-module.h:192: ModuleOrigin origin = kWasmOrigin;  // origin of the
module
On 2017/03/24 22:25:14, Mircea Trofin wrote:
> the naming style in V8 for fields would be "origin_". If we rename it, might
as
> well remove the ambiguity with the browser notion of origin. Perhaps we should
> name the field "type_" or "kind_"?

I'm a bit nervous about changing the field name from "origin_" to "type_" or
"kind_", since it is used in so many other classes as "origin_" (or "origin").

However, since the field isn't publicly accessible, it does make sense to rename
it to "origin_".

Changing the name to "origin_".

https://codereview.chromium.org/2771803005/diff/20001/src/wasm/wasm-objects.cc
File src/wasm/wasm-objects.cc (right):

https://codereview.chromium.org/2771803005/diff/20001/src/wasm/wasm-objects.c...
src/wasm/wasm-objects.cc:227: if (origin == ModuleOrigin::kWasmOrigin) {
On 2017/03/24 22:25:15, Mircea Trofin wrote:
> why not use the is_wasm() / is_asm_js() APIs here, too?

It is, look up at line 224.

[Karl, 2017-03-24 22:42:18]

The CQ bit was checked by kschimpf@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 22:42:22]

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

[Mircea Trofin, 2017-03-24 22:43:42]

https://codereview.chromium.org/2771803005/diff/20001/src/wasm/wasm-objects.cc
File src/wasm/wasm-objects.cc (right):

https://codereview.chromium.org/2771803005/diff/20001/src/wasm/wasm-objects.c...
src/wasm/wasm-objects.cc:227: if (origin == ModuleOrigin::kWasmOrigin) {
On 2017/03/24 22:41:40, Karl wrote:
> On 2017/03/24 22:25:15, Mircea Trofin wrote:
> > why not use the is_wasm() / is_asm_js() APIs here, too?
> 
> It is, look up at line 224.

that's get_origin()

[Karl, 2017-03-24 22:51:17]

https://codereview.chromium.org/2771803005/diff/20001/src/wasm/wasm-objects.cc
File src/wasm/wasm-objects.cc (right):

https://codereview.chromium.org/2771803005/diff/20001/src/wasm/wasm-objects.c...
src/wasm/wasm-objects.cc:227: if (origin == ModuleOrigin::kWasmOrigin) {
On 2017/03/24 22:43:42, Mircea Trofin wrote:
> On 2017/03/24 22:41:40, Karl wrote:
> > On 2017/03/24 22:25:15, Mircea Trofin wrote:
> > > why not use the is_wasm() / is_asm_js() APIs here, too?
> > 
> > It is, look up at line 224.
> 
> that's get_origin()

Ok. I see. I didn't read the usage well enough. Changing to use is_wasm() /
is_asm_js() methods.

[Karl, 2017-03-24 22:51:26]

The CQ bit was checked by kschimpf@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 22:51:32]

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

[bbudge, 2017-03-24 23:02:20]

I didn't realize how widespread these comparisons were. Thanks for doing this.
lgtm

[commit-bot: I haz the power, 2017-03-24 23:29:30]

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

[commit-bot: I haz the power, 2017-03-24 23:29:31]

Dry run: This issue passed the CQ dry run.

[Karl, 2017-03-25 01:08:01]

The CQ bit was checked by kschimpf@google.com

[Karl, 2017-03-25 01:08:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@chromium.org
Link to the patchset: https://codereview.chromium.org/2771803005/#ps80001
(title: "Use boolean accessors instead of get_origin.")

[commit-bot: I haz the power, 2017-03-25 01:08:03]

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-25 01:10:52]

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

[commit-bot: I haz the power, 2017-03-25 01:10:54]

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/37656)

[Karl, 2017-03-25 01:13:54]

Still waiting for approval from mtrofin for directory src/compiler

[Mircea Trofin, 2017-03-25 01:52:20]

On 2017/03/25 01:13:54, Karl wrote:
> Still waiting for approval from mtrofin for directory src/compiler

Lgtm

[Mircea Trofin, 2017-03-25 01:52:45]

The CQ bit was checked by mtrofin@chromium.org

[commit-bot: I haz the power, 2017-03-25 01:52:54]

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-25 01:54:10]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490406765767820,
"parent_rev": "a1f2239e0b7daff6bbf728d4f744ef6ee52f5299", "commit_rev":
"98ed1f9ca97882c2566646c67cdfabaa71d7ac5c"}

[commit-bot: I haz the power, 2017-03-25 01:54:16]

Description was changed from

==========
Hide WasmModule.origin field behind readable accessors.

Besides adding accessors get_origin() and set_origin(), it creates easier test
accessors is_wasm() and is_asm_js().

This allows the possibility of caching boolean flags for is_wasm() and
is_asm_js() without having to change any code except for the files containing
the class definition for WasmModule.

BUG= v8:6152
R=bbudge@chromium.org,mtrofin@chromium.org
==========

to

==========
Hide WasmModule.origin field behind readable accessors.

Besides adding accessors get_origin() and set_origin(), it creates easier test
accessors is_wasm() and is_asm_js().

This allows the possibility of caching boolean flags for is_wasm() and
is_asm_js() without having to change any code except for the files containing
the class definition for WasmModule.

BUG= v8:6152
R=bbudge@chromium.org,mtrofin@chromium.org

Review-Url: https://codereview.chromium.org/2771803005
Cr-Commit-Position: refs/heads/master@{#44130}
Committed:
https://chromium.googlesource.com/v8/v8/+/98ed1f9ca97882c2566646c67cdfabaa71d...
==========

[commit-bot: I haz the power, 2017-03-25 01:54:17]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/v8/v8/+/98ed1f9ca97882c2566646c67cdfabaa71d...