Improved extension activity logging for the chrome.webRequest API.

Improved extension activity logging for the chrome.webRequest API.

Add a new type of log item in the activitylog_urls table: WEBREQUEST,
used to summarize the changes made to an HTTP request using the
blocking WebRequest extension API.  The types of modifications made
are always logged when the extension activity log is enabled; the
details of the modification are only kept if the activity log testing
flag is enabled (as the details may contain sensitive data).

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

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

[mvrable, 2013-04-11 19:02:26]

This is an update to some of the extension activity log code to improve our
logging of HTTP request modifications made via the WebRequest API.  Would you be
able to take a look at it?

--Michael Vrable

[felt, 2013-04-11 21:13:37]

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

https://codereview.chromium.org/12491012/diff/16001/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:368: if (!log_arguments_) {
just a heads up: if issue 13726026 lands first this variable is going to change.
(no action needed now, just telling you)

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

https://codereview.chromium.org/12491012/diff/16001/chrome/browser/extensions...
chrome/browser/extensions/api/web_request/web_request_api.cc:1488: {
is this meant to be an if block?

[mvrable, 2013-04-11 21:41:26]

One small update to the code (based on Adrienne's comments), and a question of
my own.

--Michael Vrable

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

https://codereview.chromium.org/12491012/diff/16001/chrome/browser/extensions...
chrome/browser/extensions/activity_log.cc:368: if (!log_arguments_) {
On 2013/04/11 21:13:37, felt wrote:
> just a heads up: if issue 13726026 lands first this variable is going to
change.
> (no action needed now, just telling you)

Thanks for the advance notice; I'll fix this before commit if your code goes in
first.

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

https://codereview.chromium.org/12491012/diff/16001/chrome/browser/extensions...
chrome/browser/extensions/api/web_request/web_request_api.cc:1488: {
On 2013/04/11 21:13:37, felt wrote:
> is this meant to be an if block?

No, it was keeping local variables scoped more tightly.  But that probably isn't
necessary (the code is pretty simple now...I think there used to be more to it).
 I'll drop the extra braces.

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

https://codereview.chromium.org/12491012/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/api/web_request/web_request_api.cc:1482:
details->SetBoolean("cancel", true);
I'm wondering whether I should define some constants for these strings, rather
than inlining them.  Much of the code in this file refers to constants in
web_request_api_constants.cc, but I don't think that's appropriate here since
these values aren't part of the WebRequest API--they seem more a part of the
activity log.

[felt, 2013-04-11 21:43:28]

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

https://codereview.chromium.org/12491012/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/api/web_request/web_request_api.cc:1482:
details->SetBoolean("cancel", true);
You could consider adding a helper method to one of the Action classes, along
with relevant constants?

[Matt Perry, 2013-04-12 00:53:43]

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

https://codereview.chromium.org/12491012/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/api/web_request/web_request_api.cc:1592:
SummarizeResponseDelta(event_name, *delta));
This looks somewhat expensive. Ideally we wouldn't do this if logging were
disabled. We're on the IO thread here. Can we check whether logging is enabled
from the IO thread?

[mvrable, 2013-04-12 18:50:49]

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

https://codereview.chromium.org/12491012/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/api/web_request/web_request_api.cc:1482:
details->SetBoolean("cancel", true);
On 2013/04/11 21:43:28, felt wrote:
> You could consider adding a helper method to one of the Action classes, along
> with relevant constants?

I ended up creating a new file in the web_request directory for these; it does
keeps from cluttering the top-level extensions directory too much (no existing
file looked appropriate), but keeps the constants separate from the
web_request_api.cc file.

I can easily relocate the constants if somewhere else is better.

https://codereview.chromium.org/12491012/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/api/web_request/web_request_api.cc:1592:
SummarizeResponseDelta(event_name, *delta));
On 2013/04/12 00:53:43, Matt Perry wrote:
> This looks somewhat expensive. Ideally we wouldn't do this if logging were
> disabled. We're on the IO thread here. Can we check whether logging is enabled
> from the IO thread?

Done.

[Matt Perry, 2013-04-12 19:09:18]

LGTM except for comment below

https://codereview.chromium.org/12491012/diff/30001/chrome/browser/extensions...
File
chrome/browser/extensions/api/web_request/web_request_activitylog_constants.h
(right):

https://codereview.chromium.org/12491012/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/web_request/web_request_activitylog_constants.h:1:
// Copyright (c) 2013 The Chromium Authors. All rights reserved.
Sorry to be a pain, but I think these files actually belong next to the other
activity_log files. activity_log_web_request_constants.h

Speaking of which, maybe it's time to move activity_log into its own directory.

[mvrable, 2013-04-12 19:54:20]

https://codereview.chromium.org/12491012/diff/30001/chrome/browser/extensions...
File
chrome/browser/extensions/api/web_request/web_request_activitylog_constants.h
(right):

https://codereview.chromium.org/12491012/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/web_request/web_request_activitylog_constants.h:1:
// Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2013/04/12 19:09:18, Matt Perry wrote:
> Sorry to be a pain, but I think these files actually belong next to the other
> activity_log files. activity_log_web_request_constants.h
> 
> Speaking of which, maybe it's time to move activity_log into its own
directory.

I relocated these files an updated the patch.  I'm just moving it with the other
activity log files...if those get relocated, that will happen separately.

[Matt Perry, 2013-04-12 19:55:55]

lgtm

https://codereview.chromium.org/12491012/diff/30001/chrome/browser/extensions...
File
chrome/browser/extensions/api/web_request/web_request_activitylog_constants.h
(right):

https://codereview.chromium.org/12491012/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/web_request/web_request_activitylog_constants.h:1:
// Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2013/04/12 19:54:21, mvrable wrote:
> On 2013/04/12 19:09:18, Matt Perry wrote:
> > Sorry to be a pain, but I think these files actually belong next to the
other
> > activity_log files. activity_log_web_request_constants.h
> > 
> > Speaking of which, maybe it's time to move activity_log into its own
> directory.
> 
> I relocated these files an updated the patch.  I'm just moving it with the
other
> activity log files...if those get relocated, that will happen separately.

Yep, sounds good. Thanks!

[mvrable, 2013-04-12 20:06:37]

brettw,

I added two files as part of some extension activity log work.  felt@ and
mpcomplete@ have reviewed most of the work, but I think I need another approver
for the changes to chrome/chrome_browser_extensions.gypi.  Could you take a look
at that file and approve if it looks good to you?

Thanks.

--Michael Vrable

[mvrable, 2013-04-15 19:10:08]

I made one very minor change: updated the code for a member variable that was
renamed in revision 194034 by felt.  No other changes.

brettw: Could you review the one .gypi file and approve if the change looks
good?

--Michael Vrable

[felt, 2013-04-15 19:12:59]

lgtm

On 2013/04/15 19:10:08, mvrable wrote:
> I made one very minor change: updated the code for a member variable that was
> renamed in revision 194034 by felt.  No other changes.
> 
> brettw: Could you review the one .gypi file and approve if the change looks
> good?
> 
> --Michael Vrable

[mvrable, 2013-04-18 18:35:32]

Ben,

This patch is ready to be submitted, except for a review for a two-line change
to a .gypi file.  Would you be able to review the changes in
chrome/chrome_browser_extensions.gypi give an LGTM if they look fine?

--Michael Vrable

[Ben Goodger (Google), 2013-04-19 16:15:47]

lgtm

[commit-bot: I haz the power, 2013-04-19 16:44:26]

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

[commit-bot: I haz the power, 2013-04-19 16:46:35]

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

[commit-bot: I haz the power, 2013-04-19 17:03:11]

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

[commit-bot: I haz the power, 2013-04-19 17:09:51]

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

[commit-bot: I haz the power, 2013-04-19 17:58:24]

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

[commit-bot: I haz the power, 2013-04-19 20:34:14]

Change committed as 195265

[commit-bot: I haz the power, 2013-04-19 22:24:49]

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

[mvrable, 2013-04-19 22:27:09]

Change was reverted due to a memory leak; updating and resubmitting.

[commit-bot: I haz the power, 2013-04-19 23:08:01]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-19 23:12:02]

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

[commit-bot: I haz the power, 2013-04-20 02:06:35]

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

[commit-bot: I haz the power, 2013-04-20 18:49:16]

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

[commit-bot: I haz the power, 2013-04-20 19:01:19]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-20 20:38:02]

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

[commit-bot: I haz the power, 2013-04-20 20:46:54]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-22 17:00:22]

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

[commit-bot: I haz the power, 2013-04-22 21:16:29]

Change committed as 195596