Extension activity log database refactoring (step 3)

Extension activity log database refactoring (step 3)

This is a continuation of work in
https://codereview.chromium.org/18660004/ and
https://codereview.chromium.org/19234003/.

Flatten the Extension::Action hierarchy, getting rid of all subclasses
and keeping just the top class.  The new Action class has a number of
data fields (some optional), so any of the old extension actions can be
encoded into it.  It maps in a straightforward fashion onto the current
database schema.

Change all the old sites that used specific Action subclasses to use the
new version, and fix up unit tests.

The extension activity log private API will be updated soon (by felt) to
more closely match the new Action class.  There is some basic
compatibility in place so that the end-to-end tests still pass, but the
private API unit tests have been disabled pending the rewrite.

BUG=255730
TBR=jhawkins@chromium.org

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

[mvrable, 2013-07-17 22:17:46]

This is the next step in the activity database refactoring work.  Since the
previous patch hasn't been landed yet this one will show up as the combination
of both while reviewing.

If you have a chance you can review now, or if you'd rather you can wait until
the previous patch is in.

[mvrable, 2013-07-18 22:45:31]

Now that the previous patch has been committed I have rebased this to make it
easier to review.

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

A few questions about design choices, other than that it's looking good.

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

https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_actions.h:57: void
set_api_name(const std::string api_name) { api_name_ = api_name; }
Why are these all setters instead of constructors? Is this to handle optional
fields?

One benefit of putting them in a constructor is that you could check whether the
correct set of non-optional fields have been specified for the given action
type. (And then DCHECK if it's a mismatch.)

https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_database.cc (right):

https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_database.cc:160: "ORDER BY time
DESC",
good call :)

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

https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log.h:73: // does not which to
construct the Action object itself, however those
*does not wish

https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log.h:75: void
LogAction(scoped_refptr<Action> action);
I'm not entirely sure how I feel about forcing the callers to create Action
objects. After mulling over it, I think it's OK, but I'm wondering if there are
cases where the caller won't know the right ActionType to assign to it?

Are there any changes that are dependent on this refactoring, or can you make a
separate bug & TODO for it?

https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log.h:78: // ownership transfer:
either make it const or a scoped_ptr.
I'm confused, maybe there is a word missing in this comment?

[mvrable, 2013-07-23 18:16:56]

Thanks for the good questions.  I'm open to talking more about and changing the
design if needed--I'd like to get this right since it will be important going
forward.

Also, I am hoping for a straightforward translation from the Action class to the
new private extension API when that is reworked, so if there are changes to the
Action class needed for that we should think about them too.

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

https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_actions.h:57: void
set_api_name(const std::string api_name) { api_name_ = api_name; }
On 2013/07/23 17:07:17, felt wrote:
> Why are these all setters instead of constructors? Is this to handle optional
> fields?
> 
> One benefit of putting them in a constructor is that you could check whether
the
> correct set of non-optional fields have been specified for the given action
> type. (And then DCHECK if it's a mismatch.)

Mostly to handle optional fields and to avoid having constructors with very long
argument lists.

I was thinking of using the Action class for objects loaded by a policy from the
database; because data might have been stripped out while saving (for space or
privacy reasons), we need to leave many fields as optional.  That limits the
amount of checking we can do, unless I change the design.

(Looking back, api_name is something that probably could be set in the
constructor, though--would that be a good change to make?)

https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_database.cc (right):

https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_database.cc:160: "ORDER BY time
DESC",
On 2013/07/23 17:07:17, felt wrote:
> good call :)

To be fair, this wasn't possible before since the records were spread across
three independent tables.

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

https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log.h:73: // does not which to
construct the Action object itself, however those
On 2013/07/23 17:07:17, felt wrote:
> *does not wish

Done.

https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log.h:75: void
LogAction(scoped_refptr<Action> action);
On 2013/07/23 17:07:17, felt wrote:
> I'm not entirely sure how I feel about forcing the callers to create Action
> objects. After mulling over it, I think it's OK, but I'm wondering if there
are
> cases where the caller won't know the right ActionType to assign to it?

I haven't noticed cases where the caller wouldn't know which ActionType to use,
though it's possible I haven't come across them yet.

