Fix a bug where the inspector would not keep event pages alive.

Fix a bug where the inspector would not keep event pages alive.

The notification that an inspector window was opened was not always being sent.

BUG=143284
TBR=creis@chromium.org,pkasting@chromium.org


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

[yurys, 2012-08-17 07:05:01]

http://codereview.chromium.org/10837296/diff/3001/content/browser/debugger/de...
File content/browser/debugger/devtools_agent_host.h (right):

http://codereview.chromium.org/10837296/diff/3001/content/browser/debugger/de...
content/browser/debugger/devtools_agent_host.h:38: virtual void
NotifyClientOpening() = 0;
Is there any reason for not sending this notification from
DevToolsAgentHost::Attach? That way it would work for remote debugging as well,
not only for the in browser devtools.

[Matt Perry, 2012-08-17 23:11:46]

http://codereview.chromium.org/10837296/diff/3001/content/browser/debugger/de...
File content/browser/debugger/devtools_agent_host.h (right):

http://codereview.chromium.org/10837296/diff/3001/content/browser/debugger/de...
content/browser/debugger/devtools_agent_host.h:38: virtual void
NotifyClientOpening() = 0;
On 2012/08/17 07:05:01, Yury Semikhatsky wrote:
> Is there any reason for not sending this notification from
> DevToolsAgentHost::Attach? That way it would work for remote debugging as
well,
> not only for the in browser devtools.

I'm not really familiar with the devtools code. If you think that's a better
place to put it, I can move it there. Should I also move NotifyClientClosing()
to DevToolsAgentHost::Detach?

To be clear, these notifications are sent to indicate that a devtools UI window
has opened/closed.

[yurys, 2012-08-20 07:44:13]

On 2012/08/17 23:11:46, Matt Perry wrote:
>
http://codereview.chromium.org/10837296/diff/3001/content/browser/debugger/de...
> File content/browser/debugger/devtools_agent_host.h (right):
> 
>
http://codereview.chromium.org/10837296/diff/3001/content/browser/debugger/de...
> content/browser/debugger/devtools_agent_host.h:38: virtual void
> NotifyClientOpening() = 0;
> On 2012/08/17 07:05:01, Yury Semikhatsky wrote:
> > Is there any reason for not sending this notification from
> > DevToolsAgentHost::Attach? That way it would work for remote debugging as
> well,
> > not only for the in browser devtools.
> 
> I'm not really familiar with the devtools code. If you think that's a better
> place to put it, I can move it there. Should I also move NotifyClientClosing()
> to DevToolsAgentHost::Detach?
> 
> To be clear, these notifications are sent to indicate that a devtools UI
window
> has opened/closed.

There was a problem with merging NotifyClientClosing into Detach(I guess the
notification may not be sent is some cases when inspected process shuts down)
so we left it.

Whenever a devtools client attaches to an agent either DevToolsAgentHost::Attach
or Reattach will be called. So if you need to show a UI notification that a
given
RVH is being inspected it would be a good place for the logic and the clients
wouldn't
need to call NotifyClientOpening.

[Matt Perry, 2012-08-20 18:21:53]

On 2012/08/20 07:44:13, Yury Semikhatsky wrote:
> On 2012/08/17 23:11:46, Matt Perry wrote:
> >
>
http://codereview.chromium.org/10837296/diff/3001/content/browser/debugger/de...
> > File content/browser/debugger/devtools_agent_host.h (right):
> > 
> >
>
http://codereview.chromium.org/10837296/diff/3001/content/browser/debugger/de...
> > content/browser/debugger/devtools_agent_host.h:38: virtual void
> > NotifyClientOpening() = 0;
> > On 2012/08/17 07:05:01, Yury Semikhatsky wrote:
> > > Is there any reason for not sending this notification from
> > > DevToolsAgentHost::Attach? That way it would work for remote debugging as
> > well,
> > > not only for the in browser devtools.
> > 
> > I'm not really familiar with the devtools code. If you think that's a better
> > place to put it, I can move it there. Should I also move
NotifyClientClosing()
> > to DevToolsAgentHost::Detach?
> > 
> > To be clear, these notifications are sent to indicate that a devtools UI
> window
> > has opened/closed.
> 
> There was a problem with merging NotifyClientClosing into Detach(I guess the
> notification may not be sent is some cases when inspected process shuts down)
> so we left it.
> 
> Whenever a devtools client attaches to an agent either
DevToolsAgentHost::Attach
> or Reattach will be called. So if you need to show a UI notification that a
> given
> RVH is being inspected it would be a good place for the logic and the clients
> wouldn't
> need to call NotifyClientOpening.

