Extension activity log database refactoring (step 2)

Extension activity log database refactoring (step 2)

This is a continuation of work started in
https://codereview.chromium.org/18660004/ (committed as r211234).

Start work on merging all the activity types into a single database
table.  This patch creates just a single table and has each of the
Action types record to that table, however the class hierarchy of
multiple action types is still in place.  Eventually there will be just
a single Action type, but the merging of that code will happen in a
following patch.

As a stopgap, until the merger is done, there is a WatchdogAction
subclass which is used to represent records read back from the database.

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

[mvrable, 2013-07-15 21:51:51]

This is an early version of the next activity database refactoring patch.  Could
you take a look?

I've tried to switch to the new database schema but keep the old Action class
hierarchy in place (as a way of splitting a bit change into more manageable
pieces).  The schema might get tweaked a tiny bit more once I've gotten rid of
more of the legacy code.

The next step after this will be to collapse the Action hierarchy, which will
require changes to the Javascript API (this patch doesn't need that).  If you'd
like to sync to talk about that (and whether any changes to this patch would
make the API simpler), we can do that.

[felt, 2013-07-16 06:44:03]

Hi Michael, looking good, thanks for breaking this up into multiple CLs.

I take it that the next CL will be the one that adds more properties to the
Action class, and once that's done, then I can update the API?

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

https://codereview.chromium.org/19234003/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/activity_actions.cc:53: bool
WatchdogAction::Record(sql::Connection* db) {
Is Record used anywhere? could you just delete it from the class?

https://codereview.chromium.org/19234003/diff/1/chrome/browser/extensions/act...
File chrome/browser/extensions/activity_log/activity_actions.h (right):

https://codereview.chromium.org/19234003/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/activity_actions.h:73: class
WatchdogAction : public Action {
Is this named WatchdogAction specifically b/c it still exists for the sake of
the chrome.activityLogPrivate API?

Also: can you mark this comment (or some subset of the comment) a TODO(felt) or
TODO(mvrable)?

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

https://codereview.chromium.org/19234003/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/activity_database_unittest.cc:191:
//ASSERT_EQ(DomActionType::MODIFIED, statement.ColumnInt(2));
miss a line?

https://codereview.chromium.org/19234003/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/activity_database_unittest.cc:193:
switches::kEnableExtensionActivityLogTesting))
If you want to test both, we should have a separate test that forces the right
case, since I believe the switch will always be in place?

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

https://codereview.chromium.org/19234003/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:56: //
TODO(dbabic) This would be a fine error handler for all sql-based policies,
can you delete this todo? At this point it seems to have lost the method it was
attached to (deleted?)

https://codereview.chromium.org/19234003/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:135: if (args) {
nit: don't need { }

[mvrable, 2013-07-16 18:12:35]

Thanks for the feedback.

Here's an updated version after going through your comments.  I also made a few
fixes to deal with some trybot failures, and and combined the two API call types
(with and without a URL) into one and made a couple of cosmetic changes to the
list of action types.

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

https://codereview.chromium.org/19234003/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/activity_actions.cc:53: bool
WatchdogAction::Record(sql::Connection* db) {
On 2013/07/16 06:44:03, felt wrote:
> Is Record used anywhere? could you just delete it from the class?

It needs to be defined since the function is pure virtual in the parent class.

https://codereview.chromium.org/19234003/diff/1/chrome/browser/extensions/act...
File chrome/browser/extensions/activity_log/activity_actions.h (right):

https://codereview.chromium.org/19234003/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/activity_actions.h:73: class
WatchdogAction : public Action {
On 2013/07/16 06:44:03, felt wrote:
> Is this named WatchdogAction specifically b/c it still exists for the sake of
> the chrome.activityLogPrivate API?

Not any particularly good reason, I just needed another name.  If you have a
better suggestion I can use it (though it probably doesn't matter much since the
class is temporary, unless we keep this name instead of Action after the merge).

> Also: can you mark this comment (or some subset of the comment) a TODO(felt)
or
> TODO(mvrable)?

Marked as TODO(mvrable).

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

https://codereview.chromium.org/19234003/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/activity_database_unittest.cc:191:
//ASSERT_EQ(DomActionType::MODIFIED, statement.ColumnInt(2));
On 2013/07/16 06:44:03, felt wrote:
> miss a line?

Fixed.  verb wasn't getting saved/checked before, but now it is.

https://codereview.chromium.org/19234003/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/activity_database_unittest.cc:193:
switches::kEnableExtensionActivityLogTesting))
On 2013/07/16 06:44:03, felt wrote:
> If you want to test both, we should have a separate test that forces the right
> case, since I believe the switch will always be in place?

Is there an easy way to remove a command-line flag?

I kept this since it was in the old tests, but the tests will need a lot of
reworking over the next few refactoring patches so perhaps not worth spending
time on in this patch.

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

https://codereview.chromium.org/19234003/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:56: //
TODO(dbabic) This would be a fine error handler for all sql-based policies,
On 2013/07/16 06:44:03, felt wrote:
> can you delete this todo? At this point it seems to have lost the method it
was
> attached to (deleted?)

Done.

https://codereview.chromium.org/19234003/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:135: if (args) {
On 2013/07/16 06:44:03, felt wrote:
> nit: don't need { }

Done.

[felt, 2013-07-16 18:30:16]

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

https://codereview.chromium.org/19234003/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/activity_actions.cc:53: bool
WatchdogAction::Record(sql::Connection* db) {
On 2013/07/16 18:12:35, mvrable wrote:
> On 2013/07/16 06:44:03, felt wrote:
> > Is Record used anywhere? could you just delete it from the class?
> 
> It needs to be defined since the function is pure virtual in the parent class.

I see -- and you can't remove it from the parent class because the other action
subclasses are still using it, right? Although that will change in the next
patch? (Trying to sort out what goes where.)

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

https://codereview.chromium.org/19234003/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/activity_database_unittest.cc:193:
switches::kEnableExtensionActivityLogTesting))
On 2013/07/16 18:12:35, mvrable wrote:
> On 2013/07/16 06:44:03, felt wrote:
> > If you want to test both, we should have a separate test that forces the
right
> > case, since I believe the switch will always be in place?
> 
> Is there an easy way to remove a command-line flag?
> 
> I kept this since it was in the old tests, but the tests will need a lot of
> reworking over the next few refactoring patches so perhaps not worth spending
> time on in this patch.

