cc: Make the ScopedPtrVector and ScopedPtrDeque containers act like STL vector and deque.

cc: Make the ScopedPtrVector and ScopedPtrDeque containers act like STL vector and deque.

These classes used methods to match the webcore classes and ease
migration. Now they are at odds with the STL versions of these containers.
Rename the methods, and change arguments from indexes to iterators, to
match the STL containers.

isEmpty() => empty()
last() => back()
first() => front()
Peek() => at()
append() => push_back()
insert(index, value) => insert(iterator, value)
remove(index) => erase(iterator) and erase(iterator, iterator)
take(index) => take(iterator)
takeFront() => take_front()
takeBack() => take_back()


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175722

[enne (OOO), 2012-11-21 02:03:18]

Were you planning to convert the other functions that don't match, e.g.

last => back
first => front
Peek => at
append => push_back

Even take/insert/remove are all convenience functions, but do make
ScopedPtrVector pretty divergent from just behaving like a vector<scoped_ptr<T>
>.

[danakj, 2012-11-21 02:03:55]

Oh, thanks, good idea.

[danakj, 2012-11-21 03:31:25]

On 2012/11/21 02:03:18, enne wrote:
> Were you planning to convert the other functions that don't match, e.g.
> 
> last => back
> first => front
> Peek => at
> append => push_back
> 
> Even take/insert/remove are all convenience functions, but do make
> ScopedPtrVector pretty divergent from just behaving like a
vector<scoped_ptr<T>
> >.

Done. PTAL

[enne (OOO), 2012-11-21 04:12:57]

https://codereview.chromium.org/11418108/diff/7020/cc/scoped_ptr_deque.h
File cc/scoped_ptr_deque.h (right):

https://codereview.chromium.org/11418108/diff/7020/cc/scoped_ptr_deque.h#newc...
cc/scoped_ptr_deque.h:57: scoped_ptr<T> take_front() {
I know take is convenient, but it's also inconsistent with non-scoped_ptr
containers.  What about changing take_front calls to front()/pop_front()?

https://codereview.chromium.org/11418108/diff/7020/cc/scoped_ptr_vector.h
File cc/scoped_ptr_vector.h (right):

https://codereview.chromium.org/11418108/diff/7020/cc/scoped_ptr_vector.h#new...
cc/scoped_ptr_vector.h:57: scoped_ptr<T> take(iterator position) {
Same question here.

[danakj, 2012-11-21 04:23:21]

https://codereview.chromium.org/11418108/diff/7020/cc/scoped_ptr_deque.h
File cc/scoped_ptr_deque.h (right):

https://codereview.chromium.org/11418108/diff/7020/cc/scoped_ptr_deque.h#newc...
cc/scoped_ptr_deque.h:57: scoped_ptr<T> take_front() {
On 2012/11/21 04:12:57, enne wrote:
> I know take is convenient, but it's also inconsistent with non-scoped_ptr
> containers.  What about changing take_front calls to front()/pop_front()?

Oh, take_front is removing the element so it could make sense to rename it
here..

https://codereview.chromium.org/11418108/diff/7020/cc/scoped_ptr_vector.h
File cc/scoped_ptr_vector.h (right):

https://codereview.chromium.org/11418108/diff/7020/cc/scoped_ptr_vector.h#new...
cc/scoped_ptr_vector.h:57: scoped_ptr<T> take(iterator position) {
On 2012/11/21 04:12:57, enne wrote:
> Same question here.

But here, take is different than erase. It removes the pointer from the array
and leaves behind a NULL.

Do you want take() to instead delete the element from the vector? This will then
differ from the hash_map implementation, which leaves behind a null.

Or, should it also remove from the hash_map?

I feel like there was a reason why we let take() leave the element in the
container with a NULL. But looking at the callsites, it's not obvious to me.

So, I guess I will try do that.

[enne (OOO), 2012-11-21 16:45:02]

I was having this line of reasoning where cc's scoped_ptr_foo containers should
be made to be exactly like their underlying STL container APIs, rather than a
(useful, but weird) WebKit compatibility container shim.  "take" is a
WebKit-ism, and not something that STL containers support, which is why I was
suggesting we remove the take functions.

For things like vectors where you have direct access, I think you could just do
a scoped_ptr::swap and add just one line of code.  So, it doesn't really add
much value.

For things like hash maps, the equivalent of take is sadly at least 7-8 lines of
code.  If we're going to have any non-standard bits on cc/ scoped_ptr
containers, that's arguably the only useful one.

[danakj, 2013-01-07 16:42:01]

On 2012/11/21 16:45:02, enne wrote:
> I was having this line of reasoning where cc's scoped_ptr_foo containers
should
> be made to be exactly like their underlying STL container APIs, rather than a
> (useful, but weird) WebKit compatibility container shim.  "take" is a
> WebKit-ism, and not something that STL containers support, which is why I was
> suggesting we remove the take functions.
> 
> For things like vectors where you have direct access, I think you could just
do
> a scoped_ptr::swap and add just one line of code.  So, it doesn't really add
> much value.

I *think* you're saying we could do:

ScopedPtrVector<Foo> vector;
vector.insert(vector.begin(), scopedPtr.Pass());
vector[0].swap(scopedPtr2);

Is that right?

We don't actually store a scoped_ptr in the vector tho, so [0] would give the
raw pointer, and there's no internal scoped_ptr to swap() with. Or, is it
something else you mean?

> For things like hash maps, the equivalent of take is sadly at least 7-8 lines
of
> code.  If we're going to have any non-standard bits on cc/ scoped_ptr
> containers, that's arguably the only useful one.

[danakj, 2013-01-07 17:53:45]

+piman and cdn for the trivial cc_messages change

[danakj, 2013-01-07 23:03:12]

PTAL. This is updated against HEAD, and I didn't remove any take() methods as
per previous question.

[enne (OOO), 2013-01-07 23:17:24]

lgtm

[danakj, 2013-01-07 23:24:09]

thanks :)

[commit-bot: I haz the power, 2013-01-07 23:27:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11418108/32001

[commit-bot: I haz the power, 2013-01-08 00:38:24]

Retried try job too often on linux_rel for step(s) content_browsertests

[commit-bot: I haz the power, 2013-01-08 00:59:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11418108/32001

[piman, 2013-01-08 02:03:41]

lgtm

[commit-bot: I haz the power, 2013-01-08 02:07:21]

Retried try job too often on linux_rel for step(s) content_browsertests

[commit-bot: I haz the power, 2013-01-08 03:07:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11418108/32001

[danakj, 2013-01-08 21:29:38]

Ok, added cc::remove_if() and banned assignment to iterators (*it = foo) for
ScopedPtrVector and ScopedPtrDeque so we don't blow up the world by mistake.

[Cris Neckar, 2013-01-08 22:51:06]

IPC security review LGTM

[danakj, 2013-01-08 22:52:10]

Apparently on android, iterators are not classes, so we can't block assignment
to them for scoped_ptr containers :(

http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...

So I've guarded it on OS_ANDROID. And hopefully we don't do anything stupid with
the scoped_ptr containers inside OS_ANDROID guards in the future (yay for only
using such guards for changing LayerTreeSettings).

[commit-bot: I haz the power, 2013-01-09 00:01:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11418108/54001

[commit-bot: I haz the power, 2013-01-09 03:10:02]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-09 03:12:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11418108/54001

[commit-bot: I haz the power, 2013-01-09 03:13:32]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-01-09 03:14:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11418108/54001

[commit-bot: I haz the power, 2013-01-09 05:02:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11418108/54001

[commit-bot: I haz the power, 2013-01-09 07:22:46]

Change committed as 175722