Enable moving of credentials in HttpPasswordMigrator

Enable moving of credentials in HttpPasswordMigrator

In anticipation of only moving credentials on HSTS enabled hosts this change
adds the option to move credentials in HttpPasswordMigrator. When moving the old
HTTP credential will be removed from the password store.

BUG=571580, 687968
R=vasilii@chromium.org

Review-Url: https://codereview.chromium.org/2667363003
Cr-Commit-Position: refs/heads/master@{#447796}
Committed: https://chromium.googlesource.com/chromium/src/+/d2b7ed2ef23c46cce1d041ab2763132ca435a7bc

[jdoerrie, 2017-02-02 13:51:00]

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

[jdoerrie, 2017-02-02 13:51:10]

Hi Vasilii, please take a look.

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

https://codereview.chromium.org/2667363003/diff/1/components/password_manager...
components/password_manager/core/browser/http_password_migrator.cc:47: const
auto old_form = *form;
I'd like to inline this variable, but this would mean we delete the old form
before we get a chance to save the new form. This seems unsafe to me, thoughts?

[commit-bot: I haz the power, 2017-02-02 13:51:11]

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

[vasilii, 2017-02-02 14:09:13]

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

https://codereview.chromium.org/2667363003/diff/1/components/password_manager...
components/password_manager/core/browser/http_password_migrator.cc:44: // Add
the new credentials to the password store. The HTTP forms aren't
Update the comment.

https://codereview.chromium.org/2667363003/diff/1/components/password_manager...
components/password_manager/core/browser/http_password_migrator.cc:47: const
auto old_form = *form;
On 2017/02/02 13:51:10, jdoerrie wrote:
> I'd like to inline this variable, but this would mean we delete the old form
> before we get a chance to save the new form. This seems unsafe to me,
thoughts?

The best would be to create a stack variable |new_form| without auto. Then set
all the members and add it to the store. After that you can remove |*form|.

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

https://codereview.chromium.org/2667363003/diff/1/components/password_manager...
components/password_manager/core/browser/http_password_migrator_unittest.cc:147:
TEST_F(HttpPasswordMigratorTest, FullStoreWithCopy) {
As the tests are almost identical, I'd write a helper method with a parameter to
avoid code duplication.

[jdoerrie, 2017-02-02 14:15:36]

Description was changed from

==========
Enable moving of credentials in HttpPasswordMigrator

In anticipation of only moving credentials on HSTS enabled hosts this change
adds the option to move credentials in HttpPasswordMigrator. When moving the old
HTTP credential will be removed from the password store.

BUG=571580
R=vasilii@chromium.org
==========

to

==========
Enable moving of credentials in HttpPasswordMigrator

In anticipation of only moving credentials on HSTS enabled hosts this change
adds the option to move credentials in HttpPasswordMigrator. When moving the old
HTTP credential will be removed from the password store.

BUG=571580, 687968
R=vasilii@chromium.org
==========

[commit-bot: I haz the power, 2017-02-02 14:34:25]

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

[commit-bot: I haz the power, 2017-02-02 14:34:26]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2017-02-02 14:44:58]

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

https://codereview.chromium.org/2667363003/diff/1/components/password_manager...
components/password_manager/core/browser/http_password_migrator.cc:44: // Add
the new credentials to the password store. The HTTP forms aren't
On 2017/02/02 14:09:13, vasilii wrote:
> Update the comment.

Done.

https://codereview.chromium.org/2667363003/diff/1/components/password_manager...
components/password_manager/core/browser/http_password_migrator.cc:47: const
auto old_form = *form;
On 2017/02/02 14:09:13, vasilii wrote:
> On 2017/02/02 13:51:10, jdoerrie wrote:
> > I'd like to inline this variable, but this would mean we delete the old form
> > before we get a chance to save the new form. This seems unsafe to me,
> thoughts?
> 
> The best would be to create a stack variable |new_form| without auto. Then set
> all the members and add it to the store. After that you can remove |*form|.

Done.

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

https://codereview.chromium.org/2667363003/diff/1/components/password_manager...
components/password_manager/core/browser/http_password_migrator_unittest.cc:147:
TEST_F(HttpPasswordMigratorTest, FullStoreWithCopy) {
On 2017/02/02 14:09:13, vasilii wrote:
> As the tests are almost identical, I'd write a helper method with a parameter
to
> avoid code duplication.

Done.

[vasilii, 2017-02-02 15:05:28]

lgtm

https://codereview.chromium.org/2667363003/diff/20001/components/password_man...
File components/password_manager/core/browser/http_password_migrator.cc (right):

https://codereview.chromium.org/2667363003/diff/20001/components/password_man...
components/password_manager/core/browser/http_password_migrator.cc:64: *form =
new_form;
std::move() may improve the performance here once somebody adds a move
constructor to PasswordForm.

https://codereview.chromium.org/2667363003/diff/20001/components/password_man...
File components/password_manager/core/browser/http_password_migrator_unittest.cc
(right):

https://codereview.chromium.org/2667363003/diff/20001/components/password_man...
components/password_manager/core/browser/http_password_migrator_unittest.cc:98:
MockConsumer& consumer) {
I'd make them methods so you don't need useless parameters.

https://codereview.chromium.org/2667363003/diff/20001/components/password_man...
components/password_manager/core/browser/http_password_migrator_unittest.cc:126:
EXPECT_CALL(store, RemoveLogin(form));
Optionally: EXPECT_CALL(store, RemoveLogin(form)).Times(mode ==
HttpPasswordMigrator::MigrationMode::MOVE);

[jdoerrie, 2017-02-02 15:36:35]

https://codereview.chromium.org/2667363003/diff/20001/components/password_man...
File components/password_manager/core/browser/http_password_migrator.cc (right):

https://codereview.chromium.org/2667363003/diff/20001/components/password_man...
components/password_manager/core/browser/http_password_migrator.cc:64: *form =
new_form;
On 2017/02/02 15:05:28, vasilii wrote:
> std::move() may improve the performance here once somebody adds a move
> constructor to PasswordForm.

Done.

https://codereview.chromium.org/2667363003/diff/20001/components/password_man...
File components/password_manager/core/browser/http_password_migrator_unittest.cc
(right):

https://codereview.chromium.org/2667363003/diff/20001/components/password_man...
components/password_manager/core/browser/http_password_migrator_unittest.cc:98:
MockConsumer& consumer) {
On 2017/02/02 15:05:28, vasilii wrote:
> I'd make them methods so you don't need useless parameters.

Done.

https://codereview.chromium.org/2667363003/diff/20001/components/password_man...
components/password_manager/core/browser/http_password_migrator_unittest.cc:126:
EXPECT_CALL(store, RemoveLogin(form));
On 2017/02/02 15:05:28, vasilii wrote:
> Optionally: EXPECT_CALL(store, RemoveLogin(form)).Times(mode ==
> HttpPasswordMigrator::MigrationMode::MOVE);

Done.

[jdoerrie, 2017-02-02 15:36:40]

The CQ bit was checked by jdoerrie@chromium.org

[jdoerrie, 2017-02-02 15:36:41]

The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org
Link to the patchset: https://codereview.chromium.org/2667363003/#ps40001
(title: "Comments.")

[commit-bot: I haz the power, 2017-02-02 15:36:50]

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

[vasilii, 2017-02-02 16:04:37]

https://codereview.chromium.org/2667363003/diff/40001/components/password_man...
File components/password_manager/core/browser/http_password_migrator_unittest.cc
(right):

https://codereview.chromium.org/2667363003/diff/40001/components/password_man...
components/password_manager/core/browser/http_password_migrator_unittest.cc:89:
void TestEmptyStore(HttpPasswordMigrator::MigrationMode mode) {
We shouldn't inline such a big method per our style guide.

[jdoerrie, 2017-02-02 16:39:21]

The CQ bit was unchecked by jdoerrie@chromium.org

[jdoerrie, 2017-02-02 16:44:55]

https://codereview.chromium.org/2667363003/diff/40001/components/password_man...
File components/password_manager/core/browser/http_password_migrator_unittest.cc
(right):

https://codereview.chromium.org/2667363003/diff/40001/components/password_man...
components/password_manager/core/browser/http_password_migrator_unittest.cc:89:
void TestEmptyStore(HttpPasswordMigrator::MigrationMode mode) {
On 2017/02/02 16:04:37, vasilii wrote:
> We shouldn't inline such a big method per our style guide.

Done.

[jdoerrie, 2017-02-02 16:44:59]

The CQ bit was checked by jdoerrie@chromium.org

[jdoerrie, 2017-02-02 16:45:00]

The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org
Link to the patchset: https://codereview.chromium.org/2667363003/#ps60001
(title: "Moved test methods out of class")

[commit-bot: I haz the power, 2017-02-02 16:45:16]

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, 2017-02-02 18:06:32]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486053899659390,
"parent_rev": "56ca77a740bca9199956f474009626487dc5aade", "commit_rev":
"d2b7ed2ef23c46cce1d041ab2763132ca435a7bc"}

[commit-bot: I haz the power, 2017-02-02 18:07:11]

Description was changed from

==========
Enable moving of credentials in HttpPasswordMigrator

In anticipation of only moving credentials on HSTS enabled hosts this change
adds the option to move credentials in HttpPasswordMigrator. When moving the old
HTTP credential will be removed from the password store.

BUG=571580, 687968
R=vasilii@chromium.org
==========

to

==========
Enable moving of credentials in HttpPasswordMigrator

In anticipation of only moving credentials on HSTS enabled hosts this change
adds the option to move credentials in HttpPasswordMigrator. When moving the old
HTTP credential will be removed from the password store.

BUG=571580, 687968
R=vasilii@chromium.org

Review-Url: https://codereview.chromium.org/2667363003
Cr-Commit-Position: refs/heads/master@{#447796}
Committed:
https://chromium.googlesource.com/chromium/src/+/d2b7ed2ef23c46cce1d041ab2763...
==========

[commit-bot: I haz the power, 2017-02-02 18:07:12]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/d2b7ed2ef23c46cce1d041ab2763...