[Activity Log] when extension is uninstalled, delete data about it

[Activity Log] when extension is uninstalled, delete data about it

When a user uninstalls an extension, all of the data about that extension is removed from the activity log database. This is a privacy measure.

BUG=244841
R=mpcomplete@chromium.org

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

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

[felt, 2013-09-09 14:31:07]

Hi Michael, can you take a quick peek at the if-statement?

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

https://codereview.chromium.org/23983014/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/counting_policy.cc:586: if
(string_table_.StringToInt(db, extension_id, &id)) {
Michael, can you check this if-statement? Is the else clause needed for safety,
or can we be sure this won't happen?

[felt, 2013-09-09 14:31:50]

Hi Matt, can you please review this CL? Added per privacy review request.

[mvrable, 2013-09-09 17:39:44]

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

https://codereview.chromium.org/23983014/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/counting_policy.cc:586: if
(string_table_.StringToInt(db, extension_id, &id)) {
On 2013/09/09 14:31:08, felt wrote:
> Michael, can you check this if-statement? Is the else clause needed for
safety,
> or can we be sure this won't happen?

The else clause is not needed.  All writes to the database will use an integer
ID and not a string.

[Matt Perry, 2013-09-09 19:02:02]

lgtm

https://codereview.chromium.org/23983014/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_log.cc (right):

https://codereview.chromium.org/23983014/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log.cc:323: if (policy_ &&
(extension->id() != kActivityLogExtensionId))
nit: I'd restructure these two ifs to an if-else on whether it matches the
activity log id.

[felt, 2013-09-09 21:19:44]

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

https://codereview.chromium.org/23983014/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/counting_policy.cc:586: if
(string_table_.StringToInt(db, extension_id, &id)) {
On 2013/09/09 17:39:44, mvrable wrote:
> On 2013/09/09 14:31:08, felt wrote:
> > Michael, can you check this if-statement? Is the else clause needed for
> safety,
> > or can we be sure this won't happen?
> 
> The else clause is not needed.  All writes to the database will use an integer
> ID and not a string.

Thanks, changed to a return.

https://codereview.chromium.org/23983014/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_log.cc (right):

https://codereview.chromium.org/23983014/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log.cc:323: if (policy_ &&
(extension->id() != kActivityLogExtensionId))
On 2013/09/09 19:02:02, Matt Perry wrote:
> nit: I'd restructure these two ifs to an if-else on whether it matches the
> activity log id.

Done, although sadly it's still rather ugly.

[commit-bot: I haz the power, 2013-09-09 21:21:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/felt@chromium.org/23983014/11001

[commit-bot: I haz the power, 2013-09-09 22:41:06]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-10 00:41:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/felt@chromium.org/23983014/11001

[commit-bot: I haz the power, 2013-09-10 09:32:53]

Change committed as 222242

[commit-bot: I haz the power, 2013-09-13 14:27:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/felt@chromium.org/23983014/49001

[commit-bot: I haz the power, 2013-09-13 20:28:41]

Change committed as 223113