I must have missed this in a previous review. No, there's no way to remove the
flag. What we ought to be doing is switching policies here, I think. Can you add
a TODO and a link to crbug.com/260784 ?

[mvrable, 2013-07-16 18:38:39]

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

https://codereview.chromium.org/19234003/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/activity_actions.cc:53: bool
WatchdogAction::Record(sql::Connection* db) {
On 2013/07/16 18:30:17, felt wrote:
> On 2013/07/16 18:12:35, mvrable wrote:
> > On 2013/07/16 06:44:03, felt wrote:
> > > Is Record used anywhere? could you just delete it from the class?
> > 
> > It needs to be defined since the function is pure virtual in the parent
class.
> 
> I see -- and you can't remove it from the parent class because the other
action
> subclasses are still using it, right? Although that will change in the next
> patch? (Trying to sort out what goes where.)

Right.

(Or possibly changed in the patch after that, depending on how work is divided
between the patches).

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

https://codereview.chromium.org/19234003/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/activity_database_unittest.cc:193:
switches::kEnableExtensionActivityLogTesting))
On 2013/07/16 18:30:17, felt wrote:
> On 2013/07/16 18:12:35, mvrable wrote:
> > On 2013/07/16 06:44:03, felt wrote:
> > > If you want to test both, we should have a separate test that forces the
> right
> > > case, since I believe the switch will always be in place?
> > 
> > Is there an easy way to remove a command-line flag?
> > 
> > I kept this since it was in the old tests, but the tests will need a lot of
> > reworking over the next few refactoring patches so perhaps not worth
spending
> > time on in this patch.
> 
> I must have missed this in a previous review. No, there's no way to remove the
> flag. What we ought to be doing is switching policies here, I think. Can you
add
> a TODO and a link to crbug.com/260784 ?

Done.

[felt, 2013-07-17 01:46:49]

I'm pretty happy with this, please feel free to send to matt when you're ready.

[mvrable, 2013-07-17 03:42:46]

Matt,

This is the next patch in the extension activity log refactoring work--could you
take a look.  This one establishes the new database table (a single table which
holds all the activity log records), defined in one of the policies, and updates
all the action types to write to this table.  I've also created some initial
code for reading from the new table.

There are still at least a couple more patches in this patch series, but this
was another piece which could be done in isolation to make the work easier to
review.

I'm trying to keep the unit tests all passing throughout the patch series.  It's
possible there may be some minor updates if the trybots uncover any additional
test failures.

