change SkAnnotation to not inherit from SkFlattenable (does not need dynamic factories)

change SkAnnotation to not inherit from SkFlattenable (does not need dynamic factories)

BUG=
R=scroggo@google.com

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

[reed1, 2013-10-14 18:32:41]

patch #2 makes empty-annotations correctly write/read a 0 for length

[scroggo, 2013-10-14 18:47:19]

https://codereview.chromium.org/26606004/diff/4001/include/core/SkAnnotation.h
File include/core/SkAnnotation.h (right):

https://codereview.chromium.org/26606004/diff/4001/include/core/SkAnnotation....
include/core/SkAnnotation.h:11: #include "SkFlattenable.h"
Can this be removed?

https://codereview.chromium.org/26606004/diff/4001/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

https://codereview.chromium.org/26606004/diff/4001/src/pipe/SkGPipeRead.cpp#n...
src/pipe/SkGPipeRead.cpp:677: size_t size = reader->readInt();
Should this be cast from int32_t to size_t?

Also, size could be const.

https://codereview.chromium.org/26606004/diff/4001/src/pipe/SkGPipeRead.cpp#n...
src/pipe/SkGPipeRead.cpp:681: p->setAnnotation(SkNEW_ARGS(SkAnnotation,
(buffer)))->unref();
If the buffer has size 0, won't SkAnnotation's constructor fail when it tries to
read from the buffer?

I think you'll want to call setAnnotation(NULL) if the size was zero.

https://codereview.chromium.org/26606004/diff/4001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.chromium.org/26606004/diff/4001/src/pipe/SkGPipeWrite.cpp#...
src/pipe/SkGPipeWrite.cpp:1132: fWriter.write32(0); // we have no data
0 was already written by writeOp. Why write it again?

https://codereview.chromium.org/26606004/diff/4001/src/pipe/SkGPipeWrite.cpp#...
src/pipe/SkGPipeWrite.cpp:1141: this->writeOp(kSetAnnotation_DrawOp, 0, size);
Do you think Annotations might be reused (but not immediately) and may be large?

The old code stored an Annotation, and it could get reused without flattening it
again. Now we'll write it each time we see it (and we changed paints). So for
the following case:

draw(paint with annotation)
draw(some other paint)
draw(paint with same annotation)

we will have written the annotation twice.

Probably not an issue (does anyone use the annotation?), but just wanted to make
sure it was considered.

https://codereview.chromium.org/26606004/diff/4001/src/pipe/SkGPipeWrite.cpp#...
src/pipe/SkGPipeWrite.cpp:1142: fWriter.write32(size);
As in the NULL annotation case, size was written by writeOp. Why write it again?
Are you concerned that it might be too big to fit in DRAWOPS_DATA_BITS (20)?

drawData handles that by checking if it's too big, and if so, it puts 0 in the
writeOp call and then writes the size. Not sure the extra complication is worth
saving the extra 4 bytes, but it does seem like the two should be similar.

Either way it seems odd to me to write the same number twice.

[reed1, 2013-10-14 19:38:41]

https://codereview.chromium.org/26606004/diff/4001/include/core/SkAnnotation.h
File include/core/SkAnnotation.h (right):

https://codereview.chromium.org/26606004/diff/4001/include/core/SkAnnotation....
include/core/SkAnnotation.h:11: #include "SkFlattenable.h"
On 2013/10/14 18:47:19, scroggo wrote:
> Can this be removed?

Done.

https://codereview.chromium.org/26606004/diff/4001/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

https://codereview.chromium.org/26606004/diff/4001/src/pipe/SkGPipeRead.cpp#n...
src/pipe/SkGPipeRead.cpp:677: size_t size = reader->readInt();
On 2013/10/14 18:47:19, scroggo wrote:
> Should this be cast from int32_t to size_t?
> 
> Also, size could be const. 

const -- done
changed readInt to readU32, to match other call-sites that assign to size_t

