Store more behavior of SkFlatDictionary in SkFlatController.

Store more behavior of SkFlatDictionary in SkFlatController.

Code refactoring for simplicity.

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

[reed1, 2012-07-18 18:01:33]

just an idea. what if we replace virtual getters with inline non-virtuals, and
have the baseclass store pointers that subclass can set as they please?

baseclass {
public:
    Foo* getFoo() const { return fFoo; }
protected:
    Foo*  fFoo;
};

baseclass::baseclass() : fFoo(NULL) {}

subclass::subclass() {
    fFoo = &fPrivateFooObj;
}

[DerekS, 2012-07-20 20:50:42]

I like Mike's idea too, but other than that and my naming request it lgtm.

http://codereview.appspot.com/6427046/diff/1/src/core/SkPictureFlat.h
File src/core/SkPictureFlat.h (right):

http://codereview.appspot.com/6427046/diff/1/src/core/SkPictureFlat.h#newcode183
src/core/SkPictureFlat.h:183: virtual SkRefCntSet* getRefSet() { return NULL; }
can we change the name to make it explicit that this is only used for pixel
references?

[Leon, 2012-07-23 19:27:58]

I have turned the virtual calls into non-virtual calls.

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

https://codereview.appspot.com/6427046/diff/1/src/core/SkPictureFlat.h#newcod...
src/core/SkPictureFlat.h:183: virtual SkRefCntSet* getRefSet() { return NULL; }
On 2012/07/20 20:50:43, DerekS wrote:
> can we change the name to make it explicit that this is only used for pixel
> references? 

Done.

https://codereview.appspot.com/6427046/diff/4001/src/core/SkPicturePlayback.cpp
File src/core/SkPicturePlayback.cpp (right):

https://codereview.appspot.com/6427046/diff/4001/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:76: record.fHeap.setupPlaybacks();
I was hoping that I could avoid this call, but I cannot see how to do that
without making getTypefacePlayback and getRefCntPlayback virtual.

What do you guys think is best here?
- use virtual, so the subclass can ensure they are reset properly
- the way it is in this changelist (where unflattenDictionary does not work
unless setupPlaybacks is called on the SkChunkAllocator)
- the original way (pass in the playbacks)
- some other method I haven't thought of

[DerekS, 2012-07-24 03:41:50]

http://codereview.appspot.com/6427046/diff/4001/src/core/SkPicturePlayback.cpp
File src/core/SkPicturePlayback.cpp (right):

http://codereview.appspot.com/6427046/diff/4001/src/core/SkPicturePlayback.cp...
src/core/SkPicturePlayback.cpp:76: record.fHeap.setupPlaybacks();
The way you've coded it is probably the way I would have done it as I don't see
any better solution at the moment.

On 2012/07/23 19:27:59, scroggo-work wrote:
> I was hoping that I could avoid this call, but I cannot see how to do that
> without making getTypefacePlayback and getRefCntPlayback virtual.
> 
> What do you guys think is best here?
> - use virtual, so the subclass can ensure they are reset properly
> - the way it is in this changelist (where unflattenDictionary does not work
> unless setupPlaybacks is called on the SkChunkAllocator)
> - the original way (pass in the playbacks)
> - some other method I haven't thought of

http://codereview.appspot.com/6427046/diff/4001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

http://codereview.appspot.com/6427046/diff/4001/src/pipe/SkGPipeWrite.cpp#new...
src/pipe/SkGPipeWrite.cpp:67: this->setFactorySet(fFactorySet);
why not just have the parent do the safeRef in the setter and safeUnref in the
parent destructor? It seems cleaner and only avoids 3-4 extra null checks in the
parent destructor.

[Leon, 2012-07-24 15:08:53]

New patch.

http://codereview.appspot.com/6427046/diff/4001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

http://codereview.appspot.com/6427046/diff/4001/src/pipe/SkGPipeWrite.cpp#new...
src/pipe/SkGPipeWrite.cpp:67: this->setFactorySet(fFactorySet);
On 2012/07/24 03:41:50, DerekS wrote:
> why not just have the parent do the safeRef in the setter and safeUnref in the
> parent destructor? It seems cleaner and only avoids 3-4 extra null checks in
the
> parent destructor.

I agree that that is cleaner. Looking at this again also makes me realize that
FlattenableHeap does not need to hold on to the SkFactorySet.

I think the reason I did not make the SkFlatController ref the SkFactorySet is
because I do not make the SkFlatController ref the other objects (SkRefCntSets,
Playbacks) so that they can be owned by the subclass.

