Implements extensions devtools API

Exposes a chrome.devtools object to extensions.  This allows extensions to call chrome.devtools.connect() to open up a Port by which it can receive devtools messages to implement the proposed perf trace extensions API documented here:

http://code.google.com/p/chromium/wiki/ExtensionsPerfTraceAPI

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=24158

[jamesr1, 2009-08-05 01:51:41]

This patch implements a basic devtools<->extensions bridge.  The issue
description describes the motivation and background - briefly it is for this
extensions API: http://code.google.com/p/chromium/wiki/ExtensionsPerfTraceAPI.

It has a few issues that need to be resolved before it lands but I would like
feedback on the approach and general design.  In its current state it can not
land, but I want to get feedback on the general approach from the extension and
devtools folks.

The first issue is that this connects extensions to _all_ devtools messages, not
just perf trace ones.  It would be easy to implement filtering in the devtools
bridge class once all of the perf trace code has landed.

The second issue is that currently the ExtensionDevToolsBridge class leaks - it
is allocated by new in the ExtensionMessageService class and never deleted.  The
normal channel teardown paths do not do any explicit teardowns of their
endpoints because the endpoints are typically the ResourceMessageFilter for a
renderer process, which are managed separately.  In the devtools case, the
bridge itself needs to be deleted after the port is shut down.  One way to
resolve this would be to make the channel endpoints an interface with an
explicit Shutdown() call rather than IPC::Message::Sender*s, but I would like to
hear what the extension folks think.

[Matt Perry, 2009-08-05 18:39:00]

Sorry I didn't get a chance to comment on the proposed API before now, but this
seems like the wrong approach to me.  You're overloading the messaging system to
provide event notifications, when we already have an event notification system
in place.

What if you structured it like this:
chrome.devtools:
  Event onDomEvent;
  Event onDomEventDispatch;
  ...

and an extension could use it like so:
  chrome.devtools.onDomEvent.addListener(function (data) {
    console.log(data.duration);
  });

If you want fewer entry points for extensions, you could instead (or maybe
additionally) have more generic events:
  chrome.devtools.onPageEvent.addListener(function (type, data) {
    console.log(data.duration);
    if (type == 'DOM_EVENT')
      somethingDomEventSpecific();
  });
For this situation, you could dispatch both onDomEvent and
onPageEvent('DOM_EVENT').  That way extensions could handle either only the
events they care about, or handle all events at once.

[Kelly Norton, 2009-08-06 05:13:07]

Matt,

James is out of town for the rest of the week, so I'm going to take over his
patch for him. Actually, he's out near you guys so he might stop by sometime.

Moving to a more event-based API is fine with us. There were a few reasons we
had made it more port/message like:

#1 - It exposed less actual API. This one probably doesn't buy us anything.
Having the messages that arrive on the port change isn't really much different
than having an event object change. So this reason we can ignore.

#2 - We need to be able to turn on tracing on a tab-by-tab basic, so some kind
of channel abstraction where you connect to the tab and receive messages made
sense from this perspective. We can still do this with events, but it would
require us to add listeners to an event object that is associated with a
particular tab.

