Record event activity to the extension activity log.

Record event activity to the extension activity log.

Log when an extension listens for an event in the extensions API, when
removing listeners, and when an event callback is triggered.

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

[felt, 2013-01-22 22:45:52]

Figured out why they are backwards

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

https://codereview.chromium.org/11946028/diff/7005/chrome/browser/extensions/...
chrome/browser/extensions/activity_log.cc:176:
ActivityLog::ACTIVITY_EVENT_DISPATCH,
This should be ACTIVITY_EXTENSION_API_CALL

https://codereview.chromium.org/11946028/diff/7005/chrome/browser/extensions/...
chrome/browser/extensions/activity_log.cc:210:
ActivityLog::ACTIVITY_EXTENSION_API_CALL,
This should be ACTIVITY_EXTENSION_EVENT_DISPATCH

[felt, 2013-01-22 23:02:11]

https://codereview.chromium.org/11946028/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/api_actions.cc (right):

https://codereview.chromium.org/11946028/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api_actions.cc:18: "api_category LONGVARCHAR NOT NULL,
"
I think that the schema change is good.

https://codereview.chromium.org/11946028/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api_actions.cc:45: " api_call, extra) VALUES
(?,?,?,?,?,?,?)";
What happens if you had the table created with the old version (without
api_category) and then run this code? (You'll be missing a column-- probably
leading to a crash here.) Perhaps it would make sense for this change (and any
future schema change) to have a check in ActivityDatabase->InitializeTable to
make sure all the columns are present. Maybe SQL::Connection gives a built in
way to do this?

[mvrable, 2013-01-23 16:54:07]

Adrienne: Thanks for taking a look over the code.  If you have more comments or
suggestions please let me know.

Eric: Would you be able to take a look at this patch which adds logging of
event-related extension activity?  I figure that you should take an initial
look, and if it looks fine to you I'll add other reviewers as needed (not sure
who that will be yet).

--Michael Vrable

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

https://codereview.chromium.org/11946028/diff/7005/chrome/browser/extensions/...
chrome/browser/extensions/activity_log.cc:176:
ActivityLog::ACTIVITY_EVENT_DISPATCH,
On 2013/01/22 22:45:52, felt wrote:
> This should be ACTIVITY_EXTENSION_API_CALL

Thanks for catching this--I was having trouble figuring out why the values were
swapped around in the UI.

https://codereview.chromium.org/11946028/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/api_actions.cc (right):

https://codereview.chromium.org/11946028/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api_actions.cc:45: " api_call, extra) VALUES
(?,?,?,?,?,?,?)";
On 2013/01/22 23:02:11, felt wrote:
> What happens if you had the table created with the old version (without
> api_category) and then run this code? (You'll be missing a column-- probably
> leading to a crash here.) Perhaps it would make sense for this change (and any
> future schema change) to have a check in ActivityDatabase->InitializeTable to
> make sure all the columns are present. Maybe SQL::Connection gives a built in
> way to do this? 

I've added some code to update the database schema if the new column is missing.
 I've put this in a new method in api_actions.cc, but if you'd rather I can
inline the code in activity_database.cc.

[felt, 2013-01-23 17:54:06]

lgtm

[Eric Dingle, 2013-01-24 16:16:34]

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

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:216: LOG(ERROR) << "Unknown API call!
" << name;
Is this the correct error message for this function?

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

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:40: ACTIVITY_EVENT_DISPATCH,       //
Event sent to listener in extension.
Looking back, it would have been nice if those first two enum values were just
ACTIVITY_API_(CALL|BLOCK). Oh well. Clean up change for later.

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:76: // This will crate an APIActiion
for storage in the database.
*create
*APIAction

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:79: const ListValue* args,      //
arguments to the callback
All comments should begin capitalized and end with a period.

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:85: const std::string& blocked_call, 
// eg tabs.get
Be consistent with your e.g.,

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
File chrome/browser/extensions/api_actions.cc (right):

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/api_actions.cc:51: // Old versions of the table lack
the api_category column.  Add it if
This seems lousy to me. What if you want to add another column in the future? Is
it not possible to check the existing db's schema with the schema above, and
just blow away the table and start from scratch if the schemas are different?
Losing the data would be bad, but at least that would let you change the schema
over time without having to do checks like this.

