Remove stl_util's STLDeleteContainerPointers from autofill.

Remove stl_util's STLDeleteContainerPointers and ScopedVector from autofill, do more general ownership cleanup, and fix random typos.

BUG=555865, 554289
Committed: https://crrev.com/1ce1e60022566d580ddcf30dcb1f03d0426731a2
Cr-Commit-Position: refs/heads/master@{#426232}

[Avi (use Gerrit), 2016-10-10 03:33:15]

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

[commit-bot: I haz the power, 2016-10-10 03:33:19]

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-10 03:45:08]

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

[commit-bot: I haz the power, 2016-10-10 03:45:08]

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-10-12 15:09:26]

Description was changed from

==========
Remove stl_util's STLDeleteContainerPointers from autofill.

BUG=555865, 554289
==========

to

==========
Remove stl_util's STLDeleteContainerPointers from autofill, and do more general
ownership cleanup.

BUG=555865, 554289
==========

[Avi (use Gerrit), 2016-10-12 15:26:04]

Description was changed from

==========
Remove stl_util's STLDeleteContainerPointers from autofill, and do more general
ownership cleanup.

BUG=555865, 554289
==========

to

==========
Remove stl_util's STLDeleteContainerPointers from autofill, do more general
ownership cleanup, and fix random typos.

BUG=555865, 554289
==========

[Avi (use Gerrit), 2016-10-12 19:41:46]

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

[commit-bot: I haz the power, 2016-10-12 19:42:19]

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-12 19:54:23]

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

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

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...)

[Avi (use Gerrit), 2016-10-12 19:55:43]

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

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

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-12 20:09:38]

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

[commit-bot: I haz the power, 2016-10-12 20:09:41]

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-10-12 22:17:07]

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

[commit-bot: I haz the power, 2016-10-12 22:17:28]

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-12 23:02:54]

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

[commit-bot: I haz the power, 2016-10-12 23:02:55]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-10-13 14:53:20]

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

[commit-bot: I haz the power, 2016-10-13 14:53: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-10-13 14:56:56]

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

[commit-bot: I haz the power, 2016-10-13 14:56:57]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Avi (use Gerrit), 2016-10-13 15:26:30]

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

[commit-bot: I haz the power, 2016-10-13 15:26:58]

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

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

[commit-bot: I haz the power, 2016-10-13 16:15:58]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-10-13 18:06:02]

avi@chromium.org changed reviewers:
+ atwilson@chromium.org, bauerb@chromium.org, isherman@chromium.org,
michaelbai@chromium.org, pkasting@chromium.org, zea@chromium.org

[Avi (use Gerrit), 2016-10-13 18:06:03]

michaelbai: aw/
isherman: components/autofill, chrome/browser/autofill,
chrome/browser/password_manager
zea: components/browser_sync, chrome/browser/sync
bauerb: components/browsing_data
pkasting: components/search_engines, components/webdata,
chrome/browser/ui/search_engines
atwilson: chrome/browser/signin

[Nicolas Zea, 2016-10-14 02:17:43]

sync lgtm

[vabr (Chromium), 2016-10-14 07:24:42]

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

[vabr (Chromium), 2016-10-14 07:24:43]

Thanks for this great clean-up!

components/autofill LGTM with some optional comments.

Cheers,
Vaclav

https://codereview.chromium.org/2403773002/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_manager.h (left):

https://codereview.chromium.org/2403773002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_manager.h:18: #include
"base/memory/scoped_vector.h"
optional: Perhaps mention removing ScopedVector in the description explicitly
and maybe add http://crbug.com/554289 to the list in BUG?
Up to you, I'm also fine with not doing that.

https://codereview.chromium.org/2403773002/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_metrics_unittest.cc (right):

https://codereview.chromium.org/2403773002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_metrics_unittest.cc:119: for (auto
iter = profiles->begin(); iter != profiles->end(); ++iter)
optional: Since you already change the for-loop, what about making it
range-based?

for (const auto& profile : *profiles) {
  web_profiles_push_back(base::MakeUnique<AutofillProfile>(profile));
}