--Michael Vrable

[Matt Perry, 2013-07-17 22:13:19]

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

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_database.cc:191: if
(raw_value.release()->GetAsDictionary(&dict)) {
If raw_value is a List, how can you get it as a Dict? Also, if this fails,
raw_value is leaked.

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/api_actions.h (right):

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.h:37: const base::ListValue&
args_list,  // same as above, as a list
why have this redundant data? APIAction should just store one or the other.

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/dom_actions.cc (right):

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/dom_actions.cc:87: database_url = url_;
remove this line

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right):

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:52:
"LONGVARCHAR", "LONGVARCHAR", "LONGVARCHAR"
The database structure is spread across two files. ActivityDatabase needs to
know this structure in order to do both InitializeTable and GetActions. Seems
like that logic should move to this file.

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:78: return
ActivityDatabase::InitializeTable(db,
OnDatabaseInit is called from ActivityDatabase. It's odd that ActivityDatabase
calls its delegate, which calls back to ActivityDatabase to initialize the
table. Shouldn't InitializeTable be in this file?

[mvrable, 2013-07-17 22:56:35]

Thanks for taking a look.  I've made a couple of small fixes based on your
comments.  Some of your other concerns will I think be addressed in patches to
follow; if you want a preview https://codereview.chromium.org/19690003/ contains
the next set of changes.

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

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_database.cc:191: if
(raw_value.release()->GetAsDictionary(&dict)) {
On 2013/07/17 22:13:19, Matt Perry wrote:
> If raw_value is a List, how can you get it as a Dict? Also, if this fails,
> raw_value is leaked.

This was a typo; it should have been IsType(Value::TYPE_DICTIONARY).  I had
actually caught and fixed that in other changes in the next patch in this
series, but I've now fixed it here too.  The conversion shouldn't fail (now that
the type is fixed) so there shouldn't be a memory leak.

I wasn't able to come up with a version that I was entirely happy with.  A
version I had settled on in the next patch in the series (against a slightly
different API) looks like:
  if (parsed_value && parsed_value->IsType(Value::TYPE_DICTIONARY)) {
    DictionaryValue* converted = NULL;
    CHECK(parsed_value.release()->GetAsDictionary(&converted));
    action->set_other(make_scoped_ptr(converted));
  }

Thoughts on what you'd prefer, or other suggestions for improvements?

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/api_actions.h (right):

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.h:37: const base::ListValue&
args_list,  // same as above, as a list
On 2013/07/17 22:13:19, Matt Perry wrote:
> why have this redundant data? APIAction should just store one or the other.

We're moving towards just using the ListValue, but keeping both around made some
of the transition easier.  In the patch after this one, the APIAction class is
gone entirely and so this is cleaned up.

If you think it's worth spending the effort I could try to get rid of args in
this patch.

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/dom_actions.cc (right):

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/dom_actions.cc:87: database_url = url_;
On 2013/07/17 22:13:19, Matt Perry wrote:
> remove this line

Done.

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right):

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:52:
"LONGVARCHAR", "LONGVARCHAR", "LONGVARCHAR"
On 2013/07/17 22:13:19, Matt Perry wrote:
> The database structure is spread across two files. ActivityDatabase needs to
> know this structure in order to do both InitializeTable and GetActions. Seems
> like that logic should move to this file.

Yes, that will happen; the plan is that any code that depends on the database
schema will be in a policy class.  I was trying to get the Action class and
subclasses cleaned up before relocating the code.

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:78: return
ActivityDatabase::InitializeTable(db,
On 2013/07/17 22:13:19, Matt Perry wrote:
> OnDatabaseInit is called from ActivityDatabase. It's odd that ActivityDatabase
> calls its delegate, which calls back to ActivityDatabase to initialize the
> table. Shouldn't InitializeTable be in this file?

I was thinking that IntializeTable was code which could potentially be shared
between multiple policies (each using a different database schema). 
ActivityDatabase seemed one possible place to put it but it could be moved
somewhere else such as ActivityLogPolicy.

[Matt Perry, 2013-07-17 23:10:27]

lgtm

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

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_database.cc:191: if
(raw_value.release()->GetAsDictionary(&dict)) {
On 2013/07/17 22:56:35, mvrable wrote:
> On 2013/07/17 22:13:19, Matt Perry wrote:
> > If raw_value is a List, how can you get it as a Dict? Also, if this fails,
> > raw_value is leaked.
> 
> This was a typo; it should have been IsType(Value::TYPE_DICTIONARY).  I had
> actually caught and fixed that in other changes in the next patch in this
> series, but I've now fixed it here too.  The conversion shouldn't fail (now
that
> the type is fixed) so there shouldn't be a memory leak.
> 
> I wasn't able to come up with a version that I was entirely happy with.  A
> version I had settled on in the next patch in the series (against a slightly
> different API) looks like:
>   if (parsed_value && parsed_value->IsType(Value::TYPE_DICTIONARY)) {
>     DictionaryValue* converted = NULL;
>     CHECK(parsed_value.release()->GetAsDictionary(&converted));
>     action->set_other(make_scoped_ptr(converted));
>   }
> 
> Thoughts on what you'd prefer, or other suggestions for improvements?

GetAsDictionary is redundant with checking the type. Do either:
if (raw_value && raw_value->IsType(DICT)) {
  other.reset(static_cast<DictionaryValue*>(raw_value.release()));
}
or
DictionaryValue* dict;
if (raw_value && raw_value->GetAsDictionary(&dict)) {
  other.reset(dict);
  raw_value.release();  // ownership transferred to other
}

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/api_actions.h (right):

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.h:37: const base::ListValue&
args_list,  // same as above, as a list
On 2013/07/17 22:56:35, mvrable wrote:
> On 2013/07/17 22:13:19, Matt Perry wrote:
> > why have this redundant data? APIAction should just store one or the other.
> 
> We're moving towards just using the ListValue, but keeping both around made
some
> of the transition easier.  In the patch after this one, the APIAction class is
> gone entirely and so this is cleaned up.
> 
> If you think it's worth spending the effort I could try to get rid of args in
> this patch.

No that's fine, good to hear you're planning on fixing it.

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right):

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:78: return
ActivityDatabase::InitializeTable(db,
On 2013/07/17 22:56:35, mvrable wrote:
> On 2013/07/17 22:13:19, Matt Perry wrote:
> > OnDatabaseInit is called from ActivityDatabase. It's odd that
ActivityDatabase
> > calls its delegate, which calls back to ActivityDatabase to initialize the
> > table. Shouldn't InitializeTable be in this file?
> 
> I was thinking that IntializeTable was code which could potentially be shared
> between multiple policies (each using a different database schema). 
> ActivityDatabase seemed one possible place to put it but it could be moved
> somewhere else such as ActivityLogPolicy.

I see. Leave it for now, and we'll see how it looks when this is all done.

[mvrable, 2013-07-18 16:39:01]

I rebased to fix some merge conflicts and made a few other minor fixes.

Adrienne, do you have any additional comments on this or does it look good to
submit?

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

https://codereview.chromium.org/19234003/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_database.cc:191: if
(raw_value.release()->GetAsDictionary(&dict)) {
On 2013/07/17 23:10:27, Matt Perry wrote:
> On 2013/07/17 22:56:35, mvrable wrote:
> > On 2013/07/17 22:13:19, Matt Perry wrote:
> > > If raw_value is a List, how can you get it as a Dict? Also, if this fails,
> > > raw_value is leaked.
> > 
> > This was a typo; it should have been IsType(Value::TYPE_DICTIONARY).  I had
> > actually caught and fixed that in other changes in the next patch in this
> > series, but I've now fixed it here too.  The conversion shouldn't fail (now
> that
> > the type is fixed) so there shouldn't be a memory leak.
> > 
> > I wasn't able to come up with a version that I was entirely happy with.  A
> > version I had settled on in the next patch in the series (against a slightly
> > different API) looks like:
> >   if (parsed_value && parsed_value->IsType(Value::TYPE_DICTIONARY)) {
> >     DictionaryValue* converted = NULL;
> >     CHECK(parsed_value.release()->GetAsDictionary(&converted));
> >     action->set_other(make_scoped_ptr(converted));
> >   }
> > 
> > Thoughts on what you'd prefer, or other suggestions for improvements?
> 
> GetAsDictionary is redundant with checking the type. Do either:
> if (raw_value && raw_value->IsType(DICT)) {
>   other.reset(static_cast<DictionaryValue*>(raw_value.release()));
> }
> or
> DictionaryValue* dict;
> if (raw_value && raw_value->GetAsDictionary(&dict)) {
>   other.reset(dict);
>   raw_value.release();  // ownership transferred to other
> }

Done (changed to use static_cast).

[felt, 2013-07-18 17:09:59]

lgtm assuming you fix the comments below. Sorry about the delay in my review, my
hotel did not have wifi as promised yesterday. >(

Also, I'm assuming you've tested this code actually in the browser in addition
to the unit/browser tests? Both in fresh installs, and in versions of the
browser that have the old databases already in existence (so testing the
deletion code)? (I'm sorry to ask, but I've assumed incorrectly when doing
reviews before so I'm now always double checking.)

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

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_actions.cc:96: result +=
base::StringPrintf("type%d", static_cast<int>(action_type_));
Should this be NOTREACHED()?

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

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_actions.h:25: // the database,
so keep them stable.
Can you make this explicit: "Append only."

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_database.h (right):

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_database.h:143: void
LogInitFailure();
Do you need to rebase again? (I thought this was added/moved in 18360006?)

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_database_unittest.cc
(right):

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_database_unittest.cc:238:
//ASSERT_EQ(1, statement.ColumnInt(4));
I think you missed this commented out line

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log_browsertest.cc (right):

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log_browsertest.cc:51:
"PAGE_URL=http://www.google.com.bo:%d/test.html
OTHER={\"dom_verb\":3,\"extra\":\"(prerender)\",\"page_title\":\"www.google.com.bo:%d/test.html\"}",
line wrap

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log_unittest.cc (right):

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log_unittest.cc:113:
"ARGS=[\"hello\",\"world\"] OTHER={}";
is the spacing like that on purpose?

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

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:105: std::string
Serialize(const base::Value& value) {
I notice there is also a new Serialize in activity_actions.cc. Is there a reason
for two separate ones? Also, the other definition has a null check that seems to
be missing in this one.

[mvrable, 2013-07-18 18:20:34]

Thanks for the additional comments.  I think I've fixed them all now.

I had done some testing with the browser earlier on, but hadn't checked
recently.  I checked again (both the upgrade case and the new database case) and
database handling seems correct still.

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

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_actions.cc:96: result +=
base::StringPrintf("type%d", static_cast<int>(action_type_));
On 2013/07/18 17:09:59, felt wrote:
> Should this be NOTREACHED()?

We might have an action of an unknown type loaded from the database and don't
want an assertion failure when attempting to print it out.

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

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_actions.h:25: // the database,
so keep them stable.
On 2013/07/18 17:09:59, felt wrote:
> Can you make this explicit: "Append only."

Done.

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_database.h (right):

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_database.h:143: void
LogInitFailure();
On 2013/07/18 17:09:59, felt wrote:
> Do you need to rebase again? (I thought this was added/moved in 18360006?)

I had relocated it myself a while back but to a slightly different location than
you.  Updated now to reduce the code delta (only fix a typo now).

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_database_unittest.cc
(right):

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_database_unittest.cc:238:
//ASSERT_EQ(1, statement.ColumnInt(4));
On 2013/07/18 17:09:59, felt wrote:
> I think you missed this commented out line

Done.

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log_browsertest.cc (right):

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log_browsertest.cc:51:
"PAGE_URL=http://www.google.com.bo:%d/test.html
OTHER={\"dom_verb\":3,\"extra\":\"(prerender)\",\"page_title\":\"www.google.com.bo:%d/test.html\"}",
On 2013/07/18 17:09:59, felt wrote:
> line wrap

Done.

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log_unittest.cc (right):

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log_unittest.cc:113:
"ARGS=[\"hello\",\"world\"] OTHER={}";
On 2013/07/18 17:09:59, felt wrote:
> is the spacing like that on purpose?

I'm not sure how it ended up like that, but fixed now.

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

https://codereview.chromium.org/19234003/diff/65002/chrome/browser/extensions...
chrome/browser/extensions/activity_log/api_actions.cc:105: std::string
Serialize(const base::Value& value) {
On 2013/07/18 17:09:59, felt wrote:
> I notice there is also a new Serialize in activity_actions.cc. Is there a
reason
> for two separate ones? Also, the other definition has a null check that seems
to
> be missing in this one.

I wanted the function in activity_actions.cc to be local to the file.  This one
is temporary and is going away in the next patch, so it wasn't worth creating a
common version.

In the case of APIAction the list is always non-null so a check wasn't needed.

[commit-bot: I haz the power, 2013-07-18 19:14:22]

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

[commit-bot: I haz the power, 2013-07-18 21:26:06]

Change committed as 212429