Added ActivityLog tests and associated bugfixes/extra logging.

This tackles a number of ActivityLog-related corner cases. I wrote tests and then added fixes as needed.

1) Added test for chrome.app.* API calls
2) Added missing logging for chrome.app.* API calls
3) Removed event listener registration logging; added TODO
4) Added test for operations on objects returned from API calls
5) Added message passing tests
6) Added missing logging for message passing

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

[felt, 2013-04-09 01:50:04]

Hi Matt,

I added some new tests and added/removed logging in a few places. Could you take
a look?

Thanks,
Adrienne

[Matt Perry, 2013-04-09 20:56:59]

Please add a BUG number to the CL description. Also, put a general summary of
the CL in the first line of the description.

https://codereview.chromium.org/13726026/diff/10001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log.h (right):

https://codereview.chromium.org/13726026/diff/10001/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:203: // log_arguments controls whether
to log API call arguments. By default, we
fix comment

https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/app_custom_bindings.js (right):

https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_custom_bindings.js:30: if
(!extensionId) {
When can the extensionId be NULL?

https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_custom_bindings.js:31: var app = {
I think you need to declare "var app;" at toplevel scope. otherwise this will
only exist withing the if-block (unless I'm wrong about JavaScript, which is
possible).

https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/binding.js (right):

https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:56: var slicer =
Array.prototype.slice;
why is this necessary?

https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/miscellaneous_bindings.js (right):

https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
chrome/renderer/resources/extensions/miscellaneous_bindings.js:180: var
eventName = (isSendMessage ?
you can use requestEvent_.eventName_

https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
chrome/renderer/resources/extensions/miscellaneous_bindings.js:230:
"runtime.onConnectExternal" : "runtime.onConnect");
same here, with connectEvent

https://codereview.chromium.org/13726026/diff/17001/chrome/renderer/extension...
File chrome/renderer/extensions/api_activity_logger.cc (right):

https://codereview.chromium.org/13726026/diff/17001/chrome/renderer/extension...
chrome/renderer/extensions/api_activity_logger.cc:56: new
ExtensionHostMsg_AddAPIActionToActivityLog(call_type,
It'd be better if call_type were converted to an enum here. You'll need to add
the enum type to chrome/common so that it can be used by both ends of the IPC
message.

Or maybe there should be 2 message types: one for events, one for API calls.

[felt, 2013-04-09 21:06:59]

Hi Matt, thanks! A few questions for clarification:

> Please add a BUG number to the CL description. Also, put a general summary of
> the CL in the first line of the description.

The Activity Log work doesn't really correspond to a bug, because I'm the only
one keeping track of the Activity Log progress. Should I start filing bugs and
assigning them to myself even though no one outside of me + reviewers will look
at them? Or should I just make one Activity Log bug and keep it open and assign
subsequent reviews to it?

>
https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
> chrome/renderer/resources/extensions/binding.js:56: var slicer =
> Array.prototype.slice;
> why is this necessary?
> 

I was worried about prototype poisoning. This code will run in the extension's
or website's scope, right? It seems like function.prototype can't be modified
because the apps object is frozen, but array.slice could be unexpectedly altered
when the code runs. (Or am I misunderstanding how scoping is happening with the
bindings code?)

>
https://codereview.chromium.org/13726026/diff/17001/chrome/renderer/extension...
> chrome/renderer/extensions/api_activity_logger.cc:56: new
> ExtensionHostMsg_AddAPIActionToActivityLog(call_type,
> It'd be better if call_type were converted to an enum here. You'll need to add
> the enum type to chrome/common so that it can be used by both ends of the IPC
> message.
> 
> Or maybe there should be 2 message types: one for events, one for API calls.

Since all the arguments are the same, I thought it would make sense to use the
same IPC message to keep things simple. I'll add the enum unless you feel
strongly that there should be a second message type.

[Matt Perry, 2013-04-09 21:17:34]

On 2013/04/09 21:06:59, felt wrote:
> Hi Matt, thanks! A few questions for clarification:
> 
> > Please add a BUG number to the CL description. Also, put a general summary
of
> > the CL in the first line of the description.
> 
> The Activity Log work doesn't really correspond to a bug, because I'm the only
> one keeping track of the Activity Log progress. Should I start filing bugs and
> assigning them to myself even though no one outside of me + reviewers will
look
> at them? Or should I just make one Activity Log bug and keep it open and
assign
> subsequent reviews to it?

Yeah, 1 bug is fine. It will help with tracking purposes.

> 
> >
>
https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
> > chrome/renderer/resources/extensions/binding.js:56: var slicer =
> > Array.prototype.slice;
> > why is this necessary?
> > 
> 
> I was worried about prototype poisoning. This code will run in the extension's
> or website's scope, right? It seems like function.prototype can't be modified
> because the apps object is frozen, but array.slice could be unexpectedly
altered
> when the code runs. (Or am I misunderstanding how scoping is happening with
the
> bindings code?)

Ah, I see. That's a good point - but unfortunately your fix isn't enough to
protect against that. We lazily load the extension bindings, so this could run
after client code. kalman has fixed similar issues (see
https://codereview.chromium.org/12440030 ), I'll talk to him about it. For now,
you can just leave it.

> 
> >
>
https://codereview.chromium.org/13726026/diff/17001/chrome/renderer/extension...
> > chrome/renderer/extensions/api_activity_logger.cc:56: new
> > ExtensionHostMsg_AddAPIActionToActivityLog(call_type,
> > It'd be better if call_type were converted to an enum here. You'll need to
add
> > the enum type to chrome/common so that it can be used by both ends of the
IPC
> > message.
> > 
> > Or maybe there should be 2 message types: one for events, one for API calls.
> 
> Since all the arguments are the same, I thought it would make sense to use the
> same IPC message to keep things simple. I'll add the enum unless you feel
> strongly that there should be a second message type.

I have a slight preference for a second message type, but I'll leave it up to
you.

[felt, 2013-04-10 00:21:38]

Matt, made most of the changes. I'm a little confused about the prototype
poisoning issue. Does all of the bindings code run in the scope of the extension
then, sharing global variables and prototype chains? Can extensions access all
of the global variables declared there? Does that mean that every use of an
inherited method or variable in the binding code (regardless of whether it's in
a function that's exported) is possibly suspect?

https://codereview.chromium.org/13726026/diff/10001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log.h (right):

https://codereview.chromium.org/13726026/diff/10001/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:203: // log_arguments controls whether
to log API call arguments. By default, we
On 2013/04/09 20:56:59, Matt Perry wrote:
> fix comment

Done.

https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/app_custom_bindings.js (right):

https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_custom_bindings.js:30: if
(!extensionId) {
On 2013/04/09 20:56:59, Matt Perry wrote:
> When can the extensionId be NULL?

If this is a website, extensionId is null and we don't want to do any logging.

https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_custom_bindings.js:31: var app = {
On 2013/04/09 20:56:59, Matt Perry wrote:
> I think you need to declare "var app;" at toplevel scope. otherwise this will
> only exist withing the if-block (unless I'm wrong about JavaScript, which is
> possible).

It works fine actually. But maybe it's more clear with var app; first. (Done.)

https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/miscellaneous_bindings.js (right):

https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
chrome/renderer/resources/extensions/miscellaneous_bindings.js:180: var
eventName = (isSendMessage ?
On 2013/04/09 20:56:59, Matt Perry wrote:
> you can use requestEvent_.eventName_

Done.

https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
chrome/renderer/resources/extensions/miscellaneous_bindings.js:230:
"runtime.onConnectExternal" : "runtime.onConnect");
On 2013/04/09 20:56:59, Matt Perry wrote:
> same here, with connectEvent

Done.

[Matt Perry, 2013-04-10 00:26:21]

On 2013/04/10 00:21:38, felt wrote:
> Matt, made most of the changes. I'm a little confused about the prototype
> poisoning issue. Does all of the bindings code run in the scope of the
extension
> then, sharing global variables and prototype chains? Can extensions access all
> of the global variables declared there? Does that mean that every use of an
> inherited method or variable in the binding code (regardless of whether it's
in
> a function that's exported) is possibly suspect?

The bindings run in the same context, but a different scope. So global variables
and prototypes are shared. It's as if the bindings code is surrounded by an
anonymous function, e.g.
  (function() {
    runBindings();
  })();
(and in fact, I think that's exactly what happens behind the scenes.)

(Also, looks like you didn't upload the newest version yet.)

> 
>
https://codereview.chromium.org/13726026/diff/10001/chrome/browser/extensions...
> File chrome/browser/extensions/activity_log.h (right):
> 
>
https://codereview.chromium.org/13726026/diff/10001/chrome/browser/extensions...
> chrome/browser/extensions/activity_log.h:203: // log_arguments controls
whether
> to log API call arguments. By default, we
> On 2013/04/09 20:56:59, Matt Perry wrote:
> > fix comment
> 
> Done.
> 
>
https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
> File chrome/renderer/resources/extensions/app_custom_bindings.js (right):
> 
>
https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
> chrome/renderer/resources/extensions/app_custom_bindings.js:30: if
> (!extensionId) {
> On 2013/04/09 20:56:59, Matt Perry wrote:
> > When can the extensionId be NULL?
> 
> If this is a website, extensionId is null and we don't want to do any logging.
> 
>
https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
> chrome/renderer/resources/extensions/app_custom_bindings.js:31: var app = {
> On 2013/04/09 20:56:59, Matt Perry wrote:
> > I think you need to declare "var app;" at toplevel scope. otherwise this
will
> > only exist withing the if-block (unless I'm wrong about JavaScript, which is
> > possible).
> 
> It works fine actually. But maybe it's more clear with var app; first. (Done.)
> 
>
https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
> File chrome/renderer/resources/extensions/miscellaneous_bindings.js (right):
> 
>
https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
> chrome/renderer/resources/extensions/miscellaneous_bindings.js:180: var
> eventName = (isSendMessage ?
> On 2013/04/09 20:56:59, Matt Perry wrote:
> > you can use requestEvent_.eventName_
> 
> Done.
> 
>
https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
> chrome/renderer/resources/extensions/miscellaneous_bindings.js:230:
> "runtime.onConnectExternal" : "runtime.onConnect");
> On 2013/04/09 20:56:59, Matt Perry wrote:
> > same here, with connectEvent
> 
> Done.

[felt, 2013-04-10 00:27:01]

Chris & Justin, 

You previously gave me some feedback on the new IPC for Activity Logging that I
introduced in https://codereview.chromium.org/12517011/. It turns out I need to
expand that communication, to log both API calls and events. I have two choices:
add a new IPC message to log events, or overload the existing one to handle both
API calls and events (and add an extra parameter that's an enum that says
whether it's an API call or event). The choice to add a new IPC message is
slightly cleaner, but gives you an additional IPC to review that's almost
identical to another one. Do you have a preference?

[felt, 2013-04-10 00:33:11]

Sorry, it's there now -- I'd been writing the other e-mail simultaneously.

On 2013/04/10 00:26:21, Matt Perry wrote:
> On 2013/04/10 00:21:38, felt wrote:
> > Matt, made most of the changes. I'm a little confused about the prototype
> > poisoning issue. Does all of the bindings code run in the scope of the
> extension
> > then, sharing global variables and prototype chains? Can extensions access
all
> > of the global variables declared there? Does that mean that every use of an
> > inherited method or variable in the binding code (regardless of whether it's
> in
> > a function that's exported) is possibly suspect?
> 
> The bindings run in the same context, but a different scope. So global
variables
> and prototypes are shared. It's as if the bindings code is surrounded by an
> anonymous function, e.g.
>   (function() {
>     runBindings();
>   })();
> (and in fact, I think that's exactly what happens behind the scenes.)
> 
> (Also, looks like you didn't upload the newest version yet.)
> 
> > 
> >
>
https://codereview.chromium.org/13726026/diff/10001/chrome/browser/extensions...
> > File chrome/browser/extensions/activity_log.h (right):
> > 
> >
>
https://codereview.chromium.org/13726026/diff/10001/chrome/browser/extensions...
> > chrome/browser/extensions/activity_log.h:203: // log_arguments controls
> whether
> > to log API call arguments. By default, we
> > On 2013/04/09 20:56:59, Matt Perry wrote:
> > > fix comment
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
> > File chrome/renderer/resources/extensions/app_custom_bindings.js (right):
> > 
> >
>
https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
> > chrome/renderer/resources/extensions/app_custom_bindings.js:30: if
> > (!extensionId) {
> > On 2013/04/09 20:56:59, Matt Perry wrote:
> > > When can the extensionId be NULL?
> > 
> > If this is a website, extensionId is null and we don't want to do any
logging.
> > 
> >
>
https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
> > chrome/renderer/resources/extensions/app_custom_bindings.js:31: var app = {
> > On 2013/04/09 20:56:59, Matt Perry wrote:
> > > I think you need to declare "var app;" at toplevel scope. otherwise this
> will
> > > only exist withing the if-block (unless I'm wrong about JavaScript, which
is
> > > possible).
> > 
> > It works fine actually. But maybe it's more clear with var app; first.
(Done.)
> > 
> >
>
https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
> > File chrome/renderer/resources/extensions/miscellaneous_bindings.js (right):
> > 
> >
>
https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
> > chrome/renderer/resources/extensions/miscellaneous_bindings.js:180: var
> > eventName = (isSendMessage ?
> > On 2013/04/09 20:56:59, Matt Perry wrote:
> > > you can use requestEvent_.eventName_
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
> > chrome/renderer/resources/extensions/miscellaneous_bindings.js:230:
> > "runtime.onConnectExternal" : "runtime.onConnect");
> > On 2013/04/09 20:56:59, Matt Perry wrote:
> > > same here, with connectEvent
> > 
> > Done.

[felt, 2013-04-10 22:25:04]

Matt: I removed the Array.slice stuff for now. Separately, Ankur is going to
take a look at what can be done with v8::context since he is more familiar with
V8 & WebKit.

https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/binding.js (right):

https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:56: var slicer =
Array.prototype.slice;
On 2013/04/09 20:56:59, Matt Perry wrote:
> why is this necessary?

Removed for now.

https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/miscellaneous_bindings.js (right):

https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
chrome/renderer/resources/extensions/miscellaneous_bindings.js:180: var
eventName = (isSendMessage ?
I reverted this change -- eventName_ is there, but doesn't have a value. (For
both.)

On 2013/04/10 00:21:38, felt wrote:
> On 2013/04/09 20:56:59, Matt Perry wrote:
> > you can use requestEvent_.eventName_
> 
> Done.

[palmer, 2013-04-10 22:46:38]

On 2013/04/10 00:27:01, felt wrote:
> Chris & Justin, 
> 
> You previously gave me some feedback on the new IPC for Activity Logging that
I
> introduced in https://codereview.chromium.org/12517011/. It turns out I need
to
> expand that communication, to log both API calls and events. I have two
choices:
> add a new IPC message to log events, or overload the existing one to handle
both
> API calls and events (and add an extra parameter that's an enum that says
> whether it's an API call or event). The choice to add a new IPC message is
> slightly cleaner, but gives you an additional IPC to review that's almost
> identical to another one. Do you have a preference?

Can they share implementation code underneath? I'd go with whichever one results
in less code, and if that is equal, go with whatever is cleaner to you.

[Matt Perry, 2013-04-10 22:48:05]

https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/miscellaneous_bindings.js (right):

https://codereview.chromium.org/13726026/diff/10001/chrome/renderer/resources...
chrome/renderer/resources/extensions/miscellaneous_bindings.js:180: var
eventName = (isSendMessage ?
On 2013/04/10 22:25:04, felt wrote:
> I reverted this change -- eventName_ is there, but doesn't have a value. (For
> both.)

Doh, sorry about that.

> 
> On 2013/04/10 00:21:38, felt wrote:
> > On 2013/04/09 20:56:59, Matt Perry wrote:
> > > you can use requestEvent_.eventName_
> > 
> > Done.
>

https://codereview.chromium.org/13726026/diff/41001/chrome/renderer/extension...
File chrome/renderer/extensions/api_activity_logger.cc (right):

https://codereview.chromium.org/13726026/diff/41001/chrome/renderer/extension...
chrome/renderer/extensions/api_activity_logger.cc:56: new
ExtensionHostMsg_AddAPIActionToActivityLog(call_type,
This still should become an enum or 2 IPC messages.

[palmer, 2013-04-10 23:52:48]

https://codereview.chromium.org/13726026/diff/41001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/13726026/diff/41001/chrome/browser/renderer_h...
chrome/browser/renderer_host/chrome_render_message_filter.cc:83: if (call_type
== "API")
Hmm, how open-ended are these call types? Maybe an enum and then a switch/case
instead?

https://codereview.chromium.org/13726026/diff/41001/chrome/common/extensions/...
File chrome/common/extensions/extension_messages.h (right):

https://codereview.chromium.org/13726026/diff/41001/chrome/common/extensions/...
chrome/common/extensions/extension_messages.h:605: std::string /* type of API
call */,
Definitely preferable to send a more primitive type (like an enum).

https://codereview.chromium.org/13726026/diff/41001/chrome/renderer/extension...
File chrome/renderer/extensions/api_activity_logger.cc (right):

https://codereview.chromium.org/13726026/diff/41001/chrome/renderer/extension...
chrome/renderer/extensions/api_activity_logger.cc:56: new
ExtensionHostMsg_AddAPIActionToActivityLog(call_type,
> This still should become an enum or 2 IPC messages.

I'm happy either way, but not so much with a string.

https://codereview.chromium.org/13726026/diff/41001/chrome/test/data/extensio...
File chrome/test/data/extensions/activity_log/manifest.json (right):

https://codereview.chromium.org/13726026/diff/41001/chrome/test/data/extensio...
chrome/test/data/extensions/activity_log/manifest.json:10: "content_scripts": [
google.bw doesn't exist?

[felt, 2013-04-11 20:56:57]

I refactored all of the code that sends a record of an api call/event from the
bindings code to the Activity Log. There is now:

-- Two separate calls exposed to bindings code (LogEvent & LogAPICall), so it's
no longer a parameter
-- Two separate ExtensionHostMsg API calls
-- A single underlying implementation in chrome_render_message_filter.cc that
uses an enum

https://codereview.chromium.org/13726026/diff/41001/chrome/test/data/extensio...
File chrome/test/data/extensions/activity_log/manifest.json (right):

https://codereview.chromium.org/13726026/diff/41001/chrome/test/data/extensio...
chrome/test/data/extensions/activity_log/manifest.json:10: "content_scripts": [
On 2013/04/10 23:52:48, Chris P. wrote:
> google.bw doesn't exist?

Done.

[palmer, 2013-04-11 21:53:39]

IPC security LGTM

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

https://codereview.chromium.org/13726026/diff/53001/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:153: testing_mode_ =
CommandLine::ForCurrentProcess()->
NIT: Fine by me, but some Chromies might want you to put |HasSwitch| on the same
line, then indent the argument by 4 spaces on the next line.

https://codereview.chromium.org/13726026/diff/53001/chrome/common/extensions/...
File chrome/common/extensions/extension_messages.h (right):

https://codereview.chromium.org/13726026/diff/53001/chrome/common/extensions/...
chrome/common/extensions/extension_messages.h:38: 
NIT: Spurious trailing spaces here?

[Matt Perry, 2013-04-12 01:03:17]

lgtm

https://codereview.chromium.org/13726026/diff/53001/chrome/renderer/extension...
File chrome/renderer/extensions/api_activity_logger.cc (right):

https://codereview.chromium.org/13726026/diff/53001/chrome/renderer/extension...
chrome/renderer/extensions/api_activity_logger.cc:65: if (call_type == kApiCall)
nit: use braces for multi-line ifs

https://codereview.chromium.org/13726026/diff/53001/chrome/renderer/extension...
File chrome/renderer/extensions/api_activity_logger.h (right):

https://codereview.chromium.org/13726026/diff/53001/chrome/renderer/extension...
chrome/renderer/extensions/api_activity_logger.h:41: kEvent
Prevailing chrome style is to use ALL_CAPS for enum names.

[felt, 2013-04-12 02:07:21]

Hi Brett, can I please get OWNERS approval for
chrome/browser/renderer_host/chrome_render_message_filter.[cc|h]?

(Finished Matt and Chris's last nits.)

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

https://codereview.chromium.org/13726026/diff/53001/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:153: testing_mode_ =
CommandLine::ForCurrentProcess()->
On 2013/04/11 21:53:39, Chris P. wrote:
> NIT: Fine by me, but some Chromies might want you to put |HasSwitch| on the
same
> line, then indent the argument by 4 spaces on the next line.

Done.

https://codereview.chromium.org/13726026/diff/53001/chrome/common/extensions/...
File chrome/common/extensions/extension_messages.h (right):

https://codereview.chromium.org/13726026/diff/53001/chrome/common/extensions/...
chrome/common/extensions/extension_messages.h:38: 
On 2013/04/11 21:53:39, Chris P. wrote:
> NIT: Spurious trailing spaces here?

Done.

https://codereview.chromium.org/13726026/diff/53001/chrome/renderer/extension...
File chrome/renderer/extensions/api_activity_logger.cc (right):

https://codereview.chromium.org/13726026/diff/53001/chrome/renderer/extension...
chrome/renderer/extensions/api_activity_logger.cc:65: if (call_type == kApiCall)
On 2013/04/12 01:03:17, Matt Perry wrote:
> nit: use braces for multi-line ifs

Done.

https://codereview.chromium.org/13726026/diff/53001/chrome/renderer/extension...
File chrome/renderer/extensions/api_activity_logger.h (right):

https://codereview.chromium.org/13726026/diff/53001/chrome/renderer/extension...
chrome/renderer/extensions/api_activity_logger.h:41: kEvent
On 2013/04/12 01:03:17, Matt Perry wrote:
> Prevailing chrome style is to use ALL_CAPS for enum names.

The style guide says "New code should prefer constant-style naming if possible"
(first time writing a new enum in C++ so I looked it up this morning).
https://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Enumerator_Names

[Matt Perry, 2013-04-12 17:04:43]

https://codereview.chromium.org/13726026/diff/53001/chrome/renderer/extension...
File chrome/renderer/extensions/api_activity_logger.h (right):

https://codereview.chromium.org/13726026/diff/53001/chrome/renderer/extension...
chrome/renderer/extensions/api_activity_logger.h:41: kEvent
On 2013/04/12 02:07:21, felt wrote:
> On 2013/04/12 01:03:17, Matt Perry wrote:
> > Prevailing chrome style is to use ALL_CAPS for enum names.
> 
> The style guide says "New code should prefer constant-style naming if
possible"
> (first time writing a new enum in C++ so I looked it up this morning).
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Enumerator_Names

Chrome has a list of addendums: http://dev.chromium.org/developers/coding-style

Search for 'enum'.

[brettw, 2013-04-12 18:14:22]

lgtm

[felt, 2013-04-12 20:37:23]

https://codereview.chromium.org/13726026/diff/53001/chrome/renderer/extension...
File chrome/renderer/extensions/api_activity_logger.h (right):

https://codereview.chromium.org/13726026/diff/53001/chrome/renderer/extension...
chrome/renderer/extensions/api_activity_logger.h:41: kEvent
Ah, thanks!

On 2013/04/12 17:04:43, Matt Perry wrote:
> On 2013/04/12 02:07:21, felt wrote:
> > On 2013/04/12 01:03:17, Matt Perry wrote:
> > > Prevailing chrome style is to use ALL_CAPS for enum names.
> > 
> > The style guide says "New code should prefer constant-style naming if
> possible"
> > (first time writing a new enum in C++ so I looked it up this morning).
> >
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Enumerator_Names
> 
> Chrome has a list of addendums:
http://dev.chromium.org/developers/coding-style
> 
> Search for 'enum'.

[commit-bot: I haz the power, 2013-04-12 20:38:16]

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

[commit-bot: I haz the power, 2013-04-12 20:38:32]

Presubmit check for 13726026-37002 failed and returned exit status 1.

INFO:root:Found 15 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  chrome/test/data/extensions/activity_log_app/background.js, line 14, 82 chars

Presubmit checks took 2.5s to calculate.

[commit-bot: I haz the power, 2013-04-12 20:41:33]

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

[Matt Perry, 2013-04-12 20:50:33]

https://codereview.chromium.org/13726026/diff/78001/chrome/renderer/extension...
File chrome/renderer/extensions/api_activity_logger.h (right):

https://codereview.chromium.org/13726026/diff/78001/chrome/renderer/extension...
chrome/renderer/extensions/api_activity_logger.h:40: APICALL,
nit: API_CALL

[felt, 2013-04-12 20:51:46]

https://codereview.chromium.org/13726026/diff/78001/chrome/renderer/extension...
File chrome/renderer/extensions/api_activity_logger.h (right):

https://codereview.chromium.org/13726026/diff/78001/chrome/renderer/extension...
chrome/renderer/extensions/api_activity_logger.h:40: APICALL,
I get a compiler error if I do API_CALL. I'm not sure why. Is it important
enough for me to track down the cause?

On 2013/04/12 20:50:34, Matt Perry wrote:
> nit: API_CALL

[Matt Perry, 2013-04-12 20:53:51]

https://codereview.chromium.org/13726026/diff/78001/chrome/renderer/extension...
File chrome/renderer/extensions/api_activity_logger.h (right):

https://codereview.chromium.org/13726026/diff/78001/chrome/renderer/extension...
chrome/renderer/extensions/api_activity_logger.h:40: APICALL,
Oh weird. Nah, this is fine.

On 2013/04/12 20:51:46, felt wrote:
> I get a compiler error if I do API_CALL. I'm not sure why. Is it important
> enough for me to track down the cause?
> 
> On 2013/04/12 20:50:34, Matt Perry wrote:
> > nit: API_CALL
>

[commit-bot: I haz the power, 2013-04-12 22:56:45]

Change committed as 194034