> Are there any changes that are dependent on this refactoring, or can you make
a
> separate bug & TODO for it?

This is mostly independent, though passing Action objects directly makes it
easier to change exactly what is logged, or even add new types of items that are
logged, without having to change the intermediate function calls.

There's one change for incognito handling where this might be relevant.  When do
we want to strip out the URLs for incognito tabs?  Do we want to leave the URL
accessible purely in memory (for example, in the activity log developer console
that is watching raw events before they are sent to the policy)?  An option is
to log the raw URL in the Action object, but add a flag to mark it as an
incognito URL.  When the policy processes the Action, it would scrub the URL at
that point.  For this change, passing along an Action object is simpler than
adding an extra boolean parameter or two at each level to track incognito state.
 I am still undecided whether this is how we should handle incognito.

https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log.h:78: // ownership transfer:
either make it const or a scoped_ptr.
On 2013/07/23 17:07:17, felt wrote:
> I'm confused, maybe there is a word missing in this comment?

Reworded.  I understand the code a bit better since writing this comment, so I
added more details.

[felt, 2013-07-23 19:06:32]

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

https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_actions.h:57: void
set_api_name(const std::string api_name) { api_name_ = api_name; }
api_name is sometimes null but usually not. Maybe add it to the constructor just
out of convenience?

You have a good point about loading from the database, so this does make sense.