That said, if we think it is better overall, I can do it that way.

[DerekS, 2012-07-24 15:43:50]

http://codereview.appspot.com/6427046/diff/8002/src/core/SkPictureFlat.cpp
File src/core/SkPictureFlat.cpp (right):

http://codereview.appspot.com/6427046/diff/8002/src/core/SkPictureFlat.cpp#ne...
src/core/SkPictureFlat.cpp:71: SkSafeUnref(fFactorySet);
my thought was that we would safe unref all of them here and do the ref when we
call the setters for all of them.  Is there some reason we want the subclass to
own the objects?

[Leon, 2012-07-24 15:55:47]

http://codereview.appspot.com/6427046/diff/8002/src/core/SkPictureFlat.cpp
File src/core/SkPictureFlat.cpp (right):

http://codereview.appspot.com/6427046/diff/8002/src/core/SkPictureFlat.cpp#ne...
src/core/SkPictureFlat.cpp:71: SkSafeUnref(fFactorySet);
On 2012/07/24 15:43:50, DerekS wrote:
> my thought was that we would safe unref all of them here and do the ref when
we
> call the setters for all of them.  Is there some reason we want the subclass
to
> own the objects?

I guess I do not see a better place for them. In the case of the SkFactorySet,
SkGPipeCanvas has two objects of (a subclass of) SkFlatController, which need to
share the SkFactorySet, so it did not make sense for that subclass to own it.

For the SkPictureRecord, however, the SkRefCntSets are never used by
SkPictureRecord, so it made sense to me that they live in the
SkChunkFlatController.

[DerekS, 2012-07-24 21:06:12]

On 2012/07/24 15:55:47, scroggo-work wrote:
> http://codereview.appspot.com/6427046/diff/8002/src/core/SkPictureFlat.cpp
> File src/core/SkPictureFlat.cpp (right):
> 
>
http://codereview.appspot.com/6427046/diff/8002/src/core/SkPictureFlat.cpp#ne...
> src/core/SkPictureFlat.cpp:71: SkSafeUnref(fFactorySet);
> On 2012/07/24 15:43:50, DerekS wrote:
> > my thought was that we would safe unref all of them here and do the ref when
> we
> > call the setters for all of them.  Is there some reason we want the subclass
> to
> > own the objects?
> 
> I guess I do not see a better place for them. In the case of the SkFactorySet,
> SkGPipeCanvas has two objects of (a subclass of) SkFlatController, which need
to
> share the SkFactorySet, so it did not make sense for that subclass to own it.
> 
> For the SkPictureRecord, however, the SkRefCntSets are never used by
> SkPictureRecord, so it made sense to me that they live in the
> SkChunkFlatController.

I'm probably just missing something obvious, but perhaps we can chat about it in
person tomorrow to clear things up.  However if it is blocking you it lgtm
otherwise so if needed you can go ahead and commit.

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

On 2012/07/24 21:06:12, DerekS wrote:
> On 2012/07/24 15:55:47, scroggo-work wrote:
> > http://codereview.appspot.com/6427046/diff/8002/src/core/SkPictureFlat.cpp
> > File src/core/SkPictureFlat.cpp (right):
> > 
> >
>
http://codereview.appspot.com/6427046/diff/8002/src/core/SkPictureFlat.cpp#ne...
> > src/core/SkPictureFlat.cpp:71: SkSafeUnref(fFactorySet);
> > On 2012/07/24 15:43:50, DerekS wrote:
> > > my thought was that we would safe unref all of them here and do the ref
when
> > we
> > > call the setters for all of them.  Is there some reason we want the
subclass
> > to
> > > own the objects?
> > 
> > I guess I do not see a better place for them. In the case of the
SkFactorySet,
> > SkGPipeCanvas has two objects of (a subclass of) SkFlatController, which
need
> to
> > share the SkFactorySet, so it did not make sense for that subclass to own
it.
> > 
> > For the SkPictureRecord, however, the SkRefCntSets are never used by
> > SkPictureRecord, so it made sense to me that they live in the
> > SkChunkFlatController.
> 
> I'm probably just missing something obvious, but perhaps we can chat about it
in
> person tomorrow to clear things up.  However if it is blocking you it lgtm
> otherwise so if needed you can go ahead and commit.

Not blocking me at all. I am happy to discuss tomorrow.

[Leon, 2012-08-02 16:19:08]

New patch.