Implement origin-based deletion for password manager's auto-signin bit.

Implement origin-based deletion for password manager's auto-signin bit.

BUG=589586
Committed: https://crrev.com/338c59d1485de17dd38ce8b94dc4cfb90677fa0e
Cr-Commit-Position: refs/heads/master@{#404910}

[msramek, 2016-07-08 17:14:57]

Patchset #1 (id:1) has been deleted

[msramek, 2016-07-08 17:15:57]

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

[msramek, 2016-07-08 17:15:58]

Hi Václav,

Please have a look!

Thanks,
Martin

[vabr (Chromium), 2016-07-08 19:22:04]

LGTM, this looks reasonable. Just a couple of comments below.

Cheers,
Vaclav

https://codereview.chromium.org/2132063002/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/2132063002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/native_backend_gnome_x.cc:710: if
(origin_filter.Run(form->origin) && !form->skip_zero_click) {
Recalling your earlier planned to accept a "null" callback and treat as always
true here, you don't do it because nobody calls this for all origins, right?

https://codereview.chromium.org/2132063002/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc
(right):

https://codereview.chromium.org/2132063002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc:1039:
ExpectationArray expected;
nit: Would you mind moving this back after line 1041? Declaring variables near
to the first usage helps readability.

https://codereview.chromium.org/2132063002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_store_default.cc (right):

https://codereview.chromium.org/2132063002/diff/20001/components/password_man...
components/password_manager/core/browser/password_store_default.cc:147: if
(!login_db_->DisableAutoSignInForOrigin(form->origin))
This looks rather inefficient. Did you consider just changing the
skip_zero_click bit in |*form| and calling Update on it? (I'm not sure if that's
better, though.)

Alternatively, we could at least keep a set of the origins for which you already
called line 147. If that's not too bad for readability, that would probably be
my preferred solution.

But I have no opinion too strong here, because this is not a frequent action to
be taken.

[msramek, 2016-07-11 13:09:17]

I addressed your comments and added password_store_proxy_mac which I missed for
obvious reasons.

PTAL!

https://codereview.chromium.org/2132063002/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/2132063002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/native_backend_gnome_x.cc:710: if
(origin_filter.Run(form->origin) && !form->skip_zero_click) {
On 2016/07/08 19:22:03, vabr (Chromium) wrote:
> Recalling your earlier planned to accept a "null" callback and treat as always
> true here, you don't do it because nobody calls this for all origins, right?

Correct - I would leave that for another pass when we start supporting the null
callbacks on BrowsingDataRemover side.

In the meantime, I don't think there's a significant performance hit by not
supporting it, since
- the native backends always did an iteration anyway
- the SQL backend has now been optimized to one call per origin; that is still
slower than it used to be, but the operation is uncommon, and its repeated calls
will be faster (since we don't call the SQL update where unnecessary)

https://codereview.chromium.org/2132063002/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc
(right):

https://codereview.chromium.org/2132063002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc:1039:
ExpectationArray expected;
On 2016/07/08 19:22:03, vabr (Chromium) wrote:
> nit: Would you mind moving this back after line 1041? Declaring variables near
> to the first usage helps readability.

Done.

In my opinion, it helps readability to process data structures from the largest
to the smallest (ExpectationArray->vector->PasswordForm), but I'm not going to
bikeshed about that :)

https://codereview.chromium.org/2132063002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_store_default.cc (right):

https://codereview.chromium.org/2132063002/diff/20001/components/password_man...
components/password_manager/core/browser/password_store_default.cc:147: if
(!login_db_->DisableAutoSignInForOrigin(form->origin))
On 2016/07/08 19:22:03, vabr (Chromium) wrote:
> This looks rather inefficient. Did you consider just changing the
> skip_zero_click bit in |*form| and calling Update on it? (I'm not sure if
that's
> better, though.)
> 
> Alternatively, we could at least keep a set of the origins for which you
already
> called line 147. If that's not too bad for readability, that would probably be
> my preferred solution.
> 
> But I have no opinion too strong here, because this is not a frequent action
to
> be taken.

Indeed, I did the change with the expectation that this is not a frequent
action. But I implemented your preferred solution with caching origins.

I rewrote the method quite a bit, because I wanted to avoid the scenario where
form1 and form2 have the same origin, the SQL query fails when called first on
form1, then succeeds second time when called on form2 and we only send |changes|
for form2 where in fact form1 was now also updated.

[vabr (Chromium), 2016-07-11 13:21:16]

Thanks, this LGTM!
Vaclav

[msramek, 2016-07-12 13:45:51]

I updated PasswordStoreMac (pretty much a copy-paste from PasswordStoreDefault)
and its tests. The test should be finally green. Please have one last look!

https://codereview.chromium.org/2132063002/diff/60001/chrome/browser/password...
File chrome/browser/password_manager/password_store_x_unittest.cc (right):

https://codereview.chromium.org/2132063002/diff/60001/chrome/browser/password...
chrome/browser/password_manager/password_store_x_unittest.cc:234:
all_forms_[index] = all_forms_.back();
This got here from a rebase over
https://chromium.googlesource.com/chromium/src/+/84abdbb71ec1b3fc6872b73c3a9e...

[vabr (Chromium), 2016-07-12 14:21:43]

lgtm

[msramek, 2016-07-12 14:23:11]

Thanks!

[msramek, 2016-07-12 14:23:17]

The CQ bit was checked by msramek@chromium.org

[commit-bot: I haz the power, 2016-07-12 14:24:16]

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-07-12 17:15:41]

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

[commit-bot: I haz the power, 2016-07-12 17:15:43]

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

[msramek, 2016-07-12 17:24:51]

The CQ bit was checked by msramek@chromium.org

[commit-bot: I haz the power, 2016-07-12 17:26:35]

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-07-12 22:38:33]

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

[commit-bot: I haz the power, 2016-07-12 22:38:34]

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

[msramek, 2016-07-12 23:29:39]

The CQ bit was checked by msramek@chromium.org

[commit-bot: I haz the power, 2016-07-12 23:32:38]

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-07-13 02:26:07]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-07-13 02:27:30]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 02:27:49]

Description was changed from

==========
Implement origin-based deletion for password manager's auto-signin bit.

BUG=589586
==========

to

==========
Implement origin-based deletion for password manager's auto-signin bit.

BUG=589586

Committed: https://crrev.com/338c59d1485de17dd38ce8b94dc4cfb90677fa0e
Cr-Commit-Position: refs/heads/master@{#404910}
==========

[commit-bot: I haz the power, 2016-07-13 02:27:50]

Patchset 3 (id:??) landed as
https://crrev.com/338c59d1485de17dd38ce8b94dc4cfb90677fa0e
Cr-Commit-Position: refs/heads/master@{#404910}