Cleanups in the Mojo package.

Cleanups in the Mojo package.


Remove unused argument to startRead|Write. This is a breaking change.
Rename constants (to lower case) so that they are consistent. This is a breaking change.
  Keeping the MojoResult constants ("kOk", ...) since changing them would be *really* breaking.

Make the internal library the owner of the constants. That is, the constants are exported from the internal library. The package is just a user of them.

[floitsch, 2016-02-16 16:36:59]

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

[zra, 2016-02-16 16:58:26]

https://codereview.chromium.org/1694413003/diff/1/mojo/dart/packages/mojo/lib...
File mojo/dart/packages/mojo/lib/src/buffer.dart (right):

https://codereview.chromium.org/1694413003/diff/1/mojo/dart/packages/mojo/lib...
mojo/dart/packages/mojo/lib/src/buffer.dart:8: static const int createFlagNone =
MojoConstants.kNone;
This indirection increases the amount of compilation we have to do at startup,
which will hurt startup time. What is the reason for the indirection?

[floitsch, 2016-02-16 17:03:54]

https://codereview.chromium.org/1694413003/diff/1/mojo/dart/packages/mojo/lib...
File mojo/dart/packages/mojo/lib/src/buffer.dart (right):

https://codereview.chromium.org/1694413003/diff/1/mojo/dart/packages/mojo/lib...
mojo/dart/packages/mojo/lib/src/buffer.dart:8: static const int createFlagNone =
MojoConstants.kNone;
On 2016/02/16 16:58:26, zra wrote:
> This indirection increases the amount of compilation we have to do at startup,
> which will hurt startup time. What is the reason for the indirection?

The constants are defined by the internal library and not the public package.
For example, the documentation of the internal library can't reference these
constants (even though it's the own defining what they actually mean).

Ideally the constants in MojoConstants should reference the ones in C, but that
seems to be too complicated.

[floitsch, 2016-02-17 15:33:14]

ping.

If the const-redirects feel like an issue, I could run benchmarks. Are there
already startup benchmarks somewhere?

[zra, 2016-02-17 16:54:08]

On 2016/02/17 15:33:14, floitsch wrote:
> ping.
> 
> If the const-redirects feel like an issue, I could run benchmarks. Are there
> already startup benchmarks somewhere?

Sorry for the delay. You can run startup benchmarks with:

~/mojo/src$ ./mojo/devtools/common/mojo_benchmark --release
mojo/tools/data/benchmarks

dart_init__warm_start and dart_init__warm_start are the ones to keep an eye on.

I don't think this change will make a measurable difference, but looking at a
trace of the VM shows that time spent parsing and compiling constants is
non-trivial. And in general, over many CLs, adding abstraction layers will have
a cost even if no individual CL does. I would like some reassurance that that
cost will be bounded somehow.

[zra, 2016-02-22 20:20:04]

lgtm with nits and a question

https://codereview.chromium.org/1694413003/diff/1/mojo/dart/packages/mojo/sdk...
File mojo/dart/packages/mojo/sdk_ext/src/constants.dart (right):

https://codereview.chromium.org/1694413003/diff/1/mojo/dart/packages/mojo/sdk...
mojo/dart/packages/mojo/sdk_ext/src/constants.dart:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2016

https://codereview.chromium.org/1694413003/diff/1/mojo/dart/packages/mojo/sdk...
mojo/dart/packages/mojo/sdk_ext/src/constants.dart:5: part of internal;
Please add comment explaining where these values come from with a reminder to
keep them in-sync. Maybe like a github link to the C header file in the Mojo
repo.

https://codereview.chromium.org/1694413003/diff/1/mojo/dart/packages/mojo/sdk...
mojo/dart/packages/mojo/sdk_ext/src/constants.dart:8: // MojoResult:
Should these be doc comments?

https://codereview.chromium.org/1694413003/diff/1/mojo/dart/packages/mojo/sdk...
mojo/dart/packages/mojo/sdk_ext/src/constants.dart:34: // DataPipeProducer and
Consumer
Consumer:

[floitsch, 2016-02-26 18:48:08]

https://codereview.chromium.org/1694413003/diff/1/mojo/dart/packages/mojo/sdk...
File mojo/dart/packages/mojo/sdk_ext/src/constants.dart (right):

https://codereview.chromium.org/1694413003/diff/1/mojo/dart/packages/mojo/sdk...
mojo/dart/packages/mojo/sdk_ext/src/constants.dart:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2016/02/22 20:20:04, zra wrote:
> 2016

Done.

https://codereview.chromium.org/1694413003/diff/1/mojo/dart/packages/mojo/sdk...
mojo/dart/packages/mojo/sdk_ext/src/constants.dart:5: part of internal;
On 2016/02/22 20:20:04, zra wrote:
> Please add comment explaining where these values come from with a reminder to
> keep them in-sync. Maybe like a github link to the C header file in the Mojo
> repo.

Done.

https://codereview.chromium.org/1694413003/diff/1/mojo/dart/packages/mojo/sdk...
mojo/dart/packages/mojo/sdk_ext/src/constants.dart:8: // MojoResult:
On 2016/02/22 20:20:04, zra wrote:
> Should these be doc comments?

Dartdoc doesn't support sections yet. This means, that the constant would only
be applied to the first one ('kOk') which wouldn't be helpful.

https://codereview.chromium.org/1694413003/diff/1/mojo/dart/packages/mojo/sdk...
mojo/dart/packages/mojo/sdk_ext/src/constants.dart:34: // DataPipeProducer and
Consumer
On 2016/02/22 20:20:04, zra wrote:
> Consumer:

Done.

[zra, 2017-06-05 16:12:04]

Close this?

[floitsch, 2017-06-06 11:32:16]

Description was changed from

==========
Cleanups in the Mojo package.


Remove unused argument to startRead|Write. This is a breaking change.
Rename constants (to lower case) so that they are consistent. This is a breaking
change.
  Keeping the MojoResult constants ("kOk", ...) since changing them would be
*really* breaking.

Make the internal library the owner of the constants. That is, the constants are
exported from the internal library. The package is just a user of them.
==========

to

==========
Cleanups in the Mojo package.


Remove unused argument to startRead|Write. This is a breaking change.
Rename constants (to lower case) so that they are consistent. This is a breaking
change.
  Keeping the MojoResult constants ("kOk", ...) since changing them would be
*really* breaking.

Make the internal library the owner of the constants. That is, the constants are
exported from the internal library. The package is just a user of them.
==========