Move Credentials when migrating to HSTS page

Move Credentials when migrating to HSTS page

This change enables moving credentials during migration when the corresponding
site has HSTS enabled. Prior to this change old HTTP credentials were kept,
leading to unnecessary duplication.

R=vasilii@chromium.org,isherman@chromium.org
BUG=687968
Review-Url: https://codereview.chromium.org/2721663002
Cr-Commit-Position: refs/heads/master@{#456704}
Committed: https://chromium.googlesource.com/chromium/src/+/5cda81b4d16e323763e51f22582a9144dc64c30e

[jdoerrie, 2017-02-27 18:28:23]

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

[commit-bot: I haz the power, 2017-02-27 18:29:08]

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

[jdoerrie, 2017-02-27 18:33:12]

isherman@, please review histograms.xml.
vasilii@, please review the rest. I will add browser tests before landing this,
but I wanted to split it off the other CL and also get the histogram reviewed.

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

https://codereview.chromium.org/2721663002/diff/1/components/password_manager...
components/password_manager/core/browser/http_password_migrator.cc:70: ?
metrics_util::HTTP_PASSWORD_MIGRATION_MODE_MOVE
Having two MigrationMode enums feels kinda redundant, but I didn't feel
comfortable logging our enum directly to UMA. Also having two different enums
enables having a _COUNT member, so that in case more modes would be added at
some point we have to change less code. Not sure how relevant the last point is,
though. WDYT?

[commit-bot: I haz the power, 2017-02-27 19:05:57]

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

[commit-bot: I haz the power, 2017-02-27 19:05:57]

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

[Ilya Sherman, 2017-02-27 19:34:17]

Metrics LGTM % a nit:

https://codereview.chromium.org/2721663002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2721663002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:94724: +  <int value="1" label="HTTP
credentials are copied during migration to HTTPS"/>
nit: I'd shorten these labels to "Moved" and "Copied", for ease of scanning in
the UI.  If you like, you can add extra hover text, like so:

<int value="0" label="Moved">HTTP credentials were moved during migration to
HTTPS</int>

[vasilii, 2017-02-28 09:10:37]

Looking forward to the test.

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

https://codereview.chromium.org/2721663002/diff/1/components/password_manager...
components/password_manager/core/browser/form_fetcher_impl.cc:113: const auto
migration_mode =
HttpPasswordMigrator::MigrationMode is better

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

https://codereview.chromium.org/2721663002/diff/1/components/password_manager...
components/password_manager/core/browser/http_password_migrator.cc:70: ?
metrics_util::HTTP_PASSWORD_MIGRATION_MODE_MOVE
On 2017/02/27 18:33:12, jdoerrie wrote:
> Having two MigrationMode enums feels kinda redundant, but I didn't feel
> comfortable logging our enum directly to UMA. Also having two different enums
> enables having a _COUNT member, so that in case more modes would be added at
> some point we have to change less code. Not sure how relevant the last point
is,
> though. WDYT?

You did everything right.

https://codereview.chromium.org/2721663002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2721663002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:94724: +  <int value="1" label="HTTP
credentials are copied during migration to HTTPS"/>
On 2017/02/27 19:34:17, Ilya Sherman wrote:
> nit: I'd shorten these labels to "Moved" and "Copied", for ease of scanning in
> the UI.  If you like, you can add extra hover text, like so:
> 
> <int value="0" label="Moved">HTTP credentials were moved during migration to
> HTTPS</int>

+1

[jdoerrie, 2017-03-06 17:53:22]

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

[commit-bot: I haz the power, 2017-03-06 17:53:42]

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

[jdoerrie, 2017-03-06 17:54:28]

Hi Vasilii, please have another look :)

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

https://codereview.chromium.org/2721663002/diff/1/components/password_manager...
components/password_manager/core/browser/form_fetcher_impl.cc:113: const auto
migration_mode =
On 2017/02/28 09:10:36, vasilii wrote:
> HttpPasswordMigrator::MigrationMode is better

