Support origin-based deletion for password store statistics

Support origin-based deletion for password store statistics

We iterate over the rows of the SQL DB to find entries matching the origin
filter. Then, we issue a cached DELETE statement for each of the entries.

It would also be possible to issue one "DELETE .. IN (?,?,?)" statement for all
matched origins, however, this requires us to generate the statement runtime
to add the correct number of "?". This is more complex and probably doesn't
bring a performance gain in practice.

BUG=589586
Committed: https://crrev.com/9acfdb3cd901f3aec451ba3aa5305a2cc2266739
Cr-Commit-Position: refs/heads/master@{#417262}

[msramek, 2016-09-07 16:47:54]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 16:48:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2016-09-07 16:52:43]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 16:52:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2016-09-07 16:53:21]

Patchset #1 (id:1) has been deleted

[msramek, 2016-09-07 16:54:13]

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

[msramek, 2016-09-07 16:54:13]

Hi Václav,

Please have a look! This is the CL that I mentioned earlier today.

Thanks,
Martin

[msramek, 2016-09-07 16:54:14]

Hi Václav,

Please have a look! This is the CL that I mentioned earlier today.

Thanks,
Martin

[commit-bot: I haz the power, 2016-09-07 17:17:16]

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

[commit-bot: I haz the power, 2016-09-07 17:17:17]

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

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

LGTM with a few comments (and the assumption that you fix the non-compiling
unittest :)).
I agree that doing one SQL delete statement per origin sounds reasonable here.

Cheers,
Vaclav

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

https://codereview.chromium.org/2320763002/diff/20001/components/password_man...
components/password_manager/core/browser/password_store.cc:162:
base::Callback<bool(const GURL&)>(origin_filter), delete_begin,
Why do you need to create the temporary base::Callback<bool(const GURL&)>,
cannot you just pass the |origin_filter| reference directly to base::Bind (which
guarantees copying the callback, because you don't use ConstRef [1])?

[1]
https://cs.chromium.org/chromium/src/base/bind_helpers.h?q=bind+helpers+const...

https://codereview.chromium.org/2320763002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/2320763002/diff/20001/components/password_man...
components/password_manager/core/browser/password_store.h:145: // Removes all
the stats created in the given the date range.
nit: The added "the" in "the data range" seems out of place.

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

https://codereview.chromium.org/2320763002/diff/20001/components/password_man...
components/password_manager/core/browser/statistics_table.cc:174: success &=
origin_delete_statement.Run();
I'm feeling uncomfortable about mixing bit-wise & and bool values. In this case
it should be OK, because Run() returns bool, an hopefully any non-zero value
will be identical to |true|. However, if somebody changed the signature to a
numerical type, it might change. What about the bit more verbose:
success = success && origin_delete_statement.Run();
?

[msramek, 2016-09-08 09:47:36]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 09:47:45]

Dry run: 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-09-08 10:41:12]

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

[commit-bot: I haz the power, 2016-09-08 10:41:13]

Dry run: Exceeded global retry quota

[msramek, 2016-09-08 10:42:12]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 10:42:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2016-09-08 10:45:29]

I addressed your comments, fixed the test, and added Mac-specific code which I
missed previously.

PTAL!

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

https://codereview.chromium.org/2320763002/diff/20001/components/password_man...
components/password_manager/core/browser/password_store.cc:162:
base::Callback<bool(const GURL&)>(origin_filter), delete_begin,
On 2016/09/08 09:04:36, vabr (Chromium) wrote:
> Why do you need to create the temporary base::Callback<bool(const GURL&)>,
> cannot you just pass the |origin_filter| reference directly to base::Bind
(which
> guarantees copying the callback, because you don't use ConstRef [1])?
> 
> [1]
>
https://cs.chromium.org/chromium/src/base/bind_helpers.h?q=bind+helpers+const...

Done. Yeah, I was a bit overzealous here.

https://codereview.chromium.org/2320763002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/2320763002/diff/20001/components/password_man...
components/password_manager/core/browser/password_store.h:145: // Removes all
the stats created in the given the date range.
On 2016/09/08 09:04:36, vabr (Chromium) wrote:
> nit: The added "the" in "the data range" seems out of place.

Done. No the idea how that got there :)

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

https://codereview.chromium.org/2320763002/diff/20001/components/password_man...
components/password_manager/core/browser/statistics_table.cc:174: success &=
origin_delete_statement.Run();
On 2016/09/08 09:04:36, vabr (Chromium) wrote:
> I'm feeling uncomfortable about mixing bit-wise & and bool values. In this
case
> it should be OK, because Run() returns bool, an hopefully any non-zero value
> will be identical to |true|. However, if somebody changed the signature to a
> numerical type, it might change. What about the bit more verbose:
> success = success && origin_delete_statement.Run();
> ?

Done. Makes sense.

[commit-bot: I haz the power, 2016-09-08 11:57:19]

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

[commit-bot: I haz the power, 2016-09-08 11:57:20]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-09-08 12:08:41]

lgtm

[msramek, 2016-09-08 12:09:56]

Thanks!

[msramek, 2016-09-08 12:10:04]

The CQ bit was checked by msramek@chromium.org

[commit-bot: I haz the power, 2016-09-08 12:10: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-09-08 12:14:10]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-09-08 12:16:16]

Description was changed from

==========
Support origin-based deletion for password store statistics

We iterate over the rows of the SQL DB to find entries matching the origin
filter. Then, we issue a cached DELETE statement for each of the entries.

It would also be possible to issue one "DELETE .. IN (?,?,?)" statement for all
matched origins, however, this requires us to generate the statement runtime
to add the correct number of "?". This is more complex and probably doesn't
bring a performance gain in practice.

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

to

==========
Support origin-based deletion for password store statistics

We iterate over the rows of the SQL DB to find entries matching the origin
filter. Then, we issue a cached DELETE statement for each of the entries.

It would also be possible to issue one "DELETE .. IN (?,?,?)" statement for all
matched origins, however, this requires us to generate the statement runtime
to add the correct number of "?". This is more complex and probably doesn't
bring a performance gain in practice.

BUG=589586

Committed: https://crrev.com/9acfdb3cd901f3aec451ba3aa5305a2cc2266739
Cr-Commit-Position: refs/heads/master@{#417262}
==========

[commit-bot: I haz the power, 2016-09-08 12:16:17]

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