[Password Manager] Remove PSL matching for non-HTML forms

[Password Manager] Remove PSL matching for non-HTML forms

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

[vabr (Chromium), 2014-05-07 07:57:38]

Thanks, Garrett!
LGTM with comments.

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

https://codereview.chromium.org/269813012/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/native_backend_gnome_x.cc:169:
lookup_form->scheme != PasswordForm::SCHEME_HTML ||
optional nit: the added tests are cheaper than the IsPublicSuffixDomainMatch, so
I suggest putting them before the IsPublicSuffixDomainMatch -- if they return
true, it will spare some work.

Then again, this is probably not a performance hotspot, that's why the nit is
optional.

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

https://codereview.chromium.org/269813012/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/native_backend_gnome_x_unittest.cc:416: const
std::string& realm,
Please explain the meaning of |realm| in the comment above. In particular the
connection to the SCHEME_BASIC is not obvious from its name.

https://codereview.chromium.org/269813012/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/native_backend_gnome_x_unittest.cc:663:
TEST_F(NativeBackendGnomeTest, PSLMatchingDisabledForNonHTMLForms) {
The code change applies to all possible schemes, but the tests only care about
SCHEME_BASIC and SCHEME_HTML.

I'm not sure if it's worth the effort to add also tests for SCHEME_DIGEST and
SCHEME_OTHER. Here it would be an easy copy-paste of the added test. For the
LoginDatabase it would be probably longer.

My vote would be to cover all schemes with tests, but I don't insist on it if
you believe it's not worth it.

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

https://codereview.chromium.org/269813012/diff/1/components/password_manager/...
components/password_manager/core/browser/login_database.cc:486: new_form->scheme
!= PasswordForm::SCHEME_HTML) {
optional nit: Consider swapping the order of the two conditions (see my first
comment).

[Garrett Casto, 2014-05-08 20:01:20]

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

https://codereview.chromium.org/269813012/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/native_backend_gnome_x.cc:169:
lookup_form->scheme != PasswordForm::SCHEME_HTML ||
On 2014/05/07 07:57:38, vabr (Chromium) wrote:
> optional nit: the added tests are cheaper than the IsPublicSuffixDomainMatch,
so
> I suggest putting them before the IsPublicSuffixDomainMatch -- if they return
> true, it will spare some work.
> 
> Then again, this is probably not a performance hotspot, that's why the nit is
> optional.

Done.

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

https://codereview.chromium.org/269813012/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/native_backend_gnome_x_unittest.cc:416: const
std::string& realm,
On 2014/05/07 07:57:38, vabr (Chromium) wrote:
> Please explain the meaning of |realm| in the comment above. In particular the
> connection to the SCHEME_BASIC is not obvious from its name.

Done.

https://codereview.chromium.org/269813012/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/native_backend_gnome_x_unittest.cc:663:
TEST_F(NativeBackendGnomeTest, PSLMatchingDisabledForNonHTMLForms) {
On 2014/05/07 07:57:38, vabr (Chromium) wrote:
> The code change applies to all possible schemes, but the tests only care about
> SCHEME_BASIC and SCHEME_HTML.
> 
> I'm not sure if it's worth the effort to add also tests for SCHEME_DIGEST and
> SCHEME_OTHER. Here it would be an easy copy-paste of the added test. For the
> LoginDatabase it would be probably longer.
> 
> My vote would be to cover all schemes with tests, but I don't insist on it if
> you believe it's not worth it.

Done.

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

https://codereview.chromium.org/269813012/diff/1/components/password_manager/...
components/password_manager/core/browser/login_database.cc:486: new_form->scheme
!= PasswordForm::SCHEME_HTML) {
On 2014/05/07 07:57:38, vabr (Chromium) wrote:
> optional nit: Consider swapping the order of the two conditions (see my first
> comment).

Done.

[Garrett Casto, 2014-05-08 20:01:23]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-05-08 20:08:12]

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

[vabr (Chromium), 2014-05-09 09:19:57]

Hi Garrett.

Still LGTM, with two optional comments.

It would be great if the tests for LoginDatabase could be extended at some
point, not necessarily in this CL.

Cheers,
Vaclav

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

https://codereview.chromium.org/269813012/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x_unittest.cc:409: // Saves
|credentials| and then gets login for origin and realm |url|. Returns
nit: "|url|" -> "|url|, and |scheme|"

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

https://codereview.chromium.org/269813012/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:248:
TEST_F(LoginDatabaseTest, TestPublicSuffixDisabledForNonHTMLForms) {
Could you please also expand this test for other schemes than SCHEME_BASIC?

Most of the platforms, including Win and Mac depend on this code, so it's more
crucial to test than the Gnome backend.

[Garrett Casto, 2014-05-09 18:32:13]

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

https://codereview.chromium.org/269813012/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x_unittest.cc:409: // Saves
|credentials| and then gets login for origin and realm |url|. Returns
On 2014/05/09 09:19:58, vabr (Chromium) wrote:
> nit: "|url|" -> "|url|, and |scheme|"

Done.

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

https://codereview.chromium.org/269813012/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:248:
TEST_F(LoginDatabaseTest, TestPublicSuffixDisabledForNonHTMLForms) {
On 2014/05/09 09:19:58, vabr (Chromium) wrote:
> Could you please also expand this test for other schemes than SCHEME_BASIC?
> 
> Most of the platforms, including Win and Mac depend on this code, so it's more
> crucial to test than the Gnome backend.

Whoops, fixed.

[Garrett Casto, 2014-05-09 18:32:16]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-05-09 18:36:12]

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

[vabr (Chromium), 2014-05-09 18:56:54]

Awesome, thanks! LGTM.
Vaclav

[commit-bot: I haz the power, 2014-05-10 01:03:10]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-10 03:06:11]

Change committed as 269506