Remove Finch kill switch for PSL matching

Remove Finch kill switch for PSL matching

Following up on http://crbug.com/338289#c13, this CL removes the kill switch for PSL matching. Apart from the linked check with our PM, the consideration is that PSL matching has been in stable for a couple of releases now on all platforms (except for KDE, see below), and the bugs seen so far were of low severity, exposing problems in password manager rather than with PSL matching itself.

It also replaces a couple of callsites of IsMatchingEnabled with ShouldPSLDomainMatchingApply -- the former did not take sites with a unified login origin into consideration, and on the affected callsites it seemed like it should.

One caveat: PSL matching has not been added to KDE on Linux yet (it's planned, just not a top priority). Given that PSL is live on other Linux environments, and all other platforms, the concerns about needing the kill switch for KDE are low.

As a follow-up to this CL, the (never submitted) internal CL with the corresponding Finch config will be closed.

BUG=338289
Committed: https://crrev.com/4327ad3ce3504c5ae369855e24883079a67af811
Cr-Commit-Position: refs/heads/master@{#296371}

[vabr (Chromium), 2014-08-27 14:36:14]

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

[vabr (Chromium), 2014-08-27 14:36:14]

Hi Tommy,

Could you please take a look?

Thanks!
Vaclav

[nyquist, 2014-08-27 18:15:14]

https://codereview.chromium.org/508143002/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/508143002/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/native_backend_gnome_x.cc:204: :
PSLMatchingHelper::PSL_DOMAIN_MATCH_DISABLED,
This else-clause looks a bit off to me. It's not disabled, but just not
triggered, no?

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

https://codereview.chromium.org/508143002/diff/1/components/password_manager/...
components/password_manager/core/browser/psl_matching_helper.h:46: bool
ShouldPSLDomainMatchingApply(
Why is this not static? And if it was, I guess this wouldn't necessarily have to
be a class.

[vabr (Chromium), 2014-09-22 13:32:31]

Hi Tommy,

Please have another look.

Cheers,
Vaclav

https://codereview.chromium.org/508143002/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/508143002/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/native_backend_gnome_x.cc:204: :
PSLMatchingHelper::PSL_DOMAIN_MATCH_DISABLED,
On 2014/08/27 18:15:14, nyquist wrote:
> This else-clause looks a bit off to me. It's not disabled, but just not
> triggered, no?

There is no other disabling than not triggering, any more, I believe.
The current code in question is consistent with the LoginDatabase::GetLogins(),
which also reports PSL_DOMAIN_MATCH_DISABLED if ShouldPSLDomainMatchingApply
returns false.

Would you like me to rename the constant, or do you see deeper issues?

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

https://codereview.chromium.org/508143002/diff/1/components/password_manager/...
components/password_manager/core/browser/psl_matching_helper.h:46: bool
ShouldPSLDomainMatchingApply(
On 2014/08/27 18:15:14, nyquist wrote:
> Why is this not static? And if it was, I guess this wouldn't necessarily have
to
> be a class.

Oh, good catch. Simplified further to just helper functions.

Let me know if you want me to rename this file.

[nyquist, 2014-09-22 22:59:48]

lgtm

https://codereview.chromium.org/508143002/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/508143002/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/native_backend_gnome_x.cc:204: :
PSLMatchingHelper::PSL_DOMAIN_MATCH_DISABLED,
On 2014/09/22 13:32:31, vabr (Chromium) wrote:
> On 2014/08/27 18:15:14, nyquist wrote:
> > This else-clause looks a bit off to me. It's not disabled, but just not
> > triggered, no?
> 
> There is no other disabling than not triggering, any more, I believe.
> The current code in question is consistent with the
LoginDatabase::GetLogins(),
> which also reports PSL_DOMAIN_MATCH_DISABLED if ShouldPSLDomainMatchingApply
> returns false.
> 
> Would you like me to rename the constant, or do you see deeper issues?

No, I think it would only be a name change. Up to you though.

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

https://codereview.chromium.org/508143002/diff/1/components/password_manager/...
components/password_manager/core/browser/psl_matching_helper.h:46: bool
ShouldPSLDomainMatchingApply(
On 2014/09/22 13:32:31, vabr (Chromium) wrote:
> On 2014/08/27 18:15:14, nyquist wrote:
> > Why is this not static? And if it was, I guess this wouldn't necessarily
have
> to
> > be a class.
> 
> Oh, good catch. Simplified further to just helper functions.
> 
> Let me know if you want me to rename this file.

I don't feel strongly about that. I'd be fine with either way.

[vabr (Chromium), 2014-09-23 08:31:50]

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

[vabr (Chromium), 2014-09-23 08:31:51]

Ilya,
Could you please have a look at tools/metrics/histograms/histograms.xml ?

The new histogram value description should be correcting rather than changing,
so switching to a new histogram to prevent mixing old and new data does not seem
necessary to me.

Tommy,
Thanks for your review. Please let me know if you have objections to how I
addressed your most recent comments.

Cheers,
Vaclav

https://codereview.chromium.org/508143002/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/508143002/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/native_backend_gnome_x.cc:204: :
PSLMatchingHelper::PSL_DOMAIN_MATCH_DISABLED,
On 2014/09/22 22:59:48, nyquist wrote:
> On 2014/09/22 13:32:31, vabr (Chromium) wrote:
> > On 2014/08/27 18:15:14, nyquist wrote:
> > > This else-clause looks a bit off to me. It's not disabled, but just not
> > > triggered, no?
> > 
> > There is no other disabling than not triggering, any more, I believe.
> > The current code in question is consistent with the
> LoginDatabase::GetLogins(),
> > which also reports PSL_DOMAIN_MATCH_DISABLED if ShouldPSLDomainMatchingApply
> > returns false.
> > 
> > Would you like me to rename the constant, or do you see deeper issues?
> 
> No, I think it would only be a name change. Up to you though.

I went with changing "disabled" to a more generic "not used". Please let me know
if you dislike that change.

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

https://codereview.chromium.org/508143002/diff/1/components/password_manager/...
components/password_manager/core/browser/psl_matching_helper.h:46: bool
ShouldPSLDomainMatchingApply(
On 2014/09/22 22:59:48, nyquist wrote:
> On 2014/09/22 13:32:31, vabr (Chromium) wrote:
> > On 2014/08/27 18:15:14, nyquist wrote:
> > > Why is this not static? And if it was, I guess this wouldn't necessarily
> have
> > to
> > > be a class.
> > 
> > Oh, good catch. Simplified further to just helper functions.
> > 
> > Let me know if you want me to rename this file.
> 
> I don't feel strongly about that. I'd be fine with either way.

OK, in that case I have no strong reason to rename, so I keep the filename as it
was.

[nyquist, 2014-09-23 18:53:41]

I'm happy with this change. Thanks!

[Ilya Sherman, 2014-09-23 18:54:56]

lgtm

[Ilya Sherman, 2014-09-23 18:55:44]

On 2014/09/23 18:54:56, Ilya Sherman wrote:
> lgtm

Note: I only looked at histograms.xml.

[vabr (Chromium), 2014-09-24 08:36:04]

On 2014/09/23 18:55:44, Ilya Sherman wrote:
> On 2014/09/23 18:54:56, Ilya Sherman wrote:
> > lgtm
> 
> Note: I only looked at histograms.xml.

That's fine, the rest was reviewed by Tommy.

Thanks both, I'm sending this to the CQ now.

Cheers,
Vaclav

[vabr (Chromium), 2014-09-24 08:36:10]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-09-24 08:37:09]

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

[commit-bot: I haz the power, 2014-09-24 09:19:22]

Committed patchset #4 (id:60001) as f6a29641c1e7a3473c5a53e115b3d2be2a34a404

[commit-bot: I haz the power, 2014-09-24 09:19:58]

Patchset 4 (id:??) landed as
https://crrev.com/4327ad3ce3504c5ae369855e24883079a67af811
Cr-Commit-Position: refs/heads/master@{#296371}