[inspector] Split off interface-types.h

[inspector] Split off interface-types.h

This CL adds a new header src/debug/interface-types.h, moves the
definition of Location from the debug-interface.h to this new header,
and adds a new definition for the WasmDisassembly types.
This allows to use the types in other implementation files or headers
without having to include the entire debug-interface.h, reducing build
dependencies and compile time (especially for incremental builds).

The WasmDisassembly type replaces the old
std::pair<std::string, std::vector<std::tuple<...>>>, which was a bit
hard to unravel.

R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org

Committed: https://crrev.com/f5fb2da64cfd7adff84e6ebbfbaac24e3c47cdba
Cr-Commit-Position: refs/heads/master@{#41488}

[Clemens Hammacher, 2016-11-28 12:48:59]

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

[commit-bot: I haz the power, 2016-11-28 12:49:10]

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, 2016-11-28 13:16:52]

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

[commit-bot: I haz the power, 2016-11-28 13:16:54]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2016-11-28 14:05:01]

Description was changed from

==========
[wasm] Introduce type for disassembly result

The old return type (pair<string, vector<tuple<...>>>) was a bit hard
to unravel. By introducing a new struct in the DebugInterface, and
assigning a name to the tuple type by a "using" declaration, the
readability is improved significantly.
==========

to

==========
[wasm] Introduce type for disassembly result

The old return type (pair<string, vector<tuple<...>>>) was a bit hard
to unravel. By introducing a new struct in the DebugInterface, and
assigning a name to the tuple type by a "using" declaration, the
readability is improved significantly.

R=yangguo@chromium.org, titzer@chromium.org
==========

[Clemens Hammacher, 2016-11-28 14:05:01]

clemensh@chromium.org changed reviewers:
+ yangguo@chromium.org

[Clemens Hammacher, 2016-11-28 14:09:00]

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

[Clemens Hammacher, 2016-11-28 14:09:01]

Yang, is this an acceptable solution to improve readability? It requires us to
include the debug-interface.h in wasm code; currently it is only included from
src/inspector and src/debug.
We can also go a step further and introduce a new header which is included by
both debug-interface.h and the wasm implementations. This way, we could
completely get rid of the std::tuple. Would you prefer that? Where should the
header file go then? Something like src/inspector/wasm-inspect.h?

[kozy, 2016-11-29 23:52:44]

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

[kozy, 2016-11-29 23:52:45]

https://codereview.chromium.org/2529383002/diff/1/src/debug/debug-interface.h
File src/debug/debug-interface.h (right):

https://codereview.chromium.org/2529383002/diff/1/src/debug/debug-interface.h...
src/debug/debug-interface.h:212: using offset_table_entry = std::tuple<uint32_t,
int, int>;
Could we replace this tuple with struct as well?

[Clemens Hammacher, 2016-11-30 16:51:27]

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

[commit-bot: I haz the power, 2016-11-30 16:51:38]

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, 2016-11-30 16:54:28]

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

[commit-bot: I haz the power, 2016-11-30 16:54:29]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/17095)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/17149)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/13453)

[Clemens Hammacher, 2016-11-30 16:55:28]

Description was changed from

==========
[wasm] Introduce type for disassembly result

The old return type (pair<string, vector<tuple<...>>>) was a bit hard
to unravel. By introducing a new struct in the DebugInterface, and
assigning a name to the tuple type by a "using" declaration, the
readability is improved significantly.

R=yangguo@chromium.org, titzer@chromium.org
==========

to

==========
[inspector] Split off interface-types.h

This CL adds a new header src/debug/interface-types.h, moves the
definition of Location from the debug-interface.h to this new header,
and adds a new definition for the WasmDisassembly types.
This allows to use the types in other implementation files or headers
without having to include the entire debug-interface.h.

The WasmDisassembly type replaces the old
std::pair<std::string, std::vector<std::tuple<...>>>, which was a bit
hard to unravel.

R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org
==========

[Clemens Hammacher, 2016-11-30 16:56:44]

Description was changed from

==========
[inspector] Split off interface-types.h

This CL adds a new header src/debug/interface-types.h, moves the
definition of Location from the debug-interface.h to this new header,
and adds a new definition for the WasmDisassembly types.
This allows to use the types in other implementation files or headers
without having to include the entire debug-interface.h.

The WasmDisassembly type replaces the old
std::pair<std::string, std::vector<std::tuple<...>>>, which was a bit
hard to unravel.

R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org
==========

to