Then again, maybe you can provide a default value for all columns so that you
can alter the table each time you add a new column instead of checking each
column one by one.

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
File chrome/browser/extensions/api_actions.h (right):

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/api_actions.h:18: // TODO(mvrable): Come up with a
good name for this; I'm not entirely happy
Maybe use APIActionType here and use APIActionVerb below? Also, prefixing them
with APIAction seems to make them verbose. They're already nested inside the
APIAction class, so maybe you can just omit it: Type, Verb, Target.

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/api_actions.h:108: 
You can remove this extra blank line.

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
File chrome/browser/extensions/event_router.cc (right):

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/event_router.cc:104:
->extension_service()->extensions()->GetByID(extension_id);
I *think* that the first -> should be up on the previous line, but I can't quite
remember. Have you seen examples of one way or another?

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/event_router.cc:105:
extensions::ActivityLog::GetInstance(profile)
I would format this as:

    extensions::ActivityLog::GetInstance(profile)->LogEventAction(
        extension, event_name, event_args.get(), "");

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/event_router.cc:120:
scoped_ptr<ListValue>(event_args->DeepCopy()));
You should indent this line +4 to be aligned with above.

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
File chrome/browser/extensions/event_router.h (right):

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/event_router.h:239: ActivityLog* activity_logger_;
For consistency, this should be named activity_log_.

[mvrable, 2013-01-24 18:12:31]

Thanks for the feedback; I've uploaded some changes to the code.  This is a
first pass.  There are a few bits left to do--I haven't renamed any of the API
types since I want to check with Adrienne first, but I'll let you know when all
changes have been made.

--Michael Vrable

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

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:216: LOG(ERROR) << "Unknown API call!
" << name;
On 2013/01/24 16:16:34, Eric Dingle wrote:
> Is this the correct error message for this function?

Done.

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

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:76: // This will crate an APIActiion
for storage in the database.
On 2013/01/24 16:16:34, Eric Dingle wrote:
> *create
> *APIAction

Done.

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:79: const ListValue* args,      //
arguments to the callback
On 2013/01/24 16:16:34, Eric Dingle wrote:
> All comments should begin capitalized and end with a period.

Is that necessary even here?  There isn't much room at the end of the line.  We
could put these comments on a separate line, though the style guide mentions
"Shorter comments, such as comments at the end of a line of code, can sometimes
be less formal, but you should be consistent with your style"
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Punctu...).
 But I'll change these if you want.

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:85: const std::string& blocked_call, 
// eg tabs.get
On 2013/01/24 16:16:34, Eric Dingle wrote:
> Be consistent with your e.g.,

Done.

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
File chrome/browser/extensions/api_actions.cc (right):

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/api_actions.cc:51: // Old versions of the table lack
the api_category column.  Add it if
On 2013/01/24 16:16:34, Eric Dingle wrote:
> This seems lousy to me. What if you want to add another column in the future?
Is
> it not possible to check the existing db's schema with the schema above, and
> just blow away the table and start from scratch if the schemas are different?
> Losing the data would be bad, but at least that would let you change the
schema
> over time without having to do checks like this.
> 
> Then again, maybe you can provide a default value for all columns so that you
> can alter the table each time you add a new column instead of checking each
> column one by one.

This is similar to some code in chrome/browser/history/visit_database.cc (in
VisitDatabase::InitVisitTable).  I'm not sure if it's easy to compare the
schema; sql::Connection seems to only provide a DoesColumnExist method, not a
general way to get the schema (to get that I'd need to extend sql::Connection
myself).