Done.

https://codereview.chromium.org/2721663002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2721663002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:94724: +  <int value="1" label="HTTP
credentials are copied during migration to HTTPS"/>
On 2017/02/28 09:10:37, vasilii wrote:
> On 2017/02/27 19:34:17, Ilya Sherman wrote:
> > nit: I'd shorten these labels to "Moved" and "Copied", for ease of scanning
in
> > the UI.  If you like, you can add extra hover text, like so:
> > 
> > <int value="0" label="Moved">HTTP credentials were moved during migration to
> > HTTPS</int>
> 
> +1

Done.

[commit-bot: I haz the power, 2017-03-06 18:34:12]

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

[commit-bot: I haz the power, 2017-03-06 18:34:13]

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_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jdoerrie, 2017-03-07 14:49:32]

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

[commit-bot: I haz the power, 2017-03-07 14:50:00]

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, 2017-03-07 15:31:49]

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

[commit-bot: I haz the power, 2017-03-07 15:31:50]

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

[jdoerrie, 2017-03-07 16:24:54]

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

[commit-bot: I haz the power, 2017-03-07 16:25: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, 2017-03-07 19:10:25]

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

[commit-bot: I haz the power, 2017-03-07 19:10:28]

Dry run: This issue passed the CQ dry run.

[vasilii, 2017-03-08 13:31:16]

HttpPasswordMigrator should also support statistics removal. I'm fine with doing
it in a separate CL.

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/credential_manager_browsertest.cc (right):

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/credential_manager_browsertest.cc:195:
mock_cert_verifier().AddResultForCert(cert.get(), verify_result, net::OK);
Why do we need this setup?

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/credential_manager_browsertest.cc:223:
browser(), https_test_server().GetURL("/password/password_form.html"));
I'm afraid that you are testing the autofill web page. Can you navigate to blank
page without any password form?

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/credential_manager_browsertest.cc:228:
"navigator.credentials.get({password: true})"
I think get() should be enough to trigger the migration.

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/credential_manager_browsertest.cc:244:
observer.Wait();
I think it's all excessive.

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_test_base.cc (right):

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/password_manager_test_base.cc:60: FAIL();
ADD_FAILURE?

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/password_manager_test_base.cc:323:
base::RunLoop().RunUntilIdle();
I'd avoid it in the browser tests. How about PostTaskAndReplyWithResult?

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

https://codereview.chromium.org/2721663002/diff/80001/components/password_man...
components/password_manager/core/browser/http_password_migrator.cc:23: if
(migrator)
{}

https://codereview.chromium.org/2721663002/diff/80001/components/password_man...
File components/password_manager/core/browser/http_password_migrator.h (right):

https://codereview.chromium.org/2721663002/diff/80001/components/password_man...
components/password_manager/core/browser/http_password_migrator.h:53: void
OnHSTSQueryResult(bool is_hsts);
Is it PasswordStoreConsumer?
Why is it public?

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

https://codereview.chromium.org/2721663002/diff/80001/components/password_man...
components/password_manager/core/browser/http_password_migrator_unittest.cc:129:
.WillOnce(WithArg<1>(InvokeCallbackWith(is_hsts)));
I'd prefer to capture the callback and invoke it explicitly below as it matches
the actual behavior.

https://codereview.chromium.org/2721663002/diff/80001/components/password_man...
File components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc
(right):

https://codereview.chromium.org/2721663002/diff/80001/components/password_man...
components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc:142:
.WillOnce(WithArg<1>(InvokeCallbackWith(test_case.is_hsts)));
I'd split the callback from the expectation.

EXPECT_CALL();
cleaner.OnGetPasswordStoreResults(MakeResults({form}));
EXPECT_CALL(store(), RemoveLogin(form)).Times(test_case.is_deleted);
call the callback