#3 - The objects that are returned by the onMessage event are actually trees of
messages. For example, you might receive something like this:
  {
    type: DOM_EVENT,
    ...,
    children: [
      {
        type: DOM_EVENT_DISPATCH,
        ...,
        data: { phase: 'capture' }
     },
     {
       type: DOM_EVENT_DISPATCH,
      ...,
     data: { phase: 'bubble' }
   ]
}
This kind of nesting only happens in page events, so the more generic
onPageEvent style of dispatch would still be possible. I think the finer grain
dispatch would work against the nested structure of the messages/events.

#4 - There has been an ongoing question of whether we should pass in a list of
message types we are interested in when we call connect. I left it out of the
proposal to keep things simple assuming we could just wait to see if we needed
it. However, if we do need to filter, it is something we'll want to do in the
instrumented renderer since it is much better to just turn that part of the
instrumentation off (which causes the nodes in the tree to not get created).
Using the port API, we could have done something like:

chrome.devtools.connect(tab_id, [list of things to include]).

With events that seems a little harder to accomplish. 



Thinking about it purely from an API standpoint, I'm compelled to suggest
something like the following. However, the one thing I haven't properly
investigated is how difficult it will be to deliver events to the Controller
instances. Does any of the existing API use something similar? (I'll also take a
look for myself when I'm back at my desk.)

chrome.devtools

  class Controller { // Controller is not a name I'm particularly attached to.
    int tab_id;
    Event onPageEvent;
    Event onNetworkEvent;
    Event onUrlChangeEvent; // The two meta events could be collapsed into a
single event.
    ...
  }

  Controller get(tab_id); // If we needed filtering, an optional param could be
added here.


Thanks for taking a look at this,
/kelly

[Matt Perry, 2009-08-07 01:35:11]

On 2009/08/06 05:13:07, Kelly Norton wrote:
> #1 - It exposed less actual API. This one probably doesn't buy us anything.
> Having the messages that arrive on the port change isn't really much different
> than having an event object change. So this reason we can ignore.

Agreed, this doesn't seem that important.

> #2 - We need to be able to turn on tracing on a tab-by-tab basic

Good point, but I also like your proposal at the end to address this.

> #3 - The objects that are returned by the onMessage event are actually trees
of
> messages.

I see. However, at the limit this could be addressed by using a single
onDevtoolEvent which was the only entry point.

> #4 - There has been an ongoing question of whether we should pass in a list of
> message types we are interested in when we call connect.

I would almost say this argues for events, if not for your point #3 above. If
the granularity of what you wanted to filter matched up with the granularity of
your event objects, you could use event registration as an implicit signal to
turn on instrumentation.  For example, say in my extension I have:
  chrome.devtools.onDomEvent.addListener();
  chrome.devtools.onNetworkEvent.addListener();
then I would only receive DOM and network events.  Nesting makes this more
difficult, as you wouldn't be able to filter specific children out of a page
event, but maybe that's OK for your use case.

> Using the port API, we could have done something like:
> 
> chrome.devtools.connect(tab_id, [list of things to include]).
> 
> With events that seems a little harder to accomplish. 

Not by much. The browser process already maintains a list of what extensions are
listening to what events. You could monitor that list and notice that extension
X is listening to "onDomEvent" and "onNetworkEvent" for tab_id T, so filter out
everything but those.

The fact that you're targeting a specific tab with these events means your event
names would probably be something like "onDomEvent:7" (where 7 is the tab_id).
We do something similar for channel connect events: they take the form
"channel-connect:extensionId".

All that said, your points do illustrate that our current event system doesn't
fit your use case 100%. I hope I've made the case that it can be made to work
with events without too much effort. In the end, I think implementation
difficulty takes a back seat to what the best interface to extension developers
is. If you think messaging fits that purpose better, I'm OK if you go that
route.

[jamesr1, 2009-08-14 03:59:48]

I've updated the patch to use events instead of message ports, put it behind a
command line flag (off by default), and also renamed it 'timeline' rather than
'perftrace' at Pavel's suggestion.  This patch is still not 100% complete but
since there are some tricky lifetime issues I would appreciate feedback on parts
of the design.

The API pattern now is there is a chrome.devtools.getTab(int tabId) function
which returns an object with a number of events on it that correspond to events
produced from devtools land.  I've updated
http://code.google.com/p/chromium/wiki/ExtensionsPerfTraceAPI partially to
reflect the change - I'll finish updating that wiki page in the morning when I'm
better rested.

I haven't marked the API as 'experimental' in the docs because I can't quite
figure out how the doc generating stuff works (it appears broken on OS X?), I
will follow up offline about that.

Also it is very important to note that as of right now the timeline agent has
not landed in WebKit or chromium and so if this patch was committed today no
events would actually be fired.  That work is proceeding in
http://codereview.chromium.org/165402/show - as it gets closer to landing I'll
adjust this side to work with it.  Since the filtering in this patch is done
with string comparisons and JSON operations the build won't go red while things
fall into sync, the chrome.devtools API will just do nothing.

[Matt Perry, 2009-08-14 18:59:52]

I like this. It's very modular and not too tied into the extension system.

http://codereview.chromium.org/159882/diff/4028/3032
File chrome/browser/extensions/extension_devtools_bridge.cc (right):

http://codereview.chromium.org/159882/diff/4028/3032#newcode76
Line 76: UnregisterAsDevToolsClientHost();
isn't this redundant with the same call from the devtools manager?

http://codereview.chromium.org/159882/diff/4028/3032#newcode98
Line 98: if (message_name == kTimelineAgentClassName) {
kTimelineEventMessageName?

http://codereview.chromium.org/159882/diff/4028/3032#newcode102
Line 102: JSONReader::Read(msg, false /* allow_trailing_comma */));
Where does this RpcMessage come from? We avoid parsing JSON from untrusted
renderers or from the web in the browser process.

http://codereview.chromium.org/159882/diff/4028/3034
File chrome/browser/extensions/extension_devtools_events.cc (right):

http://codereview.chromium.org/159882/diff/4028/3034#newcode13
Line 13: static const char kDevToolsEventPrefix[] = "chrome.devtools.";
according to extension_process_bindings.js, it's not chrome.devtools, just
devtools. we don't put the chrome prefix on events.

http://codereview.chromium.org/159882/diff/4028/3036
File chrome/browser/extensions/extension_devtools_manager.cc (right):

http://codereview.chromium.org/159882/diff/4028/3036#newcode28
Line 28: // turn devtools on for this tab, if they aren't already
nit: Full sentence comments.

http://codereview.chromium.org/159882/diff/4028/3036#newcode34
Line 34: tab_id_to_render_process_ids_[tab_id].insert(tab_id);
last tab_id should be a pid? Also, you want to add the ender_process_id to the
map always, not only when creating a new bridge, right?

http://codereview.chromium.org/159882/diff/4028/3038
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/159882/diff/4028/3038#newcode123
Line 123: const CommandLine& command_line = *CommandLine::ForCurrentProcess();
I would move this check to profile.cc, so you don't even create the manager
unless the flag is set.

http://codereview.chromium.org/159882/diff/4028/3038#newcode421
Line 421: if (current->second.count(renderer->pid()) != 0) {
nit: no {} for single line ifs

[Aaron Boodman, 2009-08-14 20:48:40]

Hey James,

I'll defer to Matt for the most part on the code review. One thing I
would like to ask for is to modify the docs generation system so that
your APIs do not show up in the docs. You could add a nodocs=3D"true"
attribute or something to your module and have the generation system
skip those.

Even if something is marked experimental, my experience is that people
will use it and be frustrated when it does not work. Therefore my
preference is that we just don't show things in the docs that aren't
ready to be used yet.

- a

On Thu, Aug 13, 2009 at 8:59 PM, <jamesr@google.com> wrote:
> I've updated the patch to use events instead of message ports, put it
> behind a command line flag (off by default), and also renamed it
> 'timeline' rather than 'perftrace' at Pavel's suggestion. =A0This patch i=
s
> still not 100% complete but since there are some tricky lifetime issues
> I would appreciate feedback on parts of the design.
>
> The API pattern now is there is a chrome.devtools.getTab(int tabId)
> function which returns an object with a number of events on it that
> correspond to events produced from devtools land. =A0I've updated
> http://code.google.com/p/chromium/wiki/ExtensionsPerfTraceAPI partially
> to reflect the change - I'll finish updating that wiki page in the
> morning when I'm better rested.
>
> I haven't marked the API as 'experimental' in the docs because I can't
> quite figure out how the doc generating stuff works (it appears broken
> on OS X?), I will follow up offline about that.
>
> Also it is very important to note that as of right now the timeline
> agent has not landed in WebKit or chromium and so if this patch was
> committed today no events would actually be fired. =A0That work is
> proceeding in http://codereview.chromium.org/165402/show - as it gets
> closer to landing I'll adjust this side to work with it. =A0Since the
> filtering in this patch is done with string comparisons and JSON
> operations the build won't go red while things fall into sync, the
> chrome.devtools API will just do nothing.
>
> http://codereview.chromium.org/159882
>

[jamesr1, 2009-08-14 21:02:08]

On Fri, Aug 14, 2009 at 4:48 PM, Aaron Boodman <aa@chromium.org> wrote:

> Hey James,
>
> I'll defer to Matt for the most part on the code review. One thing I
> would like to ask for is to modify the docs generation system so that
> your APIs do not show up in the docs. You could add a nodocs="true"
> attribute or something to your module and have the generation system
> skip those.


Ah cool, I didn't know about that.  I'm currently building test_shell on
windows to generate the docs properly since build.py does not seem to work
on OS X.  I'll make sure the docs page is not linked from the main page.

- James


>
> Even if something is marked experimental, my experience is that people
> will use it and be frustrated when it does not work. Therefore my
> preference is that we just don't show things in the docs that aren't
> ready to be used yet.
>
> - a
>
> On Thu, Aug 13, 2009 at 8:59 PM, <jamesr@google.com> wrote:
> > I've updated the patch to use events instead of message ports, put it
> > behind a command line flag (off by default), and also renamed it
> > 'timeline' rather than 'perftrace' at Pavel's suggestion.  This patch is
> > still not 100% complete but since there are some tricky lifetime issues
> > I would appreciate feedback on parts of the design.
> >
> > The API pattern now is there is a chrome.devtools.getTab(int tabId)
> > function which returns an object with a number of events on it that
> > correspond to events produced from devtools land.  I've updated
> > http://code.google.com/p/chromium/wiki/ExtensionsPerfTraceAPI partially
> > to reflect the change - I'll finish updating that wiki page in the
> > morning when I'm better rested.
> >
> > I haven't marked the API as 'experimental' in the docs because I can't
> > quite figure out how the doc generating stuff works (it appears broken
> > on OS X?), I will follow up offline about that.
> >
> > Also it is very important to note that as of right now the timeline
> > agent has not landed in WebKit or chromium and so if this patch was
> > committed today no events would actually be fired.  That work is
> > proceeding in http://codereview.chromium.org/165402/show - as it gets
> > closer to landing I'll adjust this side to work with it.  Since the
> > filtering in this patch is done with string comparisons and JSON
> > operations the build won't go red while things fall into sync, the
> > chrome.devtools API will just do nothing.
> >
> > http://codereview.chromium.org/159882
> >
>

[jamesr1, 2009-08-14 22:53:26]

Updated to address review comments.  I've taken out the reference to
devtools.html in api_template.html, so there will not be a link to the
documentation from the table of contents although a page will be generated by
build.py.  I think that's fine for now.

http://codereview.chromium.org/159882/diff/4028/3032
File chrome/browser/extensions/extension_devtools_bridge.cc (right):

http://codereview.chromium.org/159882/diff/4028/3032#newcode76
Line 76: UnregisterAsDevToolsClientHost();
On 2009/08/14 18:59:53, Matt Perry wrote:
> isn't this redundant with the same call from the devtools manager?

Yes, I was just being paranoid.  Removing

http://codereview.chromium.org/159882/diff/4028/3032#newcode98
Line 98: if (message_name == kTimelineAgentClassName) {
On 2009/08/14 18:59:53, Matt Perry wrote:
> kTimelineEventMessageName?

Done. (late night coding for the loss)

http://codereview.chromium.org/159882/diff/4028/3032#newcode102
Line 102: JSONReader::Read(msg, false /* allow_trailing_comma */));
On 2009/08/14 18:59:53, Matt Perry wrote:
> Where does this RpcMessage come from? We avoid parsing JSON from untrusted
> renderers or from the web in the browser process.

The msg field of RpcMessage is the JSON-encoded message from devtools, so it is
coming from an untrusted renderer.  I'm only parsing the string out to avoid
double-encoding while building up the arguments for the event dispatch.

I'll try building up the JSON by hand (all it is is '[{"record": <msg>}]')

http://codereview.chromium.org/159882/diff/4028/3034
File chrome/browser/extensions/extension_devtools_events.cc (right):

http://codereview.chromium.org/159882/diff/4028/3034#newcode13
Line 13: static const char kDevToolsEventPrefix[] = "chrome.devtools.";
On 2009/08/14 18:59:53, Matt Perry wrote:
> according to extension_process_bindings.js, it's not chrome.devtools, just
> devtools. we don't put the chrome prefix on events.

Done.

http://codereview.chromium.org/159882/diff/4028/3036
File chrome/browser/extensions/extension_devtools_manager.cc (right):

http://codereview.chromium.org/159882/diff/4028/3036#newcode28
Line 28: // turn devtools on for this tab, if they aren't already
On 2009/08/14 18:59:53, Matt Perry wrote:
> nit: Full sentence comments.

Done.

http://codereview.chromium.org/159882/diff/4028/3036#newcode34
Line 34: tab_id_to_render_process_ids_[tab_id].insert(tab_id);
On 2009/08/14 18:59:53, Matt Perry wrote:
> last tab_id should be a pid? Also, you want to add the ender_process_id to the
> map always, not only when creating a new bridge, right?

Done and done.

http://codereview.chromium.org/159882/diff/4028/3038
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/159882/diff/4028/3038#newcode123
Line 123: const CommandLine& command_line = *CommandLine::ForCurrentProcess();
On 2009/08/14 18:59:53, Matt Perry wrote:
> I would move this check to profile.cc, so you don't even create the manager
> unless the flag is set.

Done.

http://codereview.chromium.org/159882/diff/4028/3038#newcode421
Line 421: if (current->second.count(renderer->pid()) != 0) {
On 2009/08/14 18:59:53, Matt Perry wrote:
> nit: no {} for single line ifs

Done.

[Matt Perry, 2009-08-14 22:59:55]

any ideas for how you could add unit tests for this?

http://codereview.chromium.org/159882/diff/6009/5022
File chrome/browser/extensions/extension_devtools_bridge.cc (right):

http://codereview.chromium.org/159882/diff/6009/5022#newcode17
Line 17: static const char kPerfTraceClassName[] = "PerfTraceClass";
is this used?

http://codereview.chromium.org/159882/diff/6009/5022#newcode99
Line 99: std::string json = StringPrintf("[{\"record\": %s}]", msg.c_str());
do you need to wrap %s in quotes?

http://codereview.chromium.org/159882/diff/6009/5026
File chrome/browser/extensions/extension_devtools_manager.cc (right):

http://codereview.chromium.org/159882/diff/6009/5026#newcode29
Line 29: tab_id_to_render_process_ids_[tab_id].insert(tab_id);
insert(render_process_id)

[jamesr1, 2009-08-18 23:33:18]

I've updated the patch with a few browser tests.  Specifically they test that
the following work:

*) Registering events for a tab and then firing events from the devtools side in
the browser
*) The lifecycle of the devtools bridge properly handles different extensions
registering/unregistering for events

which are the most interesting/fragile parts of the system.  The testing can
certainly be improved but I think it's a decent start.  I plan to take a look at
the extensions apitest stuff that just landed and see if I can simplify things
with that.

[Matt Perry, 2009-08-19 21:22:14]

Sorry for the delayed response. Good tests. I think this is ready for checkin.

http://codereview.chromium.org/159882/diff/8001/8007
File chrome/browser/extensions/extension_devtools_browsertests.cc (right):

http://codereview.chromium.org/159882/diff/8001/8007#newcode92
Line 92: IPC::Message tabUrlChangeEventMessage = FormatDevToolsMsg(
why not just DevToolsClientMsg_RpcMessage tabUrlChangeEventMessage(args);

http://codereview.chromium.org/159882/diff/8001/8023
File chrome/test/data/extensions/devtools/timeline_api/background.html (right):

http://codereview.chromium.org/159882/diff/8001/8023#newcode19
Line 19: devtoolsTab = chrome.devtools.getTab(tab.id);
nit: seeing this in action, I think a better name might be
chrome.devtools.forTab(). or maybe getTabEvents. what do you think?

[jamesr1, 2009-08-19 21:32:58]

Thanks for the review!  I will fix those last few nits, make sure I can get a
clean pass on all trybots, and then find one of y'all in IRC to commit when the
Chromium tree opens.

http://codereview.chromium.org/159882/diff/8001/8007
File chrome/browser/extensions/extension_devtools_browsertests.cc (right):

http://codereview.chromium.org/159882/diff/8001/8007#newcode92
Line 92: IPC::Message tabUrlChangeEventMessage = FormatDevToolsMsg(
On 2009/08/19 21:22:14, Matt Perry wrote:
> why not just DevToolsClientMsg_RpcMessage tabUrlChangeEventMessage(args);

I was under the mistaken impression that FormatDevToolsMsg would have to do more
work than it does.  Will change to what you suggest

http://codereview.chromium.org/159882/diff/8001/8023
File chrome/test/data/extensions/devtools/timeline_api/background.html (right):

http://codereview.chromium.org/159882/diff/8001/8023#newcode19
Line 19: devtoolsTab = chrome.devtools.getTab(tab.id);
On 2009/08/19 21:22:14, Matt Perry wrote:
> nit: seeing this in action, I think a better name might be
> chrome.devtools.forTab(). or maybe getTabEvents. what do you think?

I agree that looks a lot better.  I think this is pretty likely to change as
time goes on.

[jamesr1, 2009-08-24 21:55:08]

This patch passes all try bots and should be good to land.  The checks on the
devtools RPC messages in ExtensionDevToolsBridge will currently never be true
and so the tabUrlChange and pageEvent events will not fire until changes are
made upstream.  Once that happens I will submit a follow-up patch to sync up the
logic between the two layers.

[Matt Perry, 2009-08-24 21:59:07]

LGTM