Password manager: Gnome support for Public Suffix List matching

Password manager: Gnome support for Public Suffix List matching

This CL:

* Enables Public Suffix List (PSL) general utilities on Linux.
* Enables PSL flag for Linux.
* Adds Gnome Keyring support for searching and filtering multiple origins, including UMA metrics and a unit test.

BUG=324291
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244286

[vabr (Chromium), 2014-01-02 16:55:03]

Hi Garret, could you please review this?

Hi Tommy, this is just FYI. If you have any comments, they will of course be
appreciated.

This is just for Gnome, I did not get to KDE yet. Our PM suggested to look at
Windows first and Mac second, so I plan to do KDE after that.

The launch bug http://crbug.com/324302 is currently for M34, so I left the
matching enabled by default also for Gnome. If this gets punted (e.g., because
of unfinished reviews), I'll change it to disabled by default for Gnome in beta
& stable.

Cheers,
Vaclav

[Garrett Casto, 2014-01-06 07:26:45]

https://codereview.chromium.org/123223003/diff/30001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/123223003/diff/30001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:157: if
(!any_signon_realm_accepted &&
Is any_signon_realm_accepted necessary? At the moment it seems like it's the
same as original_signon_realm.empty().

https://codereview.chromium.org/123223003/diff/30001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:160: if
(!(helper.ShouldPSLDomainMatchingApply(
I think that this call is superfluous. We should already have checked if this is
okay for original_signon_realm and IsPublicSuffixDomainMatch() will require that
the registry controlled domains for original_signon_realm and form->signon_realm
are equal.

https://codereview.chromium.org/123223003/diff/30001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:285: std::string
original_signon_realm_;
Probably want to comment at least this member variable. It's value isn't
entirely obvious since sometimes it will be empty.

https://codereview.chromium.org/123223003/diff/30001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:387: if
(!helper_.ShouldPSLDomainMatchingApply(form.signon_realm))
You should use GetRegistryControlledDomain here.

https://codereview.chromium.org/123223003/diff/30001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x_unittest.cc (right):

https://codereview.chromium.org/123223003/diff/30001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x_unittest.cc:447: // Save
a password for www.facebook.com and see it suggested for m.facebook.com.
You should make sure to check the opposite as well. That is, make sure that
something where PSL matching shouldn't apply doesn't.

[vabr (Chromium), 2014-01-08 10:23:03]

Hi Garrett,
Thanks for your comments. I addressed them, please take a look.

Cheers,
Vaclav

https://codereview.chromium.org/123223003/diff/30001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/123223003/diff/30001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:157: if
(!any_signon_realm_accepted &&
On 2014/01/06 07:26:46, Garrett Casto wrote:
> Is any_signon_realm_accepted necessary? At the moment it seems like it's the
> same as original_signon_realm.empty().

Done.

https://codereview.chromium.org/123223003/diff/30001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:160: if
(!(helper.ShouldPSLDomainMatchingApply(
On 2014/01/06 07:26:46, Garrett Casto wrote:
> I think that this call is superfluous. We should already have checked if this
is
> okay for original_signon_realm and IsPublicSuffixDomainMatch() will require
that
> the registry controlled domains for original_signon_realm and
form->signon_realm
> are equal.

Done.

https://codereview.chromium.org/123223003/diff/30001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:285: std::string
original_signon_realm_;
On 2014/01/06 07:26:46, Garrett Casto wrote:
> Probably want to comment at least this member variable. It's value isn't
> entirely obvious since sometimes it will be empty.

Done.

https://codereview.chromium.org/123223003/diff/30001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:387: if
(!helper_.ShouldPSLDomainMatchingApply(form.signon_realm))
On 2014/01/06 07:26:46, Garrett Casto wrote:
> You should use GetRegistryControlledDomain here. 

Done.

https://codereview.chromium.org/123223003/diff/240001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_gnome_x_unittest.cc (right):

https://codereview.chromium.org/123223003/diff/240001/chrome/browser/password...
chrome/browser/password_manager/native_backend_gnome_x_unittest.cc:302:
form_google_.signon_realm = "http://www.google.com/";
According to the declaration of PasswordForm::signon_realm and to how
signon_realm is used in the code, the realm should be a URL, not just an
arbitrary string. I only need it to be a URL for |form_facebook_| here, but for
consistency I also changed |form_google_| and |form_isc_|.

[Garrett Casto, 2014-01-09 18:52:16]

lgtm

https://codereview.chromium.org/123223003/diff/240001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_gnome_x_unittest.cc (right):

https://codereview.chromium.org/123223003/diff/240001/chrome/browser/password...
chrome/browser/password_manager/native_backend_gnome_x_unittest.cc:495: //
m-facebook.com.
This seems fine, but for the record what I meant was testing that something on
one.google.com won't match two.google.com.

[vabr (Chromium), 2014-01-10 12:35:38]

Thanks, Garrett!

I extended the tests a bit. By doing that I pulled out some test code to be
shared by three tests to avoid duplication.

Given that, you might want to have one more look at it, so I'll wait with
sending it to CQ today. (Will have to rebase it after
https://codereview.chromium.org/131683003/ lands anyway).

Cheers,
Vaclav

[Garrett Casto, 2014-01-10 19:05:29]

LGTM

Nice.

[vabr (Chromium), 2014-01-10 19:13:00]

Thanks, Garrett!

Sending to CQ now.

Cheers,
Vaclav

[commit-bot: I haz the power, 2014-01-10 19:18:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/123223003/400001

[commit-bot: I haz the power, 2014-01-10 23:10:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/123223003/400001

[commit-bot: I haz the power, 2014-01-11 00:42:20]

Change committed as 244286