OK, I'll move it there. But for every call to NotifyClientOpening, there needs
to be a call to NotifyClientClosing (and vice versa). Does moving it to Attach
guarantee that?

[Matt Perry, 2012-08-20 19:38:41]

On 2012/08/20 18:21:53, Matt Perry wrote:
> On 2012/08/20 07:44:13, Yury Semikhatsky wrote:
> > On 2012/08/17 23:11:46, Matt Perry wrote:
> > >
> >
>
http://codereview.chromium.org/10837296/diff/3001/content/browser/debugger/de...
> > > File content/browser/debugger/devtools_agent_host.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10837296/diff/3001/content/browser/debugger/de...
> > > content/browser/debugger/devtools_agent_host.h:38: virtual void
> > > NotifyClientOpening() = 0;
> > > On 2012/08/17 07:05:01, Yury Semikhatsky wrote:
> > > > Is there any reason for not sending this notification from
> > > > DevToolsAgentHost::Attach? That way it would work for remote debugging
as
> > > well,
> > > > not only for the in browser devtools.
> > > 
> > > I'm not really familiar with the devtools code. If you think that's a
better
> > > place to put it, I can move it there. Should I also move
> NotifyClientClosing()
> > > to DevToolsAgentHost::Detach?
> > > 
> > > To be clear, these notifications are sent to indicate that a devtools UI
> > window
> > > has opened/closed.
> > 
> > There was a problem with merging NotifyClientClosing into Detach(I guess the
> > notification may not be sent is some cases when inspected process shuts
down)
> > so we left it.
> > 
> > Whenever a devtools client attaches to an agent either
> DevToolsAgentHost::Attach
> > or Reattach will be called. So if you need to show a UI notification that a
> > given
> > RVH is being inspected it would be a good place for the logic and the
clients
> > wouldn't
> > need to call NotifyClientOpening.
> 
> OK, I'll move it there. But for every call to NotifyClientOpening, there needs
> to be a call to NotifyClientClosing (and vice versa). Does moving it to Attach
> guarantee that?

To answer my own question, from what I can tell, it doesn't. Attach is called in
many more places than when a window is opened. (looking through
http://code.google.com/p/chromium/source/search?q=RegisterDevToolsClientHostF...
)

I propose we leave the open notification in DevToolsWindow, since that is what
dispatches the closing notification.

[yurys, 2012-08-21 09:39:57]

On 2012/08/20 19:38:41, Matt Perry wrote:
> On 2012/08/20 18:21:53, Matt Perry wrote:
> > On 2012/08/20 07:44:13, Yury Semikhatsky wrote:
> > > On 2012/08/17 23:11:46, Matt Perry wrote:
> > > >
> > >
> >
>
http://codereview.chromium.org/10837296/diff/3001/content/browser/debugger/de...
> > > > File content/browser/debugger/devtools_agent_host.h (right):
> > > > 
> > > >
> > >
> >
>
http://codereview.chromium.org/10837296/diff/3001/content/browser/debugger/de...
> > > > content/browser/debugger/devtools_agent_host.h:38: virtual void
> > > > NotifyClientOpening() = 0;
> > > > On 2012/08/17 07:05:01, Yury Semikhatsky wrote:
> > > > > Is there any reason for not sending this notification from
> > > > > DevToolsAgentHost::Attach? That way it would work for remote debugging
> as
> > > > well,
> > > > > not only for the in browser devtools.
> > > > 
> > > > I'm not really familiar with the devtools code. If you think that's a
> better
> > > > place to put it, I can move it there. Should I also move
> > NotifyClientClosing()
> > > > to DevToolsAgentHost::Detach?
> > > > 
> > > > To be clear, these notifications are sent to indicate that a devtools UI
> > > window
> > > > has opened/closed.
> > > 
> > > There was a problem with merging NotifyClientClosing into Detach(I guess
the
> > > notification may not be sent is some cases when inspected process shuts
> down)
> > > so we left it.
> > > 
> > > Whenever a devtools client attaches to an agent either
> > DevToolsAgentHost::Attach
> > > or Reattach will be called. So if you need to show a UI notification that
a
> > > given
> > > RVH is being inspected it would be a good place for the logic and the
> clients
> > > wouldn't
> > > need to call NotifyClientOpening.
> > 
> > OK, I'll move it there. But for every call to NotifyClientOpening, there
needs
> > to be a call to NotifyClientClosing (and vice versa). Does moving it to
Attach
> > guarantee that?
> 
> To answer my own question, from what I can tell, it doesn't. Attach is called
in
> many more places than when a window is opened. (looking through
>
http://code.google.com/p/chromium/source/search?q=RegisterDevToolsClientHostF...
> )
> 
> I propose we leave the open notification in DevToolsWindow, since that is what
> dispatches the closing notification.

