Some fixes to work better with the services framework

Some fixes to work better with the services framework

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

[Alan Knight, 2013-02-06 00:41:23]

This has some changes from making this work with Justin's serialization
framework.

- The SimpleJsonFormat works better with simple types, so if you just pass "123"
as a string, you just get "123" as output.

- Formats don't do their own stringify/parse. The Json-ish formats return data
structures that are JSON-able, but it's up to the thing using them whether to
stringify them, pass them around in memory, send to another isolate, or
whatever.

- There are a bunch more tests. 

- When we look up a mirror, we can look it up by an arbitrary name in the named
objects, but if it's not found there we'll try to look it up as a qualified name
using mirrors. 

- It uses the same format for the selfDescribingData as it uses for the rest,
and there are some fixes to the SimpleJsonFormat to make it work for that.

-  We pass the writer to extractState, because if the state is extracted to a
name that's a string, we still have to make a reference for that string, which
requires the Writer.

- With the simple json format, avoid some of the destructive operations in going
through and fixing up the structure. It does not work well with libv2 lazy
operations, especially if you ever try and debug.

[Jennifer Messerly, 2013-02-08 04:04:38]

LGTM

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/lib/src/...
File pkg/serialization/lib/src/format.dart (right):

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/format.dart:114: "rules" : w.serializedRules(),
I think I usually just indent this +2, since it has matching end curly. or could
be one line?

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/lib/src/...
File pkg/serialization/lib/src/reader_writer.dart (right):

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/reader_writer.dart:321: void keyNotFound(key) { throw
'Cannot find named object to link to: $key';}
Would be good to throw a more precise Exception here.

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/lib/src/...
File pkg/serialization/lib/src/serialization_rule.dart (right):

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/serialization_rule.dart:388: * automatically uses its
simpleName as the key into the namedObjects.
this comment looks out of date (e.g. "simpleName")

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/test/ser...
File pkg/serialization/test/serialization_test.dart (right):

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/test/ser...
pkg/serialization/test/serialization_test.dart:502: * The end of the tests and
the beginning of various helper functions to make
btw, one idea longer term would be to pull these out into their own file.

[justinfagnani, 2013-02-09 21:21:56]

https://chromiumcodereview.appspot.com/12136002/diff/2001/pkg/serialization/l...
File pkg/serialization/lib/serialization.dart (right):

