Attaching a DOM Activity Logger to all extension scripts

Attaching a DOM Activity Logger to all extension scripts

Adds a mechanism for attaching (using the WebKit API) a DOMActivityLogger
to each world that is executing extension scripts. (This would include the
main world in the case of an extension background page.) A logger is only 
attached if extension activity logging is enabled and if there does not 
already exist a logger attached to the world. Each logger object has a log
method that marshalls it's arguments and sends them over to the extension
activity log on the browser side. A HashMap in WebCore owns the logger
objects. Currently the loggers are not invoked, a later WebCore patch will
appropriately configure the DOM bindings for each world so that they invoke
the corresponding logger object on every access.

BUG=160987, 160989
TBR=brettw@chromium.org

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

[Matt Perry, 2013-03-05 02:32:37]

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/di...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/di...
chrome/renderer/extensions/dispatcher.cc:919: // DOMActivity loggers for an
extension background page get an empty title.
FYI this happens on any type of extension page (like popups, or tabs navigated
to a chrome-extension URL), not just background pages.

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/do...
File chrome/renderer/extensions/dom_activity_logger.cc (right):

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/do...
chrome/renderer/extensions/dom_activity_logger.cc:34: i,
converter->FromV8Value(args[i], v8::Context::GetCurrent()));
We're trying to get away from using v8::Context::GetCurrent(). Can you pass the
context in? Or even better, the DOMActivityLogger should have a reference to the
v8::Context it is attached to.

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/do...
File chrome/renderer/extensions/dom_activity_logger.h (right):

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/do...
chrome/renderer/extensions/dom_activity_logger.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
2013

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/do...
chrome/renderer/extensions/dom_activity_logger.h:20: string16 title);
pass these all by const ref

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/do...
chrome/renderer/extensions/dom_activity_logger.h:25: virtual void log(const
char* api_name,
This is chrome code, so use chrome style naming (e.g. Log).

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/do...
chrome/renderer/extensions/dom_activity_logger.h:28: const char* extra_info);
also, use std::string instead of const char*

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/do...
chrome/renderer/extensions/dom_activity_logger.h:37: string16 title);
const ref

[Ankur Taly, 2013-03-05 20:03:31]

Hi Matt,
Thanks a lot for the feedback.

To address some of the comments below, I will have to change the
WebDOMActivityLogger class declaration in WebKit. I will first do that and then
update this patch.

Best,
Ankur.

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/di...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/di...
chrome/renderer/extensions/dispatcher.cc:919: // DOMActivity loggers for an
extension background page get an empty title.
Yes and I intend to cover all such cases. I assume all these cases are ones
where the extension controls the main world, right?
On 2013/03/05 02:32:37, Matt Perry wrote:
> FYI this happens on any type of extension page (like popups, or tabs navigated
> to a chrome-extension URL), not just background pages.

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/do...
File chrome/renderer/extensions/dom_activity_logger.cc (right):

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/do...
chrome/renderer/extensions/dom_activity_logger.cc:34: i,
converter->FromV8Value(args[i], v8::Context::GetCurrent()));
I can change the log method in WebKit::WebDOMActivityLogger to also take in a
context argument. 
I cannot have a reference to the context in the DOMActivityLogger on the chrome
side as it gets created before contexts are initialized for the corresponding
world.

On 2013/03/05 02:32:37, Matt Perry wrote:
> We're trying to get away from using v8::Context::GetCurrent(). Can you pass
the
> context in? Or even better, the DOMActivityLogger should have a reference to
the
> v8::Context it is attached to.

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/do...
File chrome/renderer/extensions/dom_activity_logger.h (right):

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/do...
chrome/renderer/extensions/dom_activity_logger.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/03/05 02:32:37, Matt Perry wrote:
> 2013

Done.

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/do...
chrome/renderer/extensions/dom_activity_logger.h:20: string16 title);
On 2013/03/05 02:32:37, Matt Perry wrote:
> pass these all by const ref

