Change ScopedPtrHashMap's 2nd template parameter

Change ScopedPtrHashMap's 2nd template parameter

Now ScopedPtrHashMap expect the 2nd parameter is scoped_ptr<T>. For example, old usage
ScopedPtrHashMap<int, Value>
new usage
ScopedPtrHashMap<int, scoped_ptr<Value>>

With this change, ScopedPtrHashMap support scoped_ptr's custom deleter.

R=danakj@chromium.org, tzik@chromium.org
BUG=none

Committed: https://crrev.com/213a4dbd5994d5aad53acb08e8960804bbffbc07
Cr-Commit-Position: refs/heads/master@{#327341}

[danakj, 2015-04-23 20:36:22]

https://codereview.chromium.org/1099383002/diff/20001/base/containers/scoped_...
File base/containers/scoped_ptr_hash_map.h (right):

https://codereview.chromium.org/1099383002/diff/20001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map.h:19: // This type acts like a hash_map<K,
scoped_ptr<V, D> >, based on top of
I don't see this Deleter thing for hash_map, nor for unordered_map which we will
replace this class with in the future.

I don't think we should diverge from unordered_map here, we'll have to undo it
in the future.

[danakj, 2015-04-23 20:37:10]

https://codereview.chromium.org/1099383002/diff/20001/base/containers/scoped_...
File base/containers/scoped_ptr_hash_map.h (right):

https://codereview.chromium.org/1099383002/diff/20001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map.h:19: // This type acts like a hash_map<K,
scoped_ptr<V, D> >, based on top of
On 2015/04/23 20:36:22, danakj wrote:
> I don't see this Deleter thing for hash_map, nor for unordered_map which we
will
> replace this class with in the future.
> 
> I don't think we should diverge from unordered_map here, we'll have to undo it
> in the future.

Oh, never mind, I see this is an argument to scoped_ptr instead.

[danakj, 2015-04-23 20:54:51]

https://codereview.chromium.org/1099383002/diff/20001/base/containers/scoped_...
File base/containers/scoped_ptr_hash_map.h (right):

https://codereview.chromium.org/1099383002/diff/20001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map.h:23: typename Value,
Maybe we can take this opportunity to make this more like unordered_map though,
changing the Value to be a scoped_ptr<T,D> instead of a T.

So callers would change to:

ScopedPtrHashMap<key, scoped_ptr<value>> map;

We can pull out the type inside the scoped_ptr<value> with scoped_ptr::T for the
Value type inside this class. And we can use scoped_ptr::D for deleting.

[kcwu, 2015-04-24 06:30:29]

https://codereview.chromium.org/1099383002/diff/20001/base/containers/scoped_...
File base/containers/scoped_ptr_hash_map.h (right):

https://codereview.chromium.org/1099383002/diff/20001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map.h:19: // This type acts like a hash_map<K,
scoped_ptr<V, D> >, based on top of
On 2015/04/23 20:37:10, danakj wrote:
> On 2015/04/23 20:36:22, danakj wrote:
> > I don't see this Deleter thing for hash_map, nor for unordered_map which we
> will
> > replace this class with in the future.
> > 
> > I don't think we should diverge from unordered_map here, we'll have to undo
it
> > in the future.
> 
> Oh, never mind, I see this is an argument to scoped_ptr instead. 

Acknowledged.

https://codereview.chromium.org/1099383002/diff/20001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map.h:23: typename Value,
On 2015/04/23 20:54:51, danakj wrote:
> Maybe we can take this opportunity to make this more like unordered_map
though,
> changing the Value to be a scoped_ptr<T,D> instead of a T.
> 
> So callers would change to:
> 
> ScopedPtrHashMap<key, scoped_ptr<value>> map;
> 
> We can pull out the type inside the scoped_ptr<value> with scoped_ptr::T for
the
> Value type inside this class. And we can use scoped_ptr::D for deleting.

Done.
Is my current change what you expected?

[kcwu, 2015-04-24 06:40:47]

The CQ bit was checked by kcwu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-24 06:41:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1099383002/40001

[commit-bot: I haz the power, 2015-04-24 06:41:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-24 06:41:11]

Dry run: No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[danakj, 2015-04-24 17:15:10]

Wow there's already a lot more use of this class than I thought. Cool!

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
File base/containers/scoped_ptr_hash_map.h (right):

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map.h:46: deleter_type()(it->second);
Rather than using the deleter manually here, can you just put the it->second
into a ScopedPtr and reset() it? (with comment explaining why)

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map.h:64: deleter_type()(it->second);
ditto

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map.h:131: deleter_type()(temp->second);
ditto

https://codereview.chromium.org/1099383002/diff/80001/cc/output/direct_render...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/1099383002/diff/80001/cc/output/direct_render...
cc/output/direct_renderer.cc:158: for (pass_iter =
render_pass_textures_.begin();
nit: we can just "auto pass_iter =" here now.

https://codereview.chromium.org/1099383002/diff/80001/cc/surfaces/surface_fac...
File cc/surfaces/surface_factory.h (right):

https://codereview.chromium.org/1099383002/diff/80001/cc/surfaces/surface_fac...
cc/surfaces/surface_factory.h:81: base::ScopedPtrHashMap<SurfaceId,
scoped_ptr<Surface>> surface_map_;
nit: would you mind changing this to OwningSurfaceMap here?

[danakj, 2015-04-24 17:22:04]

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
File base/containers/scoped_ptr_hash_map_unittest.cc (right):

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map_unittest.cc:11: 
can you wrap everything inside this namespace with an anonymous namespace too?

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map_unittest.cc:36: static int g_count;
g_deleter_call_count?

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map_unittest.cc:42: }  // namespace
this isn't closing an anonymous namespace. i think you mean "namespace base",
but can you wrap the tests in it too?

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map_unittest.cc:79: }
can you test erase() too?

