components/autofill: scoped_ptr -> unique_ptr

components/autofill: scoped_ptr -> unique_ptr

In response to
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/roY78iTblYc/6juu-QucCgAJ,
which I hope was not an April Fool's joke. :)

BUG=600221
Committed: https://crrev.com/0233748dd66fe1517f6a336b559152f2991771c8
Cr-Commit-Position: refs/heads/master@{#384914}

[vabr (Chromium), 2016-04-04 01:14:24]

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

[vabr (Chromium), 2016-04-04 01:14:25]

Hello Mathieu,
PTAL.
Thank you!
Vaclav

[vabr (Chromium), 2016-04-04 01:42:03]

Description was changed from

==========
components/autofill: scoped_ptr -> unique_ptr

In response to
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/roY78iTblYc/6juu-...,
which I hope was not an April Fool's joke. :)

BUG=554298
==========

to

==========
components/autofill: scoped_ptr -> unique_ptr

In response to
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/roY78iTblYc/6juu-...,
which I hope was not an April Fool's joke. :)

BUG=600221
==========

[vabr (Chromium), 2016-04-04 13:34:51]

Hi Mathieu,
Inspired by your other review, I removed unused #include <memory> where I found
one here (patch set 4). Sorry for changing the CL while it was out for review,
this was the only change.

Cheers,
Vaclav

[Mathieu, 2016-04-04 13:36:17]

Oh noes! You rebased from under me. 

I guess there are 3 big questions:

1) Should we include <memory> in the cc when it's already included in the .h,
assuming we have unique_ptr in both?

2) Should we include <memory> in test files? I think so, but this CL is
inconsistent.

