Remove some uses of stl_util's STLDeleteContainerPointers.

Remove some uses of stl_util's STLDeleteContainerPointers.

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

[Avi (use Gerrit), 2016-09-06 14:56:36]

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

[commit-bot: I haz the power, 2016-09-06 14:56:45]

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-06 15:32:33]

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

[commit-bot: I haz the power, 2016-09-06 15:32:34]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Avi (use Gerrit), 2016-09-06 21:45:01]

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

[commit-bot: I haz the power, 2016-09-06 21:45:24]

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-06 23:04:55]

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

[commit-bot: I haz the power, 2016-09-06 23:04:55]

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

[Avi (use Gerrit), 2016-09-07 15:50:21]

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

[commit-bot: I haz the power, 2016-09-07 15:50:38]

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-07 16:17:06]

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

[commit-bot: I haz the power, 2016-09-07 16:17:08]

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

[Avi (use Gerrit), 2016-09-07 21:40:29]

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

[commit-bot: I haz the power, 2016-09-07 21:40:56]

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-07 23:35:39]

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

[commit-bot: I haz the power, 2016-09-07 23:35:40]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-09-08 14:52:03]

avi@chromium.org changed reviewers:
+ eranm@chromium.org, nasko@chromium.org, sdefresne@chromium.org,
thakis@chromium.org, wez@chromium.org

[Avi (use Gerrit), 2016-09-08 14:52:04]

Nico: base/, chrome/, ui/
Eran: components/CT
Sylvain: components/history
Nasko: content/
Wez: remoting/

[sdefresne, 2016-09-08 15:15:06]

components/history lgtm

[Ryan Sleevi, 2016-09-08 17:03:15]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2016-09-08 17:03:15]

//components/certificate_transparency LGTM

[Nico, 2016-09-08 17:32:00]

This looks like a mix of "well of course" and "uh..." changes in my files.

https://codereview.chromium.org/2314833002/diff/60001/base/memory/scoped_vect...
File base/memory/scoped_vector.h (right):

https://codereview.chromium.org/2314833002/diff/60001/base/memory/scoped_vect...
base/memory/scoped_vector.h:96: ++it;
how is this better than the old thing?

https://codereview.chromium.org/2314833002/diff/60001/ui/message_center/notif...
File ui/message_center/notification_list.h (right):