[kcwu, 2015-04-27 13:28:03]

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
File base/containers/scoped_ptr_hash_map.h (right):

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map.h:46: deleter_type()(it->second);
On 2015/04/24 17:15:10, danakj wrote:
> Rather than using the deleter manually here, can you just put the it->second
> into a ScopedPtr and reset() it? (with comment explaining why)

Done.

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map.h:64: deleter_type()(it->second);
On 2015/04/24 17:15:10, danakj wrote:
> ditto

Done.

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map.h:131: deleter_type()(temp->second);
On 2015/04/24 17:15:10, danakj wrote:
> ditto

Done.

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
File base/containers/scoped_ptr_hash_map_unittest.cc (right):

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map_unittest.cc:11: 
On 2015/04/24 17:22:04, danakj wrote:
> can you wrap everything inside this namespace with an anonymous namespace too?

Done.
base::DefaultDeleter have to be specialized inside namespace base. Please let me
know how to make it look better if possible.

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map_unittest.cc:36: static int g_count;
On 2015/04/24 17:22:04, danakj wrote:
> g_deleter_call_count?

Done.

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map_unittest.cc:42: }  // namespace
On 2015/04/24 17:22:04, danakj wrote:
> this isn't closing an anonymous namespace. i think you mean "namespace base",
> but can you wrap the tests in it too?

Done.

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map_unittest.cc:79: }
On 2015/04/24 17:22:04, danakj wrote:
> can you test erase() too?

It is already tested in the second block.

