Cleanups to the refactored extension activity log code

Cleanups to the refactored extension activity log code

Review the activity logging (post refactoring) with other authors, and
commit a few fixes to the logging:
  - Simplify the DOM action types (merge XHR into ACCESS)
  - Fix incognito handling for DOM actions
  - Add a few extra comments regarding API logging

BUG=253368, 267025
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215452

[mvrable, 2013-08-01 23:24:33]

These are a few minor changes that came out of discussions with you and Ulfar
earlier today.  No rush to submit these (I can hold off if you have any other
changes I should make).  When you have a chance can you review?

--Michael Vrable

[felt, 2013-08-02 00:41:53]

Thanks, Michael. This is looking good with only one small comment.

https://codereview.chromium.org/21653002/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_actions.h (right):

https://codereview.chromium.org/21653002/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_actions.h:39: // api_name is
sufficient for distinguishing.
I am fine with just making ACTION_WEB_REQUEST=6 at this point. We have no users
-- I don't think we've even caught up to this schema internally -- so we have no
real backward compatibility concerns.

[mvrable, 2013-08-02 03:33:21]

https://codereview.chromium.org/21653002/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_actions.h (right):

https://codereview.chromium.org/21653002/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_actions.h:39: // api_name is
sufficient for distinguishing.
On 2013/08/02 00:41:53, felt wrote:
> I am fine with just making ACTION_WEB_REQUEST=6 at this point. We have no
users
> -- I don't think we've even caught up to this schema internally -- so we have
no
> real backward compatibility concerns.

Done.

[felt, 2013-08-02 03:34:14]

lgtm

[mvrable, 2013-08-02 04:01:17]

Matt and James,

These are a few cleanups in the new reworked extension activity logging code. 
Would you be able to review this?

Matt: Could you review the files in chrome/browser/extensions?
James: Could you look at
chrome/browser/renderer_host/chrome_render_message_filter.cc

The changes to
chrome/test/data/extensions/api_test/activity_log_private/test/test.js are
trivial, just enabling a test covering the new behavior which had been disabled
before.

--Michael Vrable

[James Hawkins, 2013-08-02 19:19:10]

lgtm

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

https://codereview.chromium.org/21653002/diff/21001/chrome/browser/renderer_h...
chrome/browser/renderer_host/chrome_render_message_filter.cc:61: // If the
action included a URL, check whether it for an incognito profile.
nit: "it is for"

https://codereview.chromium.org/21653002/diff/21001/chrome/browser/renderer_h...
chrome/browser/renderer_host/chrome_render_message_filter.cc:529: // Whether the
URLs are incognito is checked in
Optional nit: Remove this comment, since it can easily get out of date.

[Matt Perry, 2013-08-02 19:23:03]

lgtm

https://codereview.chromium.org/21653002/diff/21001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_actions.h (right):

https://codereview.chromium.org/21653002/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_actions.h:36: ACTION_WEB_REQUEST
= 6,
Should this remain as 7? (If you add another one later, it'll be 7 and you might
misinterpret old data)

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

https://codereview.chromium.org/21653002/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:158: // arg_url;
arbitrarily take the first one.
What's the difference between arg_url and the id_list from args?

[mvrable, 2013-08-02 19:57:09]

https://codereview.chromium.org/21653002/diff/21001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_actions.h (right):

https://codereview.chromium.org/21653002/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_actions.h:36: ACTION_WEB_REQUEST
= 6,
On 2013/08/02 19:23:04, Matt Perry wrote:
> Should this remain as 7? (If you add another one later, it'll be 7 and you
might
> misinterpret old data)

I was originally going to do that, but Adrienne pointed out that this enum was
only very recently added and the risk of confusion at this point is low.  This
keeps the code cleaner for now; we'll definitely not renumber once M30 branches.

(I think the only people who will notice this change are us and people running
canaries that also have the activity log enabled via a flag so the risk is low. 
There's not yet a user interface where you could see the bad data, and even if
it persists in the database it should at worst cause some erroneous data to be
displayed but be non-fatal.)

I could switch back if you'd prefer.

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

https://codereview.chromium.org/21653002/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:158: // arg_url;
arbitrarily take the first one.
On 2013/08/02 19:23:04, Matt Perry wrote:
> What's the difference between arg_url and the id_list from args?

arg_url is a separate database column where we can store a URL (but only one);
if we encounter a URL in the API argument list we'll extract it into arg_url so
we can more easily search through the database.

A couple of tabs APIs can include a list of tabs (id_list); when we translate
those to URLs we can't fit all of them into arg_url so the full list of URLs can
only fit in the args list.

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

https://codereview.chromium.org/21653002/diff/21001/chrome/browser/renderer_h...
chrome/browser/renderer_host/chrome_render_message_filter.cc:61: // If the
action included a URL, check whether it for an incognito profile.
On 2013/08/02 19:19:10, James Hawkins wrote:
> nit: "it is for"

