DevTools: add initial support for shared workers debugging

DevTools: add initial support for shared workers debugging.

WorkerDevToolsManager is a service that is used for managing DevTools connections to DevTools agents in shared workers. It operates on the IO thread. Each debugged shared worker instance has unique devtools id which is used for addressing workers when passing messages between WorkerDevToolsManager and DevToolsManager. WorkerDevToolsManager translates those ids into WorkerProcessHost and worker_routing_id and back.

WorkerDevToolsMessageFilter passes worker messages to the WorkerDevToolsManager which in turn forwards them to DevToolsManager on the UI thread where they are dispatched to corresponding DevTools window.

Messages from DevTools frontend go through the DevToolsManager which forwards them to WorkerDevToolsManager on the IO thread where they are sent by means of corresponding WorkerProcessHost to the corresponding WorkerDevToolsAgent.

BUG=None
TEST=None

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

[yurys, 2011-07-05 17:00:48]

Dmitry, please take a look at the changes in websharedworker_stub.cc and
worker_process_host.cc. I tried to confine number of DevTools dependencies to
minimal.

[Jói, 2011-07-05 18:00:45]

http://codereview.chromium.org/7248076/diff/18/content/browser/debugger/devto...
File content/browser/debugger/devtools_manager.cc (right):

http://codereview.chromium.org/7248076/diff/18/content/browser/debugger/devto...
content/browser/debugger/devtools_manager.cc:15: #include
"chrome/browser/profiles/profile_manager.h"
Please don't add a new dependency on ProfileManager, I am working to remove all
the dependencies on Chrome listed in content/browser/debugger/DEPS.

If you really need the default profile, you could add a method to
ContentBrowserClient to retrieve it.

[pfeldman, 2011-07-06 08:29:39]

http://codereview.chromium.org/7248076/diff/18/content/browser/debugger/devto...
File content/browser/debugger/devtools_manager.cc (right):

http://codereview.chromium.org/7248076/diff/18/content/browser/debugger/devto...
content/browser/debugger/devtools_manager.cc:15: #include
"chrome/browser/profiles/profile_manager.h"
On 2011/07/05 18:00:45, Jói wrote:
> Please don't add a new dependency on ProfileManager, I am working to remove
all
> the dependencies on Chrome listed in content/browser/debugger/DEPS.
> 
> If you really need the default profile, you could add a method to
> ContentBrowserClient to retrieve it.

Do you have an estimate on when migration is complete?

[jam, 2011-07-06 13:47:40]

http://codereview.chromium.org/7248076/diff/18/content/browser/debugger/devto...
File content/browser/debugger/devtools_manager.cc (right):

http://codereview.chromium.org/7248076/diff/18/content/browser/debugger/devto...
content/browser/debugger/devtools_manager.cc:15: #include
"chrome/browser/profiles/profile_manager.h"
if a method is added to ContentBrowserClient to return a profile, it will have
to be removed soon as well since that interface shouldn't know about profiles.
it's probably less work to leave the code as below and then remove it in one
shot when DevToolsWindow doesn't need profile anymore?

On 2011/07/05 18:00:45, Jói wrote:
> Please don't add a new dependency on ProfileManager, I am working to remove
all
> the dependencies on Chrome listed in content/browser/debugger/DEPS.
> 
> If you really need the default profile, you could add a method to
> ContentBrowserClient to retrieve it.

[jam, 2011-07-06 13:55:07]

to explain: adding a temp method to get the default profile to CBC might lead
other code in content to depend on it, which would add more work to getting rid
of Profile from content. at least if ProfileManager is used in the debugger dir,
that dependency doesn't leak to other code in content\browser

On 2011/07/06 13:47:40, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/7248076/diff/18/content/browser/debugger/devto...
> File content/browser/debugger/devtools_manager.cc (right):
> 
>
http://codereview.chromium.org/7248076/diff/18/content/browser/debugger/devto...
> content/browser/debugger/devtools_manager.cc:15: #include
> "chrome/browser/profiles/profile_manager.h"
> if a method is added to ContentBrowserClient to return a profile, it will have
> to be removed soon as well since that interface shouldn't know about profiles.
> it's probably less work to leave the code as below and then remove it in one
> shot when DevToolsWindow doesn't need profile anymore?
> 
> On 2011/07/05 18:00:45, Jói wrote:
> > Please don't add a new dependency on ProfileManager, I am working to remove
> all
> > the dependencies on Chrome listed in content/browser/debugger/DEPS.
> > 
> > If you really need the default profile, you could add a method to
> > ContentBrowserClient to retrieve it.

[yurys, 2011-07-06 14:03:52]

http://codereview.chromium.org/7248076/diff/18/content/browser/debugger/devto...
File content/browser/debugger/devtools_manager.cc (right):

