[PasswordManager clean-up] Merge copies of CreatePasswordFormFromData

[PasswordManager clean-up] Merge copies of CreatePasswordFormFromData

The Mac unit-test had a separate copy of CreatePasswordFormFromData, which did almost the same as the platform-independent version. This CL merges them by replacing the platform-independent instance with the more recently updated Mac copy. It also renames the methods to make clear they are for testing only.

BUG=451018
Committed: https://crrev.com/9cd9b02ab72d51bb866b79df523b435230c87be9
Cr-Commit-Position: refs/heads/master@{#315529}

[vabr (Chromium), 2015-02-04 10:26:04]

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

[vabr (Chromium), 2015-02-04 10:26:05]

Hi Balázs,

Could you please have a look?

This is not urgent, and is based on https://codereview.chromium.org/866983003,
so currently I cannot run trybots. I'll ping this review once I run them, if you
want to postpone looking at it until then.

Cheers,
Vaclav

[engedy, 2015-02-04 13:33:09]

LGTM % tiny tiny comments. Thanks for the cleanup.

https://codereview.chromium.org/896903003/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_manager_test_utils.cc
(right):

https://codereview.chromium.org/896903003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager_test_utils.cc:23:
form->date_synced = form->date_created + base::TimeDelta::FromDays(1);
nit: This seems a bit random. Could you please add a comment to explain the
rationale behind this?

https://codereview.chromium.org/896903003/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_manager_test_utils.h
(right):

https://codereview.chromium.org/896903003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager_test_utils.h:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
nit: Just wanted to make sure you intentionally kept the year.

https://codereview.chromium.org/896903003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager_test_utils.h:6:
#define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_MANAGER_TEST_UTILS_H_
We should include <vector>.

[vabr (Chromium), 2015-02-04 15:49:02]

New patchsets have been uploaded after l-g-t-m from engedy@chromium.org

[vabr (Chromium), 2015-02-04 16:13:06]

Thanks, Balázs.

Please have a look at how I addressed your comments. It spawned into changing a
new file: chrome/browser/password_manager/password_store_mac_unittest.cc

Cheers,
Vaclav

https://codereview.chromium.org/896903003/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_manager_test_utils.cc
(right):

https://codereview.chromium.org/896903003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager_test_utils.cc:23:
form->date_synced = form->date_created + base::TimeDelta::FromDays(1);
On 2015/02/04 13:33:09, engedy wrote:
> nit: This seems a bit random. Could you please add a comment to explain the
> rationale behind this?

It is random, I just copied it over from the Mac version of this.
To improve, I defined the three random parameters as constants and commented
about them.

https://codereview.chromium.org/896903003/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_manager_test_utils.h
(right):

https://codereview.chromium.org/896903003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager_test_utils.h:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/02/04 13:33:09, engedy wrote:
> nit: Just wanted to make sure you intentionally kept the year.

Thanks for checking. Taking password_manager.h as an example, we don't seem to
bump the copyright year on moving files, therefore I did neither bump it here.

https://codereview.chromium.org/896903003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager_test_utils.h:6:
#define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_MANAGER_TEST_UTILS_H_
On 2015/02/04 13:33:09, engedy wrote:
> We should include <vector>.

Done.

[engedy, 2015-02-04 16:18:31]

LGTM++, thanks!

https://codereview.chromium.org/896903003/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_manager_test_utils.cc
(right):

https://codereview.chromium.org/896903003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager_test_utils.cc:23:
form->date_synced = form->date_created + base::TimeDelta::FromDays(1);
On 2015/02/04 16:13:06, vabr (Chromium) wrote:
> On 2015/02/04 13:33:09, engedy wrote:
> > nit: This seems a bit random. Could you please add a comment to explain the
> > rationale behind this?
> 
> It is random, I just copied it over from the Mac version of this.
> To improve, I defined the three random parameters as constants and commented
> about them.

Looks splendid!

https://codereview.chromium.org/896903003/diff/40001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_mac_unittest.cc (right):

https://codereview.chromium.org/896903003/diff/40001/chrome/browser/password_...
chrome/browser/password_manager/password_store_mac_unittest.cc:243:
SCOPED_TRACE(testing::Message << forms_label << " in test " << test_number
Nice, thanks!

[vabr (Chromium), 2015-02-06 13:19:04]

New patchsets have been uploaded after l-g-t-m from engedy@chromium.org

[vabr (Chromium), 2015-02-06 13:21:35]

Hi Balázs,
No need to re-review yet, I will ping you once
https://codereview.chromium.org/866983003/ lands and this gets also to be run on
the trybots.

To document the changes so far:
Patch set 3->4 is just rebasing, 4->5 is one more change (forgot to remove a
duplicated struct).

Cheers,
Vaclav

[vabr (Chromium), 2015-02-10 08:42:43]

Hi Balázs,

I landed the blocking CL, so this one is ready to go.

Since your last approval, there have been a lot of rebases, and one real change.
The real change is removing the struct PasswordFormData from
chrome/browser/password_manager/password_store_mac_unittest.cc, because that
struct was also duplicated.

Please let me know, if the code still looks good to you, given the above
mentioned change.

Cheers,
Vaclav

[engedy, 2015-02-10 08:54:23]

Still LGTM, thanks!

[vabr (Chromium), 2015-02-10 09:05:53]

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

[vabr (Chromium), 2015-02-10 09:05:53]

Thanks, Balázs.

Drew, Could you please rubber=stamp chrome/browser/sync/test/integration/* ?

Cheers,
Vaclav

[Andrew T Wilson (Slow), 2015-02-10 09:40:24]

sync lgtm

[vabr (Chromium), 2015-02-10 09:50:44]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2015-02-10 09:51:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/896903003/180001

[commit-bot: I haz the power, 2015-02-10 09:54:18]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-02-10 09:54:53]

Patchset 10 (id:??) landed as
https://crrev.com/9cd9b02ab72d51bb866b79df523b435230c87be9
Cr-Commit-Position: refs/heads/master@{#315529}