https://codereview.chromium.org/1099383002/diff/80001/cc/output/direct_render...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/1099383002/diff/80001/cc/output/direct_render...
cc/output/direct_renderer.cc:158: for (pass_iter =
render_pass_textures_.begin();
On 2015/04/24 17:15:10, danakj wrote:
> nit: we can just "auto pass_iter =" here now.

Done.

https://codereview.chromium.org/1099383002/diff/80001/cc/surfaces/surface_fac...
File cc/surfaces/surface_factory.h (right):

https://codereview.chromium.org/1099383002/diff/80001/cc/surfaces/surface_fac...
cc/surfaces/surface_factory.h:81: base::ScopedPtrHashMap<SurfaceId,
scoped_ptr<Surface>> surface_map_;
On 2015/04/24 17:15:10, danakj wrote:
> nit: would you mind changing this to OwningSurfaceMap here?

Done.

[danakj, 2015-04-27 16:50:09]

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
File base/containers/scoped_ptr_hash_map_unittest.cc (right):

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map_unittest.cc:11: 
On 2015/04/27 13:28:03, kcwu wrote:
> On 2015/04/24 17:22:04, danakj wrote:
> > can you wrap everything inside this namespace with an anonymous namespace
too?
> 
> Done.
> base::DefaultDeleter have to be specialized inside namespace base. Please let
me
> know how to make it look better if possible.

I meant wrap it all like:

namespace base {
namespace {

.. all the tests ..

}
}

You can use a custom deleter on the scoped_ptr instead of DefaultDeleter though?

[kcwu, 2015-04-27 17:48:17]

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
File base/containers/scoped_ptr_hash_map_unittest.cc (right):

https://codereview.chromium.org/1099383002/diff/80001/base/containers/scoped_...
base/containers/scoped_ptr_hash_map_unittest.cc:11: 
On 2015/04/27 16:50:09, danakj wrote:
> On 2015/04/27 13:28:03, kcwu wrote:
> > On 2015/04/24 17:22:04, danakj wrote:
> > > can you wrap everything inside this namespace with an anonymous namespace
> too?
> > 
> > Done.
> > base::DefaultDeleter have to be specialized inside namespace base. Please
let
> me
> > know how to make it look better if possible.
> 
> I meant wrap it all like:
> 
> namespace base {
> namespace {
> 
> .. all the tests ..
> 
> }
> }
> 
> You can use a custom deleter on the scoped_ptr instead of DefaultDeleter
though?

Done.
That doesn't work for DefaultDeleter (compile failed). Changed to custom
deleter.

[danakj, 2015-04-27 17:52:01]

https://codereview.chromium.org/1099383002/diff/140001/base/containers/scoped...
File base/containers/scoped_ptr_hash_map_unittest.cc (right):

https://codereview.chromium.org/1099383002/diff/140001/base/containers/scoped...
base/containers/scoped_ptr_hash_map_unittest.cc:10: namespace {
sorry, but wrap this in namespace base { outside, so that you don't have to
base:: everything in the tests?

[kcwu, 2015-04-27 17:56:20]

https://codereview.chromium.org/1099383002/diff/140001/base/containers/scoped...
File base/containers/scoped_ptr_hash_map_unittest.cc (right):

https://codereview.chromium.org/1099383002/diff/140001/base/containers/scoped...
base/containers/scoped_ptr_hash_map_unittest.cc:10: namespace {
On 2015/04/27 17:52:00, danakj wrote:
> sorry, but wrap this in namespace base { outside, so that you don't have to
> base:: everything in the tests?

Done.

[danakj, 2015-04-27 18:07:11]

LGTM

[danakj, 2015-04-27 18:07:53]

Can you update the patch description to say what this is doing now?

And the TEST= line is for QA people. Don't need to list automated tests there.

[kcwu, 2015-04-27 18:16:49]

On 2015/04/27 18:07:53, danakj wrote:
> Can you update the patch description to say what this is doing now?
> 
> And the TEST= line is for QA people. Don't need to list automated tests there.

Done.

[kcwu, 2015-04-27 18:21:46]

On 2015/04/27 18:16:49, kcwu wrote:
> On 2015/04/27 18:07:53, danakj wrote:
> > Can you update the patch description to say what this is doing now?
> > 
> > And the TEST= line is for QA people. Don't need to list automated tests
there.
> 
> Done.

In this case, should I add owners of individual directory or root directory for
review? what's your suggestion?

[danakj, 2015-04-27 18:23:44]

I'd try get a reviewer for each of chrome/ content/ and components/. If you
choose well they can overlap much of the rest. Then TBR the remaining tiny
pieces.

[kcwu, 2015-04-27 19:05:58]

kcwu@chromium.org changed reviewers:
+ jochen@chromium.org, piman@chromium.org, rpaquay@chromium.org,
sadrul@chromium.org, scherkus@chromium.org, stanisc@chromium.org

[kcwu, 2015-04-27 19:05:58]

owners review, thanks.

jochen: chrome/ components/
piman: content/ ppapi/
scherkus: media/
sadrul: ui/
stanisc: sync/
rpaquay: device/bluetooth/bluetooth_device_win.cc

[jochen (gone - plz use gerrit), 2015-04-27 20:11:17]

lgtm

[rpaquay, 2015-04-27 20:20:27]

lgtm device/bluetooth/bluetooth_device_win.cc

[piman, 2015-04-27 23:41:06]

lgtm

[stanisc, 2015-04-27 23:44:02]

lgtm sync/

[tzik, 2015-04-28 05:01:22]

lgtm

[sadrul, 2015-04-28 05:05:18]

ui lgtm

[scherkus (not reviewing), 2015-04-28 17:26:18]

media/ lgtm

[kcwu, 2015-04-28 17:26:56]

The CQ bit was checked by kcwu@chromium.org

[commit-bot: I haz the power, 2015-04-28 17:27:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1099383002/160001

[commit-bot: I haz the power, 2015-04-28 17:32:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-28 17:32:05]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kcwu, 2015-04-28 17:50:44]

The CQ bit was checked by kcwu@chromium.org

[kcwu, 2015-04-28 17:50:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
stanisc@chromium.org, sadrul@chromium.org, tzik@chromium.org,
piman@chromium.org, danakj@chromium.org, rpaquay@chromium.org,
jochen@chromium.org, scherkus@chromium.org
Link to the patchset: https://codereview.chromium.org/1099383002/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2015-04-28 17:51:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1099383002/180001

[commit-bot: I haz the power, 2015-04-28 18:54:45]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-04-28 18:56:10]

Patchset 10 (id:??) landed as
https://crrev.com/213a4dbd5994d5aad53acb08e8960804bbffbc07
Cr-Commit-Position: refs/heads/master@{#327341}