http://codereview.chromium.org/7248076/diff/18/content/browser/debugger/devto...
content/browser/debugger/devtools_manager.cc:15: #include
"chrome/browser/profiles/profile_manager.h"
On 2011/07/06 13:47:40, John Abd-El-Malek wrote:
> if a method is added to ContentBrowserClient to return a profile, it will have
> to be removed soon as well since that interface shouldn't know about profiles.
> it's probably less work to leave the code as below and then remove it in one
> shot when DevToolsWindow doesn't need profile anymore?
> 
> On 2011/07/05 18:00:45, Jói wrote:
> > Please don't add a new dependency on ProfileManager, I am working to remove
> all
> > the dependencies on Chrome listed in content/browser/debugger/DEPS.
> > 
> > If you really need the default profile, you could add a method to
> > ContentBrowserClient to retrieve it.
> 
My understanding is that DevToolsWindow is a Chrome-specific implementation of
DevToolsClientHost and as such should live under chrome/ where it shouldn't be a
problem to access ProfileManager.

[jam, 2011-07-06 14:53:07]

On 2011/07/06 14:03:52, Yury Semikhatsky wrote:
>
http://codereview.chromium.org/7248076/diff/18/content/browser/debugger/devto...
> File content/browser/debugger/devtools_manager.cc (right):
> 
>
http://codereview.chromium.org/7248076/diff/18/content/browser/debugger/devto...
> content/browser/debugger/devtools_manager.cc:15: #include
> "chrome/browser/profiles/profile_manager.h"
> On 2011/07/06 13:47:40, John Abd-El-Malek wrote:
> > if a method is added to ContentBrowserClient to return a profile, it will
have
> > to be removed soon as well since that interface shouldn't know about
profiles.
> > it's probably less work to leave the code as below and then remove it in one
> > shot when DevToolsWindow doesn't need profile anymore?
> > 
> > On 2011/07/05 18:00:45, Jói wrote:
> > > Please don't add a new dependency on ProfileManager, I am working to
remove
> > all
> > > the dependencies on Chrome listed in content/browser/debugger/DEPS.
> > > 
> > > If you really need the default profile, you could add a method to
> > > ContentBrowserClient to retrieve it.
> > 
> My understanding is that DevToolsWindow is a Chrome-specific implementation of
> DevToolsClientHost and as such should live under chrome/ where it shouldn't be
a
> problem to access ProfileManager.

ah ok, if that lives under chrome then using profilemanager should be fine. it
looks like right now it's under content. if it'll move back to chrome then the
devtoolsmanager code needs to go through ContentBrowserClient to create an
implementation of DevToolsClientHost.

[pfeldman, 2011-07-06 15:01:19]

Should we move debugger back to chrome altogether and agree on what parts get
moved to the content first?

It looks like neither content owners nor debugger owners have a complete picture
of what it should look like in the end. Having entire debugger moved to the
content with its deps frozen hurts us.

[Jói, 2011-07-06 15:34:56]

When John and I walked through the debugger code it didn't look particularly
impossible to break the dependencies DevToolsWindow has on Chrome, so we
thought it would be OK to move.

