Add wrapper container for a vector of OwnPtr<T>

Add wrapper container for a vector of OwnPtr<T>

In cc we frequently use WTF::Vector<WTF::OwnPtr<T> > to store lists of single-ownership
objects. In WTF this works nicely using template specialization in the container
implementation. In chromium we use STL containers and can't modify the implementation,
so this adds a wrapper to do roughly the same thing.

BUG=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157699

[jamesr, 2012-09-17 23:38:01]

I've converted some of the animation stuff over just to sanity check.  What do
you think of this direction?  I think I'll try to port CCLayerImpl::m_children
and CCRenderPass::m_sharedQuadStateList next - they will be a lot more typing.

CCQuadList inherits from Vector<OwnPtr<CCDrawQuad> > so that cleanup will be a
bit trickier but probably not too terrible.

[enne (OOO), 2012-09-17 23:45:43]

lgtm

[jamesr, 2012-09-18 01:16:59]

This converts everything in cc away from Vector<OwnPtr<T>>.  The differences
between cc::OwnPtrVector and ScopedVector from base are:

1.) cc::OwnPtrVector<T> only allows appending entries and the append takes a
PassOwnPtr<T>
2.) cc::OwnPtrVector<T> only allows removing objects via the take() function
which returns a PassOwnPtr<T>. This means if nobody adoptPtr()s an item out of
the vector, which shouldn't ever happen, ownership is unambiguous.

It's possible when we move away from OwnPtr that we'll lose both of these
benefits and should just use ScopedVector, or perhaps they'll still be helpful
with some smart use of the scoped_ptr movability.  I'm not sure yet which it'll
be.

[enne (OOO), 2012-09-18 16:59:05]

https://codereview.chromium.org/10940002/diff/8001/cc/TreeSynchronizer.cpp
File cc/TreeSynchronizer.cpp (right):

https://codereview.chromium.org/10940002/diff/8001/cc/TreeSynchronizer.cpp#ne...
cc/TreeSynchronizer.cpp:41: collectExistingCCLayerImplRecursive(oldLayers,
children.take(i));
This looks like a bug.  std::vector::erase removes and shifts everything down,
so element 1 becomes element 0, but that element will get skipped because the
loop index will increment.  I think you just want to keep the release here.

[jamesr, 2012-09-18 20:49:35]

https://codereview.chromium.org/10940002/diff/8001/cc/TreeSynchronizer.cpp
File cc/TreeSynchronizer.cpp (right):

https://codereview.chromium.org/10940002/diff/8001/cc/TreeSynchronizer.cpp#ne...
cc/TreeSynchronizer.cpp:41: collectExistingCCLayerImplRecursive(oldLayers,
children.take(i));
On 2012/09/18 16:59:05, enne wrote:
> This looks like a bug.  std::vector::erase removes and shifts everything down,
> so element 1 becomes element 0, but that element will get skipped because the
> loop index will increment.  I think you just want to keep the release here.

Whoops - I think you're right. So how do the tests pass...

[enne (OOO), 2012-09-18 21:51:18]

https://codereview.chromium.org/10940002/diff/8001/cc/TreeSynchronizer.cpp
File cc/TreeSynchronizer.cpp (right):

https://codereview.chromium.org/10940002/diff/8001/cc/TreeSynchronizer.cpp#ne...
cc/TreeSynchronizer.cpp:41: collectExistingCCLayerImplRecursive(oldLayers,
children.take(i));
On 2012/09/18 20:49:35, jamesr wrote:
> On 2012/09/18 16:59:05, enne wrote:
> > This looks like a bug.  std::vector::erase removes and shifts everything
down,
> > so element 1 becomes element 0, but that element will get skipped because
the
> > loop index will increment.  I think you just want to keep the release here.
> 
> Whoops - I think you're right. So how do the tests pass...

The tests pass because tree sync recreates new layers for all the dropped ones.
The only time it would cause tests to fail would be for cases where the main
thread layer expects the compositor thread layer to hold onto resources for it,
e.g. animations.

[tfarina, 2012-09-19 03:34:57]

https://chromiumcodereview.appspot.com/10940002/diff/8001/cc/own_ptr_vector.h
File cc/own_ptr_vector.h (right):

https://chromiumcodereview.appspot.com/10940002/diff/8001/cc/own_ptr_vector.h...
cc/own_ptr_vector.h:5: #ifndef CC_OWN_PTR_VECTOR_H
CC_OWN_PTR_VECTOR_H_

https://chromiumcodereview.appspot.com/10940002/diff/8001/cc/own_ptr_vector.h...
cc/own_ptr_vector.h:18: class OwnPtrVector {
very well written api! ;)

https://chromiumcodereview.appspot.com/10940002/diff/8001/cc/own_ptr_vector.h...
cc/own_ptr_vector.h:65: ASSERT(index < size());
can you use base/logging.h and this become:
DCHECK_LT(index, size());

?

https://chromiumcodereview.appspot.com/10940002/diff/8001/cc/own_ptr_vector.h...
cc/own_ptr_vector.h:71: STLDeleteContainerPointers(data_.begin(), data_.end());
This can be just:

STLDeleteElements(&data_);

https://chromiumcodereview.appspot.com/10940002/diff/8001/cc/own_ptr_vector.h...
cc/own_ptr_vector.h:100: }
// namespace cc

https://chromiumcodereview.appspot.com/10940002/diff/8001/cc/own_ptr_vector.h...
cc/own_ptr_vector.h:102: #endif  // CC_OWN_PTR_VECTOR_H
CC_OWN_PTR_VECTOR_H_

[jamesr, 2012-09-19 20:24:05]

Turns out TreeSynchronizerTest.syncSimpleTreeReusingLayers also catches the
reuse bug, but I was testing pre-r157425 so I didn't run it.  Oops!  I've
changed OwnPtrVector::take to do what the old code was doing (releasing in
place) to preserve that behavior and addressed tfarina's comments except for
DCHECK_LT, which can't easily be used here since base/logging.h and
wtf/Assertion.h both try to #define LOG

[enne (OOO), 2012-09-19 21:24:11]

LGTM.  Glad to hear the tests were actually failing.  :)

[commit-bot: I haz the power, 2012-09-20 00:49:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/10940002/13003

[commit-bot: I haz the power, 2012-09-20 03:34:04]

Change committed as 157699