Add getIsolateID to Service class in dart:developer

Add getIsolateID to Service class in dart:developer
Fixes https://github.com/dart-lang/sdk/issues/23924

BUG=
R=asiva@google.com

Committed: https://github.com/dart-lang/sdk/commit/6bf86eacd7db5a4c183ec4ba006587c6d079c94d

[Cutch, 2016-11-30 23:19:08]

Description was changed from

==========
Add getServiceIdForIsolate to dart:developer

Fixes https://github.com/dart-lang/sdk/issues/23924

BUG=
==========

to

==========
Add getServiceIdForIsolate to dart:developer

Fixes https://github.com/dart-lang/sdk/issues/23924

BUG=
==========

[Cutch, 2016-11-30 23:19:08]

johnmccutchan@google.com changed reviewers:
+ asiva@google.com, turnidge@google.com

[siva, 2016-12-01 00:09:11]

lgtm

https://codereview.chromium.org/2542003002/diff/1/runtime/lib/developer.cc
File runtime/lib/developer.cc (right):

https://codereview.chromium.org/2542003002/diff/1/runtime/lib/developer.cc#ne...
runtime/lib/developer.cc:182: return String::NewFormatted("isolates/%" Pd64 "",
port_id);
isolates or isolate ?

https://codereview.chromium.org/2542003002/diff/1/runtime/observatory/tests/s...
File
runtime/observatory/tests/service/developer_get_service_id_for_isolate_test.dart
(right):

https://codereview.chromium.org/2542003002/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/developer_get_service_id_for_isolate_test.dart:1:
// Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file
2016

https://codereview.chromium.org/2542003002/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/developer_get_service_id_for_isolate_test.dart:37:
Isolate initialIsolate;
The usage of Isolate here is a little confusing when reading the code, could we
import 'dart:isolate' as Core
and
import 'package:observatory/service_io.dart' as Service

https://codereview.chromium.org/2542003002/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/developer_get_service_id_for_isolate_test.dart:89:
testeeConcurrent: testeeMain);
This test would probably fail in dartium and potentially dart2js, I guess you
would need to adjust the status files accordingly.

[turnidge, 2016-12-01 19:22:45]

https://codereview.chromium.org/2542003002/diff/1/sdk/lib/developer/service.dart
File sdk/lib/developer/service.dart (right):

https://codereview.chromium.org/2542003002/diff/1/sdk/lib/developer/service.d...
sdk/lib/developer/service.dart:80: static String getServiceIdForIsolate(Isolate
isolate) {
getIsolateId?

[Lasse Reichstein Nielsen, 2016-12-01 20:29:04]

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

[Lasse Reichstein Nielsen, 2016-12-01 20:29:05]

drive-by-nitpicking.

https://codereview.chromium.org/2542003002/diff/1/sdk/lib/developer/service.dart
File sdk/lib/developer/service.dart (right):

https://codereview.chromium.org/2542003002/diff/1/sdk/lib/developer/service.d...
sdk/lib/developer/service.dart:77: /// Returns the opaque service id for
[isolate] as a [String]. This id can
id -> ID (both of them)

Not sure what "opaque" means here. Maybe "Returns a [String] token representing
the ID of [isolate]" instead?

Start with a single sentence on the first line, then an empty line, and then
continue with the rest.

Have you considered using a Capability instead of a string as the identifier?
(That obviously won't work if you need to roundtrip the value through non-Dart
code, or store it in JSON, but it is the canonical "opaque capability token"
type in Dart and it can be sent over send ports).

https://codereview.chromium.org/2542003002/diff/1/sdk/lib/developer/service.d...
sdk/lib/developer/service.dart:79: /// Dart environment does not support the
service protocol, this returns null.
Last sentence: "Returns null if the running Dart environment does not support
the service protocol". Possibly make it a new paragraph.

(It's a good idea to start sentences and paragraphs that talk about return types
with "Returns", it makes them stand out. Also, I think it's crisper that way,
and avoids a "this" :)

https://codereview.chromium.org/2542003002/diff/1/sdk/lib/developer/service.d...
sdk/lib/developer/service.dart:103: external String
_getIsolateIdFromSendPort(SendPort sp);
id -> ID.
sp -> sendPort.
(Style guide says: Avoid abbreviations :)

[Cutch, 2016-12-01 20:39:47]

Description was changed from

==========
Add getServiceIdForIsolate to dart:developer

Fixes https://github.com/dart-lang/sdk/issues/23924

BUG=
==========

to

==========
Add getIsolateID to Service class in dart:developer
Fixes https://github.com/dart-lang/sdk/issues/23924

BUG=
==========

[Cutch, 2016-12-01 20:41:30]

https://codereview.chromium.org/2542003002/diff/1/runtime/lib/developer.cc
File runtime/lib/developer.cc (right):

https://codereview.chromium.org/2542003002/diff/1/runtime/lib/developer.cc#ne...
runtime/lib/developer.cc:182: return String::NewFormatted("isolates/%" Pd64 "",
port_id);
On 2016/12/01 00:09:11, siva wrote:
> isolates or isolate ?

the above code is correct. I've moved the format string into a common place so
the two locations can share one definition.

https://codereview.chromium.org/2542003002/diff/1/runtime/observatory/tests/s...
File
runtime/observatory/tests/service/developer_get_service_id_for_isolate_test.dart
(right):

https://codereview.chromium.org/2542003002/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/developer_get_service_id_for_isolate_test.dart:1:
// Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file
On 2016/12/01 00:09:11, siva wrote:
> 2016

Done.

https://codereview.chromium.org/2542003002/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/developer_get_service_id_for_isolate_test.dart:37:
Isolate initialIsolate;
On 2016/12/01 00:09:11, siva wrote:
> The usage of Isolate here is a little confusing when reading the code, could
we
> import 'dart:isolate' as Core
> and
> import 'package:observatory/service_io.dart' as Service

Done.

https://codereview.chromium.org/2542003002/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/developer_get_service_id_for_isolate_test.dart:89:
testeeConcurrent: testeeMain);
On 2016/12/01 00:09:11, siva wrote:
> This test would probably fail in dartium and potentially dart2js, I guess you
> would need to adjust the status files accordingly.