Please let's not move all of devtools back to the Chrome side.  I understand
that the frozen DEPS hurts you, but the refactoring needs to happen and
there will always be some amount of pain around it.  I think we can agree at
least most of it is non Chrome-specific, so let's leave as much as possible
in content as we work to break the dependencies.  Certain bits should and
will move back to Chrome though, e.g. I have a pending change to move
ExtensionPortsRemoteService back to Chrome (
http://codereview.chromium.org/7309011/).

If you guys feel DevToolsWindow belongs on the Chrome side, I don't mind if
you move it back there.  My only question is whether we will then have a
functional debugger on the content side (if not, that may be OK and we can
write a less-functional debugger there, but if it can be moved wholesale
that is best).  John's suggestion to just allow the dependency on
ProfileManager for now also seems fine to me - he has much more experience
with the content refactoring than I do, and perhaps that is the simplest
solution for now.

In terms of a schedule, I'm afraid I don't have an exact one.  I think this
week I should be able to break the extensions dependency (pending) and one
or two other significant dependencies, but not everything.  This is my
full-time project this week, but I am only able to work regular hours this
week, and as of early next week I will be away for about 2 weeks because I
am moving to Iceland, but perhaps John can pick up where I leave it once he
gets back.

Cheers,
Jói



On Wed, Jul 6, 2011 at 11:01 AM, <pfeldman@chromium.org> wrote:

> Should we move debugger back to chrome altogether and agree on what parts
> get
> moved to the content first?
>
> It looks like neither content owners nor debugger owners have a complete
> picture
> of what it should look like in the end. Having entire debugger moved to the
> content with its deps frozen hurts us.
>
>
>
http://codereview.chromium.**org/7248076/<http://codereview.chromium.org/7248...
>

[jam, 2011-07-06 15:36:36]

Joi is taking on the task of removing chrome dependencies from content. I
commented because i like to review all changes to ContentClient interfaces.

we shouldn't move the code back since that will only slow down the content
refactoring. apologies if this is slowing you down, but the bug to remove the
chrome dependencies has been open for a while. it would have been best if the
dev tools team did this in the first place since you guys are most familiar with
it. given that it's not the case, some short term pain is unavoidable.

On 2011/07/06 15:01:19, pfeldman wrote:
> Should we move debugger back to chrome altogether and agree on what parts get
> moved to the content first?
> 
> It looks like neither content owners nor debugger owners have a complete
picture
> of what it should look like in the end. Having entire debugger moved to the
> content with its deps frozen hurts us.

[jam, 2011-07-06 16:02:33]

+1 to everything with one caveat: we want to have as much code as possible in
content, so that other embedders dont have to rewrite code that's in content. ie
the point of moving the devtools code to content is that the content test shell
can have a debugger good enough for any developer. I don't remember which
classes that Joi and I thought should stay behind and I dot have the code
infront of me right now. but if Joi says that we thought it should stay, then
that's probably the right solution long term. I can help remove the remaining
dependencies when Joi is gone if it makes your life easier. again, sorry if this
is causing short term pain. a lot of tasks depend on removing every last task
(see the content module wiki)

On 2011/07/06 15:34:56, Jói wrote:
> When John and I walked through the debugger code it didn't look particularly
> impossible to break the dependencies DevToolsWindow has on Chrome, so we
> thought it would be OK to move.
> 
> Please let's not move all of devtools back to the Chrome side.  I understand
> that the frozen DEPS hurts you, but the refactoring needs to happen and
> there will always be some amount of pain around it.  I think we can agree at
> least most of it is non Chrome-specific, so let's leave as much as possible
> in content as we work to break the dependencies.  Certain bits should and
> will move back to Chrome though, e.g. I have a pending change to move
> ExtensionPortsRemoteService back to Chrome (
> http://codereview.chromium.org/7309011/).
> 
> If you guys feel DevToolsWindow belongs on the Chrome side, I don't mind if
> you move it back there.  My only question is whether we will then have a
> functional debugger on the content side (if not, that may be OK and we can
> write a less-functional debugger there, but if it can be moved wholesale
> that is best).  John's suggestion to just allow the dependency on
> ProfileManager for now also seems fine to me - he has much more experience
> with the content refactoring than I do, and perhaps that is the simplest
> solution for now.
> 
> In terms of a schedule, I'm afraid I don't have an exact one.  I think this
> week I should be able to break the extensions dependency (pending) and one
> or two other significant dependencies, but not everything.  This is my
> full-time project this week, but I am only able to work regular hours this
> week, and as of early next week I will be away for about 2 weeks because I
> am moving to Iceland, but perhaps John can pick up where I leave it once he
> gets back.
> 
> Cheers,
> Jói
> 
> 
> 
> On Wed, Jul 6, 2011 at 11:01 AM, <mailto:pfeldman@chromium.org> wrote:
> 
> > Should we move debugger back to chrome altogether and agree on what parts
> > get
> > moved to the content first?
> >
> > It looks like neither content owners nor debugger owners have a complete
> > picture
> > of what it should look like in the end. Having entire debugger moved to the
> > content with its deps frozen hurts us.
> >
> >
> >
>
http://codereview.chromium.**org/7248076/%3Chttp://codereview.chromium.org/72...>
> >

[pfeldman, 2011-07-06 16:07:44]

I am fine with some pain. I also appreciate you guys stepping up and doing this
work. We could not have done it without talking to you first and understanding
the goals of the migration.

However, I was expecting a kind of a plan prior to the execution. I am looking
at the debugger/ content now and it is clear to me that the only files that
should have been migrated to the content are devtools_client_host.* and
devtools_manager.*. Everything else will need to go back.

Before we can move devtools_manager to content, we should break the
devtools_manager -> devtools_window dependency that yury is currently working
on. That's why I am thinking that rolling things back, breaking this dependency
and moving only these files make way more sense.

Also, I'd like to know if you are planning on bringing DevToolsAgent /
DevToolsClient back into RenderView and merging DevToolsHost back into the
DevToolsManager (stuff that was extracted with debugger/ being a part of chrome
in mind).

[Jói, 2011-07-06 16:23:52]

I think there may be some misunderstanding about the goal here.  The intent
is to include a full-featured debugger in content/, not just debugger
support.  Therefore I think everything that is not Chrome-specific
functionality should move to content/, and only Chrome-specific things (like
extensions support) should stay in Chrome.  There should only be a handful
of things remaining in Chrome.  If it turns out DevToolsWindow absolutely
needs to stay in Chrome after we look into that a bit (and I honestly don't
think it will need to), then we will need a similar implementation on the
content side to provide a UI for the debugger.

Cheers,
Jói


On Wed, Jul 6, 2011 at 12:07 PM, <pfeldman@chromium.org> wrote:

> I am fine with some pain. I also appreciate you guys stepping up and doing
> this
> work. We could not have done it without talking to you first and
> understanding
> the goals of the migration.
>
> However, I was expecting a kind of a plan prior to the execution. I am
> looking
> at the debugger/ content now and it is clear to me that the only files that
> should have been migrated to the content are devtools_client_host.* and
> devtools_manager.*. Everything else will need to go back.
>
> Before we can move devtools_manager to content, we should break the
> devtools_manager -> devtools_window dependency that yury is currently
> working
> on. That's why I am thinking that rolling things back, breaking this
> dependency
> and moving only these files make way more sense.
>
> Also, I'd like to know if you are planning on bringing DevToolsAgent /
> DevToolsClient back into RenderView and merging DevToolsHost back into the
> DevToolsManager (stuff that was extracted with debugger/ being a part of
> chrome
> in mind).
>
>
>
http://codereview.chromium.**org/7248076/<http://codereview.chromium.org/7248...
>

[pfeldman, 2011-07-06 16:47:36]

DevToolsWindow is highly coupled with the browser. It decides where to dock
devtools window (into which browser window) and such. We can have a way simpler
100 line DevToolsClientHost descendant that will render Dev Tools window
undocked with no additional dependencies.

Apart from the devtools_window, there are > 20 remote debugging files that I
believe should not be a part of content, yet are moved.

Not sure about the net log observer (I am fine both with moving it and leaving
in place). It is integrating into the chrome_net_log.

[Jói, 2011-07-06 16:59:21]

I'm glad we're having this conversation :)

> DevToolsWindow is highly coupled with the browser. It decides where to dock
> devtools window (into which browser window) and such. We can have a way
simpler
> 100 line DevToolsClientHost descendant that will render Dev Tools window
> undocked with no additional dependencies.

OK, that sounds like it might be a good approach.  Would this still
let us provide a multi-tab-aware debugger UI to our embedders?

The first use of the debugger in content would be in our own
multi-process test shell, which we expect to probably make
multi-tab-aware as many of the browser_tests which we hope to be able
to make content_tests (and depend only on content/ and the
multi-process test shell) require multiple tabs to work.

> Apart from the devtools_window, there are > 20 remote debugging files that I
> believe should not be a part of content, yet are moved.

Why should they not be part of content and thus available for all
embedders of content?  What makes them Chrome specific?

> Not sure about the net log observer (I am fine both with moving it and leaving
> in place). It is integrating into the chrome_net_log.

It looked like it is needed to have information about raw headers.  It
should be straightforward to move to content, NetLog is a net/ concept
and observing the NetLog should probably be a net/ concept too,
although that needs to be twisted around.  This was next on my list of
things to look at; as a first step I would probably just add a method
to ContentBrowserClient to observe the NetLog through a fully abstract
interface, but longer-term (and perhaps right away, I'll look at it)
I'd like to move the ability to observe a NetLog down to
net/base/net_log.h - will discuss with Eric Roman.

Cheers,
Jói



On Wed, Jul 6, 2011 at 12:47 PM, <pfeldman@chromium.org> wrote:
>
> DevToolsWindow is highly coupled with the browser. It decides where to dock
> devtools window (into which browser window) and such. We can have a way
simpler
> 100 line DevToolsClientHost descendant that will render Dev Tools window
> undocked with no additional dependencies.
>
> Apart from the devtools_window, there are > 20 remote debugging files that I
> believe should not be a part of content, yet are moved.
>
> Not sure about the net log observer (I am fine both with moving it and leaving
> in place). It is integrating into the chrome_net_log.
>
> http://codereview.chromium.org/7248076/

[Dmitry Titov, 2011-07-06 18:22:46]

On 2011/07/05 17:00:48, Yury Semikhatsky wrote:
> Dmitry, please take a look at the changes in websharedworker_stub.cc and
> worker_process_host.cc. I tried to confine number of DevTools dependencies to
> minimal.

Do you really need a Dedicated devtools agent? One option could be to sync plans
with dslomov@, he is close to enabling isolate-based dedicated workers so the
code for Dedicated workers may be significantly different soon.

For content/worker changes, you'll need John to look at it, they are simple and
he's the owner.

[yurys, 2011-07-07 06:40:20]

On 2011/07/06 18:22:46, Dmitry Titov wrote:
> On 2011/07/05 17:00:48, Yury Semikhatsky wrote:
> > Dmitry, please take a look at the changes in websharedworker_stub.cc and
> > worker_process_host.cc. I tried to confine number of DevTools dependencies
to
> > minimal.
> 
> Do you really need a Dedicated devtools agent? One option could be to sync
plans
> with dslomov@, he is close to enabling isolate-based dedicated workers so the
> code for Dedicated workers may be significantly different soon.
> 
I just didn't want to break existing devtools functionality related to
out-of-process dedicated workers. Once isolate-based implementation replaces
out-of-process one we will get rid of the dedicated worker agent.

[yurys, 2011-07-07 06:55:14]

On 2011/07/06 16:59:21, Jói wrote:
> I'm glad we're having this conversation :)
> 
> > DevToolsWindow is highly coupled with the browser. It decides where to dock
> > devtools window (into which browser window) and such. We can have a way
> simpler
> > 100 line DevToolsClientHost descendant that will render Dev Tools window
> > undocked with no additional dependencies.
> 
> OK, that sounds like it might be a good approach.  Would this still
> let us provide a multi-tab-aware debugger UI to our embedders?
> 
Yes, it is DevToolsManager that supports multiple inspected render view hosts,
each of them can have its own kind of DevToolsClientHost(not necessarily
DevToolsWindow). It doesn't operate in terms of UI tabs as e.g. there is no tab
for an extension background page and there won't be one in case of shared
worker.

If we need some default devtools UI which would base on the content/
abstractions solely, we could extract necessary functionality into
DevToolsWindow super class and put it under content/, but unless chrome
multi-tab UI is moved under content/ I'd leave DevToolsWindow under chrome/.


> The first use of the debugger in content would be in our own
> multi-process test shell, which we expect to probably make
> multi-tab-aware as many of the browser_tests which we hope to be able
> to make content_tests (and depend only on content/ and the
> multi-process test shell) require multiple tabs to work.
> 
> > Apart from the devtools_window, there are > 20 remote debugging files that I
> > believe should not be a part of content, yet are moved.
> 
> Why should they not be part of content and thus available for all
> embedders of content?  What makes them Chrome specific?
> 
> > Not sure about the net log observer (I am fine both with moving it and
leaving
> > in place). It is integrating into the chrome_net_log.
> 
> It looked like it is needed to have information about raw headers.  It
> should be straightforward to move to content, NetLog is a net/ concept
> and observing the NetLog should probably be a net/ concept too,
> although that needs to be twisted around.  This was next on my list of
> things to look at; as a first step I would probably just add a method
> to ContentBrowserClient to observe the NetLog through a fully abstract
> interface, but longer-term (and perhaps right away, I'll look at it)
> I'd like to move the ability to observe a NetLog down to
> net/base/net_log.h - will discuss with Eric Roman.
> 
> Cheers,
> Jói
> 
> 
> 
> On Wed, Jul 6, 2011 at 12:47 PM, <mailto:pfeldman@chromium.org> wrote:
> >
> > DevToolsWindow is highly coupled with the browser. It decides where to dock
> > devtools window (into which browser window) and such. We can have a way
> simpler
> > 100 line DevToolsClientHost descendant that will render Dev Tools window
> > undocked with no additional dependencies.
> >
> > Apart from the devtools_window, there are > 20 remote debugging files that I
> > believe should not be a part of content, yet are moved.
> >
> > Not sure about the net log observer (I am fine both with moving it and
leaving
> > in place). It is integrating into the chrome_net_log.
> >
> > http://codereview.chromium.org/7248076/

[Jói, 2011-07-07 14:52:58]

> If we need some default devtools UI which would base on the content/
> abstractions solely, we could extract necessary functionality into
> DevToolsWindow super class and put it under content/, but unless chrome
> multi-tab UI is moved under content/ I'd leave DevToolsWindow under chrome/.

OK cool, this sounds good to me.  Let's start that way and then see
how it goes once we have the multi-process test shell.  Thanks for the
change for removing knowledge of DevToolsWindow from DevToolsManager.

BTW, everybody, sorry to have hijacked this review thread :)  I'll stop now :)

Cheers,
Jói



On Thu, Jul 7, 2011 at 2:55 AM,  <yurys@chromium.org> wrote:
> On 2011/07/06 16:59:21, Jói wrote:
>>
>> I'm glad we're having this conversation :)
>
>> > DevToolsWindow is highly coupled with the browser. It decides where to
>> > dock
>> > devtools window (into which browser window) and such. We can have a way
>> simpler
>> > 100 line DevToolsClientHost descendant that will render Dev Tools window
>> > undocked with no additional dependencies.
>
>> OK, that sounds like it might be a good approach.  Would this still
>> let us provide a multi-tab-aware debugger UI to our embedders?
>
> Yes, it is DevToolsManager that supports multiple inspected render view
> hosts,
> each of them can have its own kind of DevToolsClientHost(not necessarily
> DevToolsWindow). It doesn't operate in terms of UI tabs as e.g. there is no
> tab
> for an extension background page and there won't be one in case of shared
> worker.
>
> If we need some default devtools UI which would base on the content/
> abstractions solely, we could extract necessary functionality into
> DevToolsWindow super class and put it under content/, but unless chrome
> multi-tab UI is moved under content/ I'd leave DevToolsWindow under chrome/.
>
>
>> The first use of the debugger in content would be in our own
>> multi-process test shell, which we expect to probably make
>> multi-tab-aware as many of the browser_tests which we hope to be able
>> to make content_tests (and depend only on content/ and the
>> multi-process test shell) require multiple tabs to work.
>
>> > Apart from the devtools_window, there are > 20 remote debugging files
>> > that I
>> > believe should not be a part of content, yet are moved.
>
>> Why should they not be part of content and thus available for all
>> embedders of content?  What makes them Chrome specific?
>
>> > Not sure about the net log observer (I am fine both with moving it and
>
> leaving
>>
>> > in place). It is integrating into the chrome_net_log.
>
>> It looked like it is needed to have information about raw headers.  It
>> should be straightforward to move to content, NetLog is a net/ concept
>> and observing the NetLog should probably be a net/ concept too,
>> although that needs to be twisted around.  This was next on my list of
>> things to look at; as a first step I would probably just add a method
>> to ContentBrowserClient to observe the NetLog through a fully abstract
>> interface, but longer-term (and perhaps right away, I'll look at it)
>> I'd like to move the ability to observe a NetLog down to
>> net/base/net_log.h - will discuss with Eric Roman.
>
>> Cheers,
>> Jói
>
>
>
>> On Wed, Jul 6, 2011 at 12:47 PM, <mailto:pfeldman@chromium.org> wrote:
>> >
>> > DevToolsWindow is highly coupled with the browser. It decides where to
>> > dock
>> > devtools window (into which browser window) and such. We can have a way
>> simpler
>> > 100 line DevToolsClientHost descendant that will render Dev Tools window
>> > undocked with no additional dependencies.
>> >
>> > Apart from the devtools_window, there are > 20 remote debugging files
>> > that I
>> > believe should not be a part of content, yet are moved.
>> >
>> > Not sure about the net log observer (I am fine both with moving it and
>
> leaving
>>
>> > in place). It is integrating into the chrome_net_log.
>> >
>> > http://codereview.chromium.org/7248076/
>
>
>
> http://codereview.chromium.org/7248076/
>

[pfeldman, 2011-07-08 09:17:17]

http://codereview.chromium.org/7248076/diff/12001/content/browser/debugger/de...
File content/browser/debugger/devtools_window.cc (right):

http://codereview.chromium.org/7248076/diff/12001/content/browser/debugger/de...
content/browser/debugger/devtools_window.cc:77: DevToolsWindow*
DevToolsWindow::CreateDevToolsWindow(Profile* profile) {
CreateDevToolsWindowForWorker

http://codereview.chromium.org/7248076/diff/15001/content/browser/debugger/wo...
File content/browser/debugger/worker_devtools_manager.cc (right):

http://codereview.chromium.org/7248076/diff/15001/content/browser/debugger/wo...
content/browser/debugger/worker_devtools_manager.cc:18: class
WorkerDevToolsManager::DevToolsWorkerIds {
IdsIOMap

http://codereview.chromium.org/7248076/diff/15001/content/browser/debugger/wo...
content/browser/debugger/worker_devtools_manager.cc:31: it != map_.end(); ++it)
{
Does it fit one line?

http://codereview.chromium.org/7248076/diff/15001/content/browser/debugger/wo...
content/browser/debugger/worker_devtools_manager.cc:32: if (it->a->id() ==
worker_process_host_id && it->b == route_id)
it-a-id? do we want a struct?

http://codereview.chromium.org/7248076/diff/15001/content/browser/debugger/wo...
content/browser/debugger/worker_devtools_manager.cc:39: int devtools_id,
why line break?

http://codereview.chromium.org/7248076/diff/15001/content/browser/debugger/wo...
content/browser/debugger/worker_devtools_manager.cc:42: for (IdMap::iterator it
= map_.begin();
ditto

http://codereview.chromium.org/7248076/diff/15001/content/browser/debugger/wo...
content/browser/debugger/worker_devtools_manager.cc:97:
WorkerDevToolsManager::GetInstance()->WorkerDevToolsClientClosing(
WorkerDevToolsManagerIO

http://codereview.chromium.org/7248076/diff/15001/content/browser/debugger/wo...
content/browser/debugger/worker_devtools_manager.cc:101: class
WorkerDevToolsClients : public DevToolsClientHost::CloseListener {
Can Clients be merged with the Ids?

[yurys, 2011-07-08 15:35:53]

http://codereview.chromium.org/7248076/diff/12001/content/browser/debugger/de...
File content/browser/debugger/devtools_window.cc (right):

http://codereview.chromium.org/7248076/diff/12001/content/browser/debugger/de...
content/browser/debugger/devtools_window.cc:77: DevToolsWindow*
DevToolsWindow::CreateDevToolsWindow(Profile* profile) {
On 2011/07/08 09:17:17, pfeldman wrote:
> CreateDevToolsWindowForWorker

Done.

http://codereview.chromium.org/7248076/diff/15001/content/browser/debugger/wo...
File content/browser/debugger/worker_devtools_manager.cc (right):

http://codereview.chromium.org/7248076/diff/15001/content/browser/debugger/wo...
content/browser/debugger/worker_devtools_manager.cc:18: class
WorkerDevToolsManager::DevToolsWorkerIds {
On 2011/07/08 09:17:17, pfeldman wrote:
> IdsIOMap

Done.

http://codereview.chromium.org/7248076/diff/15001/content/browser/debugger/wo...
content/browser/debugger/worker_devtools_manager.cc:31: it != map_.end(); ++it)
{
On 2011/07/08 09:17:17, pfeldman wrote:
> Does it fit one line?

Done.

http://codereview.chromium.org/7248076/diff/15001/content/browser/debugger/wo...
content/browser/debugger/worker_devtools_manager.cc:32: if (it->a->id() ==
worker_process_host_id && it->b == route_id)
On 2011/07/08 09:17:17, pfeldman wrote:
> it-a-id? do we want a struct?

Done.

http://codereview.chromium.org/7248076/diff/15001/content/browser/debugger/wo...
content/browser/debugger/worker_devtools_manager.cc:39: int devtools_id,
On 2011/07/08 09:17:17, pfeldman wrote:
> why line break?
Doesn't fit on 1 line.

http://codereview.chromium.org/7248076/diff/15001/content/browser/debugger/wo...
content/browser/debugger/worker_devtools_manager.cc:42: for (IdMap::iterator it
= map_.begin();
On 2011/07/08 09:17:17, pfeldman wrote:
> ditto

Done.

http://codereview.chromium.org/7248076/diff/15001/content/browser/debugger/wo...
content/browser/debugger/worker_devtools_manager.cc:97:
WorkerDevToolsManager::GetInstance()->WorkerDevToolsClientClosing(
On 2011/07/08 09:17:17, pfeldman wrote:
> WorkerDevToolsManagerIO

Done.

http://codereview.chromium.org/7248076/diff/15001/content/browser/debugger/wo...
content/browser/debugger/worker_devtools_manager.cc:101: class
WorkerDevToolsClients : public DevToolsClientHost::CloseListener {
On 2011/07/08 09:17:17, pfeldman wrote:
> Can Clients be merged with the Ids?

No, they live on different threads.

[jam, 2011-07-08 17:49:09]

http://codereview.chromium.org/7248076/diff/14003/content/worker/worker_devto...
File content/worker/worker_devtools_agent.cc (right):

http://codereview.chromium.org/7248076/diff/14003/content/worker/worker_devto...
content/worker/worker_devtools_agent.cc:66: class SharedWorkerDevToolsAgent :
public WorkerDevToolsAgent {
It's unfortunate that this class is duplicated for both worker types. Why can't
the debugger related methods on We worker and WebSharedWorker derive from a base
class for this purpose? Otherwise a template can be used.

[pfeldman, 2011-07-11 12:07:15]

Looks good with a couple of nits. Please send to content owners for review &
landing permission.

http://codereview.chromium.org/7248076/diff/14003/content/browser/debugger/de...
File content/browser/debugger/devtools_window.cc (right):

http://codereview.chromium.org/7248076/diff/14003/content/browser/debugger/de...
content/browser/debugger/devtools_window.cc:139: // There is now inspected_rvh
in case of shared workers.
There is _no_ inspected_rvh ?

http://codereview.chromium.org/7248076/diff/14003/content/browser/debugger/wo...
File content/browser/debugger/worker_devtools_manager_io.cc (right):

http://codereview.chromium.org/7248076/diff/14003/content/browser/debugger/wo...
content/browser/debugger/worker_devtools_manager_io.cc:20: 
Blank line

http://codereview.chromium.org/7248076/diff/14003/content/browser/debugger/wo...
content/browser/debugger/worker_devtools_manager_io.cc:28: class
WorkerDevToolsClients : public DevToolsClientHost::CloseListener {
ClientsUI ?

http://codereview.chromium.org/7248076/diff/14003/content/worker/worker_devto...
File content/worker/worker_devtools_agent.cc (right):

http://codereview.chromium.org/7248076/diff/14003/content/worker/worker_devto...
content/worker/worker_devtools_agent.cc:66: class SharedWorkerDevToolsAgent :
public WorkerDevToolsAgent {
I think templates won't harm readability of this code.

[yurys, 2011-07-11 15:13:11]

http://codereview.chromium.org/7248076/diff/14003/content/browser/debugger/de...
File content/browser/debugger/devtools_window.cc (right):

http://codereview.chromium.org/7248076/diff/14003/content/browser/debugger/de...
content/browser/debugger/devtools_window.cc:139: // There is now inspected_rvh
in case of shared workers.
On 2011/07/11 12:07:16, pfeldman wrote:
> There is _no_ inspected_rvh ?

Done.

http://codereview.chromium.org/7248076/diff/14003/content/browser/debugger/wo...
File content/browser/debugger/worker_devtools_manager_io.cc (right):

http://codereview.chromium.org/7248076/diff/14003/content/browser/debugger/wo...
content/browser/debugger/worker_devtools_manager_io.cc:20: 
On 2011/07/11 12:07:16, pfeldman wrote:
> Blank line

Done.

http://codereview.chromium.org/7248076/diff/14003/content/browser/debugger/wo...
content/browser/debugger/worker_devtools_manager_io.cc:28: class
WorkerDevToolsClients : public DevToolsClientHost::CloseListener {
On 2011/07/11 12:07:16, pfeldman wrote:
> ClientsUI ?

Done.

http://codereview.chromium.org/7248076/diff/14003/content/worker/worker_devto...
File content/worker/worker_devtools_agent.cc (right):

http://codereview.chromium.org/7248076/diff/14003/content/worker/worker_devto...
content/worker/worker_devtools_agent.cc:66: class SharedWorkerDevToolsAgent :
public WorkerDevToolsAgent {
On 2011/07/08 17:49:09, John Abd-El-Malek wrote:
> It's unfortunate that this class is duplicated for both worker types. Why
can't
> the debugger related methods on We worker and WebSharedWorker derive from a
base
> class for this purpose? Otherwise a template can be used.

WebWorker and WebSharedWorker don't have common ancestor, otherwise I would have
put these methods there. I tried to avoid a new base class for workers but if
you think that it would be justified, I could do that. But even in that case
OnMessageReceived would have two different implementations as it handles
DevToolsAgentMsg_DispatchOnInspectorBackend in one case and
WorkerDevToolsAgentMsg_DispatchOnInspectorBackend in another. Also
SendDevToolsMessage should be different for shared and dedicated workers. Given
all this I don't think it is worth complicating the code with templates just to
avoid code duplication in 2 one-line methods.

http://codereview.chromium.org/7248076/diff/14003/content/worker/worker_devto...
content/worker/worker_devtools_agent.cc:66: class SharedWorkerDevToolsAgent :
public WorkerDevToolsAgent {
On 2011/07/11 12:07:16, pfeldman wrote:
> I think templates won't harm readability of this code.

See my response to John.

[jam, 2011-07-11 16:53:37]

http://codereview.chromium.org/7248076/diff/14003/content/worker/worker_devto...
File content/worker/worker_devtools_agent.cc (right):

http://codereview.chromium.org/7248076/diff/14003/content/worker/worker_devto...
content/worker/worker_devtools_agent.cc:66: class SharedWorkerDevToolsAgent :
public WorkerDevToolsAgent {
On 2011/07/11 15:13:11, Yury Semikhatsky wrote:
> On 2011/07/08 17:49:09, John Abd-El-Malek wrote:
> > It's unfortunate that this class is duplicated for both worker types. Why
> can't
> > the debugger related methods on We worker and WebSharedWorker derive from a
> base
> > class for this purpose? Otherwise a template can be used.
> 
> WebWorker and WebSharedWorker don't have common ancestor, otherwise I would
have
> put these methods there. I tried to avoid a new base class for workers but if
> you think that it would be justified, I could do that. But even in that case
> OnMessageReceived would have two different implementations as it handles
> DevToolsAgentMsg_DispatchOnInspectorBackend in one case and
> WorkerDevToolsAgentMsg_DispatchOnInspectorBackend in another. 

You can send DevToolsAgentMsg_DispatchOnInspectorBackend in both cases. It's
common for messages that are sent to different child processes to reuse the same
message class, instead of duplicating it.

> Also
> SendDevToolsMessage should be different for shared and dedicated workers.

This one function would be different, but the rest of the 50 line class would be
the same.
>  Given
> all this I don't think it is worth complicating the code with templates just
to
> avoid code duplication in 2 one-line methods.

[yurys, 2011-07-12 13:58:46]

http://codereview.chromium.org/7248076/diff/14003/content/worker/worker_devto...
File content/worker/worker_devtools_agent.cc (right):

http://codereview.chromium.org/7248076/diff/14003/content/worker/worker_devto...
content/worker/worker_devtools_agent.cc:66: class SharedWorkerDevToolsAgent :
public WorkerDevToolsAgent {
On 2011/07/11 16:53:37, John Abd-El-Malek wrote:
> On 2011/07/11 15:13:11, Yury Semikhatsky wrote:
> > On 2011/07/08 17:49:09, John Abd-El-Malek wrote:
> > > It's unfortunate that this class is duplicated for both worker types. Why
> > can't
> > > the debugger related methods on We worker and WebSharedWorker derive from
a
> > base
> > > class for this purpose? Otherwise a template can be used.
> > 
> > WebWorker and WebSharedWorker don't have common ancestor, otherwise I would
> have
> > put these methods there. I tried to avoid a new base class for workers but
if
> > you think that it would be justified, I could do that. But even in that case
> > OnMessageReceived would have two different implementations as it handles
> > DevToolsAgentMsg_DispatchOnInspectorBackend in one case and
> > WorkerDevToolsAgentMsg_DispatchOnInspectorBackend in another. 
> 
> You can send DevToolsAgentMsg_DispatchOnInspectorBackend in both cases. It's
> common for messages that are sent to different child processes to reuse the
same
> message class, instead of duplicating it.
> 
> > Also
> > SendDevToolsMessage should be different for shared and dedicated workers.
> 
> This one function would be different, but the rest of the 50 line class would
be
> the same.
> >  Given
> > all this I don't think it is worth complicating the code with templates just
> to
> > avoid code duplication in 2 one-line methods.
> 

Done. Converted the class into template and reused
DevToolsAgentMsg_DispatchOnInspectorBackend for both shared and dedicated worker
processes.

[jam, 2011-07-12 18:19:55]

lgtm

http://codereview.chromium.org/7248076/diff/22001/content/worker/worker_devto...
File content/worker/worker_devtools_agent.cc (right):

http://codereview.chromium.org/7248076/diff/22001/content/worker/worker_devto...
content/worker/worker_devtools_agent.cc:65: void
WorkerDevToolsAgentImpl<WebWorker>::SendDevToolsMessage(
nit: it might be easier to read if the constructor takes a boolean for
shared/dedicated, and uses that to send different messages..