Save extension activity log to a file.

Saves activity log calls into a database.  Database interaction is loosely modeled after how history and bookmarks are saved, although I am not currently archiving the log after 3mo.

Also gives the activity log some structure in preparation for the new UI.

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

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

[felt, 2012-12-01 01:30:21]

Ulfar, Michael-- you might want to take a peek at url_actions.h and
manager_actions.h.  (This CL isn't ready for review, just uploading so you can
see the new log format that's coming.)

[mvrable, 2012-12-04 01:00:13]

I made a pass over the code looking for style issues; if there are
Chrome-specific style rules I may have missed them (but I tried to look at the
Chrome style guide first).  In some cases the same issue arises multiple times
but I only made a comment once.

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
File chrome/browser/extensions/activity_actions.h (right):

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/activity_actions.h:16: public:
There's extra indentation here: the visibility specifiers ("public:") should be
indented one space only, and the rest of the contents of the class two spaces.

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/activity_actions.h:18: virtual std::string
PrettyPrintForil8n() OVERRIDE;
OVERRIDE should be used in subclasses when overriding a method in the parent. 
It shouldn't be used in the parent class.  If there's no implementation of this
method in the parent class (it's a pure virtual method), define it like this:
  virtual std::string PrettyPrintForil8n() = 0;

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/activity_actions.h:25: }
Leave a blank line between the two closing braces, and mark the second one as
  }  // namespace activity

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/activity_actions.h:27: #endif  //
CHROME_BROWSER_EXTENSIONS_ACTIVITY_ACTIONS_H_
lint in the web interface complains about no newline at the end of the
file--that would be worth checking.

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
File chrome/browser/extensions/activity_database.h (right):

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/activity_database.h:29: virtual ~ActivityDatabase();
If ActivityDatabase will not be extended it is not necessary to mark the
destructor virtual (though doing so is still safe, so in my opinion this could
go either way).

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/activity_database.h:33: sql::ErrorDelegate*
error_delegate);
Arguments in continuation lines should all line up.  Write either
  sql::InitStatus Init(const FilePath& db_name,
                       sql::ErrorDelegate* error_delegate);
or
  sql::InitStatus Init(
      const FilePath& db_name,
      sql::ErrorDelegate* error_delegate);

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/activity_database.h:35: // Record a UrlAction in the
database.
Describe the meaning of the return value.

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
File chrome/browser/extensions/manager_actions.cc (right):

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/manager_actions.cc:15: : extension_id_(extension_id),
There should be four spaces of indentation before the colon (since the line has
been wrapped from the one above):

ManagerAction::ManagerAction(const std::string& extension_id,
                             const ManagerActionType& verb,
                             const ManagerTargetType& target,
                             const std::string& api_call,
                             const base::Time& time)
    : extension_id_(extension_id),
      verb_(verb),
      target_(target),
      api_call_(api_call),
      time_(time) { }

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/manager_actions.cc:32: switch (verb_) {
The identation for the switch statement is correct; do not change it.

(Even though classes with visibility specifiers look superficially similar, the
formatting rules are different.)

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
File chrome/browser/extensions/manager_actions.h (right):

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/manager_actions.h:21: "target_type LONGVARCHAR NOT
NULL"
Include spaces at before the closing quote so that there is whitespace when the
lines are joined.  As written this will be interpreted as
  "... LONGVARCHAR NOT NULLapi_call LONGVARCHAR NOT NULL)"

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/manager_actions.h:54: const ManagerActionType& verb,
An enum is like an integer so you should pass by value instead of reference;
just use:
  ManagerActionType verb,

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/manager_actions.h:61: std::string
PrettyPrintForil8n();
This (and the following PrettyPrint method) can probably be marked const (unless
you expect them to have any side-effects).

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
File chrome/browser/extensions/url_actions.h (right):

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/url_actions.h:17: static const std::string
url_table_name_ = "activitylog-urls";
"Static or global variables of class type are forbidden"
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static...)

Instead of string (which is a class type), use a character array:
  static const char url_table_name_[] = "activitylog-urls";

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/url_actions.h:17: static const std::string
url_table_name_ = "activitylog-urls";
The static keyword has several overloaded meanings in C++.  In this case,
"static" means "restrict visibility to the current file only" (roughly
equivalent to enclosing in an unnamed namespace).  However, since this is a
header file which is textually included into various .cc files, the net effect
is to declare a separate file-scoped variable called url_table_name in each C++
file where the .h file is included.

What you want instead is
 declare the variable in the .h file (this one):
  const char url_table_name_[];
 define it in the .cc file:
  const char url_table_name_[] = "activitylog-urls";

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/url_actions.h:18: static const std::string
url_table_creator_ = "CREATE TABLE (" +
For the reasons above, constructing this string dynamically can't be done when
initializing a static variable.  You'll need to either make the value constant
or redo this as a function that returns the appropriate string.

[felt, 2012-12-07 19:27:45]

Thanks for the style check Michael!

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
File chrome/browser/extensions/activity_actions.h (right):

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/activity_actions.h:16: public:
On 2012/12/04 01:00:13, mvrable wrote:
> There's extra indentation here: the visibility specifiers ("public:") should
be
> indented one space only, and the rest of the contents of the class two spaces.

Done.

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/activity_actions.h:18: virtual std::string
PrettyPrintForil8n() OVERRIDE;
On 2012/12/04 01:00:13, mvrable wrote:
> OVERRIDE should be used in subclasses when overriding a method in the parent. 
> It shouldn't be used in the parent class.  If there's no implementation of
this
> method in the parent class (it's a pure virtual method), define it like this:
>   virtual std::string PrettyPrintForil8n() = 0;

Done.

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/activity_actions.h:25: }
On 2012/12/04 01:00:13, mvrable wrote:
> Leave a blank line between the two closing braces, and mark the second one as
>   }  // namespace activity

Done.

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/activity_actions.h:27: #endif  //
CHROME_BROWSER_EXTENSIONS_ACTIVITY_ACTIONS_H_
On 2012/12/04 01:00:13, mvrable wrote:
> lint in the web interface complains about no newline at the end of the
> file--that would be worth checking.

Done.

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
File chrome/browser/extensions/activity_database.h (right):

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/activity_database.h:29: virtual ~ActivityDatabase();
On 2012/12/04 01:00:13, mvrable wrote:
> If ActivityDatabase will not be extended it is not necessary to mark the
> destructor virtual (though doing so is still safe, so in my opinion this could
> go either way).