==========
[inspector] Split off interface-types.h

This CL adds a new header src/debug/interface-types.h, moves the
definition of Location from the debug-interface.h to this new header,
and adds a new definition for the WasmDisassembly types.
This allows to use the types in other implementation files or headers
without having to include the entire debug-interface.h.

The WasmDisassembly type replaces the old
std::pair<std::string, std::vector<std::tuple<...>>>, which was a bit
hard to unravel. In the process of changing the
wasm::DisassembleFunction function to use the new type, I also made it
a method of the WasmCompiledModule class, where it belongs.

R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org
==========

[Clemens Hammacher, 2016-11-30 16:57:28]

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

[commit-bot: I haz the power, 2016-11-30 16:57:39]

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, 2016-11-30 17:02:26]

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

[commit-bot: I haz the power, 2016-11-30 17:02:27]

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

[Clemens Hammacher, 2016-11-30 17:04:01]

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

[commit-bot: I haz the power, 2016-11-30 17:04:04]

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

[Clemens Hammacher, 2016-11-30 17:06:20]

On 2016/11/29 at 23:52:45, kozyatinskiy wrote:
>
https://codereview.chromium.org/2529383002/diff/1/src/debug/debug-interface.h...
> src/debug/debug-interface.h:212: using offset_table_entry =
std::tuple<uint32_t, int, int>;
> Could we replace this tuple with struct as well?

In order to achieve this, I now created a new header file
src/debug/interface-types.h, where we can define such types.
This avoids having to include the debug-interface.h header everywhere.
I am now defining a new struct for the offset table entry, and the
WasmDisassembly struct there, and use it directly where the data is created.

I changed the description of this CL accordingly.
PTAL.

[commit-bot: I haz the power, 2016-11-30 17:08:11]

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

[commit-bot: I haz the power, 2016-11-30 17:08:12]

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

[titzer, 2016-11-30 17:09:05]

On 2016/11/30 17:06:20, Clemens Hammacher wrote:
> On 2016/11/29 at 23:52:45, kozyatinskiy wrote:
> >
>
https://codereview.chromium.org/2529383002/diff/1/src/debug/debug-interface.h...
> > src/debug/debug-interface.h:212: using offset_table_entry =
> std::tuple<uint32_t, int, int>;
> > Could we replace this tuple with struct as well?
> 
> In order to achieve this, I now created a new header file
> src/debug/interface-types.h, where we can define such types.
> This avoids having to include the debug-interface.h header everywhere.
> I am now defining a new struct for the offset table entry, and the
> WasmDisassembly struct there, and use it directly where the data is created.
> 
> I changed the description of this CL accordingly.
> PTAL.

I like this approach. WASM stuff lgtm

[Clemens Hammacher, 2016-11-30 17:09:33]

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

[commit-bot: I haz the power, 2016-11-30 17:09:41]

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

[kozy, 2016-11-30 17:28:15]

On 2016/11/30 17:06:20, Clemens Hammacher wrote:
> On 2016/11/29 at 23:52:45, kozyatinskiy wrote:
> >
>
https://codereview.chromium.org/2529383002/diff/1/src/debug/debug-interface.h...
> > src/debug/debug-interface.h:212: using offset_table_entry =
> std::tuple<uint32_t, int, int>;
> > Could we replace this tuple with struct as well?
> 
> In order to achieve this, I now created a new header file
> src/debug/interface-types.h, where we can define such types.
> This avoids having to include the debug-interface.h header everywhere.
> I am now defining a new struct for the offset table entry, and the
> WasmDisassembly struct there, and use it directly where the data is created.
> 
> I changed the description of this CL accordingly.
> PTAL.

I'm not sure that one more header file is needed. We use debug-interface.h as
interface between V8 and inspector so we can put any types that is used on both
sides in this file. Probably we can rename debug-interface in something more
general.
Deferring to Yang.

rs inspector lgtm.

[commit-bot: I haz the power, 2016-11-30 17:36:27]

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

[commit-bot: I haz the power, 2016-11-30 17:36:28]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2016-11-30 17:46:05]