https://codereview.chromium.org/2721663002/diff/80001/components/password_man...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/2721663002/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager_client.h:65: //
result.
on the calling thread

https://codereview.chromium.org/2721663002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2721663002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:46607: +    on HTTPS password form load
when there are no credentials saved.
Do we record when nothing is done (no credentials for HTTP)? If yes then we
probably shouldn't.

[jdoerrie, 2017-03-09 18:35:01]

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

[commit-bot: I haz the power, 2017-03-09 18:35:33]

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

[jdoerrie, 2017-03-09 18:35:50]

Next round. I agree with supporting statistics removal in a follow-up CL.

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/credential_manager_browsertest.cc (right):

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/credential_manager_browsertest.cc:195:
mock_cert_verifier().AddResultForCert(cert.get(), verify_result, net::OK);
On 2017/03/08 13:31:16, vasilii wrote:
> Why do we need this setup?

Because otherwise |CredentialManagerImpl::Get| never seems to get called. I
don't fully understand this yet, will further investigate.

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/credential_manager_browsertest.cc:223:
browser(), https_test_server().GetURL("/password/password_form.html"));
On 2017/03/08 13:31:16, vasilii wrote:
> I'm afraid that you are testing the autofill web page. Can you navigate to
blank
> page without any password form?

Done.

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/credential_manager_browsertest.cc:228:
"navigator.credentials.get({password: true})"
On 2017/03/08 13:31:16, vasilii wrote:
> I think get() should be enough to trigger the migration.

Done.

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/credential_manager_browsertest.cc:244:
observer.Wait();
On 2017/03/08 13:31:16, vasilii wrote:
> I think it's all excessive.

Done.

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_test_base.cc (right):

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/password_manager_test_base.cc:60: FAIL();
On 2017/03/08 13:31:16, vasilii wrote:
> ADD_FAILURE?

Done.

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/password_manager_test_base.cc:323:
base::RunLoop().RunUntilIdle();
On 2017/03/08 13:31:16, vasilii wrote:
> I'd avoid it in the browser tests. How about PostTaskAndReplyWithResult?

Could you elaborate on what you are thinking of? I could image adding a callback
argument here, calling |PostTaskAndReply| instead of |PostTask| and move all of
|ObsoleteHttpCredentialMovedOnMigrationToHstsSite| in say
|ObsoleteHttpCredentialMovedOnMigrationToHstsSiteImpl| and simply call
AddHSTSHost(host, ...HstsSiteImpl) in ...HstsSite. Were you thinking of
something similar?

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

https://codereview.chromium.org/2721663002/diff/80001/components/password_man...
components/password_manager/core/browser/http_password_migrator.cc:23: if
(migrator)
On 2017/03/08 13:31:16, vasilii wrote:
> {}

Done.

https://codereview.chromium.org/2721663002/diff/80001/components/password_man...
File components/password_manager/core/browser/http_password_migrator.h (right):

https://codereview.chromium.org/2721663002/diff/80001/components/password_man...
components/password_manager/core/browser/http_password_migrator.h:53: void
OnHSTSQueryResult(bool is_hsts);
On 2017/03/08 13:31:16, vasilii wrote:
> Is it PasswordStoreConsumer?
> Why is it public?

Not in PasswordStoreConsumer. This is public because we call it in
OnHSTSQueryResultHelper out of the anonymous namespace.

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

https://codereview.chromium.org/2721663002/diff/80001/components/password_man...
components/password_manager/core/browser/http_password_migrator_unittest.cc:129:
.WillOnce(WithArg<1>(InvokeCallbackWith(is_hsts)));
On 2017/03/08 13:31:16, vasilii wrote:
> I'd prefer to capture the callback and invoke it explicitly below as it
matches
> the actual behavior.

Done.

https://codereview.chromium.org/2721663002/diff/80001/components/password_man...
File components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc
(right):