Done.

https://codereview.chromium.org/21653002/diff/21001/chrome/browser/renderer_h...
chrome/browser/renderer_host/chrome_render_message_filter.cc:529: // Whether the
URLs are incognito is checked in
On 2013/08/02 19:19:10, James Hawkins wrote:
> Optional nit: Remove this comment, since it can easily get out of date.

Done.

[felt, 2013-08-02 19:59:31]

https://codereview.chromium.org/21653002/diff/21001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_actions.h (right):

https://codereview.chromium.org/21653002/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_actions.h:36: ACTION_WEB_REQUEST
= 6,
I have a strong preference for keeping the code clean; we don't have any users
except for a few canary users, and since we're going to start deleting data more
than 24h old it won't even matter anyway.

On 2013/08/02 19:57:09, mvrable wrote:
> On 2013/08/02 19:23:04, Matt Perry wrote:
> > Should this remain as 7? (If you add another one later, it'll be 7 and you
> might
> > misinterpret old data)
> 
> I was originally going to do that, but Adrienne pointed out that this enum was
> only very recently added and the risk of confusion at this point is low.  This
> keeps the code cleaner for now; we'll definitely not renumber once M30
branches.
> 
> (I think the only people who will notice this change are us and people running
> canaries that also have the activity log enabled via a flag so the risk is
low. 
> There's not yet a user interface where you could see the bad data, and even if
> it persists in the database it should at worst cause some erroneous data to be
> displayed but be non-fatal.)
> 
> I could switch back if you'd prefer.

[Matt Perry, 2013-08-02 20:26:04]

https://codereview.chromium.org/21653002/diff/21001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_actions.h (right):

https://codereview.chromium.org/21653002/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_actions.h:36: ACTION_WEB_REQUEST
= 6,
SGTM

On 2013/08/02 19:59:31, felt wrote:
> I have a strong preference for keeping the code clean; we don't have any users
> except for a few canary users, and since we're going to start deleting data
more
> than 24h old it won't even matter anyway.
> 
> On 2013/08/02 19:57:09, mvrable wrote:
> > On 2013/08/02 19:23:04, Matt Perry wrote:
> > > Should this remain as 7? (If you add another one later, it'll be 7 and you
> > might
> > > misinterpret old data)
> > 
> > I was originally going to do that, but Adrienne pointed out that this enum
was
> > only very recently added and the risk of confusion at this point is low. 
This
> > keeps the code cleaner for now; we'll definitely not renumber once M30
> branches.
> > 
> > (I think the only people who will notice this change are us and people
running
> > canaries that also have the activity log enabled via a flag so the risk is
> low. 
> > There's not yet a user interface where you could see the bad data, and even
if
> > it persists in the database it should at worst cause some erroneous data to
be
> > displayed but be non-fatal.)
> > 
> > I could switch back if you'd prefer.
>

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

https://codereview.chromium.org/21653002/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:158: // arg_url;
arbitrarily take the first one.
On 2013/08/02 19:57:09, mvrable wrote:
> On 2013/08/02 19:23:04, Matt Perry wrote:
> > What's the difference between arg_url and the id_list from args?
> 
> arg_url is a separate database column where we can store a URL (but only one);
> if we encounter a URL in the API argument list we'll extract it into arg_url
so
> we can more easily search through the database.
> 
> A couple of tabs APIs can include a list of tabs (id_list); when we translate
> those to URLs we can't fit all of them into arg_url so the full list of URLs
can
> only fit in the args list.

Might be worth briefly mentioning that in the comment.

[mvrable, 2013-08-02 20:54:48]

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

https://codereview.chromium.org/21653002/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:158: // arg_url;
arbitrarily take the first one.
On 2013/08/02 20:26:05, Matt Perry wrote:
> On 2013/08/02 19:57:09, mvrable wrote:
> > On 2013/08/02 19:23:04, Matt Perry wrote:
> > > What's the difference between arg_url and the id_list from args?
> > 
> > arg_url is a separate database column where we can store a URL (but only
one);
> > if we encounter a URL in the API argument list we'll extract it into arg_url
> so
> > we can more easily search through the database.
> > 
> > A couple of tabs APIs can include a list of tabs (id_list); when we
translate
> > those to URLs we can't fit all of them into arg_url so the full list of URLs
> can
> > only fit in the args list.
> 
> Might be worth briefly mentioning that in the comment.

I reworked the comments to try to make this clearer.

[commit-bot: I haz the power, 2013-08-02 20:56:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvrable@chromium.org/21653002/39001

[commit-bot: I haz the power, 2013-08-02 21:02:01]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-08-02 21:55:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvrable@chromium.org/21653002/39001

[commit-bot: I haz the power, 2013-08-03 01:59:43]

Change committed as 215452