3) In a test file, should we have the .h under test be included at the top of
the file, or within all other includes. I think the former.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
File
components/autofill/content/browser/request_autocomplete_manager_unittest.cc
(right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
components/autofill/content/browser/request_autocomplete_manager_unittest.cc:5:
#include "components/autofill/content/browser/request_autocomplete_manager.h"
in another CL,
https://codereview.chromium.org/1857663002/diff/1/chrome/browser/autofill/aut...,
the opposite was done (bringing the tested include in the normal include list).
We should be consistent.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
components/autofill/content/browser/request_autocomplete_manager_unittest.cc:9:
#include <tuple>
do you think we have to include <memory> for the uses of unique_ptr

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
File components/autofill/content/browser/risk/fingerprint_browsertest.cc
(right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
components/autofill/content/browser/risk/fingerprint_browsertest.cc:44: const
base::Callback<void(std::unique_ptr<Fingerprint>)>& callback);
Is it a pattern not to include <memory> for tests? If so, disregard my previous
comments to that effect :)

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:9: #include
<memory>
it's also been included in the .h for this file. In other files in this CL, this
.cc include would be omitted. We should be consistent and pick one.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:10:
#include <memory>
same comment as for password_autofill_agent.cc

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
File components/autofill/content/renderer/password_generation_agent.cc (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_generation_agent.cc:7: #include
<memory>
and here too, in most other cc files it's omitted if the .h has it (not sure
what the convention is)

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_client.h (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_client.h:8: #include <vector>
#include <memory>

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_download_manager_unittest.cc
(right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_download_manager_unittest.cc:7:
#include <stddef.h>
#include <memory>

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate_unittest.cc
(right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate_unittest.cc:5:
#include <vector>
#include <memory>

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_merge_unittest.cc (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_merge_unittest.cc:7: #include <map>
#include <memory>

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:9: #include <map>
#include <memory>

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/form_structure.cc (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/form_structure.cc:11: #include <memory>
same

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/form_structure.h (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/form_structure.h:10: #include <memory>
still needed?

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/legal_message_line.h (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/legal_message_line.h:8: #include <memory>
needed?

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/payments/payments_request.h (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/payments/payments_request.h:7: 
include <memory>?

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File
components/autofill/core/browser/ui/card_unmask_prompt_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/ui/card_unmask_prompt_controller_impl_unittest.cc:8:

#include <memory>

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_data_type_controller_unittest.cc
(right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_data_type_controller_unittest.cc:7:
#include <memory>
needed?

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service.h
(right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service.h:7:
#include <map>
#include <memory>

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_webdata.h (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_webdata.h:8: #include <memory>
needed?

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/common/password_form_fill_data.h (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/common/password_form_fill_data.h:9: #include <memory>
needed?

[vabr (Chromium), 2016-04-04 14:29:05]

Thanks, Mathieu, for the very helpful and careful review!

Please have a look at my replies below.

Thanks!
Vaclav

> 1) Should we include <memory> in the cc when it's already included in the .h,
assuming we have unique_ptr in both?
> 2) Should we include <memory> in test files? I think so, but this CL is
inconsistent.

To answer both 1 and 2: my view is that every file should #include <memory> if
it uses unique_ptr independently of other files. The only case when this is not
"independent" is when a header file puts unique_ptr in the signature, and the
.cc/unittest implements that. (See also below my response to one of your
comments.)

> 3) In a test file, should we have the .h under test be included at the top of
the file, or within all other includes. I think the former.

I agree. The place in the other CL was due to the header being in a different
directory, and the corresponding script not recognising it.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
File
components/autofill/content/browser/request_autocomplete_manager_unittest.cc
(right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
components/autofill/content/browser/request_autocomplete_manager_unittest.cc:5:
#include "components/autofill/content/browser/request_autocomplete_manager.h"
On 2016/04/04 13:36:16, Mathieu Perreault wrote:
> in another CL,
>
https://codereview.chromium.org/1857663002/diff/1/chrome/browser/autofill/aut...,
> the opposite was done (bringing the tested include in the normal include
list).
> We should be consistent.

I agree, will change the other file.

To explain: this rearranging was done by a script (linked to from the
spreadsheet in this CL description). The script recognises the matching of X.h
with X_unittest.h. This fails in case of
autofill_save_card_infobar_delegate_mobile*, because the header is in
components, while the test in //chrome.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
components/autofill/content/browser/request_autocomplete_manager_unittest.cc:9:
#include <tuple>
On 2016/04/04 13:36:16, Mathieu Perreault wrote:
> do you think we have to include <memory> for the uses of unique_ptr

The style guide does not specify this, as far as I saw.
Personally, I would expect every file to #include <memory> if it uses
unique_ptr, with the exception of a .cc file implementing a a function defined
in a header file, and the unique_ptr comes from the signature declared in the
header. If there are alternative views, I'm happy to adjust mine, though.

I'll do my best to be consistent with the above in this CL, however note that
this mechanical change only reflects the state of the code before, modulo
renaming scoped_ptr to unique_ptr and rewriting the associated #includes.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
File components/autofill/content/browser/risk/fingerprint_browsertest.cc
(right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
components/autofill/content/browser/risk/fingerprint_browsertest.cc:44: const
base::Callback<void(std::unique_ptr<Fingerprint>)>& callback);
On 2016/04/04 13:36:16, Mathieu Perreault wrote:
> Is it a pattern not to include <memory> for tests? If so, disregard my
previous
> comments to that effect :)

I believe this is a good place to have the #include. Thanks for flagging it!

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:9: #include
<memory>
On 2016/04/04 13:36:16, Mathieu Perreault wrote:
> it's also been included in the .h for this file. In other files in this CL,
this
> .cc include would be omitted. We should be consistent and pick one.

My reason to keep this here:
There are 2 kinds of unique_ptr usage in this file:
(A) As on line 1195, in a method signature coming from the .h file.
(B) As on line 131, as an implementation detail.

For (A) I would not #include <memory> in the .cc, but for (B) I would. The
reason is: if the header eliminates the unique_ptr (and the #include), the .cc
file still needs it because of (B). In most cases, the compiler would throw an
error, but it mail fail to do so (if there are other indirect #include
<memory>), resulting in a broken IWYU.

Having said all of that, I'm not strongly opposed to not #including stuff that
is already in the header as a rule of thumb, if you see enough benefit in that.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:10:
#include <memory>
On 2016/04/04 13:36:16, Mathieu Perreault wrote:
> same comment as for password_autofill_agent.cc

Here it all derives from the header, so I agree to remove <memory> from here.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
File components/autofill/content/renderer/password_generation_agent.cc (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_generation_agent.cc:7: #include
<memory>
On 2016/04/04 13:36:17, Mathieu Perreault wrote:
> and here too, in most other cc files it's omitted if the .h has it (not sure
> what the convention is)

Here, on the other hand, line 207, for example, seems like an implementation
detail independent of the header, so I'd keep it.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_client.h (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_client.h:8: #include <vector>
On 2016/04/04 13:36:17, Mathieu Perreault wrote:
> #include <memory>

Good catch!
Done.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_download_manager_unittest.cc
(right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_download_manager_unittest.cc:7:
#include <stddef.h>
On 2016/04/04 13:36:17, Mathieu Perreault wrote:
> #include <memory>

Done.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate_unittest.cc
(right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate_unittest.cc:5:
#include <vector>
On 2016/04/04 13:36:17, Mathieu Perreault wrote:
> #include <memory>

Done.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_merge_unittest.cc (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_merge_unittest.cc:7: #include <map>
On 2016/04/04 13:36:17, Mathieu Perreault wrote:
> #include <memory>

Done.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:9: #include <map>
On 2016/04/04 13:36:17, Mathieu Perreault wrote:
> #include <memory>

Done.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/form_structure.cc (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/form_structure.cc:11: #include <memory>
On 2016/04/04 13:36:17, Mathieu Perreault wrote:
> same

Done.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/form_structure.h (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/form_structure.h:10: #include <memory>
On 2016/04/04 13:36:17, Mathieu Perreault wrote:
> still needed?

No, done.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/legal_message_line.h (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/legal_message_line.h:8: #include <memory>
On 2016/04/04 13:36:17, Mathieu Perreault wrote:
> needed?

Done.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/payments/payments_request.h (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/payments/payments_request.h:7: 
On 2016/04/04 13:36:17, Mathieu Perreault wrote:
> include <memory>?

Done.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File
components/autofill/core/browser/ui/card_unmask_prompt_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/ui/card_unmask_prompt_controller_impl_unittest.cc:8:

On 2016/04/04 13:36:17, Mathieu Perreault wrote:
> #include <memory>

Done.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_data_type_controller_unittest.cc
(right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_data_type_controller_unittest.cc:7:
#include <memory>
On 2016/04/04 13:36:17, Mathieu Perreault wrote:
> needed?

No, removed.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service.h
(right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service.h:7:
#include <map>
On 2016/04/04 13:36:17, Mathieu Perreault wrote:
> #include <memory>

Done.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_webdata.h (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_webdata.h:8: #include <memory>
On 2016/04/04 13:36:17, Mathieu Perreault wrote:
> needed?

No, removed.

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
File components/autofill/core/common/password_form_fill_data.h (right):

https://codereview.chromium.org/1859453002/diff/20001/components/autofill/cor...
components/autofill/core/common/password_form_fill_data.h:9: #include <memory>
On 2016/04/04 13:36:17, Mathieu Perreault wrote:
> needed?

No, removed.

[Mathieu, 2016-04-04 14:38:01]

Thanks, I agree with your principles, and I'm glad we were able to clean up the
includes as well :) lgtm

[vabr (Chromium), 2016-04-04 14:38:26]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2016-04-04 14:38:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1859453002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1859453002/100001

[commit-bot: I haz the power, 2016-04-04 15:39:16]

Description was changed from

==========
components/autofill: scoped_ptr -> unique_ptr

In response to
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/roY78iTblYc/6juu-...,
which I hope was not an April Fool's joke. :)

BUG=600221
==========

to

==========
components/autofill: scoped_ptr -> unique_ptr

In response to
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/roY78iTblYc/6juu-...,
which I hope was not an April Fool's joke. :)

BUG=600221
==========

[commit-bot: I haz the power, 2016-04-04 15:39:18]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-04 15:41:05]

Description was changed from

==========
components/autofill: scoped_ptr -> unique_ptr

In response to
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/roY78iTblYc/6juu-...,
which I hope was not an April Fool's joke. :)

BUG=600221
==========

to

==========
components/autofill: scoped_ptr -> unique_ptr

In response to
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/roY78iTblYc/6juu-...,
which I hope was not an April Fool's joke. :)

BUG=600221

Committed: https://crrev.com/0233748dd66fe1517f6a336b559152f2991771c8
Cr-Commit-Position: refs/heads/master@{#384914}
==========

[commit-bot: I haz the power, 2016-04-04 15:41:06]

Patchset 6 (id:??) landed as
https://crrev.com/0233748dd66fe1517f6a336b559152f2991771c8
Cr-Commit-Position: refs/heads/master@{#384914}