https://codereview.chromium.org/2314833002/diff/60001/ui/message_center/notif...
ui/message_center/notification_list.h:42: bool operator()(const T& n1, const T&
n2) {
why move this inline?

how does _EXPORT on a template make sense?

https://codereview.chromium.org/2314833002/diff/60001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield_model.cc (right):

https://codereview.chromium.org/2314833002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_model.cc:770: edit->Redo(this);
Getting a raw ptr to a unique_ptr that's moved from and then calling something
on that pointer looks dangerous. The old code makes it clear that this is safe,
with the new code it's not clear.

https://codereview.chromium.org/2314833002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_model.cc:798: edit->Redo(this);
ditto

[Wez, 2016-09-08 20:22:30]

wez@chromium.org changed reviewers:
+ sergeyu@chromium.org
- wez@chromium.org

[Wez, 2016-09-08 20:22:30]

-wez, +sergeyu

[nasko, 2016-09-08 21:00:55]

content/ LGTM

[Avi (use Gerrit), 2016-09-13 14:36:11]

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

[Avi (use Gerrit), 2016-09-13 14:36:13]

https://codereview.chromium.org/2314833002/diff/60001/base/memory/scoped_vect...
File base/memory/scoped_vector.h (right):

https://codereview.chromium.org/2314833002/diff/60001/base/memory/scoped_vect...
base/memory/scoped_vector.h:96: ++it;
On 2016/09/08 17:32:00, Nico wrote:
> how is this better than the old thing?

I'm trying to remove STLDeleteContainerPointers, and my patch was getting big,
so I'm trying to commit some of it. It's better in that it doesn't use that
function.

Eventually we want to kill ScopedVector too, but until then, I want to remove
uses of the STL functions as I go.

https://codereview.chromium.org/2314833002/diff/60001/ui/message_center/notif...
File ui/message_center/notification_list.h (right):

https://codereview.chromium.org/2314833002/diff/60001/ui/message_center/notif...
ui/message_center/notification_list.h:42: bool operator()(const T& n1, const T&
n2) {
On 2016/09/08 17:32:00, Nico wrote:
> why move this inline?

Templates; don't I need to define it all to use them?

> how does _EXPORT on a template make sense?

Removed.

https://codereview.chromium.org/2314833002/diff/60001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield_model.cc (right):

https://codereview.chromium.org/2314833002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_model.cc:770: edit->Redo(this);
On 2016/09/08 17:32:00, Nico wrote:
> Getting a raw ptr to a unique_ptr that's moved from and then calling something
> on that pointer looks dangerous. The old code makes it clear that this is
safe,
> with the new code it's not clear.

This entire code is insane re ownership. I would disagree that the old code is
clearly safe. I have zero idea what's going on from reading the old code. It
tosses around pointers, owned by who knows, and sometimes deletes them and
sometimes does not.

If you'd prefer, I'll remove these files from the patch and figure out a better
ownership rewrite.

[commit-bot: I haz the power, 2016-09-13 14:36:30]

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

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

[commit-bot: I haz the power, 2016-09-13 16:16:33]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-09-14 19:28:53]

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

[commit-bot: I haz the power, 2016-09-14 19:29:22]

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

[Avi (use Gerrit), 2016-09-14 19:30:18]

Nico, I removed the controversial files; ptal.
Sergey, ping?

[Sergey Ulanov, 2016-09-14 19:33:34]

lgtm

[commit-bot: I haz the power, 2016-09-14 20:29:51]

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

[commit-bot: I haz the power, 2016-09-14 20:29:53]

Dry run: This issue passed the CQ dry run.

[Nico, 2016-09-15 16:07:16]

chrome/ looks great now, thanks. ui/ needs a bit more work imho:

https://codereview.chromium.org/2314833002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ping_manager.cc (right):

https://codereview.chromium.org/2314833002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ping_manager.cc:105:
safebrowsing_reports_.insert(std::move(report_ptr));
nit: If you do 

  (*safebrowsing_reports_.insert(std::move(report_ptr)))->Start();

here, you don't need the raw pointer. Not sure if that's better though.

https://codereview.chromium.org/2314833002/diff/100001/ui/message_center/noti...
File ui/message_center/notification_list.h (right):

https://codereview.chromium.org/2314833002/diff/100001/ui/message_center/noti...
ui/message_center/notification_list.h:43: if (n1->timestamp() > n2->timestamp())
 // Newer come first.
I think making this a full template is confusing, since n1 is either a
Notification* or a unique_ptr<Notification>. I think this might be a better
approach:

template <typename T, bool PlainCompare(T*, T*)>
struct UniquePtrCompare {
  bool operator(const std::unique_ptr<T>& n1, const std::unique_ptr<T>& n2) {
    return PlainCompare(n1.get, n2.get());
  }
};

That will work for any situation where you have a comparator for the plain type
and want to compare unique_ptrs – and it'll also allow you to keep the
definition of ComparePriorityTimestampSerial and CompareTimestampSerial out of
line.

[Avi (use Gerrit), 2016-09-15 19:52:18]

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

[commit-bot: I haz the power, 2016-09-15 19:52:42]

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

[Avi (use Gerrit), 2016-09-15 19:53:46]

Nico, ptal.

https://codereview.chromium.org/2314833002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ping_manager.cc (right):

https://codereview.chromium.org/2314833002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ping_manager.cc:105:
safebrowsing_reports_.insert(std::move(report_ptr));
On 2016/09/15 16:07:16, Nico wrote:
> nit: If you do 
> 
>   (*safebrowsing_reports_.insert(std::move(report_ptr)))->Start();
> 
> here, you don't need the raw pointer. Not sure if that's better though.

I suppose so, but I'll pass for simplicity/clarity.

https://codereview.chromium.org/2314833002/diff/100001/ui/message_center/noti...
File ui/message_center/notification_list.h (right):

https://codereview.chromium.org/2314833002/diff/100001/ui/message_center/noti...
ui/message_center/notification_list.h:43: if (n1->timestamp() > n2->timestamp())
 // Newer come first.
That doesn't actually work.

error: template argument for non-type template parameter must be an expression
               UniquePtrCompare<Notification, ComparePriorityTimestampSerial>>;
                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: template parameter is declared here
template <typename T, bool PlainCompare(T*, T*)>
                           ^

I switched to using a type parameter.

[commit-bot: I haz the power, 2016-09-15 20:41:52]

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

[commit-bot: I haz the power, 2016-09-15 20:41:53]

Dry run: This issue passed the CQ dry run.

[Nico, 2016-09-16 19:38:30]

lgtm

[Avi (use Gerrit), 2016-09-16 19:38:55]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2016-09-16 19:38:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
rsleevi@chromium.org, sdefresne@chromium.org, nasko@chromium.org,
sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2314833002/#ps120001
(title: "cleanup")

[commit-bot: I haz the power, 2016-09-16 19:39:40]

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 19:45:11]

Committed patchset #7 (id:120001)

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

Description was changed from

==========
Remove some uses of stl_util's STLDeleteContainerPointers.

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

to

==========
Remove some uses of stl_util's STLDeleteContainerPointers.

BUG=555865

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

[commit-bot: I haz the power, 2016-09-16 19:46:44]

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