If we wanted something generic, I could change kTableStructure from a string
into something structured, and write code to create the table or add columns as
needed.  Whether that is worthwhile depends on how much we expect the schema to
be changing--if there are few changes, one-off changes like this might be
simpler, but I don't mind writing the more generic code if you think that is
preferable.

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
File chrome/browser/extensions/api_actions.h (right):

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/api_actions.h:18: // TODO(mvrable): Come up with a
good name for this; I'm not entirely happy
On 2013/01/24 16:16:34, Eric Dingle wrote:
> Maybe use APIActionType here and use APIActionVerb below? Also, prefixing them
> with APIAction seems to make them verbose. They're already nested inside the
> APIAction class, so maybe you can just omit it: Type, Verb, Target.

That sounds good to me.  Adrienne, are you fine with me making that change?

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/api_actions.h:108: 
On 2013/01/24 16:16:34, Eric Dingle wrote:
> You can remove this extra blank line.

Done (and fixed a couple of other files too).

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
File chrome/browser/extensions/event_router.cc (right):

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/event_router.cc:104:
->extension_service()->extensions()->GetByID(extension_id);
On 2013/01/24 16:16:34, Eric Dingle wrote:
> I *think* that the first -> should be up on the previous line, but I can't
quite
> remember. Have you seen examples of one way or another?

I think you are right.  I did a quick search and found one example in the
extensions directory of a -> at the start of a line, and many examples at the
end of a line.  I've changed my code to match that style.

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/event_router.cc:105:
extensions::ActivityLog::GetInstance(profile)
On 2013/01/24 16:16:34, Eric Dingle wrote:
> I would format this as:
> 
>     extensions::ActivityLog::GetInstance(profile)->LogEventAction(
>         extension, event_name, event_args.get(), "");

Done.

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/event_router.cc:120:
scoped_ptr<ListValue>(event_args->DeepCopy()));
On 2013/01/24 16:16:34, Eric Dingle wrote:
> You should indent this line +4 to be aligned with above.

Done.

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
File chrome/browser/extensions/event_router.h (right):

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/event_router.h:239: ActivityLog* activity_logger_;
On 2013/01/24 16:16:34, Eric Dingle wrote:
> For consistency, this should be named activity_log_.

Done.

[felt, 2013-01-24 18:15:14]

renaming sounds fine, it'll make the different actions more consistently named.