This won't work for remote debugging where different DevToolsClientHost
implementation is used. Ideally there shouldn't be this NotifyClientClosing at
all. DevToolsAgentHost can be in two states: a client is attached to it or there
is no attached client. We can make DevToolsManager send a notification whenever
a client is de/attached to agent. There should be extra pair of Detach/Reattach
in case of cross-process navigation which may cause some problems in your case.
Could you describe the way the notification is handled? As I understand it is
used to prevent extension popup from closing while it is being inspected in
which case sending this attach/detach notifications should be enough but I may
be missing something.

[Matt Perry, 2012-08-21 18:50:04]

On 2012/08/21 09:39:57, Yury Semikhatsky wrote:
> On 2012/08/20 19:38:41, Matt Perry wrote:
> > On 2012/08/20 18:21:53, Matt Perry wrote:
> > > On 2012/08/20 07:44:13, Yury Semikhatsky wrote:
> > > > On 2012/08/17 23:11:46, Matt Perry wrote:
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/10837296/diff/3001/content/browser/debugger/de...
> > > > > File content/browser/debugger/devtools_agent_host.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/10837296/diff/3001/content/browser/debugger/de...
> > > > > content/browser/debugger/devtools_agent_host.h:38: virtual void
> > > > > NotifyClientOpening() = 0;
> > > > > On 2012/08/17 07:05:01, Yury Semikhatsky wrote:
> > > > > > Is there any reason for not sending this notification from
> > > > > > DevToolsAgentHost::Attach? That way it would work for remote
debugging
> > as
> > > > > well,
> > > > > > not only for the in browser devtools.
> > > > > 
> > > > > I'm not really familiar with the devtools code. If you think that's a
> > better
> > > > > place to put it, I can move it there. Should I also move
> > > NotifyClientClosing()
> > > > > to DevToolsAgentHost::Detach?
> > > > > 
> > > > > To be clear, these notifications are sent to indicate that a devtools
UI
> > > > window
> > > > > has opened/closed.
> > > > 
> > > > There was a problem with merging NotifyClientClosing into Detach(I guess
> the
> > > > notification may not be sent is some cases when inspected process shuts
> > down)
> > > > so we left it.
> > > > 
> > > > Whenever a devtools client attaches to an agent either
> > > DevToolsAgentHost::Attach
> > > > or Reattach will be called. So if you need to show a UI notification
that
> a
> > > > given
> > > > RVH is being inspected it would be a good place for the logic and the
> > clients
> > > > wouldn't
> > > > need to call NotifyClientOpening.
> > > 
> > > OK, I'll move it there. But for every call to NotifyClientOpening, there
> needs
> > > to be a call to NotifyClientClosing (and vice versa). Does moving it to
> Attach
> > > guarantee that?
> > 
> > To answer my own question, from what I can tell, it doesn't. Attach is
called
> in
> > many more places than when a window is opened. (looking through
> >
>
http://code.google.com/p/chromium/source/search?q=RegisterDevToolsClientHostF...
> > )
> > 
> > I propose we leave the open notification in DevToolsWindow, since that is
what
> > dispatches the closing notification.
> 
> This won't work for remote debugging where different DevToolsClientHost
> implementation is used. Ideally there shouldn't be this NotifyClientClosing at
> all. DevToolsAgentHost can be in two states: a client is attached to it or
there
> is no attached client. We can make DevToolsManager send a notification
whenever
> a client is de/attached to agent. There should be extra pair of
Detach/Reattach
> in case of cross-process navigation which may cause some problems in your
case.
> Could you describe the way the notification is handled? As I understand it is
> used to prevent extension popup from closing while it is being inspected in
> which case sending this attach/detach notifications should be enough but I may
> be missing something.

That's correct - it keeps popups alive. It now also keeps event pages alive.
Ideally, we want notifications whenever a devtools agent is attached or
detached. It would seem that DevToolsAgentHost::Attach/Detach are the right
places for these notifications. You said there was a problem with Detach
sometimes not firing - but it looks to me that Detach is called almost right
after NotifyClientClosing.

I'll try moving both notifications, and do some testing to ensure it works.

[Matt Perry, 2012-08-21 19:34:45]

OK, new version with the notifications moved to Attach/Detach. I also renamed
them to reflect what they do. It seems to work fine.

[yurys, 2012-08-21 19:52:55]

A few comments but I like the new approach.