https://codereview.chromium.org/2403773002/diff/100001/components/autofill/co...
File components/autofill/core/browser/personal_data_manager_unittest.cc (right):

https://codereview.chromium.org/2403773002/diff/100001/components/autofill/co...
components/autofill/core/browser/personal_data_manager_unittest.cc:4538:
AutofillProfile* profile1 =
Neither profile1 nor profile2 are used after being pushed to existing profiles.
What about making them unique_ptrs and std::moving them during the push_back?

https://codereview.chromium.org/2403773002/diff/100001/components/autofill/co...
File
components/autofill/core/browser/webdata/autofill_wallet_syncable_service.cc
(right):

https://codereview.chromium.org/2403773002/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_wallet_syncable_service.cc:182:
for (const auto& cur_new : data) {
Here I would prefer the original "const Data&" to using auto. It is equally
short, and more descriptive. In particular, it would help the reader to
understand that unlike cur_existing, which is a pointer, cur_new is a reference
to Data directly.

[vabr (Chromium), 2016-10-14 07:26:15]

On 2016/10/14 07:24:43, vabr (Chromium) wrote:
> Thanks for this great clean-up!
> 
> components/autofill LGTM with some optional comments.

Also chrome/browser/autofill and chrome/browser/password_manager LGTM.

[vabr (Chromium), 2016-10-14 07:27:25]

https://codereview.chromium.org/2403773002/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_manager.h (left):

https://codereview.chromium.org/2403773002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_manager.h:18: #include
"base/memory/scoped_vector.h"
On 2016/10/14 07:24:43, vabr (Chromium) wrote:
> optional: Perhaps mention removing ScopedVector in the description explicitly
> and maybe add http://crbug.com/554289 to the list in BUG?
> Up to you, I'm also fine with not doing that.

I see that you already have 554289 in BUG, please ignore that part of my
comment.

[Avi (use Gerrit), 2016-10-14 14:47:03]

Description was changed from

==========
Remove stl_util's STLDeleteContainerPointers from autofill, do more general
ownership cleanup, and fix random typos.

BUG=555865, 554289
==========

to

==========
Remove stl_util's STLDeleteContainerPointers and ScopedVector from autofill, do
more general ownership cleanup, and fix random typos.

BUG=555865, 554289
==========

[Avi (use Gerrit), 2016-10-14 15:07:40]

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

[commit-bot: I haz the power, 2016-10-14 15:07: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-10-14 15:58:48]

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

[commit-bot: I haz the power, 2016-10-14 15:58:50]

Dry run: This issue passed the CQ dry run.

[michaelbai, 2016-10-14 17:09:36]

android_webview LGTM

[Ilya Sherman, 2016-10-14 23:42:08]

isherman@chromium.org changed reviewers:
- isherman@chromium.org

[Avi (use Gerrit), 2016-10-16 20:05:54]

https://codereview.chromium.org/2403773002/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_manager.h (left):

https://codereview.chromium.org/2403773002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_manager.h:18: #include
"base/memory/scoped_vector.h"
On 2016/10/14 07:27:25, vabr (OOO until 17 Oct) wrote:
> On 2016/10/14 07:24:43, vabr (Chromium) wrote:
> > optional: Perhaps mention removing ScopedVector in the description
explicitly
> > and maybe add http://crbug.com/554289 to the list in BUG?
> > Up to you, I'm also fine with not doing that.
> 
> I see that you already have 554289 in BUG, please ignore that part of my
> comment.

Done.

https://codereview.chromium.org/2403773002/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_metrics_unittest.cc (right):

https://codereview.chromium.org/2403773002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_metrics_unittest.cc:119: for (auto
iter = profiles->begin(); iter != profiles->end(); ++iter)
On 2016/10/14 07:24:43, vabr (OOO until 17 Oct) wrote:
> optional: Since you already change the for-loop, what about making it
> range-based?
> 
> for (const auto& profile : *profiles) {
>   web_profiles_push_back(base::MakeUnique<AutofillProfile>(profile));
> }

