DevTools: don't show the unresponsive dialog for inspected tabs

DevTools: don't show the unresponsive dialog for inspected tabs


BUG=66458
TEST=DevToolsManagerTest.NoUnresponsiveDialogInInspectedTab

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

[jam, 2011-08-03 16:17:05]

tab_contents.cc lgtm

[pfeldman1, 2011-08-03 16:28:49]

http://codereview.chromium.org/7565008/diff/4001/content/browser/debugger/dev...
File content/browser/debugger/devtools_manager.h (right):

http://codereview.chromium.org/7565008/diff/4001/content/browser/debugger/dev...
content/browser/debugger/devtools_manager.h:67: bool
IsBeingDebugged(RenderViewHost* inspected_rvh);
Call site could use GetDevToolsClientHostFor instead.

http://codereview.chromium.org/7565008/diff/4001/content/browser/debugger/dev...
File content/browser/debugger/devtools_manager_unittest.cc (right):

http://codereview.chromium.org/7565008/diff/4001/content/browser/debugger/dev...
content/browser/debugger/devtools_manager_unittest.cc:156: new
MessageLoop::QuitTask(), 10);
I suspect it'll flake due to timer resolution/error. Is there a way to use mock
timer?

http://codereview.chromium.org/7565008/diff/4001/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/7565008/diff/4001/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:1765: if (devtools_manager &&
devtools_manager->IsBeingDebugged(rvh))
What if the page has in fact hit the infinite loop? Consider the following
scenario:

- developer observes watchdog
- he wants to investigate the cause
- he opens devtools and watchdog vanishes

I think we should send the heart beat from the renderer (once a second or so)
while paused. That way we will reset the watchdog in browser and it will never
hit the limit while paused.

[Paweł Hajdan Jr., 2011-08-03 16:50:29]

Drive-by with testing comments (chrome/test/OWNERS, you need approval for
modifying TestingBrowserProcess).

http://codereview.chromium.org/7565008/diff/4001/content/browser/debugger/dev...
File content/browser/debugger/devtools_manager_unittest.cc (right):

http://codereview.chromium.org/7565008/diff/4001/content/browser/debugger/dev...
content/browser/debugger/devtools_manager_unittest.cc:146:
testing_browser_process_.get()->SetDevToolsManager(manager.get());
How about making TestingBrowserProcess own DevToolsManager? That seems much
safer to me and also more following the existing convention.

http://codereview.chromium.org/7565008/diff/4001/content/browser/debugger/dev...
content/browser/debugger/devtools_manager_unittest.cc:156: new
MessageLoop::QuitTask(), 10);
On 2011/08/03 16:28:50, pfeldman1 wrote:
> I suspect it'll flake due to timer resolution/error. Is there a way to use
mock
> timer?

Yeah, this is a bit suspicious. I'd suggest removing the "not received" test
here (there's not really a good way to write those), and also see my suggestion
below.

http://codereview.chromium.org/7565008/diff/4001/content/browser/debugger/dev...
content/browser/debugger/devtools_manager_unittest.cc:165:
MessageLoop::current()->PostDelayedTask(FROM_HERE,
How about just waiting for the hang monitor to kick in, i.e. wait for an event
and not for a fixed amount of time?

[yurys, 2011-08-04 08:59:14]

http://codereview.chromium.org/7565008/diff/4001/content/browser/debugger/dev...
File content/browser/debugger/devtools_manager.h (right):

http://codereview.chromium.org/7565008/diff/4001/content/browser/debugger/dev...
content/browser/debugger/devtools_manager.h:67: bool
IsBeingDebugged(RenderViewHost* inspected_rvh);
On 2011/08/03 16:28:50, pfeldman1 wrote:
> Call site could use GetDevToolsClientHostFor instead.

Done.

http://codereview.chromium.org/7565008/diff/4001/content/browser/debugger/dev...
File content/browser/debugger/devtools_manager_unittest.cc (right):

http://codereview.chromium.org/7565008/diff/4001/content/browser/debugger/dev...
content/browser/debugger/devtools_manager_unittest.cc:146:
testing_browser_process_.get()->SetDevToolsManager(manager.get());
On 2011/08/03 16:50:30, Paweł Hajdan Jr. wrote:
> How about making TestingBrowserProcess own DevToolsManager? That seems much
> safer to me and also more following the existing convention.

Done.

http://codereview.chromium.org/7565008/diff/4001/content/browser/debugger/dev...
content/browser/debugger/devtools_manager_unittest.cc:156: new
MessageLoop::QuitTask(), 10);
On 2011/08/03 16:50:30, Paweł Hajdan Jr. wrote:
> On 2011/08/03 16:28:50, pfeldman1 wrote:
> > I suspect it'll flake due to timer resolution/error. Is there a way to use
> mock
> > timer?
> 
> Yeah, this is a bit suspicious. I'd suggest removing the "not received" test
> here (there's not really a good way to write those), and also see my
suggestion
> below.
The whole point of this test is to check that the unresponsive dialog is not
shown when the hung tab is being debugged. Removing this means that we can
remove this test completely and leave this functionality untested.

http://codereview.chromium.org/7565008/diff/4001/content/browser/debugger/dev...
content/browser/debugger/devtools_manager_unittest.cc:156: new
MessageLoop::QuitTask(), 10);
On 2011/08/03 16:28:50, pfeldman1 wrote:
> I suspect it'll flake due to timer resolution/error. Is there a way to use
mock
> timer?

This is how StopAndStartHangMonitorTimeout
test(http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/browser/renderer_host/render_widget_host_unittest.cc&exact_package=chromium&q=StopAndStartHangMonitorTimeout&type=cs&l=722)
works and the only guarantee we need  here is that the task posted later will
fire later.

http://codereview.chromium.org/7565008/diff/4001/content/browser/debugger/dev...
content/browser/debugger/devtools_manager_unittest.cc:165:
MessageLoop::current()->PostDelayedTask(FROM_HERE,
On 2011/08/03 16:50:30, Paweł Hajdan Jr. wrote:
> How about just waiting for the hang monitor to kick in, i.e. wait for an event
> and not for a fixed amount of time?
It would work if we were testing the watchdog timer but here we'd like to make
sure that the event is handled by the tab_contets properly after devtools client
has detached from the tab.

[pfeldman1, 2011-08-04 10:11:32]

lgtm

http://codereview.chromium.org/7565008/diff/6001/content/browser/debugger/dev...
File content/browser/debugger/devtools_manager_unittest.cc (right):

http://codereview.chromium.org/7565008/diff/6001/content/browser/debugger/dev...
content/browser/debugger/devtools_manager_unittest.cc:170:
testing_browser_process_.get()->SetDevToolsManager(NULL);
You should not need this now.

[yurys, 2011-08-04 11:39:19]

http://codereview.chromium.org/7565008/diff/6001/content/browser/debugger/dev...
File content/browser/debugger/devtools_manager_unittest.cc (right):

http://codereview.chromium.org/7565008/diff/6001/content/browser/debugger/dev...
content/browser/debugger/devtools_manager_unittest.cc:170:
testing_browser_process_.get()->SetDevToolsManager(NULL);
On 2011/08/04 10:11:32, pfeldman1 wrote:
> You should not need this now.

Done.

[yurys, 2011-08-05 06:15:12]

Paweł are you OK with the changes to TestingBrowserProcess ?

[Paweł Hajdan Jr., 2011-08-05 18:02:55]

LGTM

I'm not really happy with the way the unit test is written, but feel free to
test your luck. ;-)