Done.

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/activity_database.h:33: sql::ErrorDelegate*
error_delegate);
On 2012/12/04 01:00:13, mvrable wrote:
> Arguments in continuation lines should all line up.  Write either
>   sql::InitStatus Init(const FilePath& db_name,
>                        sql::ErrorDelegate* error_delegate);
> or
>   sql::InitStatus Init(
>       const FilePath& db_name,
>       sql::ErrorDelegate* error_delegate);

Done.

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/activity_database.h:35: // Record a UrlAction in the
database.
On 2012/12/04 01:00:13, mvrable wrote:
> Describe the meaning of the return value.

Done.

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
File chrome/browser/extensions/manager_actions.cc (right):

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/manager_actions.cc:15: : extension_id_(extension_id),
On 2012/12/04 01:00:13, mvrable wrote:
> There should be four spaces of indentation before the colon (since the line
has
> been wrapped from the one above):
> 
> ManagerAction::ManagerAction(const std::string& extension_id,
>                              const ManagerActionType& verb,
>                              const ManagerTargetType& target,
>                              const std::string& api_call,
>                              const base::Time& time)
>     : extension_id_(extension_id),
>       verb_(verb),
>       target_(target),
>       api_call_(api_call),
>       time_(time) { }

Done.

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/manager_actions.cc:32: switch (verb_) {
On 2012/12/04 01:00:13, mvrable wrote:
> The identation for the switch statement is correct; do not change it.
> 
> (Even though classes with visibility specifiers look superficially similar,
the
> formatting rules are different.)

Done.

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
File chrome/browser/extensions/manager_actions.h (right):

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/manager_actions.h:21: "target_type LONGVARCHAR NOT
NULL"
On 2012/12/04 01:00:13, mvrable wrote:
> Include spaces at before the closing quote so that there is whitespace when
the
> lines are joined.  As written this will be interpreted as
>   "... LONGVARCHAR NOT NULLapi_call LONGVARCHAR NOT NULL)"

Done.

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/manager_actions.h:54: const ManagerActionType& verb,
On 2012/12/04 01:00:13, mvrable wrote:
> An enum is like an integer so you should pass by value instead of reference;
> just use:
>   ManagerActionType verb,

Done.

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/manager_actions.h:61: std::string
PrettyPrintForil8n();
On 2012/12/04 01:00:13, mvrable wrote:
> This (and the following PrettyPrint method) can probably be marked const
(unless
> you expect them to have any side-effects).

Done.

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
File chrome/browser/extensions/url_actions.h (right):

https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
chrome/browser/extensions/url_actions.h:17: static const std::string
url_table_name_ = "activitylog-urls";
On 2012/12/04 01:00:13, mvrable wrote:
> The static keyword has several overloaded meanings in C++.  In this case,
> "static" means "restrict visibility to the current file only" (roughly
> equivalent to enclosing in an unnamed namespace).  However, since this is a
> header file which is textually included into various .cc files, the net effect
> is to declare a separate file-scoped variable called url_table_name in each
C++
> file where the .h file is included.
> 
> What you want instead is
>  declare the variable in the .h file (this one):
>   const char url_table_name_[];
>  define it in the .cc file:
>   const char url_table_name_[] = "activitylog-urls";

Done.

[felt, 2012-12-13 16:17:00]

ericdingle@, can you do the first review pass?  I'll add Aaron after you take a
look.  BTW this is my first attempt at writing C++ so I apologize in advance for
noob mistakes.

On 2012/12/07 19:27:45, felt wrote:
> Thanks for the style check Michael!
> 
>
https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
> File chrome/browser/extensions/activity_actions.h (right):
> 
>
https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
> chrome/browser/extensions/activity_actions.h:16: public:
> On 2012/12/04 01:00:13, mvrable wrote:
> > There's extra indentation here: the visibility specifiers ("public:") should
> be
> > indented one space only, and the rest of the contents of the class two
spaces.
> 
> Done.
> 
>
https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
> chrome/browser/extensions/activity_actions.h:18: virtual std::string
> PrettyPrintForil8n() OVERRIDE;
> On 2012/12/04 01:00:13, mvrable wrote:
> > OVERRIDE should be used in subclasses when overriding a method in the
parent. 
> > It shouldn't be used in the parent class.  If there's no implementation of
> this
> > method in the parent class (it's a pure virtual method), define it like
this:
> >   virtual std::string PrettyPrintForil8n() = 0;
> 
> Done.
> 
>
https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
> chrome/browser/extensions/activity_actions.h:25: }
> On 2012/12/04 01:00:13, mvrable wrote:
> > Leave a blank line between the two closing braces, and mark the second one
as
> >   }  // namespace activity
> 
> Done.
> 
>
https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
> chrome/browser/extensions/activity_actions.h:27: #endif  //
> CHROME_BROWSER_EXTENSIONS_ACTIVITY_ACTIONS_H_
> On 2012/12/04 01:00:13, mvrable wrote:
> > lint in the web interface complains about no newline at the end of the
> > file--that would be worth checking.
> 
> Done.
> 
>
https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
> File chrome/browser/extensions/activity_database.h (right):
> 
>
https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
> chrome/browser/extensions/activity_database.h:29: virtual ~ActivityDatabase();
> On 2012/12/04 01:00:13, mvrable wrote:
> > If ActivityDatabase will not be extended it is not necessary to mark the
> > destructor virtual (though doing so is still safe, so in my opinion this
could
> > go either way).
> 
> Done.
> 
>
https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
> chrome/browser/extensions/activity_database.h:33: sql::ErrorDelegate*
> error_delegate);
> On 2012/12/04 01:00:13, mvrable wrote:
> > Arguments in continuation lines should all line up.  Write either
> >   sql::InitStatus Init(const FilePath& db_name,
> >                        sql::ErrorDelegate* error_delegate);
> > or
> >   sql::InitStatus Init(
> >       const FilePath& db_name,
> >       sql::ErrorDelegate* error_delegate);
> 
> Done.
> 
>
https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
> chrome/browser/extensions/activity_database.h:35: // Record a UrlAction in the
> database.
> On 2012/12/04 01:00:13, mvrable wrote:
> > Describe the meaning of the return value.
> 
> Done.
> 
>
https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
> File chrome/browser/extensions/manager_actions.cc (right):
> 
>
https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
> chrome/browser/extensions/manager_actions.cc:15: :
extension_id_(extension_id),
> On 2012/12/04 01:00:13, mvrable wrote:
> > There should be four spaces of indentation before the colon (since the line
> has
> > been wrapped from the one above):
> > 
> > ManagerAction::ManagerAction(const std::string& extension_id,
> >                              const ManagerActionType& verb,
> >                              const ManagerTargetType& target,
> >                              const std::string& api_call,
> >                              const base::Time& time)
> >     : extension_id_(extension_id),
> >       verb_(verb),
> >       target_(target),
> >       api_call_(api_call),
> >       time_(time) { }
> 
> Done.
> 
>
https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
> chrome/browser/extensions/manager_actions.cc:32: switch (verb_) {
> On 2012/12/04 01:00:13, mvrable wrote:
> > The identation for the switch statement is correct; do not change it.
> > 
> > (Even though classes with visibility specifiers look superficially similar,
> the
> > formatting rules are different.)
> 
> Done.
> 
>
https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
> File chrome/browser/extensions/manager_actions.h (right):
> 
>
https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
> chrome/browser/extensions/manager_actions.h:21: "target_type LONGVARCHAR NOT
> NULL"
> On 2012/12/04 01:00:13, mvrable wrote:
> > Include spaces at before the closing quote so that there is whitespace when
> the
> > lines are joined.  As written this will be interpreted as
> >   "... LONGVARCHAR NOT NULLapi_call LONGVARCHAR NOT NULL)"
> 
> Done.
> 
>
https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
> chrome/browser/extensions/manager_actions.h:54: const ManagerActionType& verb,
> On 2012/12/04 01:00:13, mvrable wrote:
> > An enum is like an integer so you should pass by value instead of reference;
> > just use:
> >   ManagerActionType verb,
> 
> Done.
> 
>
https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
> chrome/browser/extensions/manager_actions.h:61: std::string
> PrettyPrintForil8n();
> On 2012/12/04 01:00:13, mvrable wrote:
> > This (and the following PrettyPrint method) can probably be marked const
> (unless
> > you expect them to have any side-effects).
> 
> Done.
> 
>
https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
> File chrome/browser/extensions/url_actions.h (right):
> 
>
https://codereview.chromium.org/11421192/diff/4002/chrome/browser/extensions/...
> chrome/browser/extensions/url_actions.h:17: static const std::string
> url_table_name_ = "activitylog-urls";
> On 2012/12/04 01:00:13, mvrable wrote:
> > The static keyword has several overloaded meanings in C++.  In this case,
> > "static" means "restrict visibility to the current file only" (roughly
> > equivalent to enclosing in an unnamed namespace).  However, since this is a
> > header file which is textually included into various .cc files, the net
effect
> > is to declare a separate file-scoped variable called url_table_name in each
> C++
> > file where the .h file is included.
> > 
> > What you want instead is
> >  declare the variable in the .h file (this one):
> >   const char url_table_name_[];
> >  define it in the .cc file:
> >   const char url_table_name_[] = "activitylog-urls";
> 
> Done.

[Eric Dingle, 2012-12-13 20:37:34]

Mostly style stuff, but looking good.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/activity_actions.h (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_actions.h:24: protected:
You should also define a constructor, even if it does nothing. And in general,
the constructor and destructor can be defined as the first methods of the public
section. There's no need to make them protected/private unless you REALLY don't
want them being run from outside the class. This applies to all files in this
CL.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_actions.h:25: virtual ~Action() { }
Don't forget:

DISALLOW_COPY_AND_ASSIGN(Action);

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_actions.h:25: virtual ~Action() { }
I think the common style is to have no space between {} or to have the trailing
} on the next line. This applies to all files in this CL.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_actions.h:28: }  // namespace
// namespace extensions

