Extend DebugAgent API with AsyncDebugRunner

Extend DebugAgent API with AsyncDebugRunner

[Peter Rybin, 2009-12-09 20:16:30]

Hi Soren

I'm preparing sample for standalone V8 application. For this sample I'd like 2
features from Debugger Agent:

- wait until remote has connected
- flush debugger messages callback

I think I implemented the first one and I'm adding API for the second feature.

I remember you asked me to turn my abstract class into a function pointer.
However I'd like to ask you to confirm on this one more time; if we are sure V8
will remain singleton, I'm fine with function; however if things like CL#435003
are probable, class may be a better option.

Could you please take a look?

Peter

[Maxim.Mossienko, 2009-12-09 22:26:33]

As to CL#435003 (several v8 instances in process) there is no much difference to
hold function pointer or instance of abstract class inside V8Context.

On 2009/12/09 20:16:30, Peter Rybin wrote:
> Hi Soren
> 
> I'm preparing sample for standalone V8 application. For this sample I'd like 2
> features from Debugger Agent:
> 
> - wait until remote has connected
> - flush debugger messages callback
> 
> I think I implemented the first one and I'm adding API for the second feature.
> 
> I remember you asked me to turn my abstract class into a function pointer.
> However I'd like to ask you to confirm on this one more time; if we are sure
V8
> will remain singleton, I'm fine with function; however if things like
CL#435003
> are probable, class may be a better option.
> 
> Could you please take a look?
> 
> Peter

[Søren Thygesen Gjesse, 2009-12-10 14:58:09]

Sorry for the delay - please use my @chromium.org address (I should get the
@gmail.com one removed).

I am not sure what happens here when the wait for debugger connection flag is
set. Isn't V8 execution supposed to wait until a debugger comes along and
processes the first debugger message? Should the debug break flag be raised to
ensure that a break will happen as soon as any code is executed?

You will need to provide some test which uses and tests this new flag. There are
some tests covering the DebugAgent, but they probably needs to be extended.

Please don't add the AsyncDebugRunner until it is actually used, and I still
think we should stick the the function approach used by the rest of the API.

http://codereview.chromium.org/481005/diff/2001/2002
File include/v8-debug.h (right):

http://codereview.chromium.org/481005/diff/2001/2002#newcode85
include/v8-debug.h:85: * pending.
This comment does not say anything about what you will achieve by implementing a
class inheriting from this.

http://codereview.chromium.org/481005/diff/2001/2002#newcode87
include/v8-debug.h:87: class AsyncDebugRunner {
Don't add this until you are actually using it.

http://codereview.chromium.org/481005/diff/2001/2002#newcode282
include/v8-debug.h:282: static bool EnableAgent(const char* name, int port, bool
wait_for_connection,
We should add default values for new parameters to avoid breaking existing
clients.

[Peter Rybin, 2009-12-10 20:30:08]

On 2009/12/09 22:26:33, Maxim.Mossienko wrote:
> As to CL#435003 (several v8 instances in process) there is no much difference
to
> hold function pointer or instance of abstract class inside V8Context.

Soren, Maxim

Let me explain my point about point function vs. pointer to object of abstract
class in more words.

What seems important to me is that object can hold some data, while function
cannot.
If you should have N instances of V8 (like with CL#435003), you can provide N
different instances of abstract class, but you can't provide N different
functions.
And in function you'll have a challenge to tell which instance of V8 it should
work with and furthermore to get some embedder-specific data for this instance.

On the other hand functions may be a bit more cheaper, but since we are talking
about debugging, it doesn't really make sense.

[Peter Rybin, 2009-12-10 21:00:17]

http://codereview.chromium.org/481005/diff/2001/2002
File include/v8-debug.h (right):

http://codereview.chromium.org/481005/diff/2001/2002#newcode85
include/v8-debug.h:85: * pending.
On 2009/12/10 14:58:11, Søren Gjesse wrote:
> This comment does not say anything about what you will achieve by implementing
a
> class inheriting from this.

I added a sentence about this.

http://codereview.chromium.org/481005/diff/2001/2002#newcode87
include/v8-debug.h:87: class AsyncDebugRunner {
On 2009/12/10 14:58:11, Søren Gjesse wrote:
> Don't add this until you are actually using it.

I'm ready to use it from sample program "lineprocessor".

http://codereview.chromium.org/481005/diff/2001/2002#newcode282
include/v8-debug.h:282: static bool EnableAgent(const char* name, int port, bool
wait_for_connection,
On 2009/12/10 14:58:11, Søren Gjesse wrote:
> We should add default values for new parameters to avoid breaking existing
> clients.

Done.

[Peter Rybin, 2009-12-10 21:09:49]

I have split the change in two.

From now on, "wait for connection" feature is in
http://codereview.chromium.org/489005/show

[Peter Rybin, 2009-12-10 21:13:05]

> Sorry for the delay - please use my @chromium.org address (I should get the
> @gmail.com one removed).

My bad. I just sent to address, whichever appeared in my browser from cache.

> Please don't add the AsyncDebugRunner until it is actually used, and I still
> think we should stick the the function approach used by the rest of the API.

I think I'm ready to use this API in my sample application (goes in a separate
CL).

I converted class into pointer to function type.

[Maxim.Mossienko, 2009-12-10 21:25:22]

Context dependent function callbacks have "client_data" or "context" parameter
which usually passed during callback registration, it turns out to be the same
thing as abstract class but more archaic :). 

Anyway, the question of what type of callback to use seems to be matter of style
and preserving compile time compatibility IMO is more important (the latter is
broken as current version of patch).
By later reason patch CL#435003 does not change any api signature and at time of
your/other api call particular V8 instance to be used is supposed to be already
chosen.

On 2009/12/10 20:30:08, Peter Rybin wrote:
> On 2009/12/09 22:26:33, Maxim.Mossienko wrote:
> > As to CL#435003 (several v8 instances in process) there is no much
difference
> to
> > hold function pointer or instance of abstract class inside V8Context.
> 
> Soren, Maxim
> 
> Let me explain my point about point function vs. pointer to object of abstract
> class in more words.
> 
> What seems important to me is that object can hold some data, while function
> cannot.
> If you should have N instances of V8 (like with CL#435003), you can provide N
> different instances of abstract class, but you can't provide N different
> functions.
> And in function you'll have a challenge to tell which instance of V8 it should
> work with and furthermore to get some embedder-specific data for this
instance.
> 
> On the other hand functions may be a bit more cheaper, but since we are
talking
> about debugging, it doesn't really make sense.