Replaced enum strings with ints in Activity Log database

Replaced enum strings with ints in Activity Log database

Several of the Activity Log actions have fields that are enums. Previously we were saving them as strings. To save a little database space they are now going to be saved as ints.

R=mpcomplete@chromium.org
BUG=238256
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201316

[felt, 2013-05-17 05:07:07]

Hi Matt, please review this CL. Thanks!

[Matt Perry, 2013-05-17 19:08:55]

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
File chrome/browser/extensions/activity_log/api_actions.cc (right):

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/api_actions.cc:41: {"api_type",
"api_call", "args", "extra"};
You could probably store the api_call as an integer too. We have a large enum of
them in extension_function_histogram_value.h.

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/api_actions.cc:67:
type_((Type)s.ColumnInt(2)),
style guide forbids c-style casts. use static_cast<Type>()

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/api_actions.cc:114: statement.BindInt(2,
(int)type_);
cast style

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
File chrome/browser/extensions/activity_log/api_actions.h (right):

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/api_actions.h:17: enum Type {
Add a comment here warning that these values shouldn't be changed. Ditto for the
other enums.

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
File chrome/browser/extensions/activity_log/blocked_actions.cc (right):

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/blocked_actions.cc:37:
reason_((Reason)s.ColumnInt(4)),
cast style

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/blocked_actions.cc:83:
statement.BindInt(4, (int)reason_);
cast style

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
File chrome/browser/extensions/activity_log/dom_actions.cc (right):

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/dom_actions.cc:42:
verb_((DOMActionType)s.ColumnInt(2)),
cast style

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/dom_actions.cc:92: statement.BindInt(2,
(int)verb_);
cast style

[felt, 2013-05-18 00:27:18]

Hi Matt,

I fixed the casting and added a similar mapping for API calls (that was a good
idea).

Adrienne

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
File chrome/browser/extensions/activity_log/api_actions.cc (right):

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/api_actions.cc:41: {"api_type",
"api_call", "args", "extra"};
This is a good idea. I added code that does this. However the histogram was
missing events and a number of apps APIs so I added my own constants list.

On 2013/05/17 19:08:55, Matt Perry wrote:
> You could probably store the api_call as an integer too. We have a large enum
of
> them in extension_function_histogram_value.h.

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/api_actions.cc:67:
type_((Type)s.ColumnInt(2)),
whoops, I didn't remember that. Done.

On 2013/05/17 19:08:55, Matt Perry wrote:
> style guide forbids c-style casts. use static_cast<Type>()

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/api_actions.cc:114: statement.BindInt(2,
(int)type_);
On 2013/05/17 19:08:55, Matt Perry wrote:
> cast style

Done.

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
File chrome/browser/extensions/activity_log/api_actions.h (right):

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/api_actions.h:17: enum Type {
On 2013/05/17 19:08:55, Matt Perry wrote:
> Add a comment here warning that these values shouldn't be changed. Ditto for
the
> other enums.

Done.

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
File chrome/browser/extensions/activity_log/blocked_actions.cc (right):

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/blocked_actions.cc:37:
reason_((Reason)s.ColumnInt(4)),
On 2013/05/17 19:08:55, Matt Perry wrote:
> cast style

Done.

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/blocked_actions.cc:83:
statement.BindInt(4, (int)reason_);
On 2013/05/17 19:08:55, Matt Perry wrote:
> cast style

Done.

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/blocked_actions.cc:83:
statement.BindInt(4, (int)reason_);
On 2013/05/17 19:08:55, Matt Perry wrote:
> cast style

Done.

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
File chrome/browser/extensions/activity_log/dom_actions.cc (right):

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/dom_actions.cc:42:
verb_((DOMActionType)s.ColumnInt(2)),
On 2013/05/17 19:08:55, Matt Perry wrote:
> cast style

Done.

https://codereview.chromium.org/14774012/diff/11/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/dom_actions.cc:92: statement.BindInt(2,
(int)verb_);
On 2013/05/17 19:08:55, Matt Perry wrote:
> cast style

Done.

[Matt Perry, 2013-05-18 00:38:19]

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/api_actions.cc (right):

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:51: for (int i = 0; i <
activity_log_api_name_constants::kNumNames; ++i) {
use arraysize(kNames) and you won't need this constant

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:55: num << i;
stringstreams are against the style guide. Use base::IntToString.

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:56:
names_to_nums_.insert(std::pair<std::string, std::string>(name,
shorter: names_to_nums_.insert(std::make_pair(name, num));
shortest: names_to_nums_[name] = num;

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:69: std::string
ApiToShortname(std::string api_call) {
pass and return by const ref

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/api_name_constants.h (right):

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_name_constants.h:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
Is this list auto-generated? It would be great if you could generate this as a
presubmit step. See
https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...
for where the histogram values are updated.

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_name_constants.h:16: {"alarms.clear",
"alarms.clearAll", "alarms.create", "alarms.get",
style nit:
const char* const kNames[] = {
  ...
};

[felt, 2013-05-18 01:25:21]

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/api_actions.cc (right):

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:51: for (int i = 0; i <
activity_log_api_name_constants::kNumNames; ++i) {
On 2013/05/18 00:38:19, Matt Perry wrote:
> use arraysize(kNames) and you won't need this constant

Done.

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:55: num << i;
On 2013/05/18 00:38:19, Matt Perry wrote:
> stringstreams are against the style guide. Use base::IntToString.

Done.

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:56:
names_to_nums_.insert(std::pair<std::string, std::string>(name,
The shorter version populates the map but causes the find() lookup to always
fail. I don't know why they aren't equivalent.

On 2013/05/18 00:38:19, Matt Perry wrote:
> shorter: names_to_nums_.insert(std::make_pair(name, num));
> shortest: names_to_nums_[name] = num;

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:69: std::string
ApiToShortname(std::string api_call) {
On 2013/05/18 00:38:19, Matt Perry wrote:
> pass and return by const ref

Done.

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/api_name_constants.h (right):

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_name_constants.h:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
Yes it was auto-generated although the script is not great (it doesn't currently
account for changes in the list, which will need to be identified and appended).
I'm pulling down from http://chrome-api.storage.googleapis.com/apps_latest.json
and http://chrome-api.storage.googleapis.com/extensions_latest.json.

If you don't mind I'd rather do that in a separate CL to (1) make very sure I
get it right, and (2) learn Python. [[This existing CL is currently blocking
Domagoj.]]

On 2013/05/18 00:38:19, Matt Perry wrote:
> Is this list auto-generated? It would be great if you could generate this as a
> presubmit step. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...
> for where the histogram values are updated.

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_name_constants.h:16: {"alarms.clear",
"alarms.clearAll", "alarms.create", "alarms.get",
On 2013/05/18 00:38:19, Matt Perry wrote:
> style nit:
> const char* const kNames[] = {
>   ...
> };

Done.

[Matt Perry, 2013-05-20 17:46:14]

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/api_actions.cc (right):

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:55: num << i;
On 2013/05/18 01:25:21, felt wrote:
> On 2013/05/18 00:38:19, Matt Perry wrote:
> > stringstreams are against the style guide. Use base::IntToString.
> 
> Done.

This one still...

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:56:
names_to_nums_.insert(std::pair<std::string, std::string>(name,
Which find() lookup? That doesn't make sense to me...

On 2013/05/18 01:25:21, felt wrote:
> The shorter version populates the map but causes the find() lookup to always
> fail. I don't know why they aren't equivalent.
> 
> On 2013/05/18 00:38:19, Matt Perry wrote:
> > shorter: names_to_nums_.insert(std::make_pair(name, num));
> > shortest: names_to_nums_[name] = num;
>

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/api_name_constants.h (right):

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_name_constants.h:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
Yep, that's a good plan.

On 2013/05/18 01:25:21, felt wrote:
> Yes it was auto-generated although the script is not great (it doesn't
currently
> account for changes in the list, which will need to be identified and
appended).
> I'm pulling down from
http://chrome-api.storage.googleapis.com/apps_latest.json
> and http://chrome-api.storage.googleapis.com/extensions_latest.json.
> 
> If you don't mind I'd rather do that in a separate CL to (1) make very sure I
> get it right, and (2) learn Python. [[This existing CL is currently blocking
> Domagoj.]]
> 
> On 2013/05/18 00:38:19, Matt Perry wrote:
> > Is this list auto-generated? It would be great if you could generate this as
a
> > presubmit step. See
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...
> > for where the histogram values are updated.
>

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_name_constants.h:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
nit: drop the (c) in the file header. See
http://dev.chromium.org/developers/coding-style#TOC-File-headers

[felt, 2013-05-20 21:10:38]

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/api_actions.cc (right):

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:55: num << i;
I'm confused -- the newest version uses IntToString:

std::string name =
    std::string(activity_log_api_name_constants::kNames[i]);
std::string num = base::IntToString(i);

Is that not what you wanted?

On 2013/05/20 17:46:14, Matt Perry wrote:
> On 2013/05/18 01:25:21, felt wrote:
> > On 2013/05/18 00:38:19, Matt Perry wrote:
> > > stringstreams are against the style guide. Use base::IntToString.
> > 
> > Done.
> 
> This one still...

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:56:
names_to_nums_.insert(std::pair<std::string, std::string>(name,
In ShortnameToApi. If I change the syntax to brackets, find() *always* returns
the end() iterator (as if the entry weren't there). I don't know if there's
something wrong with ShortnameToApi but it looks fine to me so I am not sure why
find() only works if I use insert().

On 2013/05/20 17:46:14, Matt Perry wrote:
> Which find() lookup? That doesn't make sense to me...
> 
> On 2013/05/18 01:25:21, felt wrote:
> > The shorter version populates the map but causes the find() lookup to always
> > fail. I don't know why they aren't equivalent.
> > 
> > On 2013/05/18 00:38:19, Matt Perry wrote:
> > > shorter: names_to_nums_.insert(std::make_pair(name, num));
> > > shortest: names_to_nums_[name] = num;
> > 
>

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/api_name_constants.h (right):

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_name_constants.h:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
On 2013/05/20 17:46:14, Matt Perry wrote:
> nit: drop the (c) in the file header. See
> http://dev.chromium.org/developers/coding-style#TOC-File-headers

Done. By the way-- how did you know about this? Do you go back and re-read the
style guide periodically, or is there a style mailing list that announces
changes?

[Matt Perry, 2013-05-20 21:20:48]

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/api_actions.cc (right):

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:55: num << i;
Sorry, I didn't see the latest version uploaded.

On 2013/05/20 21:10:38, felt wrote:
> I'm confused -- the newest version uses IntToString:
> 
> std::string name =
>     std::string(activity_log_api_name_constants::kNames[i]);
> std::string num = base::IntToString(i);
> 
> Is that not what you wanted?
> 
> On 2013/05/20 17:46:14, Matt Perry wrote:
> > On 2013/05/18 01:25:21, felt wrote:
> > > On 2013/05/18 00:38:19, Matt Perry wrote:
> > > > stringstreams are against the style guide. Use base::IntToString.
> > > 
> > > Done.
> > 
> > This one still...
>

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:56:
names_to_nums_.insert(std::pair<std::string, std::string>(name,
Maybe your use of const in ShortnameToApi is to blame? if you change it to a
const std::string&, does that fix it?

If not, what about std::make_pair instead of std::pair<> ?

On 2013/05/20 21:10:38, felt wrote:
> In ShortnameToApi. If I change the syntax to brackets, find() *always* returns
> the end() iterator (as if the entry weren't there). I don't know if there's
> something wrong with ShortnameToApi but it looks fine to me so I am not sure
why
> find() only works if I use insert().
> 
> On 2013/05/20 17:46:14, Matt Perry wrote:
> > Which find() lookup? That doesn't make sense to me...
> > 
> > On 2013/05/18 01:25:21, felt wrote:
> > > The shorter version populates the map but causes the find() lookup to
always
> > > fail. I don't know why they aren't equivalent.
> > > 
> > > On 2013/05/18 00:38:19, Matt Perry wrote:
> > > > shorter: names_to_nums_.insert(std::make_pair(name, num));
> > > > shortest: names_to_nums_[name] = num;
> > > 
> > 
>

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/api_name_constants.h (right):

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_name_constants.h:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
On 2013/05/20 21:10:38, felt wrote:
> On 2013/05/20 17:46:14, Matt Perry wrote:
> > nit: drop the (c) in the file header. See
> > http://dev.chromium.org/developers/coding-style#TOC-File-headers
> 
> Done. By the way-- how did you know about this? Do you go back and re-read the
> style guide periodically, or is there a style mailing list that announces
> changes?

I just happened to overhear someone mentioning this change. I don't reread the
style guide unless someone calls me out on a style violation.

https://codereview.chromium.org/14774012/diff/37001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/api_actions.cc (right):

https://codereview.chromium.org/14774012/diff/37001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:52: for (int i = 0;
use size_t here and you can drop the static_cast

https://codereview.chromium.org/14774012/diff/37001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:69: const std::string
ApiToShortname(const std::string api_call) {
const ref is:
"const std::string& ApiToShortname(const std::string& api_call) {"

Passing by reference means it just passes a pointer instead of making a copy.

[felt, 2013-05-20 22:34:36]

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/api_actions.cc (right):

https://codereview.chromium.org/14774012/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:56:
names_to_nums_.insert(std::pair<std::string, std::string>(name,
You were right, the problem was that I had const std::string instead of const
std::string&. It works now with the bracket notation. Thanks for spotting that!

On 2013/05/20 21:20:48, Matt Perry wrote:
> Maybe your use of const in ShortnameToApi is to blame? if you change it to a
> const std::string&, does that fix it?
> 
> If not, what about std::make_pair instead of std::pair<> ?
> 
> On 2013/05/20 21:10:38, felt wrote:
> > In ShortnameToApi. If I change the syntax to brackets, find() *always*
returns
> > the end() iterator (as if the entry weren't there). I don't know if there's
> > something wrong with ShortnameToApi but it looks fine to me so I am not sure
> why
> > find() only works if I use insert().
> > 
> > On 2013/05/20 17:46:14, Matt Perry wrote:
> > > Which find() lookup? That doesn't make sense to me...
> > > 
> > > On 2013/05/18 01:25:21, felt wrote:
> > > > The shorter version populates the map but causes the find() lookup to
> always
> > > > fail. I don't know why they aren't equivalent.
> > > > 
> > > > On 2013/05/18 00:38:19, Matt Perry wrote:
> > > > > shorter: names_to_nums_.insert(std::make_pair(name, num));
> > > > > shortest: names_to_nums_[name] = num;
> > > > 
> > > 
> > 
>

https://codereview.chromium.org/14774012/diff/37001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/api_actions.cc (right):

https://codereview.chromium.org/14774012/diff/37001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:52: for (int i = 0;
On 2013/05/20 21:20:48, Matt Perry wrote:
> use size_t here and you can drop the static_cast

Done.

https://codereview.chromium.org/14774012/diff/37001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:69: const std::string
ApiToShortname(const std::string api_call) {
On 2013/05/20 21:20:48, Matt Perry wrote:
> const ref is:
> "const std::string& ApiToShortname(const std::string& api_call) {"
> 
> Passing by reference means it just passes a pointer instead of making a copy.

Done.

[Matt Perry, 2013-05-20 22:35:34]

lgtm

[commit-bot: I haz the power, 2013-05-20 23:59:44]

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

[commit-bot: I haz the power, 2013-05-21 16:41:37]

Change committed as 201316