On 2013/01/24 18:12:31, mvrable wrote:
> Thanks for the feedback; I've uploaded some changes to the code.  This is a
> first pass.  There are a few bits left to do--I haven't renamed any of the API
> types since I want to check with Adrienne first, but I'll let you know when
all
> changes have been made.
> 
> --Michael Vrable
> 
>
https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
> File chrome/browser/extensions/activity_log.cc (right):
> 
>
https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
> chrome/browser/extensions/activity_log.cc:216: LOG(ERROR) << "Unknown API
call!
> " << name;
> On 2013/01/24 16:16:34, Eric Dingle wrote:
> > Is this the correct error message for this function?
> 
> Done.
> 
>
https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
> File chrome/browser/extensions/activity_log.h (right):
> 
>
https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
> chrome/browser/extensions/activity_log.h:76: // This will crate an APIActiion
> for storage in the database.
> On 2013/01/24 16:16:34, Eric Dingle wrote:
> > *create
> > *APIAction
> 
> Done.
> 
>
https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
> chrome/browser/extensions/activity_log.h:79: const ListValue* args,      //
> arguments to the callback
> On 2013/01/24 16:16:34, Eric Dingle wrote:
> > All comments should begin capitalized and end with a period.
> 
> Is that necessary even here?  There isn't much room at the end of the line. 
We
> could put these comments on a separate line, though the style guide mentions
> "Shorter comments, such as comments at the end of a line of code, can
sometimes
> be less formal, but you should be consistent with your style"
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Punctu...).
>  But I'll change these if you want.
> 
>
https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
> chrome/browser/extensions/activity_log.h:85: const std::string& blocked_call, 
> // eg tabs.get
> On 2013/01/24 16:16:34, Eric Dingle wrote:
> > Be consistent with your e.g.,
> 
> Done.
> 
>
https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
> File chrome/browser/extensions/api_actions.cc (right):
> 
>
https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
> chrome/browser/extensions/api_actions.cc:51: // Old versions of the table lack
> the api_category column.  Add it if
> On 2013/01/24 16:16:34, Eric Dingle wrote:
> > This seems lousy to me. What if you want to add another column in the
future?
> Is
> > it not possible to check the existing db's schema with the schema above, and
> > just blow away the table and start from scratch if the schemas are
different?
> > Losing the data would be bad, but at least that would let you change the
> schema
> > over time without having to do checks like this.
> > 
> > Then again, maybe you can provide a default value for all columns so that
you
> > can alter the table each time you add a new column instead of checking each
> > column one by one.
> 
> This is similar to some code in chrome/browser/history/visit_database.cc (in
> VisitDatabase::InitVisitTable).  I'm not sure if it's easy to compare the
> schema; sql::Connection seems to only provide a DoesColumnExist method, not a
> general way to get the schema (to get that I'd need to extend sql::Connection
> myself).
> 
> If we wanted something generic, I could change kTableStructure from a string
> into something structured, and write code to create the table or add columns
as
> needed.  Whether that is worthwhile depends on how much we expect the schema
to
> be changing--if there are few changes, one-off changes like this might be
> simpler, but I don't mind writing the more generic code if you think that is
> preferable.
> 
>
https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
> File chrome/browser/extensions/api_actions.h (right):
> 
>
https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
> chrome/browser/extensions/api_actions.h:18: // TODO(mvrable): Come up with a
> good name for this; I'm not entirely happy
> On 2013/01/24 16:16:34, Eric Dingle wrote:
> > Maybe use APIActionType here and use APIActionVerb below? Also, prefixing
them
> > with APIAction seems to make them verbose. They're already nested inside the
> > APIAction class, so maybe you can just omit it: Type, Verb, Target.
> 
> That sounds good to me.  Adrienne, are you fine with me making that change?
> 
>
https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
> chrome/browser/extensions/api_actions.h:108: 
> On 2013/01/24 16:16:34, Eric Dingle wrote:
> > You can remove this extra blank line.
> 
> Done (and fixed a couple of other files too).
> 
>
https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
> File chrome/browser/extensions/event_router.cc (right):
> 
>
https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
> chrome/browser/extensions/event_router.cc:104:
> ->extension_service()->extensions()->GetByID(extension_id);
> On 2013/01/24 16:16:34, Eric Dingle wrote:
> > I *think* that the first -> should be up on the previous line, but I can't
> quite
> > remember. Have you seen examples of one way or another?
> 
> I think you are right.  I did a quick search and found one example in the
> extensions directory of a -> at the start of a line, and many examples at the
> end of a line.  I've changed my code to match that style.
> 
>
https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
> chrome/browser/extensions/event_router.cc:105:
> extensions::ActivityLog::GetInstance(profile)
> On 2013/01/24 16:16:34, Eric Dingle wrote:
> > I would format this as:
> > 
> >     extensions::ActivityLog::GetInstance(profile)->LogEventAction(
> >         extension, event_name, event_args.get(), "");
> 
> Done.
> 
>
https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
> chrome/browser/extensions/event_router.cc:120:
> scoped_ptr<ListValue>(event_args->DeepCopy()));
> On 2013/01/24 16:16:34, Eric Dingle wrote:
> > You should indent this line +4 to be aligned with above.
> 
> Done.
> 
>
https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
> File chrome/browser/extensions/event_router.h (right):
> 
>
https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
> chrome/browser/extensions/event_router.h:239: ActivityLog* activity_logger_;
> On 2013/01/24 16:16:34, Eric Dingle wrote:
> > For consistency, this should be named activity_log_.
> 
> Done.

