Remove stl_util's deletion function use from components/history/.

Remove stl_util's deletion function use from components/history/.

BUG=555865
Committed: https://crrev.com/53aeef9b765f032230f359efc5455dd77e0e188b
Cr-Commit-Position: refs/heads/master@{#427123}

[Avi (use Gerrit), 2016-10-23 00:33:25]

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

[commit-bot: I haz the power, 2016-10-23 00:33: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-10-23 01:40:18]

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

[commit-bot: I haz the power, 2016-10-23 01:40:19]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-10-23 02:30:54]

avi@chromium.org changed reviewers:
+ sky@chromium.org

[Avi (use Gerrit), 2016-10-23 02:30:55]

Oh, you own history! Have at it.

[sky, 2016-10-24 16:04:11]

https://codereview.chromium.org/2441223002/diff/1/components/history/core/bro...
File components/history/core/browser/web_history_service.h (right):

https://codereview.chromium.org/2441223002/diff/1/components/history/core/bro...
components/history/core/browser/web_history_service.h:216: std::map<Request*,
std::unique_ptr<Request>> pending_expire_requests_;
Using a map purely for the ownership is bizarre. Can you instead use a custom
comparison function?

[Avi (use Gerrit), 2016-10-24 16:09:01]

https://codereview.chromium.org/2441223002/diff/1/components/history/core/bro...
File components/history/core/browser/web_history_service.h (right):

https://codereview.chromium.org/2441223002/diff/1/components/history/core/bro...
components/history/core/browser/web_history_service.h:216: std::map<Request*,
std::unique_ptr<Request>> pending_expire_requests_;
On 2016/10/24 16:04:10, sky wrote:
> Using a map purely for the ownership is bizarre. Can you instead use a custom
> comparison function?

It's not about comparison functions.

In general, I convert to map<pointer, owned pointer> because I've gotten
feedback that people don't like losing logarithmic lookup. I can switch to a
set, and use find_if if that isn't an issue.

The real tricky thing is that you can't extract ownership from the set once you
give it to the set. So I'd have to rewrite the functions that do the removal to
move the set element deletion to be the last thing. In this case I don't believe
it would matter.

[sky, 2016-10-24 16:40:07]

LGTM

https://codereview.chromium.org/2441223002/diff/1/components/history/core/bro...
File components/history/core/browser/web_history_service.h (right):

https://codereview.chromium.org/2441223002/diff/1/components/history/core/bro...
components/history/core/browser/web_history_service.h:216: std::map<Request*,
std::unique_ptr<Request>> pending_expire_requests_;
On 2016/10/24 16:09:01, Avi wrote:
> On 2016/10/24 16:04:10, sky wrote:
> > Using a map purely for the ownership is bizarre. Can you instead use a
custom
> > comparison function?
> 
> It's not about comparison functions.
> 
> In general, I convert to map<pointer, owned pointer> because I've gotten
> feedback that people don't like losing logarithmic lookup. I can switch to a
> set, and use find_if if that isn't an issue.
> 
> The real tricky thing is that you can't extract ownership from the set once
you
> give it to the set. So I'd have to rewrite the functions that do the removal
to
> move the set element deletion to be the last thing. In this case I don't
believe
> it would matter.



I was hoping a comparison function would give the logarithmic lookup time. But
that would require searches to be done by placing the pointer in a unique_ptr,
then doing the search. Which isn't really what you want, and is easy to get
wrong (have to be careful to remove the pointer from the unique_ptr after the
search).

It seems to me trying to use stl containers with unique_ptr to enforce ownership
is quirky... Was there ever a discussion of stl containers with unique_ptrs on
chromium-dev?

I'll approve this since this appears to be the path we're going down, but I
think we should consider ways to address the need of containers that own their
contents without the quirks that stl + unique_ptr gives you.

[Avi (use Gerrit), 2016-10-24 17:05:34]

https://codereview.chromium.org/2441223002/diff/1/components/history/core/bro...
File components/history/core/browser/web_history_service.h (right):

https://codereview.chromium.org/2441223002/diff/1/components/history/core/bro...
components/history/core/browser/web_history_service.h:216: std::map<Request*,
std::unique_ptr<Request>> pending_expire_requests_;
There was a thread on cxx
(https://groups.google.com/a/chromium.org/d/msg/cxx/xJpM3J_gpZY/-EA_DuGdCwAJ)
but there were no solid conclusions.

[Avi (use Gerrit), 2016-10-24 18:25:55]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2016-10-24 18:26:42]

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-10-24 19:45:22]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-24 19:55:55]

Description was changed from

==========
Remove stl_util's deletion function use from components/history/.

BUG=555865
==========

to

==========
Remove stl_util's deletion function use from components/history/.

BUG=555865

Committed: https://crrev.com/53aeef9b765f032230f359efc5455dd77e0e188b
Cr-Commit-Position: refs/heads/master@{#427123}
==========

[commit-bot: I haz the power, 2016-10-24 19:55:56]

Patchset 1 (id:??) landed as
https://crrev.com/53aeef9b765f032230f359efc5455dd77e0e188b
Cr-Commit-Position: refs/heads/master@{#427123}