Introducing the activityLogPrivate.deleteActivities() API call.

Introducing the activityLogPrivate.deleteActivities() API
call. The call takes an array of activity IDs (each activity
ID is a string) and deletes the matching activities form the
Activity Log database.

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

[pmarch, 2014-02-11 02:18:36]

Michael, Adrienne, please, review this CL.

[mvrable, 2014-02-11 17:44:34]

https://codereview.chromium.org/154053004/diff/1/chrome/browser/extensions/ac...
File chrome/browser/extensions/activity_log/counting_policy.cc (right):

https://codereview.chromium.org/154053004/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/counting_policy.cc:521: void
CountingPolicy::DoRemoveActions(const std::vector<int64>& action_ids) {
Have you done any comparisons between this version and no batching (just calling
"DELETE FROM %s WHERE rowid = ?" in a loop)?  This version should work, but my
concern is that batching adds a fair bit of complexity and we don't even know if
it is faster.  With the call to GetUniqueStatement SQLite has to re-parse the
SQL statement on each batch.  With no batching and a fixed query string it's
easy to switch to using GetCachedStatement so I'm not even sure it would be
slower.

https://codereview.chromium.org/154053004/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/counting_policy.cc:530: 
I would wrap all the deletions in a transaction (see FlushDatabase).  That will
give better performance when issuing multiple statements to the database.

https://codereview.chromium.org/154053004/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/counting_policy.cc:558: int start = iter
* batch_size;
What happens when action_ids.size() == batch_size?  I think this second query
runs with no row ids and we may want to avoid that.

https://codereview.chromium.org/154053004/diff/1/chrome/browser/extensions/ac...
File chrome/browser/extensions/activity_log/counting_policy_unittest.cc (right):

https://codereview.chromium.org/154053004/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/counting_policy_unittest.cc:1239: //
policy->RemoveActions() deletes actions in batches of 50 itemis. Test the
s/itemis/items/

[pmarch, 2014-02-12 18:19:03]

https://codereview.chromium.org/154053004/diff/1/chrome/browser/extensions/ac...
File chrome/browser/extensions/activity_log/counting_policy.cc (right):

https://codereview.chromium.org/154053004/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/counting_policy.cc:521: void
CountingPolicy::DoRemoveActions(const std::vector<int64>& action_ids) {
On 2014/02/11 17:44:35, mvrable wrote:
> Have you done any comparisons between this version and no batching (just
calling
> "DELETE FROM %s WHERE rowid = ?" in a loop)?  This version should work, but my
> concern is that batching adds a fair bit of complexity and we don't even know
if
> it is faster.  With the call to GetUniqueStatement SQLite has to re-parse the
> SQL statement on each batch.  With no batching and a fixed query string it's
> easy to switch to using GetCachedStatement so I'm not even sure it would be
> slower.
I removed the complexity and now the function deletes one raw at a time.

https://codereview.chromium.org/154053004/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/counting_policy.cc:530: 
On 2014/02/11 17:44:35, mvrable wrote:
> I would wrap all the deletions in a transaction (see FlushDatabase).  That
will
> give better performance when issuing multiple statements to the database.

Done.

https://codereview.chromium.org/154053004/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/counting_policy.cc:558: int start = iter
* batch_size;
On 2014/02/11 17:44:35, mvrable wrote:
> What happens when action_ids.size() == batch_size?  I think this second query
> runs with no row ids and we may want to avoid that.
Correct. I removed this code.

https://codereview.chromium.org/154053004/diff/1/chrome/browser/extensions/ac...
File chrome/browser/extensions/activity_log/counting_policy_unittest.cc (right):

https://codereview.chromium.org/154053004/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/counting_policy_unittest.cc:1239: //
policy->RemoveActions() deletes actions in batches of 50 itemis. Test the
On 2014/02/11 17:44:35, mvrable wrote:
> s/itemis/items/

Done.

[mvrable, 2014-02-12 18:38:54]

lgtm

A couple of minor suggestions, but I'm happy even without these.

https://codereview.chromium.org/154053004/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/counting_policy.cc (right):

https://codereview.chromium.org/154053004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/activity_log/counting_policy.cc:539: std::string
query_str =
query_str can be moved outside the loop since it doesn't change.  In fact, I
think you can even move getting the statement outside the loop and call
query.Reset() after each iteration.

Minor nit: maybe rename from query since that implies a SELECT statement to me. 
Perhaps statement, or delete_stmt, or something similar?

[pmarch, 2014-02-12 19:03:23]

https://codereview.chromium.org/154053004/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/counting_policy.cc (right):

https://codereview.chromium.org/154053004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/activity_log/counting_policy.cc:539: std::string
query_str =
On 2014/02/12 18:38:55, mvrable wrote:
> query_str can be moved outside the loop since it doesn't change.  In fact, I
> think you can even move getting the statement outside the loop and call
> query.Reset() after each iteration.
> 
> Minor nit: maybe rename from query since that implies a SELECT statement to
me. 
> Perhaps statement, or delete_stmt, or something similar?

Done.

[pmarch, 2014-02-12 19:04:36]

Hi Ben

Could you please approve changes to the following files:
	 chrome/browser/extensions/extension_function_histogram_value.h
chrome/common/extensions/api/activity_log_private.json

Thanks

[not at google - send to devlin, 2014-02-12 20:28:27]

I don't know what the use case. Rubberstamp lgtm but see comment.

https://codereview.chromium.org/154053004/diff/320001/chrome/common/extension...
File chrome/common/extensions/api/activity_log_private.json (right):

https://codereview.chromium.org/154053004/diff/320001/chrome/common/extension...
chrome/common/extensions/api/activity_log_private.json:83: "name":
"deleteActivities",
I suppose another interesting API would be to delete based on a list of Filters?

[pmarch, 2014-02-12 20:36:36]

https://codereview.chromium.org/154053004/diff/320001/chrome/common/extension...
File chrome/common/extensions/api/activity_log_private.json (right):

https://codereview.chromium.org/154053004/diff/320001/chrome/common/extension...
chrome/common/extensions/api/activity_log_private.json:83: "name":
"deleteActivities",
On 2014/02/12 20:28:27, kalman wrote:
> I suppose another interesting API would be to delete based on a list of
Filters?

Within the team, we discussed an alternative implementation of the
deleteActivities API call that takes a Filter object as a parameter and deletes
activities that matches the Filter object. Our conclusion was that filtered
deletion would be an overkill for the required use case which is deletion of
individual activities. However, we have implementation of filtered deletion
ready, and may reconsider our decision if there will be demand for such API
call.

[felt, 2014-02-12 20:43:38]

On 2014/02/12 20:36:36, pmarch wrote:
>
https://codereview.chromium.org/154053004/diff/320001/chrome/common/extension...
> File chrome/common/extensions/api/activity_log_private.json (right):
> 
>
https://codereview.chromium.org/154053004/diff/320001/chrome/common/extension...
> chrome/common/extensions/api/activity_log_private.json:83: "name":
> "deleteActivities",
> On 2014/02/12 20:28:27, kalman wrote:
> > I suppose another interesting API would be to delete based on a list of
> Filters?
> 
> Within the team, we discussed an alternative implementation of the
> deleteActivities API call that takes a Filter object as a parameter and
deletes
> activities that matches the Filter object. Our conclusion was that filtered
> deletion would be an overkill for the required use case which is deletion of
> individual activities. However, we have implementation of filtered deletion
> ready, and may reconsider our decision if there will be demand for such API
> call.


I can't review for a few hours please wait for my review before sending to cq

[felt, 2014-02-13 00:58:55]

https://codereview.chromium.org/154053004/diff/320001/chrome/browser/extensio...
File
chrome/browser/extensions/api/activity_log_private/activity_log_private_api.cc
(right):

https://codereview.chromium.org/154053004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/activity_log_private/activity_log_private_api.cc:186:
if (base::StringToInt64(params->activity_ids[i], &value))
should you be throwing an exception if it fails to parse? is that possible? ben
might know the answer

[felt, 2014-02-13 01:00:57]

another thing, can you invoke this from the browser test? so that we get an
end-to-end test of it?

[pmarch, 2014-02-13 18:31:55]

On 2014/02/13 00:58:55, felt wrote:
>
https://codereview.chromium.org/154053004/diff/320001/chrome/browser/extensio...
> File
> chrome/browser/extensions/api/activity_log_private/activity_log_private_api.cc
> (right):
> 
>
https://codereview.chromium.org/154053004/diff/320001/chrome/browser/extensio...
>
chrome/browser/extensions/api/activity_log_private/activity_log_private_api.cc:186:
> if (base::StringToInt64(params->activity_ids[i], &value))
> should you be throwing an exception if it fails to parse? is that possible?
ben
> might know the answer

I think the default behavior should ignore an activity ID if there is no
activity with such ID similar to the Tabs API calls which ignore non-existing
tabs ID.

[pmarch, 2014-02-13 18:32:14]

On 2014/02/13 01:00:57, felt wrote:
> another thing, can you invoke this from the browser test? so that we get an
> end-to-end test of it?

Added e2e test.

[felt, 2014-02-13 18:53:24]

lgtm

[pmarch, 2014-02-13 20:25:02]

The CQ bit was checked by pmarch@chromium.org

[commit-bot: I haz the power, 2014-02-13 20:28:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pmarch@chromium.org/154053004/580001

[commit-bot: I haz the power, 2014-02-13 20:52:47]

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

[commit-bot: I haz the power, 2014-02-13 20:52:47]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[pmarch, 2014-02-13 23:21:51]

The CQ bit was checked by pmarch@chromium.org

[commit-bot: I haz the power, 2014-02-13 23:23:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pmarch@chromium.org/154053004/580001

[commit-bot: I haz the power, 2014-02-14 00:40:51]

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

[commit-bot: I haz the power, 2014-02-14 00:40:52]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[pmarch, 2014-02-14 02:34:14]

The CQ bit was checked by pmarch@chromium.org

[commit-bot: I haz the power, 2014-02-14 02:35:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pmarch@chromium.org/154053004/580001

[commit-bot: I haz the power, 2014-02-14 05:59:59]

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

[commit-bot: I haz the power, 2014-02-14 05:59:59]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[pmarch, 2014-02-14 12:49:05]

The CQ bit was checked by pmarch@chromium.org

[commit-bot: I haz the power, 2014-02-14 12:49:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pmarch@chromium.org/154053004/580001

[commit-bot: I haz the power, 2014-02-14 14:48:02]

Change committed as 251311

[pmarch, 2014-02-14 23:07:02]

A revert of this CL has been created in
https://codereview.chromium.org/167883002/ by pmarch@chromium.org.

The reason for reverting is: CountingPolicyTest.RemoveActions fails on try jobs.

https://code.google.com/p/chromium/issues/detail?id=344042.