Switch base::IDMap to use unique_ptr.

Switch base::IDMap to use unique_ptr.

This old base class is a thin wrapper around STL hashmap, providing A)
less easily invalidated iterators and B) a unique_ptr equivalent.  At
this point there is nothing stopping this from *actually* using
unique_ptr, so I switch it over to using that in its IDMapOwnPointer mode.

I made one semantic change, to Replace(), which used to "leak" the
return pointer.  I changed it to instead directly return a unique_ptr
when in that mode, and also to not support replacing an invalid ID.
Both of those changes suit the only user of Replace (in
ServiceWorkerContextCore).

BUG=647091
Committed: https://crrev.com/aed214d0ef7a3da3efa968dd6646e126efe91963
Cr-Commit-Position: refs/heads/master@{#420809}

[aelias_OOO_until_Jul13, 2016-09-15 00:59:47]

aelias@chromium.org changed reviewers:
+ danakj@chromium.org, michaeln@chromium.org

[aelias_OOO_until_Jul13, 2016-09-15 00:59:51]

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

[commit-bot: I haz the power, 2016-09-15 01:00:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 01:19:14]

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

[commit-bot: I haz the power, 2016-09-15 01:19:16]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[aelias_OOO_until_Jul13, 2016-09-15 02:03:25]

Hi, danakj@ for base/ and michaeln@ for service_worker/.

I ran into this last week and it bothered me a bit.  If this change looks good,
the next step would be to change Add() and AddWithID() to take V as parameter
(and fix ~100 callsites).

[danakj, 2016-09-15 20:08:40]

https://codereview.chromium.org/2340813003/diff/1/base/id_map.h
File base/id_map.h (right):

https://codereview.chromium.org/2340813003/diff/1/base/id_map.h#newcode46
base/id_map.h:46: typedef typename std::conditional<OS == IDMapExternalPointer,
can you change these both to "using" instead of typedef

https://codereview.chromium.org/2340813003/diff/1/base/id_map.h#newcode72
base/id_map.h:72: KeyType Add(T* data) {
Can this not take a V instead?

https://codereview.chromium.org/2340813003/diff/1/base/id_map.h#newcode86
base/id_map.h:86: void AddWithID(T* data, KeyType id) {
And this?

[aelias_OOO_until_Jul13, 2016-09-15 21:04:25]

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

[commit-bot: I haz the power, 2016-09-15 21:04:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aelias_OOO_until_Jul13, 2016-09-15 21:09:36]

https://codereview.chromium.org/2340813003/diff/1/base/id_map.h
File base/id_map.h (right):

https://codereview.chromium.org/2340813003/diff/1/base/id_map.h#newcode46
base/id_map.h:46: typedef typename std::conditional<OS == IDMapExternalPointer,
On 2016/09/15 at 20:08:40, danakj wrote:
> can you change these both to "using" instead of typedef

Done.

https://codereview.chromium.org/2340813003/diff/1/base/id_map.h#newcode72
base/id_map.h:72: KeyType Add(T* data) {
On 2016/09/15 at 20:08:40, danakj wrote:
> Can this not take a V instead?

Yes, it can and should, but it would require expanding the scope of the patch to
touch ~100 files and I wanted initial reviewer feedback before embarking on
that.

This patch has a small risk of affecting perfbots or correctness and getting
reverted, so I'm thinking it would make sense to land this small one first, then
do the big no-op boilerplatey one once this proves to stick?  WDYT?  I added a
TODO(aelias) to change to Vs.

[commit-bot: I haz the power, 2016-09-15 23:49:38]

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

[commit-bot: I haz the power, 2016-09-15 23:49:39]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-09-16 00:30:28]

LGTM

https://codereview.chromium.org/2340813003/diff/1/base/id_map.h
File base/id_map.h (right):

https://codereview.chromium.org/2340813003/diff/1/base/id_map.h#newcode72
base/id_map.h:72: KeyType Add(T* data) {
On 2016/09/15 21:09:35, aelias wrote:
> On 2016/09/15 at 20:08:40, danakj wrote:
> > Can this not take a V instead?
> 
> Yes, it can and should, but it would require expanding the scope of the patch
to
> touch ~100 files and I wanted initial reviewer feedback before embarking on
> that.
> 
> This patch has a small risk of affecting perfbots or correctness and getting
> reverted, so I'm thinking it would make sense to land this small one first,
then
> do the big no-op boilerplatey one once this proves to stick?  WDYT?  I added a
> TODO(aelias) to change to Vs.

Cool cool. Can you add a V version now and the comment on the T* one say to not
use it and use the V version?

https://codereview.chromium.org/2340813003/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/2340813003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_core.cc:637:
ServiceWorkerProviderHost* replacement = new ServiceWorkerProviderHost(
mind making this a unique_ptr instead of using WrapUnique below?

[aelias_OOO_until_Jul13, 2016-09-16 01:36:58]

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

[commit-bot: I haz the power, 2016-09-16 01:37:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aelias_OOO_until_Jul13, 2016-09-16 01:39:19]

All done, I just need service_worker/ OWNERS review from michaeln@ to land.

https://codereview.chromium.org/2340813003/diff/1/base/id_map.h
File base/id_map.h (right):

https://codereview.chromium.org/2340813003/diff/1/base/id_map.h#newcode72
base/id_map.h:72: KeyType Add(T* data) {
On 2016/09/16 at 00:30:27, danakj wrote:
> Cool cool. Can you add a V version now and the comment on the T* one say to
not use it and use the V version?

Done.  Note that because V can be the same as T*, I had to add an explicit
unique_ptr<T> variant and make the V variant a private method.

https://codereview.chromium.org/2340813003/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/2340813003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_core.cc:637:
ServiceWorkerProviderHost* replacement = new ServiceWorkerProviderHost(
On 2016/09/16 at 00:30:27, danakj wrote:
> mind making this a unique_ptr instead of using WrapUnique below?

Done, and I also changed the rest of service_worker/ to use the new
Add()/AddWithID() to cover the rest of this component while I'm at it.

[commit-bot: I haz the power, 2016-09-16 01:42:45]

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

[commit-bot: I haz the power, 2016-09-16 01:42:46]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aelias_OOO_until_Jul13, 2016-09-16 02:10:22]

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

[commit-bot: I haz the power, 2016-09-16 02:10:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 03:27:39]

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

[commit-bot: I haz the power, 2016-09-16 03:27:40]

Dry run: This issue passed the CQ dry run.

[aelias_OOO_until_Jul13, 2016-09-21 18:39:46]

Ping michaeln@ for OWNERS for no-op changes in components/service_worker/.

[michaeln, 2016-09-23 21:11:18]

lgtm

[aelias_OOO_until_Jul13, 2016-09-23 21:39:09]

The CQ bit was checked by aelias@chromium.org

[aelias_OOO_until_Jul13, 2016-09-23 21:39:09]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2340813003/#ps40001
(title: "unique_ptr<> versions of Add/AddWithId")

[commit-bot: I haz the power, 2016-09-23 21:39:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 23:30:56]

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

[commit-bot: I haz the power, 2016-09-23 23:30:57]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[aelias_OOO_until_Jul13, 2016-09-23 23:36:50]

The CQ bit was checked by aelias@chromium.org

[commit-bot: I haz the power, 2016-09-23 23:37:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-24 01:27:02]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-24 01:30:37]

Description was changed from

==========
Switch base::IDMap to use unique_ptr.

This old base class is a thin wrapper around STL hashmap, providing A)
less easily invalidated iterators and B) a unique_ptr equivalent.  At
this point there is nothing stopping this from *actually* using
unique_ptr, so I switch it over to using that in its IDMapOwnPointer mode.

I made one semantic change, to Replace(), which used to "leak" the
return pointer.  I changed it to instead directly return a unique_ptr
when in that mode, and also to not support replacing an invalid ID.
Both of those changes suit the only user of Replace (in
ServiceWorkerContextCore).

BUG=647091
==========

to

==========
Switch base::IDMap to use unique_ptr.

This old base class is a thin wrapper around STL hashmap, providing A)
less easily invalidated iterators and B) a unique_ptr equivalent.  At
this point there is nothing stopping this from *actually* using
unique_ptr, so I switch it over to using that in its IDMapOwnPointer mode.

I made one semantic change, to Replace(), which used to "leak" the
return pointer.  I changed it to instead directly return a unique_ptr
when in that mode, and also to not support replacing an invalid ID.
Both of those changes suit the only user of Replace (in
ServiceWorkerContextCore).

BUG=647091

Committed: https://crrev.com/aed214d0ef7a3da3efa968dd6646e126efe91963
Cr-Commit-Position: refs/heads/master@{#420809}
==========

[commit-bot: I haz the power, 2016-09-24 01:30:38]

Patchset 3 (id:??) landed as
https://crrev.com/aed214d0ef7a3da3efa968dd6646e126efe91963
Cr-Commit-Position: refs/heads/master@{#420809}