Stop prerendering of a site if it uses the Credential Manager API.

Stop prerendering of a site if it uses the Credential Manager API.

BUG=658732
Committed: https://crrev.com/7926e90cb2f6b25fd19ed9a4e37bbbd0e37b3845
Cr-Commit-Position: refs/heads/master@{#428007}

[vasilii, 2016-10-25 17:09:53]

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

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

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

[vasilii, 2016-10-25 17:12:52]

Description was changed from

==========
Stop prerendering of a site if it uses the Credential Manager API.

BUG=658732
==========

to

==========
Stop prerendering of a site if it uses the Credential Manager API.

BUG=658732
==========

[vasilii, 2016-10-25 17:12:52]

vasilii@chromium.org changed reviewers:
+ mmenke@chromium.org, vabr@chromium.org

[vasilii, 2016-10-25 17:14:10]

vasilii@chromium.org changed reviewers:
+ isherman@chromium.org

[vasilii, 2016-10-25 17:16:34]

Hi guys,

please review

isherman@: histograms.xml
vabr@, mmenke@: everything.

[mmenke, 2016-10-25 17:24:07]

mmenke@chromium.org changed reviewers:
+ pasko@chromium.org

[mmenke, 2016-10-25 17:24:08]

[+pasko]:  Do you want to take this one, or should I?

Just skimming over this one, cancellation looks safe, haven't dug into the
browser test, though.

[commit-bot: I haz the power, 2016-10-25 18:03:02]

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

[commit-bot: I haz the power, 2016-10-25 18:03:03]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2016-10-25 20:28:39]

histograms.xml lgtm

[vabr (Chromium), 2016-10-26 08:27:40]

LGTM with nits and a question.

Thanks!
Vaclav

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

https://codereview.chromium.org/2447143002/diff/1/components/password_manager...
components/password_manager/core/browser/password_manager_client.h:67: // Checks
if the Credential Manager API is allowed to run on the page. It's
nit: One thing to mention is that even if this returns true, it does not mean
that Credential Manager API will work. It is disabled when the old password
manager is disabled.

https://codereview.chromium.org/2447143002/diff/1/components/password_manager...
components/password_manager/core/browser/password_manager_client.h:68: // not
allowed while prerendering and the tab will be destroyed in this case.
nit: "the tab" might be ambiguous; perhaps "the pre-rendered WebContents"?

https://codereview.chromium.org/2447143002/diff/1/components/password_manager...
components/password_manager/core/browser/password_manager_client.h:68: // not
allowed while prerendering and the tab will be destroyed in this case.
question: Did you comment on the relation to prerendering here instead of at the
ChromePasswordManagerClient override, because the plan is to do the same for
iOS? Or does iOS have no prerendering?

If the latter, is it worth having this in the PasswordManagerClient as opposed
to just ChromePasswordManagerClient?

[pasko, 2016-10-26 10:05:59]

On 2016/10/25 17:24:08, mmenke wrote:
> [+pasko]:  Do you want to take this one, or should I?
> 
> Just skimming over this one, cancellation looks safe, haven't dug into the
> browser test, though.

mmenke: Thanks for taking the initial look. I will review it today.

[vasilii, 2016-10-26 11:39:27]

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

[commit-bot: I haz the power, 2016-10-26 11:39:37]

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

[vasilii, 2016-10-26 11:41:05]

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

[vasilii, 2016-10-26 11:41:07]

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

https://codereview.chromium.org/2447143002/diff/1/components/password_manager...
components/password_manager/core/browser/password_manager_client.h:67: // Checks
if the Credential Manager API is allowed to run on the page. It's
On 2016/10/26 08:27:40, vabr (Chromium) wrote:
> nit: One thing to mention is that even if this returns true, it does not mean
> that Credential Manager API will work. It is disabled when the old password
> manager is disabled.

Done.

https://codereview.chromium.org/2447143002/diff/1/components/password_manager...
components/password_manager/core/browser/password_manager_client.h:68: // not
allowed while prerendering and the tab will be destroyed in this case.
On 2016/10/26 08:27:40, vabr (Chromium) wrote:
> nit: "the tab" might be ambiguous; perhaps "the pre-rendered WebContents"?

