[Password Manager] Disable saving and filling on sync signup

[Password Manager] Disable saving and filling on sync signup

BUG=386390, 357233
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281896

[Garrett Casto, 2014-06-23 20:46:12]

Vaclav - Please review.
Roger - Can you verify signup always goes through chrome://chrome-signup?

[vabr (Chromium), 2014-06-24 08:49:49]

LGTM with nits.
Thanks!
Vaclav

https://codereview.chromium.org/344033008/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/344033008/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:112:
NOTREACHED();
Could we leave out the NOTREACHED?
I'm currently working on http://crbug.com/384328, which deals with a very
similar NOTREACHED in DidLastPageLoadEncounterSSLErrors, triggered during Basic
HTTP Auth, which is apparently processed before the entry is committed.
Even though I'm not sure yet if IsPasswordManagerEnabledForCurrentPage could be
triggered this way, I'd feel more comfortable if the password manager code
allowed for being called before the entry is committed.

https://codereview.chromium.org/344033008/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/344033008/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:115:
client_->IsPasswordManagerEnabledForCurrentPage();
Should this method check for password_manager_enabled_, instead of having that
check extra added in IsSavingenabledForCurrentPage?

https://codereview.chromium.org/344033008/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/344033008/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager_unittest.cc:623:
TEST_F(PasswordManagerTest, SavingNotEnabledForSyncSignin) {
The connection between Sync and IsPasswordManagerEnabledForCurrentPage is not
apparent.
My preference would be to replace "SyncSignin", e.g.,
SavingDisabledIfManagerDisabled.
Similarly below.

If you want a test for SyncSignin, that test should indeed attempt to load the
sign-in page. (I'm not sure we need such test.)

[vabr (Chromium), 2014-06-24 14:28:24]

https://codereview.chromium.org/344033008/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/344033008/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:112:
NOTREACHED();
I looked into the NOTREACHED a bit more: http://crbug.com/388246

It seems to me like it expresses a wrong expectation. Since it is not affecting
the Release builds anyway, I don't think we make the situation worse if we
remove the NOTREACHED from this CL.

You might want to put here a TODO mentioning http://crbug.com/388246, which is
tracking the problem.

[Roger Tawa OOO till Jul 10th, 2014-06-26 14:15:05]

Looks good. A couple of questions below.

https://codereview.chromium.org/344033008/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/344033008/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:118: return
entry->GetURL().host() != chrome::kChromeUIChromeSigninHost;
The chrome://chrome-signin/ page hosts a regular gaia page at
https://accounts.google.com/  That's OK?

https://codereview.chromium.org/344033008/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_manager_client.cc
(right):

https://codereview.chromium.org/344033008/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager_client.cc:14: return
true;
Why is the default true?  Why is this not pure virtual?

[Garrett Casto, 2014-07-07 21:09:03]

Hey Vaclav, do you mind taking another look at this? We decided that this is
initially going to be an experiment that is defaulted off as there is some
possibility that disallowing autofill in this situation will affect signin
rates.

https://codereview.chromium.org/344033008/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/344033008/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:112:
NOTREACHED();
On 2014/06/24 14:28:24, vabr (Chromium) wrote:
> I looked into the NOTREACHED a bit more: http://crbug.com/388246
> 
> It seems to me like it expresses a wrong expectation. Since it is not
affecting
> the Release builds anyway, I don't think we make the situation worse if we
> remove the NOTREACHED from this CL.
> 
> You might want to put here a TODO mentioning http://crbug.com/388246, which is
> tracking the problem.

Added.

https://codereview.chromium.org/344033008/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:118: return
entry->GetURL().host() != chrome::kChromeUIChromeSigninHost;
On 2014/06/26 14:15:05, Roger Tawa (OOO til July 25th) wrote:
> The chrome://chrome-signin/ page hosts a regular gaia page at
> https://accounts.google.com/  That's OK?

Yeah, as long as we can uniquely identify the page that is used for signin it
doesn't particularly matter what the actual content is.

[vabr (Chromium), 2014-07-08 07:49:20]

LGTM.

Controlling this through Finch seems like a good idea, and the default value
being enabled seems appropriate.

I repeated my two earlier comments to make sure you saw them, but they are not
blockers.

Cheers,
Vaclav

https://codereview.chromium.org/344033008/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/344033008/diff/40001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:120: return
*password_manager_enabled_ && !driver_->IsOffTheRecord() &&
I wanted to highlight once more:
Why is the check for *password_manager_enabled_ part of
IsSavingEnabledForCurrentPage instead of IsEnabledForCurrentPage? To me it would
make more sense in IsEnabledForCurrentPage.
(It's fine if you disagree, I'd like to hear you thoughts on that, though.)

https://codereview.chromium.org/344033008/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/344033008/diff/40001/components/password_mana...
components/password_manager/core/browser/password_manager_unittest.cc:631:
TEST_F(PasswordManagerTest, SavingNotEnabledForSyncSignin) {
Pinging (and clarifying) my previous comment about test names:

The connection between Sync and IsPasswordManagerEnabledForCurrentPage is not
apparent, so I find the test name a bit confusing.
My suggestion for the test name is SavingDisabledIfManagerDisabled.
Similarly below.

If you want a test for SyncSignin, that test should indeed attempt to load the
sign-in page. (I'm not sure we need such test.)

[Garrett Casto, 2014-07-08 19:58:49]

My apologies for not responding to these earlier, I just lost them over the
Holidays.

https://codereview.chromium.org/344033008/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/344033008/diff/40001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:120: return
*password_manager_enabled_ && !driver_->IsOffTheRecord() &&
On 2014/07/08 07:49:20, vabr (Chromium) wrote:
> I wanted to highlight once more:
> Why is the check for *password_manager_enabled_ part of
> IsSavingEnabledForCurrentPage instead of IsEnabledForCurrentPage? To me it
would
> make more sense in IsEnabledForCurrentPage.
> (It's fine if you disagree, I'd like to hear you thoughts on that, though.)

There was a decision made a while ago that disabling the the pref means
disabling saving not filing (crbug.com/158296). There has been some recent
discussions about this when we started exposing this for extensions, but
changing this is something we shouldn't do here.

https://codereview.chromium.org/344033008/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/344033008/diff/40001/components/password_mana...
components/password_manager/core/browser/password_manager_unittest.cc:631:
TEST_F(PasswordManagerTest, SavingNotEnabledForSyncSignin) {
On 2014/07/08 07:49:20, vabr (Chromium) wrote:
> Pinging (and clarifying) my previous comment about test names:
> 
> The connection between Sync and IsPasswordManagerEnabledForCurrentPage is not
> apparent, so I find the test name a bit confusing.
> My suggestion for the test name is SavingDisabledIfManagerDisabled.
> Similarly below.
> 
> If you want a test for SyncSignin, that test should indeed attempt to load the
> sign-in page. (I'm not sure we need such test.)

Changed.

[Garrett Casto, 2014-07-08 20:02:03]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-07-08 20:06:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gcasto@chromium.org/344033008/60001

[commit-bot: I haz the power, 2014-07-09 00:12:29]

Change committed as 281896