Remove stl_util's deletion function use from chrome/browser/extensions/api/content_settings.

Remove stl_util's deletion function use from chrome/browser/extensions/api/content_settings.

BUG=555865
Committed: https://crrev.com/49d2ee2e272c94b197737746a799844779d57b72
Cr-Commit-Position: refs/heads/master@{#438624}

[Avi (use Gerrit), 2016-10-20 15:43:18]

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

[commit-bot: I haz the power, 2016-10-20 15:43:36]

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-20 16:13:20]

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

[commit-bot: I haz the power, 2016-10-20 16:13:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2016-12-13 21:54:29]

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

[commit-bot: I haz the power, 2016-12-13 21:55:02]

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-12-13 22:15:22]

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

[commit-bot: I haz the power, 2016-12-13 22:15:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2016-12-13 23:56:39]

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

[commit-bot: I haz the power, 2016-12-13 23:57:06]

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-12-14 00:44:35]

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

[commit-bot: I haz the power, 2016-12-14 00:44:35]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-12-14 03:39:38]

avi@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Avi (use Gerrit), 2016-12-14 03:39:38]

FYI:

- Chrome Linux's C++ stdlib doesn't support unique_ptr in multimap yet. Since
the only reason for a multimap was to do sorting by time, I switched to an
explicitly-sorted vector. It's better for memory locality too.

- Chrome Linux's C++ stdlib also doesn't support using a const_iterator to erase
from a vector yet. That's why I maintain the two find functions despite the fact
I should only really need the const_iterator one.

PTAL.

[Devlin, 2016-12-14 15:54:01]

https://codereview.chromium.org/2436023002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/content_settings/content_settings_store.cc
(right):

https://codereview.chromium.org/2436023002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_store.cc:69: //
Iterate the extensions based on install time (last installed extensions
not your code, but something like "most recently installed" sounds better to me
than "last installed".

https://codereview.chromium.org/2436023002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_store.cc:129:
auto i = FindEntry(ext_id);
Using auto with all these FindEntry's makes me a little nervous when we have
const and non-const iterator versions, since it makes it easier to accidentally
modify the value (the compiler will happily return the non-const iterator if you
perform a modification, even if you didn't mean to).  If we keep these, I wonder
if we could either not use auto or name them differently?

That said, it looks like when we get the const iterators, we really only need
the value.  I think it might clean some of these methods up to have FindEntry()
return a const ExtensionEntry* (or nullptr) (and then maybe rename the iterator
FindEntry to FindIterator or something?).  This would make it clear what the
purpose was in each case, and would get rid of a lot of the (*i)-> syntax.

e.g.

const ExtensionEntry* FindEntry(const std::string& id) {
  auto iter = FindIterator(id);
  return iter == entries_.end() ? nullptr : iter->get();
}

ExtensionEntries::iterator FindIterator(const std::string& id) {
  std::find_if(...);
}

WDYT?

https://codereview.chromium.org/2436023002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_store.cc:130:
ExtensionEntry* entry;
init to nullptr

https://codereview.chromium.org/2436023002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_store.cc:137:
std::stable_sort(entries_.begin(), entries_.end(),
Would it be better to use std::find_if() to find the position of the new item
and then use vector::insert?  Realistically, probably won't make that much of a
difference since the vector should be otherwise sorted, but it makes it more
clear what the state is, IMO.

https://codereview.chromium.org/2436023002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/content_settings/content_settings_store.h
(right):

https://codereview.chromium.org/2436023002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_store.h:137: //
The entries. Maintained in reverse-chronological order (newest items first)
I wonder if the part about reverse-chronological order is more useful above the
using declaration?  Similar to how for we would suggest a comment above using x
= std::map<std::string, int> describing what the string and int are.

[Avi (use Gerrit), 2016-12-14 17:59:49]

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

[commit-bot: I haz the power, 2016-12-14 18:00:32]

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-12-14 18:19:01]

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

[commit-bot: I haz the power, 2016-12-14 18:19:02]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Avi (use Gerrit), 2016-12-14 18:29:52]

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

[commit-bot: I haz the power, 2016-12-14 18:30: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-12-14 19:28:17]

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

[commit-bot: I haz the power, 2016-12-14 19:28:18]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-12-14 19:29:06]

PTAL.

https://codereview.chromium.org/2436023002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/content_settings/content_settings_store.cc
(right):

