Update Isolate API.

Update Isolate API.

Remove AS_EVENT as priority of ping/kill. It wasn't very usable or predictable.
Make priority consistently named and a named parameter.
Add response object to ping/addExitListener so it doesn't have to send null.
This is useful for cases where you want to use the same receive port for
multiple purposes.

R=sgjesse@google.com

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

[Lasse Reichstein Nielsen, 2015-04-10 15:51:59]

lrn@google.com changed reviewers:
+ floitsch@google.com, iposva@google.com, sgjesse@google.com

[kevmoo, 2015-04-10 19:03:09]

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

[kevmoo, 2015-04-10 19:03:10]

DBC.

This is why we should avoid positional arguments.

Sorry for the duplicate comment.

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

https://codereview.chromium.org/1074223002/diff/1/sdk/lib/isolate/isolate.dar...
sdk/lib/isolate/isolate.dart:330: external void kill({int priority:
BEFORE_NEXT_EVENT});
This is a breaking change.

https://codereview.chromium.org/1074223002/diff/1/sdk/lib/isolate/isolate.dar...
sdk/lib/isolate/isolate.dart:356: external void ping(SendPort responsePort,
{Object response,
This is a breaking change.

https://codereview.chromium.org/1074223002/diff/1/sdk/lib/isolate/isolate.dar...
sdk/lib/isolate/isolate.dart:356: external void ping(SendPort responsePort,
{Object response,
this is a breaking change

[Lasse Reichstein Nielsen, 2015-04-10 19:38:02]

We should fix the language so positional optional parameters don't block adding
named optional parameters. 

When a positional parameter is the right thing to use, we *should* use it, but
then we need to be willing to break things until we are satisfied with an API.
Locking an API too early is worse, in the long run, than breaking it once or
twice.

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

https://codereview.chromium.org/1074223002/diff/1/sdk/lib/isolate/isolate.dar...
sdk/lib/isolate/isolate.dart:279: external void addOnExitListener(SendPort
responsePort, {Object response});
I am slightly worried about sending the "response" object. It can be very
useful, but it does open the possibility of sending something that can't be
received at the other end, and thereby failing, silently, to add the listener.

https://codereview.chromium.org/1074223002/diff/1/sdk/lib/isolate/isolate.dar...
sdk/lib/isolate/isolate.dart:357: int priority: IMMEDIATE});
This is indeed a breaking change.
Luckily, the function is still marked as experimental, which means that we can
break it to improve it.

Now, if we could have both positional and named optional arguments on the same
function, then it wouldn't be a problem, the we could just add {response}
independently of what other arguments we already have.
Not having that is really cramping out style.

I would have preferred a signature of:
  void ping(SendPort responsePort, [int priority = IMMEDIATE],
                                   {Object response});
which is non-breaking, but we can't wait for the language to get unbroken.

[Søren Gjesse, 2015-04-13 08:25:55]

lgtm

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

https://codereview.chromium.org/1074223002/diff/1/sdk/lib/isolate/isolate.dar...
sdk/lib/isolate/isolate.dart:268: * Adding the same poort more than once will
only cause it to receive one
poort -> port

https://codereview.chromium.org/1074223002/diff/1/sdk/lib/isolate/isolate.dar...
sdk/lib/isolate/isolate.dart:279: external void addOnExitListener(SendPort
responsePort, {Object response});
On 2015/04/10 19:38:02, Lasse Reichstein Nielsen wrote:
> I am slightly worried about sending the "response" object. It can be very
> useful, but it does open the possibility of sending something that can't be
> received at the other end, and thereby failing, silently, to add the listener.

Don't we already know whether the other isolate is a spawn or spawnUri isolate?
Then we can check whether it need to be a 'simple value'.

https://codereview.chromium.org/1074223002/diff/1/sdk/lib/isolate/isolate.dar...
sdk/lib/isolate/isolate.dart:330: external void kill({int priority:
BEFORE_NEXT_EVENT});
On 2015/04/10 19:03:10, kevmoo wrote:
> This is a breaking change.

The API is still marked as experimental, and kill have only been working since
1.9, so this should be OK.

[Lasse Reichstein Nielsen, 2015-04-13 10:15:03]

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

https://codereview.chromium.org/1074223002/diff/1/sdk/lib/isolate/isolate.dar...
sdk/lib/isolate/isolate.dart:268: * Adding the same poort more than once will
only cause it to receive one
On 2015/04/13 08:25:55, Søren Gjesse wrote:
> poort -> port

Done.

https://codereview.chromium.org/1074223002/diff/1/sdk/lib/isolate/isolate.dar...
sdk/lib/isolate/isolate.dart:279: external void addOnExitListener(SendPort
responsePort, {Object response});
True, now sending an invalid value throws at the sending end. That's good enough
to prevent silent failure.

[Lasse Reichstein Nielsen, 2015-04-13 10:30:12]

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

[sra1, 2015-04-13 16:18:11]

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

[sra1, 2015-04-13 16:18:12]

This adds 6.5k to swarm.
Can you investigate why it adds to much code?

[Lasse Reichstein Nielsen, 2015-04-13 16:44:53]

On 2015/04/13 16:18:12, sra1 wrote:
> This adds 6.5k to swarm.
> Can you investigate why it adds to much code?


Immediate guess: It uses HashMap where it used to have a list.

[kevmoo, 2015-04-13 17:02:30]

On 2015/04/13 16:44:53, Lasse Reichstein Nielsen wrote:
> On 2015/04/13 16:18:12, sra1 wrote:
> > This adds 6.5k to swarm.
> > Can you investigate why it adds to much code?
> 
> 
> Immediate guess: It uses HashMap where it used to have a list.

Please add a note to CHANGELOG.md

I love the moved to named args, but we do risk breaking real users who have
started to depend on these APIs.

Marked as experimental gives us a bit of cover – but only a bit.

[ahe, 2015-06-01 20:09:06]

On 2015/04/13 17:02:30, kevmoo wrote:
> On 2015/04/13 16:44:53, Lasse Reichstein Nielsen wrote:
> > On 2015/04/13 16:18:12, sra1 wrote:
> > > This adds 6.5k to swarm.
> > > Can you investigate why it adds to much code?
> > 
> > 
> > Immediate guess: It uses HashMap where it used to have a list.
> 
> Please add a note to CHANGELOG.md
> 
> I love the moved to named args, but we do risk breaking real users who have
> started to depend on these APIs.
> 
> Marked as experimental gives us a bit of cover – but only a bit.

Where is the announcement of this breaking change?

I'd like to understand why this change is necessary, and I'd like a better
explanation than: "Lasse thinks the language should have been designed
differently" or "consistency". The explanation has to be: "it is dangerous to
...", "we are unable to implement important feature ... without this change", or
something similar.