This applies to all files in this CL.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_actions.h:31: 
Remove extra line break. This applies to all files in this CL.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/activity_database.cc (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_database.cc:26: initialized_ = false;
You should initialize this in the initialization list:

ActivityDatabase::ActivityDatabase : initialized_(false) {
}

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_database.cc:59: if
(!db_.DoesTableExist(url_table_name_)) {
This logic might be worth extracting to a helper method since you do it three
times.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_database.cc:60: std::string url_table_creator
= "CREATE TABLE " +
Replace these concatenations with StrCat(...). Here and below.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_database.cc:91: sql::InitStatus
ActivityDatabase::MarkInitFailure() {
Not sure that this method is needed. initialized_ is set to false in the
constructor and only set to true if initialization succeeds. Seems like you can
just replace calls to MarkInitFailure with sql::INIT_FAILURE.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/activity_database.h (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_database.h:61: sql::InitStatus
MarkInitFailure();
From the style guide, order of declarations:

Typedefs and Enums
Constants (static const data members)
Constructors
Destructor
Methods, including static methods
Data Members (except static const data members)

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

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:30: static const char*
kActivityThreadName = "Chrome_ActivityLogThread";
No need for static keyword in this context.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:57: class ActivityLogFactory : public
ProfileKeyedServiceFactory {
All classes should have at least one constructor and a destructor.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:119: : thread_(new
base::Thread(kActivityThreadName)) {
I'm not saying that this is wrong, but you should look into reusing the IO
thread of IO operations.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:120: 
Remove extra blank line.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:126: db_ = new ActivityDatabase();
Would suggest using db_.reset(new ActivityDatabase()) for readability.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:197: std::string verb;
std::string manager, verb;

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:226: void
ActivityLog::LogBlockedAction(const Extension* extension,
You don't notify the observers on blocked calls?

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:262:
ActivityLog::ACTIVITY_CONTENT_SCRIPT,
I assume that you'll notify for a different kind of activity for dom
manipulations or network requests?

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/activity_log.h (left):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:59: friend struct
DefaultSingletonTraits<ActivityLog>;
Is this not required anymore?

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

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
In general, I feel like you've introduced a disconnect between the calling
convention and the observing convention. Previously, the callers would call
Log() with the activity represented as an enum and the observers would get
invoked with the activity as an enum. Now, the callers must call a specific
Log*() method while the observers still only observe the one method with the
activity as an enum.

I would prefer if you choose one design pattern. Either use the enum for the
callers and the observers, or have individual Log* methods for the callers and
individual OnLog* methods for the observers.

What do you think?

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:48: explicit ActivityLog(Profile*
profile);
It's lousy that you have to make the constructor public since GetInstance should
be used. Is it possible to keep the constructor private and list
ActivityLogFactory as a friend class?

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:49: ActivityLog();
You can omit the default constructor if there's no use for it.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:62: void LogManagerAction(const
Extension* extension,
Maybe this is just me, but the name of the method doesn't pair up with the
comment above. This is the method that should be called when an extension makes
an API call?

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:123: base::Thread* thread_;
You should use a scoped_ptr for this.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/activity_log_unittest.cc (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log_unittest.cc:20: namespace {
No need for the anonymous namespace.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log_unittest.cc:43:
content::TestBrowserThread ui_thread_;
I haven't written tests with the TestBrowserThread before, but I assume that you
need them for this test to run, even though you don't access them in any way?

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/blocked_actions.h (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/blocked_actions.h:14: extern const char*
blocked_table_name_;
These should be declared as private static char array member variables. This
applies to the other action classes as well.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/blocked_actions.h:25: // Print a UrlAction with il8n
substitutions for display.
*BlockedAction

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/blocked_actions.h:28: // Print a UrlAction as a
regular string for debugging purposes.
*BlockedAction

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/blocked_actions.h:38: virtual ~BlockedAction();
No need for this to be protected. Declare it beneath the constructor. This
applies to the other action classes as well.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/manager_actions.h (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/manager_actions.h:17: // This class describes
extension actions that pertain to Content Script
This looks like the description for UrlAction. Copy and paste error?

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/manager_actions.h:48: // url_title, which can be an
empty string if the ActionType is XHR.
Also looks like it describes the UrlAction constructor.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/url_actions.cc (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/url_actions.cc:74: return UNKNOWN;  // this should
never happen!
Perhaps use NOTREACHED() instead of having an UNKNOWN type.

https://codereview.chromium.org/11421192/diff/30006/chrome/common/chrome_cons...
File chrome/common/chrome_constants.h (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/common/chrome_cons...
chrome/common/chrome_constants.h:55: extern const FilePath::CharType
kActivityLogFilename[];
Might be wise to use kExtensionActivityLogFilename since this file is not
specific to extensions.

[felt, 2012-12-15 02:51:52]

Addressed most of Eric's comments, with a few questions.

Separately, I'm going to look into reusing the existing browser IO thread.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/activity_actions.h (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_actions.h:24: protected:
On 2012/12/13 20:37:34, Eric Dingle wrote:
> You should also define a constructor, even if it does nothing. And in general,
> the constructor and destructor can be defined as the first methods of the
public
> section. There's no need to make them protected/private unless you REALLY
don't
> want them being run from outside the class. This applies to all files in this
> CL.

They have to be protected/private due to the refcounting.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_actions.h:25: virtual ~Action() { }
On 2012/12/13 20:37:34, Eric Dingle wrote:
> Don't forget:
> 
> DISALLOW_COPY_AND_ASSIGN(Action);

Done.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_actions.h:25: virtual ~Action() { }
On 2012/12/13 20:37:34, Eric Dingle wrote:
> Don't forget:
> 
> DISALLOW_COPY_AND_ASSIGN(Action);

Done.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_actions.h:28: }  // namespace
On 2012/12/13 20:37:34, Eric Dingle wrote:
> // namespace extensions
> 
> This applies to all files in this CL.

Done.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_actions.h:31: 
On 2012/12/13 20:37:34, Eric Dingle wrote:
> Remove extra line break. This applies to all files in this CL.

Done.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/activity_database.cc (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_database.cc:26: initialized_ = false;
On 2012/12/13 20:37:34, Eric Dingle wrote:
> You should initialize this in the initialization list:
> 
> ActivityDatabase::ActivityDatabase : initialized_(false) {
> }

Done.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_database.cc:59: if
(!db_.DoesTableExist(url_table_name_)) {
On 2012/12/13 20:37:34, Eric Dingle wrote:
> This logic might be worth extracting to a helper method since you do it three
> times.

Done.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_database.cc:60: std::string url_table_creator
= "CREATE TABLE " +
On 2012/12/13 20:37:34, Eric Dingle wrote:
> Replace these concatenations with StrCat(...). Here and below.

Done.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_database.cc:91: sql::InitStatus
ActivityDatabase::MarkInitFailure() {
On 2012/12/13 20:37:34, Eric Dingle wrote:
> Not sure that this method is needed. initialized_ is set to false in the
> constructor and only set to true if initialization succeeds. Seems like you
can
> just replace calls to MarkInitFailure with sql::INIT_FAILURE.

Done.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/activity_database.h (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_database.h:61: sql::InitStatus
MarkInitFailure();
On 2012/12/13 20:37:34, Eric Dingle wrote:
> From the style guide, order of declarations:
> 
> Typedefs and Enums
> Constants (static const data members)
> Constructors
> Destructor
> Methods, including static methods
> Data Members (except static const data members)

Done.

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

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:30: static const char*
kActivityThreadName = "Chrome_ActivityLogThread";
On 2012/12/13 20:37:34, Eric Dingle wrote:
> No need for static keyword in this context.

Done.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:57: class ActivityLogFactory : public
ProfileKeyedServiceFactory {
On 2012/12/13 20:37:34, Eric Dingle wrote:
> All classes should have at least one constructor and a destructor.

Done.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:120: 
On 2012/12/13 20:37:34, Eric Dingle wrote:
> Remove extra blank line.

Done.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:126: db_ = new ActivityDatabase();
On 2012/12/13 20:37:34, Eric Dingle wrote:
> Would suggest using db_.reset(new ActivityDatabase()) for readability.

The docs in base/memory/ref_counted.h say to use a = new Y(); for scoped_refptrs
(different from scoped_ptrs).

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:197: std::string verb;
On 2012/12/13 20:37:34, Eric Dingle wrote:
> std::string manager, verb;

Done.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:226: void
ActivityLog::LogBlockedAction(const Extension* extension,
On 2012/12/13 20:37:34, Eric Dingle wrote:
> You don't notify the observers on blocked calls?

Whoops.  Done.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:262:
ActivityLog::ACTIVITY_CONTENT_SCRIPT,
On 2012/12/13 20:37:34, Eric Dingle wrote:
> I assume that you'll notify for a different kind of activity for dom
> manipulations or network requests?

Yes, I'm going to change how this works as part of the new UI.  I'll be using
the enum values inside of the UrlAction to format the message to display.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/activity_log.h (left):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:59: friend struct
DefaultSingletonTraits<ActivityLog>;
On 2012/12/13 20:37:34, Eric Dingle wrote:
> Is this not required anymore?

It's in the ActivityLogFactory now.

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

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/12/13 20:37:34, Eric Dingle wrote:
> In general, I feel like you've introduced a disconnect between the calling
> convention and the observing convention. Previously, the callers would call
> Log() with the activity represented as an enum and the observers would get
> invoked with the activity as an enum. Now, the callers must call a specific
> Log*() method while the observers still only observe the one method with the
> activity as an enum.
> 
> I would prefer if you choose one design pattern. Either use the enum for the
> callers and the observers, or have individual Log* methods for the callers and
> individual OnLog* methods for the observers.
> 
> What do you think?

I'm planning on getting rid of the enum and modifying the Observers when I make
the new UI.  I'm leaving it there for right now since the current UI expects it
to exist, and I'm trying not to make any visible changes in this CL.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:48: explicit ActivityLog(Profile*
profile);
On 2012/12/13 20:37:34, Eric Dingle wrote:
> It's lousy that you have to make the constructor public since GetInstance
should
> be used. Is it possible to keep the constructor private and list
> ActivityLogFactory as a friend class?

Good idea.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:49: ActivityLog();
On 2012/12/13 20:37:34, Eric Dingle wrote:
> You can omit the default constructor if there's no use for it.

Done.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:62: void LogManagerAction(const
Extension* extension,
On 2012/12/13 20:37:34, Eric Dingle wrote:
> Maybe this is just me, but the name of the method doesn't pair up with the
> comment above. This is the method that should be called when an extension
makes
> an API call?

OK, I renamed it to be LogAPIAction.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:123: base::Thread* thread_;
On 2012/12/13 20:37:34, Eric Dingle wrote:
> You should use a scoped_ptr for this.

Done.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/activity_log_unittest.cc (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log_unittest.cc:20: namespace {
On 2012/12/13 20:37:34, Eric Dingle wrote:
> No need for the anonymous namespace.

Done.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log_unittest.cc:43:
content::TestBrowserThread ui_thread_;
On 2012/12/13 20:37:34, Eric Dingle wrote:
> I haven't written tests with the TestBrowserThread before, but I assume that
you
> need them for this test to run, even though you don't access them in any way?

Yes, it seems like they're needed by ChromeRenderViewHostTestHarness in some
way.  Everything explodes if I remove them.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/blocked_actions.h (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/blocked_actions.h:14: extern const char*
blocked_table_name_;
On 2012/12/13 20:37:34, Eric Dingle wrote:
> These should be declared as private static char array member variables. This
> applies to the other action classes as well.

These are used in ActivityDatabase::InitializeTable (which doesn't have an
actual Action on hand).  I could define them there, but I liked having them here
so that everything for each type of action is in one place.  Do you have a
strong preference for me to move them?

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/blocked_actions.h:25: // Print a UrlAction with il8n
substitutions for display.
On 2012/12/13 20:37:34, Eric Dingle wrote:
> *BlockedAction

sorry -- what does this mean?

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/blocked_actions.h:38: virtual ~BlockedAction();
On 2012/12/13 20:37:34, Eric Dingle wrote:
> No need for this to be protected. Declare it beneath the constructor. This
> applies to the other action classes as well.

It needs to be protected because of how scoped_refptrs work.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/manager_actions.h (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/manager_actions.h:17: // This class describes
extension actions that pertain to Content Script
On 2012/12/13 20:37:34, Eric Dingle wrote:
> This looks like the description for UrlAction. Copy and paste error?

Done.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/manager_actions.h:48: // url_title, which can be an
empty string if the ActionType is XHR.
On 2012/12/13 20:37:34, Eric Dingle wrote:
> Also looks like it describes the UrlAction constructor.

Done.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/url_actions.cc (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/url_actions.cc:74: return UNKNOWN;  // this should
never happen!
On 2012/12/13 20:37:34, Eric Dingle wrote:
> Perhaps use NOTREACHED() instead of having an UNKNOWN type.

Done, although I still have to return *something* to avoid a compiler error
about reading the end of a non-void function.

https://codereview.chromium.org/11421192/diff/30006/chrome/common/chrome_cons...
File chrome/common/chrome_constants.h (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/common/chrome_cons...
chrome/common/chrome_constants.h:55: extern const FilePath::CharType
kActivityLogFilename[];
On 2012/12/13 20:37:34, Eric Dingle wrote:
> Might be wise to use kExtensionActivityLogFilename since this file is not
> specific to extensions.

Done.

[felt, 2012-12-17 22:45:09]

Redid how threading works - modifications to activity_log and
extension_function_dispatcher.

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

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:119: : thread_(new
base::Thread(kActivityThreadName)) {
On 2012/12/13 20:37:34, Eric Dingle wrote:
> I'm not saying that this is wrong, but you should look into reusing the IO
> thread of IO operations.

Now using the DB thread.

[felt, 2012-12-18 18:15:14]

Aaron, this is the continuation of Eric's activity log work (which I think you
reviewed also).

[Eric Dingle, 2012-12-18 21:55:34]

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/blocked_actions.h (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/blocked_actions.h:14: extern const char*
blocked_table_name_;
On 2012/12/15 02:51:52, felt wrote:
> On 2012/12/13 20:37:34, Eric Dingle wrote:
> > These should be declared as private static char array member variables. This
> > applies to the other action classes as well.
> 
> These are used in ActivityDatabase::InitializeTable (which doesn't have an
> actual Action on hand).  I could define them there, but I liked having them
here
> so that everything for each type of action is in one place.  Do you have a
> strong preference for me to move them?

Couldn't you define this as a member variable of the BlockedAction class. You
would access them in InitializeTable using:

BlockedAction::kTableName

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/blocked_actions.h:25: // Print a UrlAction with il8n
substitutions for display.
On 2012/12/15 02:51:52, felt wrote:
> On 2012/12/13 20:37:34, Eric Dingle wrote:
> > *BlockedAction
> 
> sorry -- what does this mean?

You have a copy and paste error.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/manager_actions.h (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/manager_actions.h:17: // This class describes
extension actions that pertain to Content Script
On 2012/12/15 02:51:52, felt wrote:
> On 2012/12/13 20:37:34, Eric Dingle wrote:
> > This looks like the description for UrlAction. Copy and paste error?
> 
> Done.

I don't see any changes. Is this the right description for this class?

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/manager_actions.h:48: // url_title, which can be an
empty string if the ActionType is XHR.
On 2012/12/15 02:51:52, felt wrote:
> On 2012/12/13 20:37:34, Eric Dingle wrote:
> > Also looks like it describes the UrlAction constructor.
> 
> Done.

I don't see any changes.

https://codereview.chromium.org/11421192/diff/66030/chrome/browser/extensions...
File chrome/browser/extensions/activity_database.h (right):

https://codereview.chromium.org/11421192/diff/66030/chrome/browser/extensions...
chrome/browser/extensions/activity_database.h:55: ~ActivityDatabase();
virtual?

https://codereview.chromium.org/11421192/diff/66030/chrome/browser/extensions...
File chrome/browser/extensions/activity_log_unittest.cc (right):

https://codereview.chromium.org/11421192/diff/66030/chrome/browser/extensions...
chrome/browser/extensions/activity_log_unittest.cc:70: ListValue* args = new
ListValue();
This leaks.

[felt, 2012-12-18 23:28:53]

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/blocked_actions.h (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/blocked_actions.h:14: extern const char*
blocked_table_name_;
On 2012/12/18 21:55:35, Eric Dingle wrote:
> On 2012/12/15 02:51:52, felt wrote:
> > On 2012/12/13 20:37:34, Eric Dingle wrote:
> > > These should be declared as private static char array member variables.
This
> > > applies to the other action classes as well.
> > 
> > These are used in ActivityDatabase::InitializeTable (which doesn't have an
> > actual Action on hand).  I could define them there, but I liked having them
> here
> > so that everything for each type of action is in one place.  Do you have a
> > strong preference for me to move them?
> 
> Couldn't you define this as a member variable of the BlockedAction class. You
> would access them in InitializeTable using:
> 
> BlockedAction::kTableName

The code that uses the db name doesn't actually have a BlockedAction object
instance - InitializeTable is just setting up the tables.  I could make
empty/fake BlockedActions/ManagerActions/etc., although that seems undesirable.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/blocked_actions.h:25: // Print a UrlAction with il8n
substitutions for display.
On 2012/12/18 21:55:35, Eric Dingle wrote:
> On 2012/12/15 02:51:52, felt wrote:
> > On 2012/12/13 20:37:34, Eric Dingle wrote:
> > > *BlockedAction
> > 
> > sorry -- what does this mean?
> 
> You have a copy and paste error.

Done.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/blocked_actions.h:28: // Print a UrlAction as a
regular string for debugging purposes.
On 2012/12/13 20:37:34, Eric Dingle wrote:
> *BlockedAction

Done.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/manager_actions.h (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/manager_actions.h:17: // This class describes
extension actions that pertain to Content Script
On 2012/12/18 21:55:35, Eric Dingle wrote:
> On 2012/12/15 02:51:52, felt wrote:
> > On 2012/12/13 20:37:34, Eric Dingle wrote:
> > > This looks like the description for UrlAction. Copy and paste error?
> > 
> > Done.
> 
> I don't see any changes. Is this the right description for this class?

Very odd.  Don't know what happened.  Fixed.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/manager_actions.h:48: // url_title, which can be an
empty string if the ActionType is XHR.
On 2012/12/18 21:55:35, Eric Dingle wrote:
> On 2012/12/15 02:51:52, felt wrote:
> > On 2012/12/13 20:37:34, Eric Dingle wrote:
> > > Also looks like it describes the UrlAction constructor.
> > 
> > Done.
> 
> I don't see any changes.

Done.

https://codereview.chromium.org/11421192/diff/66030/chrome/browser/extensions...
File chrome/browser/extensions/activity_log_unittest.cc (right):

https://codereview.chromium.org/11421192/diff/66030/chrome/browser/extensions...
chrome/browser/extensions/activity_log_unittest.cc:70: ListValue* args = new
ListValue();
On 2012/12/18 21:55:35, Eric Dingle wrote:
> This leaks.

Done.

[Eric Dingle, 2012-12-19 16:04:11]

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/blocked_actions.h (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/blocked_actions.h:14: extern const char*
blocked_table_name_;
On 2012/12/18 23:28:54, felt wrote:
> On 2012/12/18 21:55:35, Eric Dingle wrote:
> > On 2012/12/15 02:51:52, felt wrote:
> > > On 2012/12/13 20:37:34, Eric Dingle wrote:
> > > > These should be declared as private static char array member variables.
> This
> > > > applies to the other action classes as well.
> > > 
> > > These are used in ActivityDatabase::InitializeTable (which doesn't have an
> > > actual Action on hand).  I could define them there, but I liked having
them
> > here
> > > so that everything for each type of action is in one place.  Do you have a
> > > strong preference for me to move them?
> > 
> > Couldn't you define this as a member variable of the BlockedAction class.
You
> > would access them in InitializeTable using:
> > 
> > BlockedAction::kTableName
> 
> The code that uses the db name doesn't actually have a BlockedAction object
> instance - InitializeTable is just setting up the tables.  I could make
> empty/fake BlockedActions/ManagerActions/etc., although that seems
undesirable.

You should declare it as:

static const char* kTableName;

within the public section of the BlockedAction class. Then you can access it
without instantiating the class.

BlockedAction::kTableName

https://codereview.chromium.org/11421192/diff/27010/chrome/browser/extensions...
File chrome/browser/extensions/activity_log_unittest.cc (right):

https://codereview.chromium.org/11421192/diff/27010/chrome/browser/extensions...
chrome/browser/extensions/activity_log_unittest.cc:88: delete args;
You could also just use scoped_ptr.

[felt, 2012-12-19 17:56:27]

Eric, made the changes.

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
File chrome/browser/extensions/blocked_actions.h (right):

https://codereview.chromium.org/11421192/diff/30006/chrome/browser/extensions...
chrome/browser/extensions/blocked_actions.h:14: extern const char*
blocked_table_name_;
On 2012/12/19 16:04:11, Eric Dingle wrote:
> On 2012/12/18 23:28:54, felt wrote:
> > On 2012/12/18 21:55:35, Eric Dingle wrote:
> > > On 2012/12/15 02:51:52, felt wrote:
> > > > On 2012/12/13 20:37:34, Eric Dingle wrote:
> > > > > These should be declared as private static char array member
variables.
> > This
> > > > > applies to the other action classes as well.
> > > > 
> > > > These are used in ActivityDatabase::InitializeTable (which doesn't have
an
> > > > actual Action on hand).  I could define them there, but I liked having
> them
> > > here
> > > > so that everything for each type of action is in one place.  Do you have
a
> > > > strong preference for me to move them?
> > > 
> > > Couldn't you define this as a member variable of the BlockedAction class.
> You
> > > would access them in InitializeTable using:
> > > 
> > > BlockedAction::kTableName
> > 
> > The code that uses the db name doesn't actually have a BlockedAction object
> > instance - InitializeTable is just setting up the tables.  I could make
> > empty/fake BlockedActions/ManagerActions/etc., although that seems
> undesirable.
> 
> You should declare it as:
> 
> static const char* kTableName;
> 
> within the public section of the BlockedAction class. Then you can access it
> without instantiating the class.
> 
> BlockedAction::kTableName

Ohhh.  I understand now.  Done.

https://codereview.chromium.org/11421192/diff/66030/chrome/browser/extensions...
File chrome/browser/extensions/activity_database.h (right):

https://codereview.chromium.org/11421192/diff/66030/chrome/browser/extensions...
chrome/browser/extensions/activity_database.h:55: ~ActivityDatabase();
On 2012/12/18 21:55:35, Eric Dingle wrote:
> virtual?

Done.

https://codereview.chromium.org/11421192/diff/27010/chrome/browser/extensions...
File chrome/browser/extensions/activity_log_unittest.cc (right):

https://codereview.chromium.org/11421192/diff/27010/chrome/browser/extensions...
chrome/browser/extensions/activity_log_unittest.cc:88: delete args;
On 2012/12/19 16:04:11, Eric Dingle wrote:
> You could also just use scoped_ptr.

Done.

[Eric Dingle, 2012-12-19 18:33:47]

LGTM

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
File chrome/browser/extensions/activity_log_unittest.cc (right):

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
chrome/browser/extensions/activity_log_unittest.cc:70: scoped_ptr<ListValue>
args;
scoped_ptr<ListValue> args(new ListValue());

[Eric Dingle, 2012-12-19 18:34:08]

I would suggest having someone on the extensions side review this before
submitting.

[felt, 2012-12-19 18:40:20]

mpcomplete@ can you review this please?  It's the continuation of Eric's work 
on the activity log.

[felt, 2013-01-03 16:46:35]

Pinging Matt since everyone's been on vacation

[Matt Perry, 2013-01-03 20:53:34]

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
File chrome/browser/extensions/activity_database.h (right):

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
chrome/browser/extensions/activity_database.h:33: void
RecordUrlAction(scoped_refptr<UrlAction> action);
It seems to me this would be better handled by the Action subclasses. i.e. you
have a single ActivityDatabase::RecordAction(Action* action) method which calls
action->RecordAction(sql_connection);

Then the database structure is self contained in each action type's file. The
ActivityDatabase doesn't need to know about it.

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

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:66: void schedule_death() {
ScheduleDeath

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:179: std::vector<std::string>(1,
call_signature));
Seems like we never Notify with multiple messages. Change OnExtensionActivity to
accept a single message.

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

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:132: mutable base::Lock lock_;
Is this lock needed? It sounds like this class can only be accessed on one
thread.

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
chrome/browser/extensions/extension_function_dispatcher.cc:51: const ListValue*
args,
This leaks |args|. Pass as a scoped_ptr to fix.

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
chrome/browser/extensions/extension_function_dispatcher.cc:56: bool known =
BrowserThread::GetCurrentThreadIdentifier(&thread_id);
Use BrowserThread::CurrentlyOn

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
File chrome/browser/extensions/manager_actions.h (right):

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
chrome/browser/extensions/manager_actions.h:16: class ManagerAction : public
Action,
Why "ManagerAction"? Why not APIAction?

[felt, 2013-01-07 23:44:22]

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
File chrome/browser/extensions/activity_database.h (right):

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
chrome/browser/extensions/activity_database.h:33: void
RecordUrlAction(scoped_refptr<UrlAction> action);
On 2013/01/03 20:53:35, Matt Perry wrote:
> It seems to me this would be better handled by the Action subclasses. i.e. you
> have a single ActivityDatabase::RecordAction(Action* action) method which
calls
> action->RecordAction(sql_connection);
> 
> Then the database structure is self contained in each action type's file. The
> ActivityDatabase doesn't need to know about it.

I agree that it would be neater to have a single
ActivityDatabase::RecordAction(Action), but I'm not sure how to do it well.  A
few options I thought about:

1) I don't want to move database logic out of ActivityDatabase and into the
action classes, since the db connection needs to be managed in a single place.
2) I can't overload ActivityDatabase::RecordAction with the 3 different types
because the templating in ScheduleAndForget fails if the method is overloaded.
3) I could have a single ActivityDatabase::RecordAction that checks what
subclass Action it's dealing with in some way -- e.g., by having a constant that
differs in value between the subclasses.  This seems like bad practice, but is
probably the best option I can think of.

I'm new to C++ so my style sense isn't the best.  Would I be best off with
option #3, is there a better way, or should I leave it as-is?

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

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:66: void schedule_death() {
On 2013/01/03 20:53:35, Matt Perry wrote:
> ScheduleDeath

Done.

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:179: std::vector<std::string>(1,
call_signature));
On 2013/01/03 20:53:35, Matt Perry wrote:
> Seems like we never Notify with multiple messages. Change OnExtensionActivity
to
> accept a single message.

Done.

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

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:132: mutable base::Lock lock_;
On 2013/01/03 20:53:35, Matt Perry wrote:
> Is this lock needed? It sounds like this class can only be accessed on one
> thread.

It should only be accessed from the main (UI) thread, but I'm not the one who
added the lock-- it was added by Eric (line 71 in the previous revision).

Eric, please let me know if I should add the lock back in.

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
File chrome/browser/extensions/activity_log_unittest.cc (right):

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
chrome/browser/extensions/activity_log_unittest.cc:70: scoped_ptr<ListValue>
args;
On 2012/12/19 18:33:47, Eric Dingle wrote:
> scoped_ptr<ListValue> args(new ListValue());

Done.

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
chrome/browser/extensions/extension_function_dispatcher.cc:51: const ListValue*
args,
On 2013/01/03 20:53:35, Matt Perry wrote:
> This leaks |args|. Pass as a scoped_ptr to fix.

I think I've done this correctly, assuming you don't also want me to change
ActivityLog::LogAPIAction to accept a scoped_ptr as well.

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
chrome/browser/extensions/extension_function_dispatcher.cc:56: bool known =
BrowserThread::GetCurrentThreadIdentifier(&thread_id);
On 2013/01/03 20:53:35, Matt Perry wrote:
> Use BrowserThread::CurrentlyOn

Done.

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
File chrome/browser/extensions/manager_actions.h (right):

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
chrome/browser/extensions/manager_actions.h:16: class ManagerAction : public
Action,
On 2013/01/03 20:53:35, Matt Perry wrote:
> Why "ManagerAction"? Why not APIAction?

Done.

[Matt Perry, 2013-01-08 00:02:01]

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
File chrome/browser/extensions/activity_database.h (right):

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
chrome/browser/extensions/activity_database.h:33: void
RecordUrlAction(scoped_refptr<UrlAction> action);
On 2013/01/07 23:44:22, felt wrote:
> On 2013/01/03 20:53:35, Matt Perry wrote:
> > It seems to me this would be better handled by the Action subclasses. i.e.
you
> > have a single ActivityDatabase::RecordAction(Action* action) method which
> calls
> > action->RecordAction(sql_connection);
> > 
> > Then the database structure is self contained in each action type's file.
The
> > ActivityDatabase doesn't need to know about it.
> 
> I agree that it would be neater to have a single
> ActivityDatabase::RecordAction(Action), but I'm not sure how to do it well.  A
> few options I thought about:
> 
> 1) I don't want to move database logic out of ActivityDatabase and into the
> action classes, since the db connection needs to be managed in a single place.

I don't understand the reason you give here. Why can't ActivityAction have a
virtual method RecordAction which takes a pointer to the db_? Then just move the
code from RecordUrlAction to UrlAction::RecordAction, etc.

The way it is now, the database logic for each database is spread across 2
files. The table format is in foo_action.cc, and the table writing logic is in
this file.

> 2) I can't overload ActivityDatabase::RecordAction with the 3 different types
> because the templating in ScheduleAndForget fails if the method is overloaded.
> 3) I could have a single ActivityDatabase::RecordAction that checks what
> subclass Action it's dealing with in some way -- e.g., by having a constant
that
> differs in value between the subclasses.  This seems like bad practice, but is
> probably the best option I can think of.
> 
> I'm new to C++ so my style sense isn't the best.  Would I be best off with
> option #3, is there a better way, or should I leave it as-is?

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
chrome/browser/extensions/extension_function_dispatcher.cc:51: const ListValue*
args,
On 2013/01/07 23:44:22, felt wrote:
> On 2013/01/03 20:53:35, Matt Perry wrote:
> > This leaks |args|. Pass as a scoped_ptr to fix.
> 
> I think I've done this correctly, assuming you don't also want me to change
> ActivityLog::LogAPIAction to accept a scoped_ptr as well.

Correct. (Only use scoped_ptr when passing ownership. LogAPIAction doesn't
assume ownership of |args| so it should take a raw ptr.)

[felt, 2013-01-08 06:38:51]

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
File chrome/browser/extensions/activity_database.h (right):

https://codereview.chromium.org/11421192/diff/76005/chrome/browser/extensions...
chrome/browser/extensions/activity_database.h:33: void
RecordUrlAction(scoped_refptr<UrlAction> action);
On 2013/01/08 00:02:01, Matt Perry wrote:
> On 2013/01/07 23:44:22, felt wrote:
> > On 2013/01/03 20:53:35, Matt Perry wrote:
> > > It seems to me this would be better handled by the Action subclasses. i.e.
> you
> > > have a single ActivityDatabase::RecordAction(Action* action) method which
> > calls
> > > action->RecordAction(sql_connection);
> > > 
> > > Then the database structure is self contained in each action type's file.
> The
> > > ActivityDatabase doesn't need to know about it.
> > 
> > I agree that it would be neater to have a single
> > ActivityDatabase::RecordAction(Action), but I'm not sure how to do it well. 
A
> > few options I thought about:
> > 
> > 1) I don't want to move database logic out of ActivityDatabase and into the
> > action classes, since the db connection needs to be managed in a single
place.
> 
> I don't understand the reason you give here. Why can't ActivityAction have a
> virtual method RecordAction which takes a pointer to the db_? Then just move
the
> code from RecordUrlAction to UrlAction::RecordAction, etc.
> 
> The way it is now, the database logic for each database is spread across 2
> files. The table format is in foo_action.cc, and the table writing logic is in
> this file.
> 
> > 2) I can't overload ActivityDatabase::RecordAction with the 3 different
types
> > because the templating in ScheduleAndForget fails if the method is
overloaded.
> > 3) I could have a single ActivityDatabase::RecordAction that checks what
> > subclass Action it's dealing with in some way -- e.g., by having a constant
> that
> > differs in value between the subclasses.  This seems like bad practice, but
is
> > probably the best option I can think of.
> > 
> > I'm new to C++ so my style sense isn't the best.  Would I be best off with
> > option #3, is there a better way, or should I leave it as-is?
> 

Done.

[Matt Perry, 2013-01-08 18:55:20]

lgtm

[felt, 2013-01-08 19:02:18]

Looking for OWNERS approval:
mirandac@chromium.org - chrome/browser/profiles
arv@chromium.org - chrome/browser/ui/webui/extensions
ben@chromium.org - chrome/common/chrome_constants.*

[Ben Goodger (Google), 2013-01-08 21:00:47]

chrome_constants lgtm

[Miranda Callahan, 2013-01-08 22:05:04]

On 2013/01/08 21:00:47, Ben Goodger (Google) wrote:
> chrome_constants lgtm

profile_dependency_manager LGTM.

[felt, 2013-01-10 19:18:44]

Pinging arv@ for OWNERS approval - chrome/browser/ui/webui/extensions

[felt, 2013-01-11 19:41:23]

estade@chromium.org, could I get OWNERS approval for
chrome/browser/ui/webui/extensions ? Trivial changes.

[Evan Stade, 2013-01-11 20:28:23]

lgtm

[commit-bot: I haz the power, 2013-01-11 20:31:39]

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

[commit-bot: I haz the power, 2013-01-11 20:31:52]

Presubmit check for 11421192-130001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2013|2012|2011|2010|2009|2008|2007|2006|2006-2008) The
Chromium Authors\. All rights reserved\.\n.*? Use of this source code is
governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?:
\*/)?\n
Found a bad license header in these files:
  chrome/browser/extensions/activity_actions.h \
  chrome/browser/extensions/activity_database.h \
  chrome/browser/extensions/api_actions.cc \
  chrome/browser/extensions/api_actions.h \
  chrome/browser/extensions/blocked_actions.cc \
  chrome/browser/extensions/blocked_actions.h \
  chrome/browser/extensions/url_actions.cc \
  chrome/browser/extensions/url_actions.h

Presubmit checks took 2.5s to calculate.

[commit-bot: I haz the power, 2013-01-11 23:54:44]

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

[commit-bot: I haz the power, 2013-01-11 23:54:56]

Presubmit check for 11421192-130001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2013|2012|2011|2010|2009|2008|2007|2006|2006-2008) The
Chromium Authors\. All rights reserved\.\n.*? Use of this source code is
governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?:
\*/)?\n
Found a bad license header in these files:
  chrome/browser/extensions/activity_actions.h \
  chrome/browser/extensions/activity_database.h \
  chrome/browser/extensions/api_actions.cc \
  chrome/browser/extensions/api_actions.h \
  chrome/browser/extensions/blocked_actions.cc \
  chrome/browser/extensions/blocked_actions.h \
  chrome/browser/extensions/url_actions.cc \
  chrome/browser/extensions/url_actions.h

Presubmit checks took 2.4s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2013-01-12 00:04:14]

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

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

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-01-12 16:51:04]

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

[commit-bot: I haz the power, 2013-01-12 21:23:24]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-01-14 17:55:55]

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

[commit-bot: I haz the power, 2013-01-14 22:38:36]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-01-15 00:29:27]

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

[commit-bot: I haz the power, 2013-01-15 05:18:16]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-01-15 23:30:40]

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

[commit-bot: I haz the power, 2013-01-16 20:08:23]

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

[commit-bot: I haz the power, 2013-01-16 22:39:21]

Change committed as 177235

[commit-bot: I haz the power, 2013-01-17 16:09:57]

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

[commit-bot: I haz the power, 2013-01-17 17:53:20]

Change committed as 177426