https://codereview.chromium.org/26606004/diff/4001/src/pipe/SkGPipeRead.cpp#n...
src/pipe/SkGPipeRead.cpp:681: p->setAnnotation(SkNEW_ARGS(SkAnnotation,
(buffer)))->unref();
On 2013/10/14 18:47:19, scroggo wrote:
> If the buffer has size 0, won't SkAnnotation's constructor fail when it tries
to
> read from the buffer?
> 
> I think you'll want to call setAnnotation(NULL) if the size was zero.

Done.

https://codereview.chromium.org/26606004/diff/4001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.chromium.org/26606004/diff/4001/src/pipe/SkGPipeWrite.cpp#...
src/pipe/SkGPipeWrite.cpp:1132: fWriter.write32(0); // we have no data
On 2013/10/14 18:47:19, scroggo wrote:
> 0 was already written by writeOp. Why write it again?

Done.

https://codereview.chromium.org/26606004/diff/4001/src/pipe/SkGPipeWrite.cpp#...
src/pipe/SkGPipeWrite.cpp:1141: this->writeOp(kSetAnnotation_DrawOp, 0, size);
On 2013/10/14 18:47:19, scroggo wrote:
> Do you think Annotations might be reused (but not immediately) and may be
large?
> 
> The old code stored an Annotation, and it could get reused without flattening
it
> again. Now we'll write it each time we see it (and we changed paints). So for
> the following case:
> 
> draw(paint with annotation)
> draw(some other paint)
> draw(paint with same annotation)
> 
> we will have written the annotation twice.
> 
> Probably not an issue (does anyone use the annotation?), but just wanted to
make
> sure it was considered.

It was considered. I think annotations are so unbelievably rare right now, that
any complexity around caching previously seen values was not worth it.

https://codereview.chromium.org/26606004/diff/4001/src/pipe/SkGPipeWrite.cpp#...
src/pipe/SkGPipeWrite.cpp:1142: fWriter.write32(size);
On 2013/10/14 18:47:19, scroggo wrote:
> As in the NULL annotation case, size was written by writeOp. Why write it
again?
> Are you concerned that it might be too big to fit in DRAWOPS_DATA_BITS (20)?
> 
> drawData handles that by checking if it's too big, and if so, it puts 0 in the
> writeOp call and then writes the size. Not sure the extra complication is
worth
> saving the extra 4 bytes, but it does seem like the two should be similar.
> 
> Either way it seems odd to me to write the same number twice.

Done.

[scroggo, 2013-10-14 19:47:27]

lgtm, so long as it doesn't throw a warning for treating unsigned as a bool.

https://codereview.chromium.org/26606004/diff/13001/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

https://codereview.chromium.org/26606004/diff/13001/src/pipe/SkGPipeRead.cpp#...
src/pipe/SkGPipeRead.cpp:677: if (PaintOp_unpackData(op32)) {
Will this fire a warning for turning unsigned to bool?

If so, SkToBool may be in order.

[reed1, 2013-10-14 19:55:44]

https://codereview.chromium.org/26606004/diff/13001/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

https://codereview.chromium.org/26606004/diff/13001/src/pipe/SkGPipeRead.cpp#...
src/pipe/SkGPipeRead.cpp:677: if (PaintOp_unpackData(op32)) {
On 2013/10/14 19:47:27, scroggo wrote:
> Will this fire a warning for turning unsigned to bool?
> 
> If so, SkToBool may be in order.

Done.

[scroggo, 2013-10-14 19:59:11]

On 2013/10/14 19:55:44, reed1 wrote:
> https://codereview.chromium.org/26606004/diff/13001/src/pipe/SkGPipeRead.cpp
> File src/pipe/SkGPipeRead.cpp (right):
> 
>
https://codereview.chromium.org/26606004/diff/13001/src/pipe/SkGPipeRead.cpp#...
> src/pipe/SkGPipeRead.cpp:677: if (PaintOp_unpackData(op32)) {
> On 2013/10/14 19:47:27, scroggo wrote:
> > Will this fire a warning for turning unsigned to bool?
> > 
> > If so, SkToBool may be in order.
> 
> Done.

lgtm

[reed1, 2013-10-14 20:02:52]

Committed patchset #4 manually as r11762 (presubmit successful).