[Eric Dingle, 2013-01-24 18:56:56]

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

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/activity_log.h:79: const ListValue* args,      //
arguments to the callback
Fine to leave as is.

On 2013/01/24 18:12:31, mvrable wrote:
> On 2013/01/24 16:16:34, Eric Dingle wrote:
> > All comments should begin capitalized and end with a period.
> 
> Is that necessary even here?  There isn't much room at the end of the line. 
We
> could put these comments on a separate line, though the style guide mentions
> "Shorter comments, such as comments at the end of a line of code, can
sometimes
> be less formal, but you should be consistent with your style"
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Punctu...).
>  But I'll change these if you want.

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
File chrome/browser/extensions/api_actions.cc (right):

https://codereview.chromium.org/11946028/diff/20001/chrome/browser/extensions...
chrome/browser/extensions/api_actions.cc:51: // Old versions of the table lack
the api_category column.  Add it if
If it's already done elsewhere, then I'm okay with it. It's just hard to know
when it's okay to remove it. Too bad it's not easier to detect schema
differences and adapt accordingly.

On 2013/01/24 18:12:31, mvrable wrote:
> On 2013/01/24 16:16:34, Eric Dingle wrote:
> > This seems lousy to me. What if you want to add another column in the
future?
> Is
> > it not possible to check the existing db's schema with the schema above, and
> > just blow away the table and start from scratch if the schemas are
different?
> > Losing the data would be bad, but at least that would let you change the
> schema
> > over time without having to do checks like this.
> > 
> > Then again, maybe you can provide a default value for all columns so that
you
> > can alter the table each time you add a new column instead of checking each
> > column one by one.
> 
> This is similar to some code in chrome/browser/history/visit_database.cc (in
> VisitDatabase::InitVisitTable).  I'm not sure if it's easy to compare the
> schema; sql::Connection seems to only provide a DoesColumnExist method, not a
> general way to get the schema (to get that I'd need to extend sql::Connection
> myself).
> 
> If we wanted something generic, I could change kTableStructure from a string
> into something structured, and write code to create the table or add columns
as
> needed.  Whether that is worthwhile depends on how much we expect the schema
to
> be changing--if there are few changes, one-off changes like this might be
> simpler, but I don't mind writing the more generic code if you think that is
> preferable.

[Eric Dingle, 2013-01-24 18:57:30]

lgtm

[mvrable, 2013-01-24 19:01:45]

One more update, this one renames the types in APIAction to shorten the names,
and changes APICategory->Type and APIActionType->Verb.

[Eric Dingle, 2013-01-24 19:02:48]

lgtm

[mvrable, 2013-01-24 20:18:46]

Matt,

Would you be able to review a patch for me?  This is an extension to the Chrome
extension activity log work by ericdingle@ and felt@.  I'm adding logging
activities related to events in extensions (for exampling,
chrome.tabs.onUpdated), both the registering listeners and when events are
delivered.  Eric and Adrienne have already looked the patch over, but whatever
feedback you have is appreciated.  Thanks for the help!

--Michael Vrable

[Matt Perry, 2013-01-25 21:24:17]

https://codereview.chromium.org/11946028/diff/24010/chrome/browser/extensions...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/11946028/diff/24010/chrome/browser/extensions...
chrome/browser/extensions/api/web_request/web_request_api.cc:1081:
(*it)->ipc_sender.get(), static_cast<Profile*>(profile),
There's a reason that |profile| is a void* - so you don't end up dereferencing
it like this :). This is on the IO thread. Even though you're pinging it over to
the UI thread eventually, it's not safe because there's no guarantee the Profile
hasn't been deleted by the time you dereference it.

See
https://code.google.com/p/chromium/codesearch#chrome/src/chrome/browser/net/c...
for the (hacky) way to handle this case. I'd prefer you keep the profile as a
void* until it makes its way all the way to the UI thread.