On 2013/07/23 18:16:56, mvrable wrote:
> On 2013/07/23 17:07:17, felt wrote:
> > Why are these all setters instead of constructors? Is this to handle
optional
> > fields?
> > 
> > One benefit of putting them in a constructor is that you could check whether
> the
> > correct set of non-optional fields have been specified for the given action
> > type. (And then DCHECK if it's a mismatch.)
> 
> Mostly to handle optional fields and to avoid having constructors with very
long
> argument lists.
> 
> I was thinking of using the Action class for objects loaded by a policy from
the
> database; because data might have been stripped out while saving (for space or
> privacy reasons), we need to leave many fields as optional.  That limits the
> amount of checking we can do, unless I change the design.
> 
> (Looking back, api_name is something that probably could be set in the
> constructor, though--would that be a good change to make?)

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

https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log.h:75: void
LogAction(scoped_refptr<Action> action);
On 2013/07/23 18:16:56, mvrable wrote:
> On 2013/07/23 17:07:17, felt wrote:
> > I'm not entirely sure how I feel about forcing the callers to create Action
> > objects. After mulling over it, I think it's OK, but I'm wondering if there
> are
> > cases where the caller won't know the right ActionType to assign to it?
> 
> I haven't noticed cases where the caller wouldn't know which ActionType to
use,
> though it's possible I haven't come across them yet.

Well, I'm worried about a few things, another one being URLs. Is the caller
going to go through the args lists, looking to see if there are URLs, and then
separate out the URL from the args list? It seems like that processing should
happen here in a single place, rather than scattered throughout the code. Maybe
we could offer convenience methods?

> 
> > Are there any changes that are dependent on this refactoring, or can you
make
> a
> > separate bug & TODO for it?
> 
> This is mostly independent, though passing Action objects directly makes it
> easier to change exactly what is logged, or even add new types of items that
are
> logged, without having to change the intermediate function calls.
> 
> There's one change for incognito handling where this might be relevant.  When
do
> we want to strip out the URLs for incognito tabs?  Do we want to leave the URL
> accessible purely in memory (for example, in the activity log developer
console
> that is watching raw events before they are sent to the policy)?  An option is
> to log the raw URL in the Action object, but add a flag to mark it as an
> incognito URL.  When the policy processes the Action, it would scrub the URL
at
> that point.  For this change, passing along an Action object is simpler than
> adding an extra boolean parameter or two at each level to track incognito
state.
>  I am still undecided whether this is how we should handle incognito.

[mvrable, 2013-07-23 19:17:54]

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

https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_actions.h:57: void
set_api_name(const std::string api_name) { api_name_ = api_name; }
On 2013/07/23 19:06:33, felt wrote:
> api_name is sometimes null but usually not. Maybe add it to the constructor
just
> out of convenience?

I'll add the API name to the constructor--patch will be coming up after I've had
a chance to make the changes everywhere.

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

https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log.h:75: void
LogAction(scoped_refptr<Action> action);
On 2013/07/23 19:06:33, felt wrote:
> Well, I'm worried about a few things, another one being URLs. Is the caller
> going to go through the args lists, looking to see if there are URLs, and then
> separate out the URL from the args list? It seems like that processing should
> happen here in a single place, rather than scattered throughout the code.
Maybe
> we could offer convenience methods?

The current plan is to keep URL scanning through the argument list in the
ActivityLog class (where tab ID lookups can happen on the UI thread)--but this
can take an Action object as input and modify the object to extract the URLs. 
If there are other common operations that are needed we can create convenience
methods.

[felt, 2013-07-23 19:22:33]

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

https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log.h:75: void
LogAction(scoped_refptr<Action> action);
On 2013/07/23 19:17:55, mvrable wrote:
> On 2013/07/23 19:06:33, felt wrote:
> > Well, I'm worried about a few things, another one being URLs. Is the caller
> > going to go through the args lists, looking to see if there are URLs, and
then
> > separate out the URL from the args list? It seems like that processing
should
> > happen here in a single place, rather than scattered throughout the code.
> Maybe
> > we could offer convenience methods?
> 
> The current plan is to keep URL scanning through the argument list in the
> ActivityLog class (where tab ID lookups can happen on the UI thread)--but this
> can take an Action object as input and modify the object to extract the URLs. 
> If there are other common operations that are needed we can create convenience
> methods.

Ah, OK. I am happy with this plan, where the object will be modified by the
ActivityLog class.

https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log.h:75: void
LogAction(scoped_refptr<Action> action);
> There's one change for incognito handling where this might be relevant.  When
do
> we want to strip out the URLs for incognito tabs?  Do we want to leave the URL
> accessible purely in memory (for example, in the activity log developer
console
> that is watching raw events before they are sent to the policy)?  An option is
> to log the raw URL in the Action object, but add a flag to mark it as an
> incognito URL.  When the policy processes the Action, it would scrub the URL
at
> that point.  For this change, passing along an Action object is simpler than
> adding an extra boolean parameter or two at each level to track incognito
state.
>  I am still undecided whether this is how we should handle incognito.

We want to send along incognito URLs to the API and database if & only if we're
in testing mode. So they should be written to disk if we are in testing mode.
(Does that answer your question?)

[mvrable, 2013-07-23 21:54:03]

The patch is updated so the api_name parameter is set in the constructor (every
previous call site was setting it anyway).

If you have concerns with the design still I'm open to more changes.

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

https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log.h:75: void
LogAction(scoped_refptr<Action> action);
On 2013/07/23 19:22:33, felt wrote:
> We want to send along incognito URLs to the API and database if & only if
we're
> in testing mode. So they should be written to disk if we are in testing mode.
> (Does that answer your question?)

I think this answers my question.  I think we don't want to bother checking
command-line flags at the point where the data is collected.  So, we'll want to
record the full URL and also a flag that indicates whether it is incognito (the
change I was proposing).

The policy can then discard incognito URLs if appropriate--the log-everything
policy will keep the URLs, and the non-testing policy will blank them out. 
Incognito scrubbing should be in a helper method, so it can also be applied to
the data forwarded directly to the API.  Or perhaps the ActivityLog can do the
scrubbing for both, depending on command-line flags.

[felt, 2013-07-23 21:57:07]

On 2013/07/23 21:54:03, mvrable wrote:
> The patch is updated so the api_name parameter is set in the constructor
(every
> previous call site was setting it anyway).
> 
> If you have concerns with the design still I'm open to more changes.
> 
>
https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
> File chrome/browser/extensions/activity_log/activity_log.h (right):
> 
>
https://codereview.chromium.org/19690003/diff/3001/chrome/browser/extensions/...
> chrome/browser/extensions/activity_log/activity_log.h:75: void
> LogAction(scoped_refptr<Action> action);
> On 2013/07/23 19:22:33, felt wrote:
> > We want to send along incognito URLs to the API and database if & only if
> we're
> > in testing mode. So they should be written to disk if we are in testing
mode.
> > (Does that answer your question?)
> 
> I think this answers my question.  I think we don't want to bother checking
> command-line flags at the point where the data is collected.  So, we'll want
to
> record the full URL and also a flag that indicates whether it is incognito
(the
> change I was proposing).
> 
> The policy can then discard incognito URLs if appropriate--the log-everything
> policy will keep the URLs, and the non-testing policy will blank them out. 
> Incognito scrubbing should be in a helper method, so it can also be applied to
> the data forwarded directly to the API.  Or perhaps the ActivityLog can do the
> scrubbing for both, depending on command-line flags.

lgtm

[mvrable, 2013-07-24 04:17:49]

Matt,

This is the next of the activity log refactoring patches--this uses a single
Action type instead of each of the subclasses from before.  I'm particularly
interested in feedback on the API design for the Action class, but comments you
have on any part of the patch are welcome.

I'm still seeing a few test failures on the trybots so I'll be working on
tracking down and fixing those, but I think this patch should otherwise be ready
for you to review.

--Michael Vrable

[Matt Perry, 2013-07-24 21:06:28]

lgtm

https://codereview.chromium.org/19690003/diff/20001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_actions.cc (right):

https://codereview.chromium.org/19690003/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_actions.cc:36: std::string
GetURLForTabId(const int tab_id, Profile* profile) {
nit: const is unnecessary for a POD type

https://codereview.chromium.org/19690003/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_actions.cc:36: std::string
GetURLForTabId(const int tab_id, Profile* profile) {
nit: Url, not URL

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

https://codereview.chromium.org/19690003/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_actions.h:53: // logged, and
determines which other fields are expected to filled in.
to be* filled in

[mvrable, 2013-07-25 00:08:42]

https://codereview.chromium.org/19690003/diff/20001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_actions.cc (right):

https://codereview.chromium.org/19690003/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_actions.cc:36: std::string
GetURLForTabId(const int tab_id, Profile* profile) {
On 2013/07/24 21:06:29, Matt Perry wrote:
> nit: const is unnecessary for a POD type

Done.

https://codereview.chromium.org/19690003/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_actions.cc:36: std::string
GetURLForTabId(const int tab_id, Profile* profile) {
On 2013/07/24 21:06:29, Matt Perry wrote:
> nit: Url, not URL

Done.

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

https://codereview.chromium.org/19690003/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_actions.h:53: // logged, and
determines which other fields are expected to filled in.
On 2013/07/24 21:06:29, Matt Perry wrote:
> to be* filled in

Done.

[commit-bot: I haz the power, 2013-07-26 00:52:07]

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

[mvrable, 2013-07-26 04:04:50]

James,

I just noticed that there is one file in this patch that needs you or another
reviewer to approve:
chrome/browser/renderer_host/chrome_render_message_filter.cc

The only change is to remove two #include lines for header files that are
deleted in the rest of this patch.  Would you be able to give a quick LGTM on
this, if it looks reasonable?  The other files have already been reviewed.

--Michael Vrable

[commit-bot: I haz the power, 2013-07-26 06:19:09]

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

[mvrable, 2013-07-26 16:57:06]

On 2013/07/26 04:04:50, mvrable wrote:
> James,
> 
> I just noticed that there is one file in this patch that needs you or another
> reviewer to approve:
> chrome/browser/renderer_host/chrome_render_message_filter.cc
> 
> The only change is to remove two #include lines for header files that are
> deleted in the rest of this patch.  Would you be able to give a quick LGTM on
> this, if it looks reasonable?  The other files have already been reviewed.
> 
> --Michael Vrable

James: The change to
chrome/browser/renderer_host/chrome_render_message_filter.cc is quite trivial,
so I'm going to mark this as TBR by you (felt@ gave me instructions on how to do
so).  I'll wait a few hours before sending to the commit queue in case you have
comments you'd like to make before then.

--Michael Vrable

[James Hawkins, 2013-07-26 16:59:42]

lgtm

[commit-bot: I haz the power, 2013-07-26 17:06:38]

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

[commit-bot: I haz the power, 2013-07-26 22:53:40]

Change committed as 213995