Pipe factory names independently from the flattenables using them.

Pipe factory names independently from the flattenables using them.

Avoids an issue where a flattenable written twice might be written
differently (the first time the flat data may have a name, whereas
the second time it will have an index).

Also add a test which confirms that identical flattenables will have
the same SkFlatData representation.

BUG=https://code.google.com/p/skia/issues/detail?id=721
TEST=FlatDataTest.cpp

Committed: https://code.google.com/p/skia/source/detail?r=4896

[Leon, 2012-07-23 19:04:02]

https://codereview.appspot.com/6431057/diff/2001/src/core/SkPictureFlat.h
File src/core/SkPictureFlat.h (right):

https://codereview.appspot.com/6431057/diff/2001/src/core/SkPictureFlat.h#new...
src/core/SkPictureFlat.h:194: SkRefCnt_SafeAssign(fFactoryNames, rec);
Making my test case made me realize that ref counting this is a problem if it is
owned by the subclass. Not a problem in my case, but I'll probably remove this
behavior to be consistent with https://codereview.appspot.com/6427046/ where I
do want the subclass to own the (SkRefCntSet, SkFactorySet, etc).

[DerekS, 2012-07-24 03:31:19]

I'm not opposed to the CL, but staring at the code does make me wonder if there
isn't a better way.  It seems like the obvious thing to do would be to always
store the name, but there is a small amount of overhead involved for both the
sender (storing a string instead of a number) and the receiver (decoding the
string to a factory ptr).

Is all this complexity worth it? If this is the right way to go how can we
better document this behavior?

http://codereview.appspot.com/6431057/diff/2001/src/core/SkOrderedWriteBuffer...
File src/core/SkOrderedWriteBuffer.cpp (right):

http://codereview.appspot.com/6431057/diff/2001/src/core/SkOrderedWriteBuffer...
src/core/SkOrderedWriteBuffer.cpp:27: *      >0: the length of the name
does this comment need amended to account for this change?

http://codereview.appspot.com/6431057/diff/2001/src/core/SkOrderedWriteBuffer...
src/core/SkOrderedWriteBuffer.cpp:71: index = fFactorySet->add(factory);
should we add an assert here that checks to make sure that fFactoryNames and
fFactorySet are the same size?

[Leon, 2012-07-24 21:21:39]

I agree that always flattening the name would be much simpler. I could write a
test to determine if there is a speed tradeoff.

I'll think about it some more, and perhaps investigate always storing the names,
but hopefully my inline comment clarifies things a little bit in the meantime.

http://codereview.appspot.com/6431057/diff/2001/src/core/SkOrderedWriteBuffer...
File src/core/SkOrderedWriteBuffer.cpp (right):

http://codereview.appspot.com/6431057/diff/2001/src/core/SkOrderedWriteBuffer...
src/core/SkOrderedWriteBuffer.cpp:27: *      >0: the length of the name
On 2012/07/24 03:31:19, DerekS wrote:
> does this comment need amended to account for this change?

Done.

http://codereview.appspot.com/6431057/diff/2001/src/core/SkOrderedWriteBuffer...
src/core/SkOrderedWriteBuffer.cpp:71: index = fFactorySet->add(factory);
On 2012/07/24 03:31:19, DerekS wrote:
> should we add an assert here that checks to make sure that fFactoryNames and
> fFactorySet are the same size?

Actually, they are not necessarily the same size. After each call to
SkFlatDictionary::find (which potentially calls writeFlattenable multiple
times), I write each new name to the stream before writing the new Flattenable
(since the reader will not be able to read it until it knows the name).

We keep the factory set around so that multiple uses of the same factory use the
same index. The name, however, only gets used once. It IS important that the two
are in the same order, i.e.

fFactorySet: 1...x...N
fFactoryNames: x...N

so that when the reader adds the corresponding factory to its fFactoryArray it
will have the same order as the writer's fFactorySet. Once the name is written
to the stream, I remove it from the list of names since it is no longer needed.

I could leave the name in fFactoryNames (in which case we could assert that
fFactoryNames and fFactorySet are the same size), but then I need to distinguish
between which names are new and which are old.

[Leon, 2012-07-31 22:48:18]

I have combined the name recorder and factory set into the same object (for my
use, while leaving the option of just using a factory set). Let me know if this
is cleaner.

[DerekS, 2012-08-01 13:08:01]

This code does look much cleaner! Thanks for working on this.

http://codereview.appspot.com/6431057/diff/16001/tests/FlatDataTest.cpp
File tests/FlatDataTest.cpp (right):

http://codereview.appspot.com/6431057/diff/16001/tests/FlatDataTest.cpp#newco...
tests/FlatDataTest.cpp:38: static void Tests(skiatest::Reporter* reporter) {
I don't see how this test exercises the SkNamedFactorySet?  Can you add some
comments describing the behavior that we are testing?

[Leon, 2012-08-01 14:47:18]

http://codereview.appspot.com/6431057/diff/16001/tests/FlatDataTest.cpp
File tests/FlatDataTest.cpp (right):

http://codereview.appspot.com/6431057/diff/16001/tests/FlatDataTest.cpp#newco...
tests/FlatDataTest.cpp:38: static void Tests(skiatest::Reporter* reporter) {
On 2012/08/01 13:08:01, DerekS wrote:
> I don't see how this test exercises the SkNamedFactorySet?  Can you add some
> comments describing the behavior that we are testing?

Done. Let me know if it's still confusing.

[DerekS, 2012-08-01 16:00:13]

lgtm.  I have a CL that moves some of the flattening code around that I'll
publish shortly so we'll need to coordinate when these CL's go in so we can
merge the changes.