Done.

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/do...
chrome/renderer/extensions/dom_activity_logger.h:25: virtual void log(const
char* api_name,
This method is supposed to override the "log" method in
WebKit::WebDOMActivityLogger and therefore follows the WebKit convention. I put
in a comment saying this.

On 2013/03/05 02:32:37, Matt Perry wrote:
> This is chrome code, so use chrome style naming (e.g. Log).

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/do...
chrome/renderer/extensions/dom_activity_logger.h:28: const char* extra_info);
This would require changing the "log" method declaration in WebKit. My plan is
to change the log method arguments to be of type WebString, from which I can
then extract std::string values and send them over to the activity log. 
On 2013/03/05 02:32:37, Matt Perry wrote:
> also, use std::string instead of const char*

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/do...
chrome/renderer/extensions/dom_activity_logger.h:37: string16 title);
On 2013/03/05 02:32:37, Matt Perry wrote:
> const ref

Done.

[Matt Perry, 2013-03-05 20:34:22]

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/di...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/di...
chrome/renderer/extensions/dispatcher.cc:919: // DOMActivity loggers for an
extension background page get an empty title.
Yes. (You should update the comment, though.)

On 2013/03/05 20:03:32, ataly1 wrote:
> Yes and I intend to cover all such cases. I assume all these cases are ones
> where the extension controls the main world, right?
> On 2013/03/05 02:32:37, Matt Perry wrote:
> > FYI this happens on any type of extension page (like popups, or tabs
navigated
> > to a chrome-extension URL), not just background pages.
>

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/do...
File chrome/renderer/extensions/dom_activity_logger.cc (right):

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/do...
chrome/renderer/extensions/dom_activity_logger.cc:34: i,
converter->FromV8Value(args[i], v8::Context::GetCurrent()));
On 2013/03/05 20:03:32, ataly1 wrote:
> I can change the log method in WebKit::WebDOMActivityLogger to also take in a
> context argument. 

Sounds good.

> I cannot have a reference to the context in the DOMActivityLogger on the
chrome
> side as it gets created before contexts are initialized for the corresponding
> world.
> 
> On 2013/03/05 02:32:37, Matt Perry wrote:
> > We're trying to get away from using v8::Context::GetCurrent(). Can you pass
> the
> > context in? Or even better, the DOMActivityLogger should have a reference to
> the
> > v8::Context it is attached to.
>

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/do...
File chrome/renderer/extensions/dom_activity_logger.h (right):

https://codereview.chromium.org/12390076/diff/1/chrome/renderer/extensions/do...
chrome/renderer/extensions/dom_activity_logger.h:25: virtual void log(const
char* api_name,
Oops, I missed that this class overrode a WebKit API.

On 2013/03/05 20:03:32, ataly1 wrote:
> This method is supposed to override the "log" method in
> WebKit::WebDOMActivityLogger and therefore follows the WebKit convention. I
put
> in a comment saying this.
> 
> On 2013/03/05 02:32:37, Matt Perry wrote:
> > This is chrome code, so use chrome style naming (e.g. Log).
>

[Ankur Taly, 2013-03-07 01:03:39]

Hi Matt,

The WebKit patch got committed, it will become part of the chrome tree in a day
or so. I have accordingly changed the chrome patch and addressed your comments.
Could you please take a look?

Thanks,
Ankur.

[Matt Perry, 2013-03-07 01:06:26]

lgtm

https://codereview.chromium.org/12390076/diff/21001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/12390076/diff/21001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:716:
lazy_bindings_map_["webactivitystore"] = InstallWebstoreBindings;
?

[Ankur Taly, 2013-03-07 01:50:26]

Oops that was unintended. I fixed that.
I will send this to the commit queue once the webkit patch lands.

Ankur.

[commit-bot: I haz the power, 2013-03-07 20:11:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ataly@chromium.org/12390076/28003

[commit-bot: I haz the power, 2013-03-07 20:11:15]

Presubmit check for 12390076-28003 failed and returned exit status 1.

INFO:root:Found 6 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/chrome_renderer.gypi

Presubmit checks took 4.1s to calculate.

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

[commit-bot: I haz the power, 2013-03-07 20:30:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ataly@chromium.org/12390076/28003

[commit-bot: I haz the power, 2013-03-08 01:57:53]

Change committed as 186845