Done.

https://codereview.chromium.org/2403773002/diff/100001/components/autofill/co...
File components/autofill/core/browser/personal_data_manager_unittest.cc (right):

https://codereview.chromium.org/2403773002/diff/100001/components/autofill/co...
components/autofill/core/browser/personal_data_manager_unittest.cc:4538:
AutofillProfile* profile1 =
On 2016/10/14 07:24:43, vabr (OOO until 17 Oct) wrote:
> Neither profile1 nor profile2 are used after being pushed to existing
profiles.
> What about making them unique_ptrs and std::moving them during the push_back?

profile2 is, but I can do that for profile 1.

https://codereview.chromium.org/2403773002/diff/100001/components/autofill/co...
File
components/autofill/core/browser/webdata/autofill_wallet_syncable_service.cc
(right):

https://codereview.chromium.org/2403773002/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_wallet_syncable_service.cc:182:
for (const auto& cur_new : data) {
On 2016/10/14 07:24:43, vabr (OOO until 17 Oct) wrote:
> Here I would prefer the original "const Data&" to using auto. It is equally
> short, and more descriptive. In particular, it would help the reader to
> understand that unlike cur_existing, which is a pointer, cur_new is a
reference
> to Data directly.

Done.

[Bernhard Bauer, 2016-10-17 14:53:18]

https://codereview.chromium.org/2403773002/diff/120001/components/browsing_da...
File components/browsing_data/core/counters/autofill_counter.cc (right):

https://codereview.chromium.org/2403773002/diff/120001/components/browsing_da...
components/browsing_data/core/counters/autofill_counter.cc:103:
std::vector<std::unique_ptr<autofill::CreditCard>> credit_cards = std::move(
Ugh, this is really not particularly readable. Could we extract a type alias for
std::vector<std::unique_ptr<autofill::CreditCard>> and use that? Maybe that will
help a bit.

[Avi (use Gerrit), 2016-10-17 15:04:10]

https://codereview.chromium.org/2403773002/diff/120001/components/browsing_da...
File components/browsing_data/core/counters/autofill_counter.cc (right):

https://codereview.chromium.org/2403773002/diff/120001/components/browsing_da...
components/browsing_data/core/counters/autofill_counter.cc:103:
std::vector<std::unique_ptr<autofill::CreditCard>> credit_cards = std::move(
On 2016/10/17 14:53:18, Bernhard Bauer wrote:
> Ugh, this is really not particularly readable. Could we extract a type alias
for
> std::vector<std::unique_ptr<autofill::CreditCard>> and use that? Maybe that
will
> help a bit.

The problem is that autofill code is full of vector<CreditCard*>,
vector<CreditCard>, and vector<unique_ptr<CreditCard>> objects, all over the
place. As bad as these types are, at least it says right there what the type is,
rather than having to think, "wait, is this a vector of pointers or a vector of
objects?"

[Avi (use Gerrit), 2016-10-17 15:40:31]

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

[commit-bot: I haz the power, 2016-10-17 15:40:46]

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-17 15:54:18]

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

[commit-bot: I haz the power, 2016-10-17 15:54:19]

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

[Bernhard Bauer, 2016-10-17 16:27:59]

https://codereview.chromium.org/2403773002/diff/120001/components/browsing_da...
File components/browsing_data/core/counters/autofill_counter.cc (right):

https://codereview.chromium.org/2403773002/diff/120001/components/browsing_da...
components/browsing_data/core/counters/autofill_counter.cc:103:
std::vector<std::unique_ptr<autofill::CreditCard>> credit_cards = std::move(
On 2016/10/17 15:04:10, Avi wrote:
> On 2016/10/17 14:53:18, Bernhard Bauer wrote:
> > Ugh, this is really not particularly readable. Could we extract a type alias
> for
> > std::vector<std::unique_ptr<autofill::CreditCard>> and use that? Maybe that
> will
> > help a bit.
> 
> The problem is that autofill code is full of vector<CreditCard*>,
> vector<CreditCard>, and vector<unique_ptr<CreditCard>> objects, all over the
> place. As bad as these types are, at least it says right there what the type
is,
> rather than having to think, "wait, is this a vector of pointers or a vector
of
> objects?"

TBH, I'd be happy with just using a local type alias here, or even just `auto
credit_cards` – the whole thing is just used to count the number of elements
anyway.

[Avi (use Gerrit), 2016-10-17 16:48:27]

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

[commit-bot: I haz the power, 2016-10-17 16:48:49]

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-10-17 16:50:33]

https://codereview.chromium.org/2403773002/diff/120001/components/browsing_da...
File components/browsing_data/core/counters/autofill_counter.cc (right):

https://codereview.chromium.org/2403773002/diff/120001/components/browsing_da...
components/browsing_data/core/counters/autofill_counter.cc:103:
std::vector<std::unique_ptr<autofill::CreditCard>> credit_cards = std::move(
On 2016/10/17 16:27:59, Bernhard Bauer wrote:
> On 2016/10/17 15:04:10, Avi wrote:
> > On 2016/10/17 14:53:18, Bernhard Bauer wrote:
> > > Ugh, this is really not particularly readable. Could we extract a type
alias
> > for
> > > std::vector<std::unique_ptr<autofill::CreditCard>> and use that? Maybe
that
> > will
> > > help a bit.
> > 
> > The problem is that autofill code is full of vector<CreditCard*>,
> > vector<CreditCard>, and vector<unique_ptr<CreditCard>> objects, all over the
> > place. As bad as these types are, at least it says right there what the type
> is,
> > rather than having to think, "wait, is this a vector of pointers or a vector
> of
> > objects?"
> 
> TBH, I'd be happy with just using a local type alias here, or even just `auto
> credit_cards` – the whole thing is just used to count the number of elements
> anyway.

Done.

[Bernhard Bauer, 2016-10-17 16:54:26]

Thanks! LGTM.

[commit-bot: I haz the power, 2016-10-17 17:32:25]

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

[commit-bot: I haz the power, 2016-10-17 17:32:26]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-10-17 18:28:38]

Only one substantial concern.

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
File components/webdata/common/web_data_request_manager.cc (right):

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
components/webdata/common/web_data_request_manager.cc:84: for (auto i =
pending_requests_.begin(); i != pending_requests_.end(); ++i) {
Nit: {} unnecessary

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
components/webdata/common/web_data_request_manager.cc:104: NOTREACHED() <<
"Canceling a nonexistent web data service request";
Nit: Consider changing these conditional return blocks to:

  DCHECK(i != pending_requests_.end());

...while you're here to avoid handling DCHECK failure.

You may want to avoid doing this in this patch if you think there's a risk of
introducing a real bug.

(2 places)

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
components/webdata/common/web_data_request_manager.cc:154:
std::unique_ptr<WDTypedResult> r = request->GetResult();
Nit: Just inline into next statement?

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
File components/webdata/common/web_data_results.h (right):

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
components/webdata/common/web_data_results.h:20: // clang-format off
If you add empty "//" comments in the #if/#endif lines below, does clang-format
preserve the comment spacing without the need for these directives?  That might
be cleaner?

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
components/webdata/common/web_data_results.h:75: T& GetValue() { return value_;
}
This second accessor worries me because it looks like we're providing access to
modify the container's value where we didn't use to.  Why do we need this?

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
File components/webdata/common/web_data_service_consumer.h (right):

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
components/webdata/common/web_data_service_consumer.h:18: // The result can be
NULL, if no result is expected or if the database could
Nit: NULL -> null

[Avi (use Gerrit), 2016-10-17 19:05:15]

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

[Avi (use Gerrit), 2016-10-17 19:05:42]

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
File components/webdata/common/web_data_request_manager.cc (right):

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
components/webdata/common/web_data_request_manager.cc:84: for (auto i =
pending_requests_.begin(); i != pending_requests_.end(); ++i) {
On 2016/10/17 18:28:37, Peter Kasting wrote:
> Nit: {} unnecessary

Done.

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
components/webdata/common/web_data_request_manager.cc:104: NOTREACHED() <<
"Canceling a nonexistent web data service request";
On 2016/10/17 18:28:37, Peter Kasting wrote:
> Nit: Consider changing these conditional return blocks to:
> 
>   DCHECK(i != pending_requests_.end());
> 
> ...while you're here to avoid handling DCHECK failure.
> 
> You may want to avoid doing this in this patch if you think there's a risk of
> introducing a real bug.
> 
> (2 places)

This isn't my code, so while this isn't how I'd write this myself, I'm a bit
wary of making that change.

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
components/webdata/common/web_data_request_manager.cc:154:
std::unique_ptr<WDTypedResult> r = request->GetResult();
On 2016/10/17 18:28:37, Peter Kasting wrote:
> Nit: Just inline into next statement?

Done.

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
File components/webdata/common/web_data_results.h (right):

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
components/webdata/common/web_data_results.h:20: // clang-format off
On 2016/10/17 18:28:37, Peter Kasting wrote:
> If you add empty "//" comments in the #if/#endif lines below, does
clang-format
> preserve the comment spacing without the need for these directives?  That
might
> be cleaner?

That works!

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
components/webdata/common/web_data_results.h:75: T& GetValue() { return value_;
}
On 2016/10/17 18:28:37, Peter Kasting wrote:
> This second accessor worries me because it looks like we're providing access
to
> modify the container's value where we didn't use to.  Why do we need this?

Because otherwise we can't move a move-only type out of the value result.

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
File components/webdata/common/web_data_service_consumer.h (right):

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
components/webdata/common/web_data_service_consumer.h:18: // The result can be
NULL, if no result is expected or if the database could
On 2016/10/17 18:28:38, Peter Kasting wrote:
> Nit: NULL -> null

Done.

[commit-bot: I haz the power, 2016-10-17 19:05:46]

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

[Peter Kasting, 2016-10-17 20:41:52]

LGTM

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
File components/webdata/common/web_data_request_manager.cc (right):

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
components/webdata/common/web_data_request_manager.cc:104: NOTREACHED() <<
"Canceling a nonexistent web data service request";
On 2016/10/17 19:05:41, Avi wrote:
> On 2016/10/17 18:28:37, Peter Kasting wrote:
> > Nit: Consider changing these conditional return blocks to:
> > 
> >   DCHECK(i != pending_requests_.end());
> > 
> > ...while you're here to avoid handling DCHECK failure.
> > 
> > You may want to avoid doing this in this patch if you think there's a risk
of
> > introducing a real bug.
> > 
> > (2 places)
> 
> This isn't my code, so while this isn't how I'd write this myself, I'm a bit
> wary of making that change.

Yeah, hence the suggestion to do it separately... I mostly just wanted to (a)
make sure this got fixed and (b) pawn the work off on someone else since I was
too lazy.  But you talked me into it. 
https://codereview.chromium.org/2422323002/

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
File components/webdata/common/web_data_results.h (right):

https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
components/webdata/common/web_data_results.h:75: T& GetValue() { return value_;
}
On 2016/10/17 19:05:41, Avi wrote:
> On 2016/10/17 18:28:37, Peter Kasting wrote:
> > This second accessor worries me because it looks like we're providing access
> to
> > modify the container's value where we didn't use to.  Why do we need this?
> 
> Because otherwise we can't move a move-only type out of the value result.

Ugh... I wonder if there's any other way to make this work that won't otherwise
allow people to muck with |value_|.

T GetValue() { return std::move(value_); }

This probably doesn't work because it won't compile if T is non-movable?

[Avi (use Gerrit), 2016-10-17 20:55:12]

On 2016/10/17 20:41:52, Peter Kasting wrote:
> LGTM
> 
>
https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
> File components/webdata/common/web_data_request_manager.cc (right):
> 
>
https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
> components/webdata/common/web_data_request_manager.cc:104: NOTREACHED() <<
> "Canceling a nonexistent web data service request";
> On 2016/10/17 19:05:41, Avi wrote:
> > On 2016/10/17 18:28:37, Peter Kasting wrote:
> > > Nit: Consider changing these conditional return blocks to:
> > > 
> > >   DCHECK(i != pending_requests_.end());
> > > 
> > > ...while you're here to avoid handling DCHECK failure.
> > > 
> > > You may want to avoid doing this in this patch if you think there's a risk
> of
> > > introducing a real bug.
> > > 
> > > (2 places)
> > 
> > This isn't my code, so while this isn't how I'd write this myself, I'm a bit
> > wary of making that change.
> 
> Yeah, hence the suggestion to do it separately... I mostly just wanted to (a)
> make sure this got fixed and (b) pawn the work off on someone else since I was
> too lazy.  But you talked me into it. 
> https://codereview.chromium.org/2422323002/
> 
>
https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
> File components/webdata/common/web_data_results.h (right):
> 
>
https://codereview.chromium.org/2403773002/diff/160001/components/webdata/com...
> components/webdata/common/web_data_results.h:75: T& GetValue() { return
value_;
> }
> On 2016/10/17 19:05:41, Avi wrote:
> > On 2016/10/17 18:28:37, Peter Kasting wrote:
> > > This second accessor worries me because it looks like we're providing
access
> > to
> > > modify the container's value where we didn't use to.  Why do we need this?
> > 
> > Because otherwise we can't move a move-only type out of the value result.
> 
> Ugh... I wonder if there's any other way to make this work that won't
otherwise
> allow people to muck with |value_|.
> 
> T GetValue() { return std::move(value_); }
> 
> This probably doesn't work because it won't compile if T is non-movable?

SFINAE, so it shouldn't be a problem if no one tries to do it. Let's give it a
whirl.

[Avi (use Gerrit), 2016-10-17 21:03:38]

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

[commit-bot: I haz the power, 2016-10-17 21:04:34]

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-17 22:38:50]

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

[commit-bot: I haz the power, 2016-10-17 22:38:52]

Dry run: This issue passed the CQ dry run.

[Andrew T Wilson (Slow), 2016-10-19 05:00:49]

browser/signin LGTM

[Avi (use Gerrit), 2016-10-19 05:52:40]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2016-10-19 05:52:41]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
zea@chromium.org, michaelbai@chromium.org, bauerb@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2403773002/#ps200001
(title: "pkasting")

[commit-bot: I haz the power, 2016-10-19 05:53:52]

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-19 06:07:59]

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

[commit-bot: I haz the power, 2016-10-19 06:08:00]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Avi (use Gerrit), 2016-10-19 16:25:10]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2016-10-19 16:25:11]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
pkasting@chromium.org, michaelbai@chromium.org, atwilson@chromium.org,
vabr@chromium.org, zea@chromium.org
Link to the patchset: https://codereview.chromium.org/2403773002/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-19 16:25: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-10-19 17:26:18]

Description was changed from

==========
Remove stl_util's STLDeleteContainerPointers and ScopedVector from autofill, do
more general ownership cleanup, and fix random typos.

BUG=555865, 554289
==========

to

==========
Remove stl_util's STLDeleteContainerPointers and ScopedVector from autofill, do
more general ownership cleanup, and fix random typos.

BUG=555865, 554289
==========

[commit-bot: I haz the power, 2016-10-19 17:26:20]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-10-21 13:09:45]

Description was changed from

==========
Remove stl_util's STLDeleteContainerPointers and ScopedVector from autofill, do
more general ownership cleanup, and fix random typos.

BUG=555865, 554289
==========

to

==========
Remove stl_util's STLDeleteContainerPointers and ScopedVector from autofill, do
more general ownership cleanup, and fix random typos.

BUG=555865, 554289

Committed: https://crrev.com/1ce1e60022566d580ddcf30dcb1f03d0426731a2
Cr-Commit-Position: refs/heads/master@{#426232}
==========

[commit-bot: I haz the power, 2016-10-21 13:09:47]

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