Enable/disable LiveEdit using the (C++) debug API.

Enable/disable LiveEdit using the (C++) debug API.

BUG=

Committed: https://code.google.com/p/v8/source/detail?r=12472

[Yang, 2012-08-27 09:31:45]

Please take a look.

[Peter Rybin, 2012-08-27 16:02:12]

LGTM

[Erik Corry, 2012-08-29 11:00:42]

LGTM.

When the support is added to the embedder I would suggest changing the default
in V8 so that liveedit is off by default.

http://codereview.chromium.org/10875072/diff/1/include/v8-debug.h
File include/v8-debug.h (right):

http://codereview.chromium.org/10875072/diff/1/include/v8-debug.h#newcode324
include/v8-debug.h:324: Handle<Value> data = Handle<Value>());
Wrong indentation.

http://codereview.chromium.org/10875072/diff/1/include/v8-debug.h#newcode394
include/v8-debug.h:394: * Optionally enable/disable LiveEdit functionality for
the given Isolate
You don't need the word "Optionally" here.

http://codereview.chromium.org/10875072/diff/1/src/debug.h
File src/debug.h (right):

http://codereview.chromium.org/10875072/diff/1/src/debug.h#newcode876
src/debug.h:876: bool is_live_edit_enabled() const { return
is_live_edit_enabled_; }
The variable should be called live_edit_enabled_ and the getters and setters
should be named in a similar way, with no "is".

[Peter Rybin, 2012-08-29 16:53:21]

Erik, gents,

I think we need a view on how V8-based applications with DebugAgent (such as
Node.JS) should handle this. Probably it would be sad if they silently lose
LiveEdit feature (well, not silently, I guess they are going to crash on
attempt).

Peter


On 2012/08/29 11:00:42, Erik Corry wrote:
> LGTM.
> 
> When the support is added to the embedder I would suggest changing the default
> in V8 so that liveedit is off by default.
> 
> http://codereview.chromium.org/10875072/diff/1/include/v8-debug.h
> File include/v8-debug.h (right):
> 
> http://codereview.chromium.org/10875072/diff/1/include/v8-debug.h#newcode324
> include/v8-debug.h:324: Handle<Value> data = Handle<Value>());
> Wrong indentation.
> 
> http://codereview.chromium.org/10875072/diff/1/include/v8-debug.h#newcode394
> include/v8-debug.h:394: * Optionally enable/disable LiveEdit functionality for
> the given Isolate
> You don't need the word "Optionally" here.
> 
> http://codereview.chromium.org/10875072/diff/1/src/debug.h
> File src/debug.h (right):
> 
> http://codereview.chromium.org/10875072/diff/1/src/debug.h#newcode876
> src/debug.h:876: bool is_live_edit_enabled() const { return
> is_live_edit_enabled_; }
> The variable should be called live_edit_enabled_ and the getters and setters
> should be named in a similar way, with no "is".

[Yang, 2012-08-31 15:53:29]

On 2012/08/29 16:53:21, Peter Rybin wrote:
> Erik, gents,
> 
> I think we need a view on how V8-based applications with DebugAgent (such as
> Node.JS) should handle this. Probably it would be sad if they silently lose
> LiveEdit feature (well, not silently, I guess they are going to crash on
> attempt).
> 
> Peter
> 
> 
> On 2012/08/29 11:00:42, Erik Corry wrote:
> > LGTM.
> > 
> > When the support is added to the embedder I would suggest changing the
default
> > in V8 so that liveedit is off by default.
> > 
> > http://codereview.chromium.org/10875072/diff/1/include/v8-debug.h
> > File include/v8-debug.h (right):
> > 
> > http://codereview.chromium.org/10875072/diff/1/include/v8-debug.h#newcode324
> > include/v8-debug.h:324: Handle<Value> data = Handle<Value>());
> > Wrong indentation.
> > 
> > http://codereview.chromium.org/10875072/diff/1/include/v8-debug.h#newcode394
> > include/v8-debug.h:394: * Optionally enable/disable LiveEdit functionality
for
> > the given Isolate
> > You don't need the word "Optionally" here.
> > 
> > http://codereview.chromium.org/10875072/diff/1/src/debug.h
> > File src/debug.h (right):
> > 
> > http://codereview.chromium.org/10875072/diff/1/src/debug.h#newcode876
> > src/debug.h:876: bool is_live_edit_enabled() const { return
> > is_live_edit_enabled_; }
> > The variable should be called live_edit_enabled_ and the getters and setters
> > should be named in a similar way, with no "is".

I was under the impression that the LiveEdit feature does not come "for free"
per se, but the embedder has to somehow adapt his application to use it (offer
user interface, actively use the LiveEdit API etc.). If that's the case, having
LiveEdit disabled by default should be rather obvious to an embedder who relies
on it.

[Peter Rybin, 2012-08-31 16:00:33]

On 2012/08/31 15:53:29, Yang wrote:
> On 2012/08/29 16:53:21, Peter Rybin wrote:
> > Erik, gents,
> > 
> > I think we need a view on how V8-based applications with DebugAgent (such as
> > Node.JS) should handle this. Probably it would be sad if they silently lose
> > LiveEdit feature (well, not silently, I guess they are going to crash on
> > attempt).
> > Peter
> 
> I was under the impression that the LiveEdit feature does not come "for free"
> per se, but the embedder has to somehow adapt his application to use it (offer
> user interface, actively use the LiveEdit API etc.). If that's the case,
having
> LiveEdit disabled by default should be rather obvious to an embedder who
relies
> on it.

As far as I understand that's not the case. The embedder merely should enable
Debug Agent (see v8::EnableAgent) that supports v8 debug protocol via TCP port,
including LiveEdit. Then he can use a debug client, for example
http://code.google.com/p/chromedevtools

For example Node.JS does it and the sample lineprocessor.cc does it (no
liveedit-specific code is there).