Better ability to have hand-written custom rules and various cleanups.

Better ability to have hand-written custom rules and various cleanups.

In particular, added CustomRule with a simplified set of methods for 
subclasses to implement, renamed ClosureToMapRule to ClosureRule
and made it a subclass of CustomRule. Custom rules that return lists will
work either in the map or list format. Those that return maps will only
work in the map format. Introduced implementations of Map and List that 
resolve references only on demand as part of the simplifications. Since 
we don't have a way to serialize CustomRule, made selfDescribing be off
by default if those are used.

Generalized the ClassMirrorRule to general support for named objects both
on read and write with a specific subclass for DeclarationMirrors so that
they are set up automatically on write. Still the user's responsibility 
on read.

Removed a bunch of declarations of things as Object, which didn't add any 
information and just made for spurious type warnings.

Simplified the API so just read/write that take a single object. Removed 
readOne/writeOne. If you want multiple objects, use a collection as the 
single object.

Slightly generalized the dumpStateInto and related flat format methods. They
should still go away in favor of a more general mechanism, but at least there's
less code duplication now.

BUG=

[Alan Knight, 2012-12-12 18:29:36]

See description for an overview of what's changed.

[Jennifer Messerly, 2012-12-12 20:38:28]

looks great, just a few minor comments

https://chromiumcodereview.appspot.com/11553012/diff/1/pkg/serialization/lib/...
File pkg/serialization/lib/serialization.dart (right):

https://chromiumcodereview.appspot.com/11553012/diff/1/pkg/serialization/lib/...
pkg/serialization/lib/serialization.dart:198: _selfDescribing = !rules.some((x)
=> x is CustomRule);
Do we know that the set of "rules" is fixed at this point? In other words, could
add a CustomRule later but this property has cached a value of true?

Also, what happens if I set selfDescribing to true but I in fact do have a
CustomRule? Does it work?

It seems potentially fragile to compute a value once and cache it if the things
used by the computation could change later. It would be good to invalidate this
property in addRule, or just don't cache it.