On 2016/11/30 at 17:28:15, kozyatinskiy wrote:
> On 2016/11/30 17:06:20, Clemens Hammacher wrote:
> > On 2016/11/29 at 23:52:45, kozyatinskiy wrote:
> > >
> >
https://codereview.chromium.org/2529383002/diff/1/src/debug/debug-interface.h...
> > > src/debug/debug-interface.h:212: using offset_table_entry =
> > std::tuple<uint32_t, int, int>;
> > > Could we replace this tuple with struct as well?
> > 
> > In order to achieve this, I now created a new header file
> > src/debug/interface-types.h, where we can define such types.
> > This avoids having to include the debug-interface.h header everywhere.
> > I am now defining a new struct for the offset table entry, and the
> > WasmDisassembly struct there, and use it directly where the data is created.
> > 
> > I changed the description of this CL accordingly.
> > PTAL.
> 
> I'm not sure that one more header file is needed. We use debug-interface.h as
interface between V8 and inspector so we can put any types that is used on both
sides in this file. Probably we can rename debug-interface in something more
general.
> Deferring to Yang.
> 
> rs inspector lgtm.

There is another problem with the types defined inside of the DebugInterface
class: You can not forward-declare them. Solving this does not require a
separate header though.
The main idea of the separate header is to reduce build dependencies and compile
time.

[titzer, 2016-11-30 18:02:37]

On 2016/11/30 17:46:05, Clemens Hammacher wrote:
> On 2016/11/30 at 17:28:15, kozyatinskiy wrote:
> > On 2016/11/30 17:06:20, Clemens Hammacher wrote:
> > > On 2016/11/29 at 23:52:45, kozyatinskiy wrote:
> > > >
> > >
>
https://codereview.chromium.org/2529383002/diff/1/src/debug/debug-interface.h...
> > > > src/debug/debug-interface.h:212: using offset_table_entry =
> > > std::tuple<uint32_t, int, int>;
> > > > Could we replace this tuple with struct as well?
> > > 
> > > In order to achieve this, I now created a new header file
> > > src/debug/interface-types.h, where we can define such types.
> > > This avoids having to include the debug-interface.h header everywhere.
> > > I am now defining a new struct for the offset table entry, and the
> > > WasmDisassembly struct there, and use it directly where the data is
created.
> > > 
> > > I changed the description of this CL accordingly.
> > > PTAL.
> > 
> > I'm not sure that one more header file is needed. We use debug-interface.h
as
> interface between V8 and inspector so we can put any types that is used on
both
> sides in this file. Probably we can rename debug-interface in something more
> general.
> > Deferring to Yang.
> > 
> > rs inspector lgtm.
> 
> There is another problem with the types defined inside of the DebugInterface
> class: You can not forward-declare them. Solving this does not require a
> separate header though.
> The main idea of the separate header is to reduce build dependencies and
compile
> time.

Yes. I like the separate header for exactly these reasons.

[kozy, 2016-11-30 18:02:53]

On 2016/11/30 17:46:05, Clemens Hammacher wrote:
> There is another problem with the types defined inside of the DebugInterface
> class: You can not forward-declare them. Solving this does not require a
> separate header though.

Let's replace class DebugInterface with namespace DebugInterface.

> The main idea of the separate header is to reduce build dependencies and
compile
> time.

I see, what is real difference in compile time before and after split into two
headers? I think we can split it later if it would be real performance issue.

[Clemens Hammacher, 2016-12-01 13:29:45]

Description was changed from

==========
[inspector] Split off interface-types.h

This CL adds a new header src/debug/interface-types.h, moves the
definition of Location from the debug-interface.h to this new header,
and adds a new definition for the WasmDisassembly types.
This allows to use the types in other implementation files or headers
without having to include the entire debug-interface.h.

The WasmDisassembly type replaces the old
std::pair<std::string, std::vector<std::tuple<...>>>, which was a bit
hard to unravel. In the process of changing the
wasm::DisassembleFunction function to use the new type, I also made it
a method of the WasmCompiledModule class, where it belongs.

R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org
==========

to

==========
[inspector] Split off interface-types.h

This CL adds a new header src/debug/interface-types.h, moves the
definition of Location from the debug-interface.h to this new header,
and adds a new definition for the WasmDisassembly types.
This allows to use the types in other implementation files or headers
without having to include the entire debug-interface.h, reducing build
dependencies and compile time (especially for incremental builds).

The WasmDisassembly type replaces the old
std::pair<std::string, std::vector<std::tuple<...>>>, which was a bit
hard to unravel.

R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org
==========

[Clemens Hammacher, 2016-12-01 13:31:17]

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

[commit-bot: I haz the power, 2016-12-01 13:31:24]

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, 2016-12-01 14:01:39]

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

[commit-bot: I haz the power, 2016-12-01 14:01:39]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-12-02 13:47:40]