Done.

https://codereview.chromium.org/2447143002/diff/1/components/password_manager...
components/password_manager/core/browser/password_manager_client.h:68: // not
allowed while prerendering and the tab will be destroyed in this case.
On 2016/10/26 08:27:40, vabr (Chromium) wrote:
> question: Did you comment on the relation to prerendering here instead of at
the
> ChromePasswordManagerClient override, because the plan is to do the same for
> iOS? Or does iOS have no prerendering?
> 
> If the latter, is it worth having this in the PasswordManagerClient as opposed
> to just ChromePasswordManagerClient?

Yes, iOS has prerendering. Although it's limited to omnibox typing (<link
rel="prerender"> isn't supported).

[commit-bot: I haz the power, 2016-10-26 11:41:20]

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

[vabr (Chromium), 2016-10-26 12:11:26]

Thanks, still LGTM!

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

https://codereview.chromium.org/2447143002/diff/1/components/password_manager...
components/password_manager/core/browser/password_manager_client.h:68: // not
allowed while prerendering and the tab will be destroyed in this case.
On 2016/10/26 11:41:06, vasilii wrote:
> On 2016/10/26 08:27:40, vabr (Chromium) wrote:
> > question: Did you comment on the relation to prerendering here instead of at
> the
> > ChromePasswordManagerClient override, because the plan is to do the same for
> > iOS? Or does iOS have no prerendering?
> > 
> > If the latter, is it worth having this in the PasswordManagerClient as
opposed
> > to just ChromePasswordManagerClient?
> 
> Yes, iOS has prerendering. Although it's limited to omnibox typing (<link
> rel="prerender"> isn't supported).

Acknowledged.

[pasko, 2016-10-26 12:37:01]

overall early destroying the PrerenderContents looks like the safest possible
thing to do, suggesting to move the test to prerender_browsertest.cc and change
it slightly.