https://codereview.chromium.org/2721663002/diff/80001/components/password_man...
components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc:142:
.WillOnce(WithArg<1>(InvokeCallbackWith(test_case.is_hsts)));
On 2017/03/08 13:31:16, vasilii wrote:
> I'd split the callback from the expectation.
> 
> EXPECT_CALL();
> cleaner.OnGetPasswordStoreResults(MakeResults({form}));
> EXPECT_CALL(store(), RemoveLogin(form)).Times(test_case.is_deleted);
> call the callback

Done.

https://codereview.chromium.org/2721663002/diff/80001/components/password_man...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/2721663002/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager_client.h:65: //
result.
On 2017/03/08 13:31:16, vasilii wrote:
> on the calling thread

Done.

https://codereview.chromium.org/2721663002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2721663002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:46607: +    on HTTPS password form load
when there are no credentials saved.
On 2017/03/08 13:31:16, vasilii wrote:
> Do we record when nothing is done (no credentials for HTTP)? If yes then we
> probably shouldn't.

Done.

[commit-bot: I haz the power, 2017-03-09 19:39:08]

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

[commit-bot: I haz the power, 2017-03-09 19:39:09]

Dry run: This issue passed the CQ dry run.

[vasilii, 2017-03-10 10:21:01]

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/credential_manager_browsertest.cc (right):

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/credential_manager_browsertest.cc:195:
mock_cert_verifier().AddResultForCert(cert.get(), verify_result, net::OK);
On 2017/03/09 18:35:49, jdoerrie wrote:
> On 2017/03/08 13:31:16, vasilii wrote:
> > Why do we need this setup?
> 
> Because otherwise |CredentialManagerImpl::Get| never seems to get called. I
> don't fully understand this yet, will further investigate.

Strange because other CredentialManagerBrowserTest tests don't require any
special set up. Something to investigate.

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_test_base.cc (right):

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/password_manager_test_base.cc:323:
base::RunLoop().RunUntilIdle();
On 2017/03/09 18:35:49, jdoerrie wrote:
> On 2017/03/08 13:31:16, vasilii wrote:
> > I'd avoid it in the browser tests. How about PostTaskAndReplyWithResult?
> 
> Could you elaborate on what you are thinking of? I could image adding a
callback
> argument here, calling |PostTaskAndReply| instead of |PostTask| and move all
of
> |ObsoleteHttpCredentialMovedOnMigrationToHstsSite| in say
> |ObsoleteHttpCredentialMovedOnMigrationToHstsSiteImpl| and simply call
> AddHSTSHost(host, ...HstsSiteImpl) in ...HstsSite. Were you thinking of
> something similar?

I meant something like this

scoped_refptr<content::MessageLoopRunner> message_loop_runner = new ...;

content::BrowserThread::PostTaskAndReplyWithResult(content::BrowserThread::IO,
FROM_HERE,
    base::Bind(&AddHSTSHostImpl, ...),
    message_loop_runner->QuitClosure());
message_loop_runner->Run();

[jdoerrie, 2017-03-10 13:47:22]

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

[commit-bot: I haz the power, 2017-03-10 13:47:30]

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

[jdoerrie, 2017-03-10 13:53:10]

Hi Vasilii, please have another look :)

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/credential_manager_browsertest.cc (right):

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/credential_manager_browsertest.cc:195:
mock_cert_verifier().AddResultForCert(cert.get(), verify_result, net::OK);
On 2017/03/10 10:21:00, vasilii wrote:
> On 2017/03/09 18:35:49, jdoerrie wrote:
> > On 2017/03/08 13:31:16, vasilii wrote:
> > > Why do we need this setup?
> > 
> > Because otherwise |CredentialManagerImpl::Get| never seems to get called. I
> > don't fully understand this yet, will further investigate.
> 
> Strange because other CredentialManagerBrowserTest tests don't require any
> special set up. Something to investigate.

The other tests pass because they are not making use of the https_test_server.
Here we need it to trigger the migration. Without the cert setup we can't
execute JavaScript due to the insecure origin.

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_test_base.cc (right):