https://chromiumcodereview.appspot.com/11553012/diff/1/pkg/serialization/lib/...
pkg/serialization/lib/serialization.dart:280: void addRule(SerializationRule
rule) {
An issue I just noticed (maybe we already have a TODO) -- rules.add is different
from addRule. If someone added a rule with "rules.add" it wouldn't have the
correct invariants (number property not set). This could be fixed in a few ways:
* not expose the raw list to the user
* expose a smart list that knows how to maintain the invariants (we do something
similar for DOM node lists -- adding to them sets their parent property -- but
it's more work)
* don't set "rule.number" or check for CustomRules up front. Instead wait until
we start serialization, and then update the properties:

https://chromiumcodereview.appspot.com/11553012/diff/1/pkg/serialization/lib/...
File pkg/serialization/lib/src/reader_writer.dart (right):

https://chromiumcodereview.appspot.com/11553012/diff/1/pkg/serialization/lib/...
pkg/serialization/lib/src/reader_writer.dart:272: return references[object];
totally optional, but this could be:
  _referenceFor(object) => references[object];

https://chromiumcodereview.appspot.com/11553012/diff/1/pkg/serialization/lib/...
File pkg/serialization/lib/src/serialization_rule.dart (right):

https://chromiumcodereview.appspot.com/11553012/diff/1/pkg/serialization/lib/...
pkg/serialization/lib/src/serialization_rule.dart:127: if
(writeLengthInFlatFormat())
fwiw, I think our style is usually either to have this on one line:

if (writeLengthInFlatFormat()) intermediate.add(eachList.length);

or use curlies:

if (writeLengthInFlatFormat()) {
  intermediate.add(eachList.length);
}

(yeah ... it's 2012 and we're still worried about dangling else and indent+close
brace mismatches :\ )

https://chromiumcodereview.appspot.com/11553012/diff/1/pkg/serialization/lib/...
pkg/serialization/lib/src/serialization_rule.dart:145: bool
writeLengthInFlatFormat() => false;
make a getter?

https://chromiumcodereview.appspot.com/11553012/diff/1/pkg/serialization/lib/...
pkg/serialization/lib/src/serialization_rule.dart:156: var subList = new List();
personally I prefer [] form

https://chromiumcodereview.appspot.com/11553012/diff/1/pkg/serialization/lib/...
pkg/serialization/lib/src/serialization_rule.dart:186: int dataLength() => 0;
getter?

https://chromiumcodereview.appspot.com/11553012/diff/1/pkg/serialization/lib/...
pkg/serialization/lib/src/serialization_rule.dart:494: => (l is List) ? new
_LazyList(l, r) : new _LazyMap(l, r);
should this check for primitive types too?

https://chromiumcodereview.appspot.com/11553012/diff/1/pkg/serialization/lib/...
pkg/serialization/lib/src/serialization_rule.dart:506: Map raw;
make these private?

[Alan Knight, 2012-12-12 21:19:33]

https://codereview.chromium.org/11553012/diff/1/pkg/serialization/lib/seriali...
File pkg/serialization/lib/serialization.dart (right):

https://codereview.chromium.org/11553012/diff/1/pkg/serialization/lib/seriali...
pkg/serialization/lib/serialization.dart:198: _selfDescribing = !rules.some((x)
=> x is CustomRule);
On 2012/12/12 20:38:28, John Messerly wrote:
> Do we know that the set of "rules" is fixed at this point? In other words,
could
> add a CustomRule later but this property has cached a value of true?
> 
> Also, what happens if I set selfDescribing to true but I in fact do have a
> CustomRule? Does it work?
> 
> It seems potentially fragile to compute a value once and cache it if the
things
> used by the computation could change later. It would be good to invalidate
this
> property in addRule, or just don't cache it.

Yes, this was a cheesy attempt to make it default to the right thing, but allow
the user to override it. It _probably_ doesn't get called until all the rules
are added, but that's pretty fragile. I think not caching the value if we
compute it is the simplest thing. Then the user can still force it. In which
case the only way it's going to work right now is if the user adds named objects
for each such rule in the meta-serialization. Which they can't actually do right
now, but ought to be able to.

https://codereview.chromium.org/11553012/diff/1/pkg/serialization/lib/seriali...
pkg/serialization/lib/serialization.dart:280: void addRule(SerializationRule
rule) {
On 2012/12/12 20:38:28, John Messerly wrote:
> An issue I just noticed (maybe we already have a TODO) -- rules.add is
different
> from addRule. If someone added a rule with "rules.add" it wouldn't have the
> correct invariants (number property not set). This could be fixed in a few
ways:
> * not expose the raw list to the user
> * expose a smart list that knows how to maintain the invariants (we do
something
> similar for DOM node lists -- adding to them sets their parent property -- but
> it's more work)
> * don't set "rule.number" or check for CustomRules up front. Instead wait
until
> we start serialization, and then update the properties:

Made rules private. If it starts seem like people really need access will look
further at one of the other approaches.

https://codereview.chromium.org/11553012/diff/1/pkg/serialization/lib/src/rea...
File pkg/serialization/lib/src/reader_writer.dart (right):

https://codereview.chromium.org/11553012/diff/1/pkg/serialization/lib/src/rea...
pkg/serialization/lib/src/reader_writer.dart:272: return references[object];
On 2012/12/12 20:38:28, John Messerly wrote:
> totally optional, but this could be:
>   _referenceFor(object) => references[object];

Done.

https://codereview.chromium.org/11553012/diff/1/pkg/serialization/lib/src/ser...
File pkg/serialization/lib/src/serialization_rule.dart (right):

https://codereview.chromium.org/11553012/diff/1/pkg/serialization/lib/src/ser...
pkg/serialization/lib/src/serialization_rule.dart:127: if
(writeLengthInFlatFormat())
On 2012/12/12 20:38:28, John Messerly wrote:
> fwiw, I think our style is usually either to have this on one line:
> 
> if (writeLengthInFlatFormat()) intermediate.add(eachList.length);
> 
> or use curlies:
> 
> if (writeLengthInFlatFormat()) {
>   intermediate.add(eachList.length);
> }
> 
> (yeah ... it's 2012 and we're still worried about dangling else and
indent+close
> brace mismatches :\ )

Done.

https://codereview.chromium.org/11553012/diff/1/pkg/serialization/lib/src/ser...
pkg/serialization/lib/src/serialization_rule.dart:145: bool
writeLengthInFlatFormat() => false;
On 2012/12/12 20:38:28, John Messerly wrote:
> make a getter?

Done.

https://codereview.chromium.org/11553012/diff/1/pkg/serialization/lib/src/ser...
pkg/serialization/lib/src/serialization_rule.dart:156: var subList = new List();
On 2012/12/12 20:38:28, John Messerly wrote:
> personally I prefer [] form

Done.

https://codereview.chromium.org/11553012/diff/1/pkg/serialization/lib/src/ser...
pkg/serialization/lib/src/serialization_rule.dart:186: int dataLength() => 0;
On 2012/12/12 20:38:28, John Messerly wrote:
> getter?

Done. Also renamed locals that were shadowing this. Shouldn't have mattered, but
potentially confusing.

https://codereview.chromium.org/11553012/diff/1/pkg/serialization/lib/src/ser...
pkg/serialization/lib/src/serialization_rule.dart:494: => (l is List) ? new
_LazyList(l, r) : new _LazyMap(l, r);
On 2012/12/12 20:38:28, John Messerly wrote:
> should this check for primitive types too?

It really expects this to be either a List or a Map, so checked for both cases
and throw an exception otherwise. Thought about whether it would be reasonable
to let the user return a completely arbitrary object, e.g. a primitive or a
Reference. But decided it was likely making the API more confusing without
adding much power.

https://codereview.chromium.org/11553012/diff/1/pkg/serialization/lib/src/ser...
pkg/serialization/lib/src/serialization_rule.dart:506: Map raw;
On 2012/12/12 20:38:28, John Messerly wrote:
> make these private?

Done.

[Jennifer Messerly, 2012-12-12 21:26:46]

lgtm

[Alan Knight, 2012-12-14 22:16:41]

This was committed as https://chromiumcodereview.appspot.com/11578037/