https://chromiumcodereview.appspot.com/12136002/diff/2001/pkg/serialization/l...
pkg/serialization/lib/serialization.dart:354: read(input, [Map externals = const
{}]) {
now that the type annotation is removed, can you document what types are allowed
for input?

https://chromiumcodereview.appspot.com/12136002/diff/2001/pkg/serialization/l...
File pkg/serialization/lib/src/format.dart (right):

https://chromiumcodereview.appspot.com/12136002/diff/2001/pkg/serialization/l...
pkg/serialization/lib/src/format.dart:55: generateOutput(Writer w) {
Style nit: Why remove the type here? Since the method does return something, at
least Object would show that. Maybe the doc needs to be update where it says
"return it as a String"?

https://chromiumcodereview.appspot.com/12136002/diff/2001/pkg/serialization/l...
pkg/serialization/lib/src/format.dart:108: generateOutput(Writer w) {
Map as the return type?

https://chromiumcodereview.appspot.com/12136002/diff/2001/pkg/serialization/l...
pkg/serialization/lib/src/format.dart:160: * Read a [json] encoded string
representing serialized data in this format
string->?

https://chromiumcodereview.appspot.com/12136002/diff/2001/pkg/serialization/l...
pkg/serialization/lib/src/format.dart:171: reader.readRules(rules);
what happens here if "rules" is just some user data? Could the
serialization-related properties be made "private" with a '_' prefix?

[Jennifer Messerly, 2013-02-10 00:20:35]

https://chromiumcodereview.appspot.com/12136002/diff/2001/pkg/serialization/l...
File pkg/serialization/lib/src/format.dart (right):

https://chromiumcodereview.appspot.com/12136002/diff/2001/pkg/serialization/l...
pkg/serialization/lib/src/format.dart:55: generateOutput(Writer w) {
On 2013/02/09 21:21:56, justinfagnani wrote:
> Style nit: Why remove the type here? Since the method does return something,
at
> least Object would show that. Maybe the doc needs to be update where it says
> "return it as a String"?

Style nit nit :)

If return type is "dynamic", we don't write "dynamic".

"dynamic" return type is more friendly than "Object". I agree about using
"Object" as a parameter though -- it says you aren't going to use any API beyond
object.

However this particular method could be declared to return Map<String, dynamic>.

https://chromiumcodereview.appspot.com/12136002/diff/2001/pkg/serialization/l...
pkg/serialization/lib/src/format.dart:108: generateOutput(Writer w) {
On 2013/02/09 21:21:56, justinfagnani wrote:
> Map as the return type?

I don't think it necessarily returns a map if not selfDescribing or not
storeRoundTripInfo

https://chromiumcodereview.appspot.com/12136002/diff/2001/pkg/serialization/l...
pkg/serialization/lib/src/format.dart:171: reader.readRules(rules);
On 2013/02/09 21:21:56, justinfagnani wrote:
> what happens here if "rules" is just some user data? Could the
> serialization-related properties be made "private" with a '_' prefix?

This is a good idea.

We should go even further IMO. User should need to specify explicitly if the
format is "self describing JSON" or normal JSON. We shouldn't have any
possibility of incorrectly recognizing the data. "_" prefix is nice but still
not enough, IMO.

[Alan Knight, 2013-02-11 23:38:41]

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/lib/seri...
File pkg/serialization/lib/serialization.dart (right):

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/lib/seri...
pkg/serialization/lib/serialization.dart:354: read(input, [Map externals = const
{}]) {
On 2013/02/09 21:21:56, justinfagnani wrote:
> now that the type annotation is removed, can you document what types are
allowed
> for input?

Done. Also fixed up the main doc comment some.

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/lib/src/...
File pkg/serialization/lib/src/format.dart (right):

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/format.dart:55: generateOutput(Writer w) {
On 2013/02/10 00:20:35, John Messerly wrote:
> On 2013/02/09 21:21:56, justinfagnani wrote:
> > Style nit: Why remove the type here? Since the method does return something,
> at
> > least Object would show that. Maybe the doc needs to be update where it says
> > "return it as a String"?
> 
> Style nit nit :)
> 
> If return type is "dynamic", we don't write "dynamic".
> 
> "dynamic" return type is more friendly than "Object". I agree about using
> "Object" as a parameter though -- it says you aren't going to use any API
beyond
> object.
> 
> However this particular method could be declared to return Map<String,
dynamic>.

Good point, changed this to return Map<String, dynamic>.

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/format.dart:108: generateOutput(Writer w) {
On 2013/02/10 00:20:35, John Messerly wrote:
> On 2013/02/09 21:21:56, justinfagnani wrote:
> > Map as the return type?
> 
> I don't think it necessarily returns a map if not selfDescribing or not
> storeRoundTripInfo

It doesn't necessarily return a Map, part of the changes in this CL are that the
SimpleJsonFormat maps simple types to themselves, so "abc" -> "abc" and 3 -> 3.
At least if not using the round trip info or selfDescribing. Makes the output
more compact and readable, but not very typeable.

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/format.dart:114: "rules" : w.serializedRules(),
On 2013/02/08 04:04:38, John Messerly wrote:
> I think I usually just indent this +2, since it has matching end curly. or
could
> be one line?

Because I made those be identifiers rather than literal strings I can't use a
literal map any more :-( So now it's cascaded assignments, which I think does
want four spaces

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/format.dart:160: * Read a [json] encoded string
representing serialized data in this format
On 2013/02/09 21:21:56, justinfagnani wrote:
> string->?

Done.

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/format.dart:171: reader.readRules(rules);
On 2013/02/10 00:20:35, John Messerly wrote:
> On 2013/02/09 21:21:56, justinfagnani wrote:
> > what happens here if "rules" is just some user data? Could the
> > serialization-related properties be made "private" with a '_' prefix?
> 
> This is a good idea.
> 
> We should go even further IMO. User should need to specify explicitly if the
> format is "self describing JSON" or normal JSON. We shouldn't have any
> possibility of incorrectly recognizing the data. "_" prefix is nice but still
> not enough, IMO.

OK, I changed this to just look at the "selfDescribing" property on the
serialization. Which I think should probably move to the format, but will do
that in a different CL, so added a TODO.

That makes this a little bit more fragile if the user saves it one way and tries
to read it the other, but simplifies the code. So I added an explicit check that

throws if it looks like it was written without selfDescribing set and is being
read with it.

Also changed these to use _rules, _data, _root for this format, and put them
into constants. I only did it for this format because this is the only one that
has the possibility of mixing user data with private serialization data, as
we're trying to write both plain JSON and JSON with our annotations. And it
minimized the extent of the change.

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/lib/src/...
File pkg/serialization/lib/src/reader_writer.dart (right):

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/reader_writer.dart:321: void keyNotFound(key) { throw
'Cannot find named object to link to: $key';}
On 2013/02/08 04:04:38, John Messerly wrote:
> Would be good to throw a more precise Exception here.

Done.

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/lib/src/...
File pkg/serialization/lib/src/serialization_rule.dart (right):

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/serialization_rule.dart:388: * automatically uses its
simpleName as the key into the namedObjects.
On 2013/02/08 04:04:38, John Messerly wrote:
> this comment looks out of date (e.g. "simpleName")

Done.

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/test/ser...
File pkg/serialization/test/serialization_test.dart (right):

https://codereview.chromium.org/12136002/diff/2001/pkg/serialization/test/ser...
pkg/serialization/test/serialization_test.dart:502: * The end of the tests and
the beginning of various helper functions to make
On 2013/02/08 04:04:38, John Messerly wrote:
> btw, one idea longer term would be to pull these out into their own file.

Yes, I'm going to do that along with splitting out the tests that require
reflection so the others can be tested on dart2js.