https://codereview.chromium.org/2721663002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/password_manager_test_base.cc:323:
base::RunLoop().RunUntilIdle();
On 2017/03/10 10:21:00, vasilii wrote:
> On 2017/03/09 18:35:49, jdoerrie wrote:
> > On 2017/03/08 13:31:16, vasilii wrote:
> > > I'd avoid it in the browser tests. How about PostTaskAndReplyWithResult?
> > 
> > Could you elaborate on what you are thinking of? I could image adding a
> callback
> > argument here, calling |PostTaskAndReply| instead of |PostTask| and move all
> of
> > |ObsoleteHttpCredentialMovedOnMigrationToHstsSite| in say
> > |ObsoleteHttpCredentialMovedOnMigrationToHstsSiteImpl| and simply call
> > AddHSTSHost(host, ...HstsSiteImpl) in ...HstsSite. Were you thinking of
> > something similar?
> 
> I meant something like this
> 
> scoped_refptr<content::MessageLoopRunner> message_loop_runner = new ...;
> 
> content::BrowserThread::PostTaskAndReplyWithResult(content::BrowserThread::IO,
> FROM_HERE,
>     base::Bind(&AddHSTSHostImpl, ...),
>     message_loop_runner->QuitClosure());
> message_loop_runner->Run();

Done.

[jdoerrie, 2017-03-10 13:53:31]

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

[commit-bot: I haz the power, 2017-03-10 13:53:42]

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, 2017-03-10 14:56:46]

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

[commit-bot: I haz the power, 2017-03-10 14:56:47]

Dry run: This issue passed the CQ dry run.

[vasilii, 2017-03-10 15:37:43]

I think you need to sync with the IO thread as well in your browser tests to
avoid flakiness.

https://codereview.chromium.org/2721663002/diff/140001/chrome/browser/passwor...
File chrome/browser/password_manager/credential_manager_browsertest.cc (right):

https://codereview.chromium.org/2721663002/diff/140001/chrome/browser/passwor...
chrome/browser/password_manager/credential_manager_browsertest.cc:178:
http_form.action = https_origin;
Are you sure we need it?

https://codereview.chromium.org/2721663002/diff/140001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/2721663002/diff/140001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:1519:
http_form.action = https_origin;
Needed?

https://codereview.chromium.org/2721663002/diff/140001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:1537:
WaitForPasswordStore();
Why don't we wait here twice?

https://codereview.chromium.org/2721663002/diff/140001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.cc (right):