All tests in this directory rely on dart:io and so we wildcard disable them on
dartium and dart2js.

https://codereview.chromium.org/2542003002/diff/1/sdk/lib/developer/service.dart
File sdk/lib/developer/service.dart (right):

https://codereview.chromium.org/2542003002/diff/1/sdk/lib/developer/service.d...
sdk/lib/developer/service.dart:77: /// Returns the opaque service id for
[isolate] as a [String]. This id can
On 2016/12/01 20:29:05, Lasse Reichstein Nielsen wrote:
> id -> ID (both of them)

Done.

> Not sure what "opaque" means here. Maybe "Returns a [String] token
representing
> the ID of [isolate]" instead?
> 

Done.

> Start with a single sentence on the first line, then an empty line, and then
> continue with the rest.
> 

Done.

> Have you considered using a Capability instead of a string as the identifier?
> (That obviously won't work if you need to roundtrip the value through non-Dart
> code, or store it in JSON, but it is the canonical "opaque capability token"
> type in Dart and it can be sent over send ports).

The context of this ID is the service protocol which is a JSON-RPC based
protocol.

https://codereview.chromium.org/2542003002/diff/1/sdk/lib/developer/service.d...
sdk/lib/developer/service.dart:79: /// Dart environment does not support the
service protocol, this returns null.
On 2016/12/01 20:29:05, Lasse Reichstein Nielsen wrote:
> Last sentence: "Returns null if the running Dart environment does not support
> the service protocol". Possibly make it a new paragraph.
> 
> (It's a good idea to start sentences and paragraphs that talk about return
types
> with "Returns", it makes them stand out. Also, I think it's crisper that way,
> and avoids a "this" :)

Done.

https://codereview.chromium.org/2542003002/diff/1/sdk/lib/developer/service.d...
sdk/lib/developer/service.dart:80: static String getServiceIdForIsolate(Isolate
isolate) {
On 2016/12/01 19:22:44, turnidge wrote:
> getIsolateId?

Done.

https://codereview.chromium.org/2542003002/diff/1/sdk/lib/developer/service.d...
sdk/lib/developer/service.dart:103: external String
_getIsolateIdFromSendPort(SendPort sp);
On 2016/12/01 20:29:05, Lasse Reichstein Nielsen wrote:
> id -> ID.
> sp -> sendPort.
> (Style guide says: Avoid abbreviations :)

Done.

[Cutch, 2016-12-01 20:47:22]

Description was changed from

==========
Add getIsolateID to Service class in dart:developer
Fixes https://github.com/dart-lang/sdk/issues/23924

BUG=
==========

to

==========
Add getIsolateID to Service class in dart:developer
Fixes https://github.com/dart-lang/sdk/issues/23924

BUG=
R=asiva@google.com

Committed:
https://github.com/dart-lang/sdk/commit/6bf86eacd7db5a4c183ec4ba006587c6d079c94d
==========

[Cutch, 2016-12-01 20:47:23]

Committed patchset #3 (id:40001) manually as
6bf86eacd7db5a4c183ec4ba006587c6d079c94d (presubmit successful).