https://codereview.chromium.org/2447143002/diff/40001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/2447143002/diff/40001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:216: bool
ChromePasswordManagerClient::IsCredentialManagerEnabledForCurrentPage() {
Naming this function by starting with "Is" suggests that it has limited side
effects, which is confusing. Can it be renamed to something like
KillCurrentPageIfPrerendered()? Then it would probably be better to return true
iff the page is killed.

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

https://codereview.chromium.org/2447143002/diff/40001/chrome/browser/password...
chrome/browser/password_manager/credential_manager_browsertest.cc:264:
std::vector<std::unique_ptr<TestPrerender>> PrerenderTestURLImpl(
in a concurrent change I am removing PrerenderTestURLImpl from
PrerenderInProcessBrowserTest.

Can you put this test inside prerender_browsertest and use PrerenderTestURL()
machinery instead? This would then be more logical to move autosignin.html to
chrome/test/data/prerender, but the good part is that prerender_autosignin.html
can be eliminated.

https://codereview.chromium.org/2447143002/diff/40001/chrome/browser/password...
chrome/browser/password_manager/credential_manager_browsertest.cc:280:
prerender->WaitForCreate();
can just WaitForStop() without WaitForCreate()

https://codereview.chromium.org/2447143002/diff/40001/chrome/browser/password...
chrome/browser/password_manager/credential_manager_browsertest.cc:281:
prerender->WaitForStop();
Do we also want to make sure that a request to password/done.html is not made?
RequestCounter in prerender_browsertest may come handy.

https://codereview.chromium.org/2447143002/diff/40001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_final_status.h (right):

https://codereview.chromium.org/2447143002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_final_status.h:70:
FINAL_STATUS_CREDENTIAL_MANAGER_API = 55,
FYI: there will be an easy-to-resolve conflict with this change:
https://codereview.chromium.org/2411863002/

[commit-bot: I haz the power, 2016-10-26 12:37:13]

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

[commit-bot: I haz the power, 2016-10-26 12:37:14]

Dry run: This issue passed the CQ dry run.

[vasilii, 2016-10-27 09:01:26]

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

[commit-bot: I haz the power, 2016-10-27 09:01:51]

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

[vasilii, 2016-10-27 09:02:09]

https://codereview.chromium.org/2447143002/diff/40001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/2447143002/diff/40001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:216: bool
ChromePasswordManagerClient::IsCredentialManagerEnabledForCurrentPage() {
On 2016/10/26 12:37:00, pasko wrote:
> Naming this function by starting with "Is" suggests that it has limited side
> effects, which is confusing. Can it be renamed to something like
> KillCurrentPageIfPrerendered()? Then it would probably be better to return
true
> iff the page is killed.

Renamed. The password manager is a component. It shouldn't know any information
about prerendering and its consequences.

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

https://codereview.chromium.org/2447143002/diff/40001/chrome/browser/password...
chrome/browser/password_manager/credential_manager_browsertest.cc:264:
std::vector<std::unique_ptr<TestPrerender>> PrerenderTestURLImpl(
On 2016/10/26 12:37:00, pasko wrote:
> in a concurrent change I am removing PrerenderTestURLImpl from
> PrerenderInProcessBrowserTest.
> 
> Can you put this test inside prerender_browsertest and use PrerenderTestURL()
> machinery instead? This would then be more logical to move autosignin.html to
> chrome/test/data/prerender, but the good part is that
prerender_autosignin.html
> can be eliminated.

Done.

https://codereview.chromium.org/2447143002/diff/40001/chrome/browser/password...
chrome/browser/password_manager/credential_manager_browsertest.cc:280:
prerender->WaitForCreate();
On 2016/10/26 12:37:00, pasko wrote:
> can just WaitForStop() without WaitForCreate()

Done.

https://codereview.chromium.org/2447143002/diff/40001/chrome/browser/password...
chrome/browser/password_manager/credential_manager_browsertest.cc:281:
prerender->WaitForStop();
On 2016/10/26 12:37:00, pasko wrote:
> Do we also want to make sure that a request to password/done.html is not made?
> RequestCounter in prerender_browsertest may come handy.

Done.

https://codereview.chromium.org/2447143002/diff/40001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_final_status.h (right):

https://codereview.chromium.org/2447143002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_final_status.h:70:
FINAL_STATUS_CREDENTIAL_MANAGER_API = 55,
On 2016/10/26 12:37:00, pasko wrote:
> FYI: there will be an easy-to-resolve conflict with this change:
> https://codereview.chromium.org/2411863002/

Acknowledged.

[commit-bot: I haz the power, 2016-10-27 09:27:24]

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

[commit-bot: I haz the power, 2016-10-27 09:27:25]

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

[vasilii, 2016-10-27 09:48:40]

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

[commit-bot: I haz the power, 2016-10-27 09:49:03]

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-27 09:50:57]

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

[commit-bot: I haz the power, 2016-10-27 09:50:58]

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...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[vasilii, 2016-10-27 10:09:14]

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

[vasilii, 2016-10-27 10:09:35]

Rebased on ToT.

[commit-bot: I haz the power, 2016-10-27 10:09:43]

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-27 11:46:33]

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

[commit-bot: I haz the power, 2016-10-27 11:46:34]

Dry run: This issue passed the CQ dry run.

[pasko, 2016-10-27 12:42:11]

lgtm

[vasilii, 2016-10-27 12:48:50]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2016-10-27 12:48:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/2447143002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-27 12:49:10]

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-27 12:54:34]

Description was changed from

==========
Stop prerendering of a site if it uses the Credential Manager API.

BUG=658732
==========

to

==========
Stop prerendering of a site if it uses the Credential Manager API.

BUG=658732
==========

[commit-bot: I haz the power, 2016-10-27 12:54:35]

Committed patchset #6 (id:100001)

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

Description was changed from

==========
Stop prerendering of a site if it uses the Credential Manager API.

BUG=658732
==========

to

==========
Stop prerendering of a site if it uses the Credential Manager API.

BUG=658732

Committed: https://crrev.com/7926e90cb2f6b25fd19ed9a4e37bbbd0e37b3845
Cr-Commit-Position: refs/heads/master@{#428007}
==========

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

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