http://codereview.chromium.org/10837296/diff/13/chrome/browser/debugger/devto...
File chrome/browser/debugger/devtools_window.cc (right):

http://codereview.chromium.org/10837296/diff/13/chrome/browser/debugger/devto...
chrome/browser/debugger/devtools_window.cc:646: if (!window->is_docked() ||
do_open) {
Revert this? As I remember style guide doesn't require braces for one line body.

http://codereview.chromium.org/10837296/diff/13/content/browser/debugger/devt...
File content/browser/debugger/devtools_agent_host.h (right):

http://codereview.chromium.org/10837296/diff/13/content/browser/debugger/devt...
content/browser/debugger/devtools_agent_host.h:38: virtual void
NotifyClientOpening() = 0;
These two notification methods should be protected or private now that they are
only called from DevToolsAgentHost. I'd probably rename these methods to
NotifyClientAttachingDetaching.

http://codereview.chromium.org/10837296/diff/13/content/browser/debugger/devt...
content/browser/debugger/devtools_agent_host.h:39: // TODO(yurys): get rid of
this method
Please remove this comment.

http://codereview.chromium.org/10837296/diff/13/content/public/browser/notifi...
File content/public/browser/notification_types.h (right):

http://codereview.chromium.org/10837296/diff/13/content/public/browser/notifi...
content/public/browser/notification_types.h:166: // Indicates that a devtools
window is opening. The source is the
Please update the comment. At least type of the details object has changed.

[yurys, 2012-08-21 20:08:54]

http://codereview.chromium.org/10837296/diff/13/content/public/browser/notifi...
File content/public/browser/notification_types.h (right):

http://codereview.chromium.org/10837296/diff/13/content/public/browser/notifi...
content/public/browser/notification_types.h:166: // Indicates that a devtools
window is opening. The source is the
On 2012/08/21 19:52:55, Yury Semikhatsky wrote:
> Please update the comment. At least type of the details object has changed.
I was wrong, it's still RVH. Ignore this comment.

[Matt Perry, 2012-08-21 20:13:46]

http://codereview.chromium.org/10837296/diff/13/chrome/browser/debugger/devto...
File chrome/browser/debugger/devtools_window.cc (right):

http://codereview.chromium.org/10837296/diff/13/chrome/browser/debugger/devto...
chrome/browser/debugger/devtools_window.cc:646: if (!window->is_docked() ||
do_open) {
On 2012/08/21 19:52:55, Yury Semikhatsky wrote:
> Revert this? As I remember style guide doesn't require braces for one line
body.

Oops, leftover change. Reverted.

http://codereview.chromium.org/10837296/diff/13/content/browser/debugger/devt...
File content/browser/debugger/devtools_agent_host.h (right):

http://codereview.chromium.org/10837296/diff/13/content/browser/debugger/devt...
content/browser/debugger/devtools_agent_host.h:38: virtual void
NotifyClientOpening() = 0;
On 2012/08/21 19:52:55, Yury Semikhatsky wrote:
> These two notification methods should be protected or private now that they
are
> only called from DevToolsAgentHost. I'd probably rename these methods to
> NotifyClientAttachingDetaching.

Done.

http://codereview.chromium.org/10837296/diff/13/content/browser/debugger/devt...
content/browser/debugger/devtools_agent_host.h:39: // TODO(yurys): get rid of
this method
On 2012/08/21 19:52:55, Yury Semikhatsky wrote:
> Please remove this comment.

Done.

http://codereview.chromium.org/10837296/diff/13/content/public/browser/notifi...
File content/public/browser/notification_types.h (right):

http://codereview.chromium.org/10837296/diff/13/content/public/browser/notifi...
content/public/browser/notification_types.h:166: // Indicates that a devtools
window is opening. The source is the
On 2012/08/21 19:52:55, Yury Semikhatsky wrote:
> Please update the comment. At least type of the details object has changed.

Updated comment. But the details are still the RVH.

[yurys, 2012-08-21 20:44:26]

lgtm

[Matt Perry, 2012-08-21 21:12:22]

pkasting: browser/ui OWNERS (just a rename)
creis: content OWNERS (ditto)

[commit-bot: I haz the power, 2012-08-21 21:12:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpcomplete@chromium.org/10837296/13002

[Charlie Reis, 2012-08-21 22:24:54]

LGTM

[commit-bot: I haz the power, 2012-08-21 23:11:20]

Try job failure for 10837296-13002 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-21 23:16:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpcomplete@chromium.org/10837296/13002

[commit-bot: I haz the power, 2012-08-22 04:15:15]

Change committed as 152733