Provide a dart2js implementation of dart:debugger

Provide a dart2js implementation of dart:debugger

BUG=
R=sra@google.com

Committed: https://code.google.com/p/dart/source/detail?r=45577

[Alan Knight, 2015-05-05 19:22:28]

alanknight@google.com changed reviewers:
+ skybrian@google.com, sra@google.com

[skybrian, 2015-05-05 19:35:39]

I'm not qualified to review the implementation but it looks okay...

https://codereview.chromium.org/1121293003/diff/1/sdk/lib/_internal/compiler/...
File sdk/lib/_internal/compiler/js_lib/debugger_patch.dart (left):

https://codereview.chromium.org/1121293003/diff/1/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/js_lib/debugger_patch.dart:13: /// not a debugger is
connected.
The last statement isn't true for the JavaScript "debugger" statement [1], so I
think we need to qualify it? "When running in the Dart VM, the behavior is the
same [...]".

[1]
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/...

[floitsch, 2015-05-05 20:29:27]

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

[floitsch, 2015-05-05 20:29:29]

DBC.

https://codereview.chromium.org/1121293003/diff/1/sdk/lib/_internal/compiler/...
File sdk/lib/_internal/compiler/js_lib/debugger_patch.dart (right):

https://codereview.chromium.org/1121293003/diff/1/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/js_lib/debugger_patch.dart:16: static void
breakHere() {
The _js_helper library has a "ForceInline()" annotation.
I would put it on this method.

https://codereview.chromium.org/1121293003/diff/1/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/js_lib/debugger_patch.dart:23: static void
breakHereIf(bool expr) {
ditto.

[sra1, 2015-05-06 04:33:10]

https://codereview.chromium.org/1121293003/diff/1/sdk/lib/_internal/compiler/...
File sdk/lib/_internal/compiler/js_lib/debugger_patch.dart (left):

https://codereview.chromium.org/1121293003/diff/1/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/js_lib/debugger_patch.dart:13: /// not a debugger is
connected.
On 2015/05/05 19:35:38, skybrian wrote:
> The last statement isn't true for the JavaScript "debugger" statement [1], so
I
> think we need to qualify it? "When running in the Dart VM, the behavior is the
> same [...]".
> 
> [1]
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/...
> 

+1

https://codereview.chromium.org/1121293003/diff/1/sdk/lib/debugger/debugger.dart
File sdk/lib/debugger/debugger.dart (right):

https://codereview.chromium.org/1121293003/diff/1/sdk/lib/debugger/debugger.d...
sdk/lib/debugger/debugger.dart:14: external static void breakHere();
Since this is now external, the VM needs a patch file too.

[Alan Knight, 2015-05-06 17:04:39]

PTAL

https://codereview.chromium.org/1121293003/diff/1/sdk/lib/_internal/compiler/...
File sdk/lib/_internal/compiler/js_lib/debugger_patch.dart (left):

https://codereview.chromium.org/1121293003/diff/1/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/js_lib/debugger_patch.dart:13: /// not a debugger is
connected.
On 2015/05/05 19:35:38, skybrian wrote:
> The last statement isn't true for the JavaScript "debugger" statement [1], so
I
> think we need to qualify it? "When running in the Dart VM, the behavior is the
> same [...]".
> 
> [1]
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/...
> 

Done.

https://codereview.chromium.org/1121293003/diff/1/sdk/lib/_internal/compiler/...
File sdk/lib/_internal/compiler/js_lib/debugger_patch.dart (right):

https://codereview.chromium.org/1121293003/diff/1/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/js_lib/debugger_patch.dart:16: static void
breakHere() {
On 2015/05/05 20:29:28, floitsch wrote:
> The _js_helper library has a "ForceInline()" annotation.
> I would put it on this method.

Done.

https://codereview.chromium.org/1121293003/diff/1/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/js_lib/debugger_patch.dart:23: static void
breakHereIf(bool expr) {
On 2015/05/05 20:29:28, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/1121293003/diff/1/sdk/lib/debugger/debugger.dart
File sdk/lib/debugger/debugger.dart (right):

https://codereview.chromium.org/1121293003/diff/1/sdk/lib/debugger/debugger.d...
sdk/lib/debugger/debugger.dart:14: external static void breakHere();
On 2015/05/06 04:33:10, sra1 wrote:
> Since this is now external, the VM needs a patch file too.

There is already a patch class in runtime/lib/debugger.dart. Is more than that
required? It appears to work with the VM.

[sra1, 2015-05-06 17:18:14]

On 2015/05/06 17:04:39, Alan Knight wrote:
> PTAL
> 
>
https://codereview.chromium.org/1121293003/diff/1/sdk/lib/_internal/compiler/...
> File sdk/lib/_internal/compiler/js_lib/debugger_patch.dart (left):
> 
>
https://codereview.chromium.org/1121293003/diff/1/sdk/lib/_internal/compiler/...
> sdk/lib/_internal/compiler/js_lib/debugger_patch.dart:13: /// not a debugger
is
> connected.
> On 2015/05/05 19:35:38, skybrian wrote:
> > The last statement isn't true for the JavaScript "debugger" statement [1],
so
> I
> > think we need to qualify it? "When running in the Dart VM, the behavior is
the
> > same [...]".
> > 
> > [1]
> >
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/...
> > 
> 
> Done.
> 
>
https://codereview.chromium.org/1121293003/diff/1/sdk/lib/_internal/compiler/...
> File sdk/lib/_internal/compiler/js_lib/debugger_patch.dart (right):
> 
>
https://codereview.chromium.org/1121293003/diff/1/sdk/lib/_internal/compiler/...
> sdk/lib/_internal/compiler/js_lib/debugger_patch.dart:16: static void
> breakHere() {
> On 2015/05/05 20:29:28, floitsch wrote:
> > The _js_helper library has a "ForceInline()" annotation.
> > I would put it on this method.
> 
> Done.
> 
>
https://codereview.chromium.org/1121293003/diff/1/sdk/lib/_internal/compiler/...
> sdk/lib/_internal/compiler/js_lib/debugger_patch.dart:23: static void
> breakHereIf(bool expr) {
> On 2015/05/05 20:29:28, floitsch wrote:
> > ditto.
> 
> Done.
> 
>
https://codereview.chromium.org/1121293003/diff/1/sdk/lib/debugger/debugger.dart
> File sdk/lib/debugger/debugger.dart (right):
> 
>
https://codereview.chromium.org/1121293003/diff/1/sdk/lib/debugger/debugger.d...
> sdk/lib/debugger/debugger.dart:14: external static void breakHere();
> On 2015/05/06 04:33:10, sra1 wrote:
> > Since this is now external, the VM needs a patch file too.
> 
> There is already a patch class in runtime/lib/debugger.dart. Is more than that
> required? It appears to work with the VM.

That is OK then.

[sra1, 2015-05-06 17:18:21]

lgtm

[Cutch, 2015-05-06 19:50:09]

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

[Cutch, 2015-05-06 19:50:10]

https://codereview.chromium.org/1121293003/diff/20001/sdk/lib/debugger/debugg...
File sdk/lib/debugger/debugger.dart (right):

https://codereview.chromium.org/1121293003/diff/20001/sdk/lib/debugger/debugg...
sdk/lib/debugger/debugger.dart:13: /// not a debugger is connected.
DBC: Update comment here.

[Alan Knight, 2015-05-06 22:30:50]

https://codereview.chromium.org/1121293003/diff/20001/sdk/lib/debugger/debugg...
File sdk/lib/debugger/debugger.dart (right):

https://codereview.chromium.org/1121293003/diff/20001/sdk/lib/debugger/debugg...
sdk/lib/debugger/debugger.dart:13: /// not a debugger is connected.
On 2015/05/06 19:50:10, Cutch wrote:
> DBC: Update comment here.

Already done from an earlier comment.

[Alan Knight, 2015-05-06 22:36:08]

Committed patchset #2 (id:20001) manually as 45577 (presubmit successful).

[skybrian, 2015-05-06 22:51:57]

https://codereview.chromium.org/1121293003/diff/20001/sdk/lib/debugger/debugg...
File sdk/lib/debugger/debugger.dart (right):

https://codereview.chromium.org/1121293003/diff/20001/sdk/lib/debugger/debugg...
sdk/lib/debugger/debugger.dart:13: /// not a debugger is connected.
On 2015/05/06 22:30:50, Alan Knight wrote:
> On 2015/05/06 19:50:10, Cutch wrote:
> > DBC: Update comment here.
> 
> Already done from an earlier comment.

There are doc comments in two places, here and in debugger_patch.dart. Which one
shows up on api.dartlang.org and in IDE's? If it's this version, then perhaps
this comment needs to be updated as well?

[Cutch, 2015-05-06 22:54:29]

On 2015/05/06 22:51:57, skybrian wrote:
>
https://codereview.chromium.org/1121293003/diff/20001/sdk/lib/debugger/debugg...
> File sdk/lib/debugger/debugger.dart (right):
> 
>
https://codereview.chromium.org/1121293003/diff/20001/sdk/lib/debugger/debugg...
> sdk/lib/debugger/debugger.dart:13: /// not a debugger is connected.
> On 2015/05/06 22:30:50, Alan Knight wrote:
> > On 2015/05/06 19:50:10, Cutch wrote:
> > > DBC: Update comment here.
> > 
> > Already done from an earlier comment.
> 
> There are doc comments in two places, here and in debugger_patch.dart. Which
one
> shows up on http://api.dartlang.org and in IDE's? If it's this version, then
perhaps
> this comment needs to be updated as well?

This was my point. The comment needs to be in sync across the various
implementations.

It's unclear where the canonical place for API documentation is. I filed a bug
years ago about this, which was ignored.

[Alan Knight, 2015-05-06 23:00:55]

Ah, I misunderstood the comment. Created
https://codereview.chromium.org/1130043002

Documentation in api.dartlang.org comes from the dart2js sources. It has no
choice in that, because it uses dart2js mirrors to extract the information.
dartdoc is using the analyzer, I'm not sure which sources it uses.