On 2016/11/30 18:02:53, kozyatinskiy wrote:
> On 2016/11/30 17:46:05, Clemens Hammacher wrote:
> > There is another problem with the types defined inside of the DebugInterface
> > class: You can not forward-declare them. Solving this does not require a
> > separate header though.
> 
> Let's replace class DebugInterface with namespace DebugInterface.
> 
> > The main idea of the separate header is to reduce build dependencies and
> compile
> > time.
> 
> I see, what is real difference in compile time before and after split into two
> headers? I think we can split it later if it would be real performance issue.

It is admittedly small, but since it's already done (and I think it's cleaner),
maybe we should just go ahead and land this one.

[Clemens Hammacher, 2016-12-02 14:01:28]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2016-12-02 14:01:29]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org,
kozyatinskiy@chromium.org
Link to the patchset: https://codereview.chromium.org/2529383002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2016-12-02 14:01: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, 2016-12-02 14:05:03]

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

[commit-bot: I haz the power, 2016-12-02 14:05: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/29866)

[Yang, 2016-12-05 12:44:49]

LGTM with comment

https://codereview.chromium.org/2529383002/diff/100001/src/debug/interface-ty...
File src/debug/interface-types.h (right):

https://codereview.chromium.org/2529383002/diff/100001/src/debug/interface-ty...
src/debug/interface-types.h:13: namespace debug {
Having Location in the namespace v8::debug, and DebugInterface in the namespace
v8 is somewhat weird.

Can we have a follow-up CL that moves everything in DebugInterface into
v8::debug as well?

[Clemens Hammacher, 2016-12-05 12:49:37]

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

[commit-bot: I haz the power, 2016-12-05 12:49:47]

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, 2016-12-05 13:17:21]

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

[commit-bot: I haz the power, 2016-12-05 13:17:22]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2016-12-05 13:30:04]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2016-12-05 13:30:05]

https://codereview.chromium.org/2529383002/diff/100001/src/debug/interface-ty...
File src/debug/interface-types.h (right):

https://codereview.chromium.org/2529383002/diff/100001/src/debug/interface-ty...
src/debug/interface-types.h:13: namespace debug {
On 2016/12/05 at 12:44:49, Yang wrote:
> Having Location in the namespace v8::debug, and DebugInterface in the
namespace v8 is somewhat weird.
> 
> Can we have a follow-up CL that moves everything in DebugInterface into
v8::debug as well?

It's here: http://crrev.com/2549133002
I don't particularly like it, since you now often have to write
debug::DebugInterface or even v8::debug::DebugInterface.

Thanks for supporting an incremental process here!

[commit-bot: I haz the power, 2016-12-05 13:30:11]

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, 2016-12-05 13:32:06]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480944604147010,
"parent_rev": "fc14a87a8b2b02d77ae994130b423b7056072216", "commit_rev":
"cdc9f0782769288310bb071c58688a1c61cee5fc"}

[commit-bot: I haz the power, 2016-12-05 13:32:12]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-05 13:32:30]

Description was changed from

==========
[inspector] Split off interface-types.h

This CL adds a new header src/debug/interface-types.h, moves the
definition of Location from the debug-interface.h to this new header,
and adds a new definition for the WasmDisassembly types.
This allows to use the types in other implementation files or headers
without having to include the entire debug-interface.h, reducing build
dependencies and compile time (especially for incremental builds).

The WasmDisassembly type replaces the old
std::pair<std::string, std::vector<std::tuple<...>>>, which was a bit
hard to unravel.

R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org
==========

to

==========
[inspector] Split off interface-types.h

This CL adds a new header src/debug/interface-types.h, moves the
definition of Location from the debug-interface.h to this new header,
and adds a new definition for the WasmDisassembly types.
This allows to use the types in other implementation files or headers
without having to include the entire debug-interface.h, reducing build
dependencies and compile time (especially for incremental builds).

The WasmDisassembly type replaces the old
std::pair<std::string, std::vector<std::tuple<...>>>, which was a bit
hard to unravel.

R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org

Committed: https://crrev.com/f5fb2da64cfd7adff84e6ebbfbaac24e3c47cdba
Cr-Commit-Position: refs/heads/master@{#41488}
==========

[commit-bot: I haz the power, 2016-12-05 13:32:32]

Patchset 6 (id:??) landed as
https://crrev.com/f5fb2da64cfd7adff84e6ebbfbaac24e3c47cdba
Cr-Commit-Position: refs/heads/master@{#41488}