Add "Version" command

Add "Version" command

Committed: http://code.google.com/p/v8/source/detail?r=3108

[Peter Rybin, 2009-10-16 20:21:46]

Hi Soren

I changed our protocol a bit by making "running" property more accurate.
However, there is no mean how client can tell whether "running" is accurate or
not.
Here I'm trying to let V8 describe version of its debugger protocol so that any
client software could check it.
I decided to support the list of "protocol features". I hope this makes it
flexible enough for a future use.

What do you think?
Are there any aspects where I'm going out of V8 style here?

Peter

[Søren Thygesen Gjesse, 2009-10-19 12:11:05]

LGTM with comments below addressed.

http://codereview.chromium.org/274080/diff/1011/8
File include/v8-debug.h (right):

http://codereview.chromium.org/274080/diff/1011/8#newcode255
Line 255: #define DEBUGGER_PROTOCOL_VERSION "1"
This #define should be in the debug.h file and not in the public header. If we
want to expose this through the API there should be a function for retrieving
it.

http://codereview.chromium.org/274080/diff/1011/10
File src/debug-delay.js (right):

http://codereview.chromium.org/274080/diff/1011/10#newcode29
Line 29: const kDebuggerProtocolVersion = 1;
You could add a function to runtome.h/runtime.cc to get this to avoid the
duplicate constant.

http://codereview.chromium.org/274080/diff/1011/10#newcode1925
Line 1925: debugProtocolVersion: kDebuggerProtocolVersion,
Please include the V8 version here as well.

http://codereview.chromium.org/274080/diff/1011/10#newcode1925
Line 1925: debugProtocolVersion: kDebuggerProtocolVersion,
How about debugProtocolVersion -> protocolVersion as debug should be implicit in
this context.

http://codereview.chromium.org/274080/diff/1011/10#newcode1927
Line 1927: 'accurate_running' // running field in response is accurate
I think that specifying this as a "feature" is not that appropriate as it being
not accurate is probably a bug. Instead we should add the list of supported
commands e.g.

  supportedCommands: [ 'continue', 'backtrace', ... ]

and dropping protocol features for now. If the suspend command is supported the
running flag is accurate. As the protocol is dynamic having supported commands
will be valuable.

[Peter Rybin, 2009-10-20 21:26:51]

Hi Soren

Now I dropped protocol version and feature list and added V8Version.

So now this command only reports version of V8 VM.

Actually DebuggerAgent already reports V8 version at shandshake moment, but
DevTools do not; but I would prefer a standard way of getting version regardless
of transport layer.

Peter

http://codereview.chromium.org/274080/diff/1011/8
File include/v8-debug.h (right):

http://codereview.chromium.org/274080/diff/1011/8#newcode255
Line 255: #define DEBUGGER_PROTOCOL_VERSION "1"
On 2009/10/19 12:11:05, Søren Gjesse wrote:
> This #define should be in the debug.h file and not in the public header. If we
> want to expose this through the API there should be a function for retrieving
> it.
> 

I'm rolling this back for now.

http://codereview.chromium.org/274080/diff/1011/10
File src/debug-delay.js (right):

http://codereview.chromium.org/274080/diff/1011/10#newcode29
Line 29: const kDebuggerProtocolVersion = 1;
On 2009/10/19 12:11:05, Søren Gjesse wrote:
> You could add a function to runtome.h/runtime.cc to get this to avoid the
> duplicate constant.

I'm dropping protocol for now, added GetV8Version as you suggested.

http://codereview.chromium.org/274080/diff/1011/10#newcode1925
Line 1925: debugProtocolVersion: kDebuggerProtocolVersion,
On 2009/10/19 12:11:05, Søren Gjesse wrote:
> Please include the V8 version here as well.

Done.

http://codereview.chromium.org/274080/diff/1011/10#newcode1925
Line 1925: debugProtocolVersion: kDebuggerProtocolVersion,
On 2009/10/19 12:11:05, Søren Gjesse wrote:
> How about debugProtocolVersion -> protocolVersion as debug should be implicit
in
> this context.

I dropped protocol version for now.

http://codereview.chromium.org/274080/diff/1011/10#newcode1927
Line 1927: 'accurate_running' // running field in response is accurate
On 2009/10/19 12:11:05, Søren Gjesse wrote:
> I think that specifying this as a "feature" is not that appropriate as it
being
> not accurate is probably a bug. Instead we should add the list of supported
> commands e.g.
> 
>   supportedCommands: [ 'continue', 'backtrace', ... ]
> 
> and dropping protocol features for now. If the suspend command is supported
the
> running flag is accurate. As the protocol is dynamic having supported commands
> will be valuable.

Dropped

[Søren Thygesen Gjesse, 2009-10-21 07:38:50]

LGTM when comments have been addressed.

http://codereview.chromium.org/274080/diff/5014/1017
File src/runtime.cc (right):

http://codereview.chromium.org/274080/diff/5014/1017#newcode7664
Line 7664: // code offset.
This comment does not seem right.

http://codereview.chromium.org/274080/diff/5014/1017#newcode7669
Line 7669: return *(Factory::LookupAsciiSymbol(v8::V8::GetVersion()));
No need to make the version a symbol. Just use NewStringFromAscii. Actually you
can drop the HandleScope and just use Heap::AllocateStringFromAscii. Please add

  NoHandleAllocation ha;

to the beginning of the function as well.

[Peter Rybin, 2009-10-21 16:58:52]

Hi Soren

Thank you for your review. I also added a test before commit.

Peter

http://codereview.chromium.org/274080/diff/5014/1017
File src/runtime.cc (right):

http://codereview.chromium.org/274080/diff/5014/1017#newcode7664
Line 7664: // code offset.
On 2009/10/21 07:38:50, Søren Gjesse wrote:
> This comment does not seem right.

Stupid me.

Done

http://codereview.chromium.org/274080/diff/5014/1017#newcode7669
Line 7669: return *(Factory::LookupAsciiSymbol(v8::V8::GetVersion()));
On 2009/10/21 07:38:50, Søren Gjesse wrote:
> No need to make the version a symbol. Just use NewStringFromAscii. Actually
you
> can drop the HandleScope and just use Heap::AllocateStringFromAscii. Please
add
> 
>   NoHandleAllocation ha;
> 
> to the beginning of the function as well.

Done.