https://codereview.chromium.org/2721663002/diff/140001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:177: void
PasswordManagerBrowserTestBase::SetUpInProcessBrowserTestFixture() {
Why did you need to overwrite those methods?

https://codereview.chromium.org/2721663002/diff/140001/components/password_ma...
File components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc
(right):

https://codereview.chromium.org/2721663002/diff/140001/components/password_ma...
components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc:144:
if (callback)
I think you know exactly when it's true.

https://codereview.chromium.org/2721663002/diff/140001/components/password_ma...
components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc:205:
if (callback)
Same as above.

https://codereview.chromium.org/2721663002/diff/140001/components/password_ma...
components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc:242:
if (callback)
And here.

https://codereview.chromium.org/2721663002/diff/140001/components/password_ma...
components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc:244:
base::RunLoop().RunUntilIdle();
A comment? I already almost forgot why it's needed.

[jdoerrie, 2017-03-13 17:25:06]

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

[commit-bot: I haz the power, 2017-03-13 17:25:33]

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

[jdoerrie, 2017-03-13 17:26:22]

https://codereview.chromium.org/2721663002/diff/140001/chrome/browser/passwor...
File chrome/browser/password_manager/credential_manager_browsertest.cc (right):

https://codereview.chromium.org/2721663002/diff/140001/chrome/browser/passwor...
chrome/browser/password_manager/credential_manager_browsertest.cc:178:
http_form.action = https_origin;
On 2017/03/10 15:37:43, vasilii wrote:
> Are you sure we need it?

No, not needed.

https://codereview.chromium.org/2721663002/diff/140001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/2721663002/diff/140001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:1519:
http_form.action = https_origin;
On 2017/03/10 15:37:43, vasilii wrote:
> Needed?

Nope.

https://codereview.chromium.org/2721663002/diff/140001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:1537:
WaitForPasswordStore();
On 2017/03/10 15:37:43, vasilii wrote:
> Why don't we wait here twice?

Testing locally the test passed without the second call. However, the situation
here should be similar to
|CredentialManagerBrowserTest.ObsoleteHttpCredentialMovedOnMigrationToHstsSite|,
so I added another call for robustness.

https://codereview.chromium.org/2721663002/diff/140001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.cc (right):

https://codereview.chromium.org/2721663002/diff/140001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:177: void
PasswordManagerBrowserTestBase::SetUpInProcessBrowserTestFixture() {
On 2017/03/10 15:37:43, vasilii wrote:
> Why did you need to overwrite those methods?

I was copying the behavior in |CertVerifierBrowserTest|

(https://codesearch.chromium.org/chromium/src/chrome/browser/ssl/cert_verifier...).
When trying to moving the logic into |{SetUp,TearDown}OnMainThread|, I obtained
a "Your connection is not private" error. So it seems like setting the
certificate in those methods is too late to have the desired effect.

https://codereview.chromium.org/2721663002/diff/140001/components/password_ma...
File components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc
(right):

https://codereview.chromium.org/2721663002/diff/140001/components/password_ma...
components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc:144:
if (callback)
On 2017/03/10 15:37:43, vasilii wrote:
> I think you know exactly when it's true.

Done.

https://codereview.chromium.org/2721663002/diff/140001/components/password_ma...
components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc:205:
if (callback)
On 2017/03/10 15:37:43, vasilii wrote:
> Same as above.

Done.

https://codereview.chromium.org/2721663002/diff/140001/components/password_ma...
components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc:242:
if (callback)
On 2017/03/10 15:37:43, vasilii wrote:
> And here.

Done.

https://codereview.chromium.org/2721663002/diff/140001/components/password_ma...
components/password_manager/core/browser/obsolete_http_cleaner_unittest.cc:244:
base::RunLoop().RunUntilIdle();
On 2017/03/10 15:37:43, vasilii wrote:
> A comment? I already almost forgot why it's needed.

Done.

[commit-bot: I haz the power, 2017-03-13 18:58:22]

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

[commit-bot: I haz the power, 2017-03-13 18:58:23]

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

[vasilii, 2017-03-14 10:48:32]

https://codereview.chromium.org/2721663002/diff/160001/chrome/browser/passwor...
File chrome/browser/password_manager/credential_manager_browsertest.cc (right):

https://codereview.chromium.org/2721663002/diff/160001/chrome/browser/passwor...
chrome/browser/password_manager/credential_manager_browsertest.cc:203: 
See my comment on another test.

https://codereview.chromium.org/2721663002/diff/160001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/2721663002/diff/160001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:1541: 
By syncing with the IO thread I meant adding some code here. NavigateToURL
triggers a request for HSTS state. It's processed on the IO thread and we want
to make sure that the migration happened before checking the password store.

https://codereview.chromium.org/2721663002/diff/160001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.cc (right):

https://codereview.chromium.org/2721663002/diff/160001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:335: // Posts
AddHSTSHostImpl task to IO thread and empty task to the UI thread to
The implementation before was correct. Now it's something strange.

[jdoerrie, 2017-03-14 12:56:51]

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

[commit-bot: I haz the power, 2017-03-14 12:57:19]

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

[jdoerrie, 2017-03-14 12:57:37]

https://codereview.chromium.org/2721663002/diff/160001/chrome/browser/passwor...
File chrome/browser/password_manager/credential_manager_browsertest.cc (right):

https://codereview.chromium.org/2721663002/diff/160001/chrome/browser/passwor...
chrome/browser/password_manager/credential_manager_browsertest.cc:203: 
On 2017/03/14 10:48:31, vasilii wrote:
> See my comment on another test.

Done.

https://codereview.chromium.org/2721663002/diff/160001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/2721663002/diff/160001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:1541: 
On 2017/03/14 10:48:32, vasilii wrote:
> By syncing with the IO thread I meant adding some code here. NavigateToURL
> triggers a request for HSTS state. It's processed on the IO thread and we want
> to make sure that the migration happened before checking the password store.

Done.

https://codereview.chromium.org/2721663002/diff/160001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.cc (right):

https://codereview.chromium.org/2721663002/diff/160001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:335: // Posts
AddHSTSHostImpl task to IO thread and empty task to the UI thread to
On 2017/03/14 10:48:32, vasilii wrote:
> The implementation before was correct. Now it's something strange. 

Done.

[vasilii, 2017-03-14 13:13:22]

lgtm

https://codereview.chromium.org/2721663002/diff/180001/chrome/browser/passwor...
File chrome/browser/password_manager/credential_manager_browsertest.cc (right):

https://codereview.chromium.org/2721663002/diff/180001/chrome/browser/passwor...
chrome/browser/password_manager/credential_manager_browsertest.cc:204: // Sync
with IO thread before continuing. The actual task is empty, because
Explain why the IO thread.

https://codereview.chromium.org/2721663002/diff/180001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/2721663002/diff/180001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:1542: // Sync
with IO thread before continuing. The actual task is empty, because
Same here

[jdoerrie, 2017-03-14 13:31:57]

https://codereview.chromium.org/2721663002/diff/180001/chrome/browser/passwor...
File chrome/browser/password_manager/credential_manager_browsertest.cc (right):

https://codereview.chromium.org/2721663002/diff/180001/chrome/browser/passwor...
chrome/browser/password_manager/credential_manager_browsertest.cc:204: // Sync
with IO thread before continuing. The actual task is empty, because
On 2017/03/14 13:13:22, vasilii wrote:
> Explain why the IO thread.

Done.

https://codereview.chromium.org/2721663002/diff/180001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/2721663002/diff/180001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:1542: // Sync
with IO thread before continuing. The actual task is empty, because
On 2017/03/14 13:13:22, vasilii wrote:
> Same here

Done.

[jdoerrie, 2017-03-14 13:32:03]

The CQ bit was checked by jdoerrie@chromium.org

[jdoerrie, 2017-03-14 13:32:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, vasilii@chromium.org
Link to the patchset: https://codereview.chromium.org/2721663002/#ps200001
(title: "More Explanation")

[commit-bot: I haz the power, 2017-03-14 13:32:25]

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-03-14 15:00:01]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1489498323996920,
"parent_rev": "ef037e9d0a143de214150814f8c16df588ef2ae7", "commit_rev":
"5cda81b4d16e323763e51f22582a9144dc64c30e"}

[commit-bot: I haz the power, 2017-03-14 15:00:47]

Description was changed from

==========
Move Credentials when migrating to HSTS page

This change enables moving credentials during migration when the corresponding
site has HSTS enabled. Prior to this change old HTTP credentials were kept,
leading to unnecessary duplication.

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

to

==========
Move Credentials when migrating to HSTS page

This change enables moving credentials during migration when the corresponding
site has HSTS enabled. Prior to this change old HTTP credentials were kept,
leading to unnecessary duplication.

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

Review-Url: https://codereview.chromium.org/2721663002
Cr-Commit-Position: refs/heads/master@{#456704}
Committed:
https://chromium.googlesource.com/chromium/src/+/5cda81b4d16e323763e51f22582a...
==========

[commit-bot: I haz the power, 2017-03-14 15:00:49]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/5cda81b4d16e323763e51f22582a...