https://codereview.chromium.org/11946028/diff/24010/chrome/browser/extensions...
File chrome/browser/extensions/event_router.cc (right):

https://codereview.chromium.org/11946028/diff/24010/chrome/browser/extensions...
chrome/browser/extensions/event_router.cc:134:
scoped_ptr<ListValue>(event_args->DeepCopy()));
Can we only do this copying if activity logging is enabled?

[mvrable, 2013-01-28 16:27:36]

Thanks for the suggestions for code improvements.  I've uploaded a new version
of the patch--can you let me know what you think?

--Michael Vrable

https://codereview.chromium.org/11946028/diff/24010/chrome/browser/extensions...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/11946028/diff/24010/chrome/browser/extensions...
chrome/browser/extensions/api/web_request/web_request_api.cc:1081:
(*it)->ipc_sender.get(), static_cast<Profile*>(profile),
On 2013/01/25 21:24:17, Matt Perry wrote:
> There's a reason that |profile| is a void* - so you don't end up dereferencing
> it like this :). This is on the IO thread. Even though you're pinging it over
to
> the UI thread eventually, it's not safe because there's no guarantee the
Profile
> hasn't been deleted by the time you dereference it.
> 
> See
>
https://code.google.com/p/chromium/codesearch#chrome/src/chrome/browser/net/c...
> for the (hacky) way to handle this case. I'd prefer you keep the profile as a
> void* until it makes its way all the way to the UI thread.

Fixed I think--does the new version look all right?

https://codereview.chromium.org/11946028/diff/24010/chrome/browser/extensions...
File chrome/browser/extensions/event_router.cc (right):

https://codereview.chromium.org/11946028/diff/24010/chrome/browser/extensions...
chrome/browser/extensions/event_router.cc:134:
scoped_ptr<ListValue>(event_args->DeepCopy()));
On 2013/01/25 21:24:17, Matt Perry wrote:
> Can we only do this copying if activity logging is enabled?

Done, if the updated code looks good to you.

[Matt Perry, 2013-01-28 21:18:58]

lgtm

[mvrable-google, 2013-01-28 21:24:25]

FYI: I'm current trying to track down a bug (segfault in the unit tests
calling ProfileManager::IsValidProfile) that shows up on the tryserver but
not on my local machine.  I have an idea, though, so hopefully I'll be able
to reproduce it shortly (when I get back to my desk) and work on fixing it.

--Michael Vrable

[mvrable, 2013-01-29 16:05:05]

mpcomplete: I made a couple of updates to the WebRequest unit tests, to ensure
that a testing version of ProfileManager is initialized so calls to
g_browser_process->profile_manager()->IsValidProfile(profile) will work.  That
seems to fix the issue I was seeing on the tryserver (a couple tests are failing
due to what seem to be unrelated compiler errors but I'm retrying).  How does
the current version of this patch look to you?

estade: Most of this patch has been reviewed, but would you be willing to look
at the files in chrome/browser/ui/webui and chrome/browser/resources/extensions?
 The changes there are very minor.

If the patch looks fine to everyone I'll submit through the commit queue.

[mvrable, 2013-01-29 18:45:09]

One more update to the patch set to fix trybot failures.  It turns out that some
Windows headers include a
  #define IsLoggingEnabled IsLoggingEnabledW
so we can't use the name IsLoggingEnabled in our code portably.  I've renamed
the method.  Hopefully this gets the patch into a working state now.

[Matt Perry, 2013-01-29 20:34:56]

lgtm

[mvrable, 2013-01-31 15:53:40]

I just discovered that estade looks to be OOO this week.

csilv: Would you be able to review the two files in chrome/browser/ui/webui and
chrome/browser/resources/extensions?  It's a total of four lines and very
trivial, but I need some owner to take a look.

[csilv, 2013-01-31 18:07:38]

webui lgtm

[commit-bot: I haz the power, 2013-01-31 18:10:23]

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

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

Change committed as 179945