https://codereview.chromium.org/2436023002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_store.cc:69: //
Iterate the extensions based on install time (last installed extensions
On 2016/12/14 15:54:01, Devlin wrote:
> not your code, but something like "most recently installed" sounds better to
me
> than "last installed".

Done.

https://codereview.chromium.org/2436023002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_store.cc:129:
auto i = FindEntry(ext_id);
On 2016/12/14 15:54:01, Devlin wrote:
> Using auto with all these FindEntry's makes me a little nervous when we have
> const and non-const iterator versions, since it makes it easier to
accidentally
> modify the value (the compiler will happily return the non-const iterator if
you
> perform a modification, even if you didn't mean to).  If we keep these, I
wonder
> if we could either not use auto or name them differently?
> 
> That said, it looks like when we get the const iterators, we really only need
> the value.  I think it might clean some of these methods up to have
FindEntry()
> return a const ExtensionEntry* (or nullptr) (and then maybe rename the
iterator
> FindEntry to FindIterator or something?).  This would make it clear what the
> purpose was in each case, and would get rid of a lot of the (*i)-> syntax.
> 
> e.g.
> 
> const ExtensionEntry* FindEntry(const std::string& id) {
>   auto iter = FindIterator(id);
>   return iter == entries_.end() ? nullptr : iter->get();
> }
> 
> ExtensionEntries::iterator FindIterator(const std::string& id) {
>   std::find_if(...);
> }
> 
> WDYT?

Sounds reasonable, though I can't delegate like that. FindEntry needs to be
const, and so it can't rely on a non-const FindIterator.

https://codereview.chromium.org/2436023002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_store.cc:130:
ExtensionEntry* entry;
On 2016/12/14 15:54:01, Devlin wrote:
> init to nullptr

Done.

https://codereview.chromium.org/2436023002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_store.cc:137:
std::stable_sort(entries_.begin(), entries_.end(),
On 2016/12/14 15:54:01, Devlin wrote:
> Would it be better to use std::find_if() to find the position of the new item
> and then use vector::insert?  Realistically, probably won't make that much of
a
> difference since the vector should be otherwise sorted, but it makes it more
> clear what the state is, IMO.

std::upper_bound?

https://codereview.chromium.org/2436023002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/content_settings/content_settings_store.h
(right):

https://codereview.chromium.org/2436023002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_store.h:137: //
The entries. Maintained in reverse-chronological order (newest items first)
On 2016/12/14 15:54:01, Devlin wrote:
> I wonder if the part about reverse-chronological order is more useful above
the
> using declaration?  Similar to how for we would suggest a comment above using
x
> = std::map<std::string, int> describing what the string and int are.

Done.

[Devlin, 2016-12-14 19:54:34]

https://codereview.chromium.org/2436023002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/content_settings/content_settings_store.cc
(right):

https://codereview.chromium.org/2436023002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_store.cc:137:
std::stable_sort(entries_.begin(), entries_.end(),
On 2016/12/14 19:29:06, Avi wrote:
> On 2016/12/14 15:54:01, Devlin wrote:
> > Would it be better to use std::find_if() to find the position of the new
item
> > and then use vector::insert?  Realistically, probably won't make that much
of
> a
> > difference since the vector should be otherwise sorted, but it makes it more
> > clear what the state is, IMO.
> 
> std::upper_bound?

Even better. :)

[Devlin, 2016-12-14 19:54:42]

whoops, and lgtm

[Avi (use Gerrit), 2016-12-14 21:05:02]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2016-12-14 21:06:03]

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-12-14 21:18:07]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481749502536440,
"parent_rev": "36a345884fb997a8c0e15f497a782d1c3571d9f1", "commit_rev":
"ada57d859f13cec3d38a7221e1876c8da940c694"}

[commit-bot: I haz the power, 2016-12-14 21:18:40]

Description was changed from

==========
Remove stl_util's deletion function use from
chrome/browser/extensions/api/content_settings.

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

to

==========
Remove stl_util's deletion function use from
chrome/browser/extensions/api/content_settings.

BUG=555865

Review-Url: https://codereview.chromium.org/2436023002
==========

[commit-bot: I haz the power, 2016-12-14 21:18:41]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-14 21:22:55]

Description was changed from

==========
Remove stl_util's deletion function use from
chrome/browser/extensions/api/content_settings.

BUG=555865

Review-Url: https://codereview.chromium.org/2436023002
==========

to

==========
Remove stl_util's deletion function use from
chrome/browser/extensions/api/content_settings.

BUG=555865

Committed: https://crrev.com/49d2ee2e272c94b197737746a799844779d57b72
Cr-Commit-Position: refs/heads/master@{#438624}
==========

[commit-bot: I haz the power, 2016-12-14 21:22:56]

Patchset 5 (id:??) landed as
https://crrev.com/49d2ee2e272c94b197737746a799844779d57b72
Cr-Commit-Position: refs/heads/master@{#438624}