Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7824039:
Implement function that hides the WebUI Hung Renderer Dialog. (Closed)
|
Created:
9 years, 3 months ago by wyck Modified:
9 years, 3 months ago CC:
chromium-reviews, arv (Not doing code reviews) Visibility:
Public. |
DescriptionImplement function that hides the WebUI Hung Renderer Dialog.
BUG=None
TEST=When frozen tab becomes reponsive, the Hung Renderer Dialog should disappear.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99877
Patch Set 1 : self review #
Total comments: 8
Patch Set 2 : Fixed timeout behaviour. #
Total comments: 10
Patch Set 3 : flackr's review issues #
Messages
Total messages: 9 (0 generated)
A little one! :) http://codereview.chromium.org/7824039/diff/1007/chrome/browser/ui/webui/hung... File chrome/browser/ui/webui/hung_renderer_dialog.cc (right): http://codereview.chromium.org/7824039/diff/1007/chrome/browser/ui/webui/hung... chrome/browser/ui/webui/hung_renderer_dialog.cc:106: DLOG(INFO) << "json does not describe a valid result"; This is now reached when the dialog is closed without clicking a button. http://codereview.chromium.org/7824039/diff/1007/chrome/browser/ui/webui/hung... File chrome/browser/ui/webui/hung_renderer_dialog.h (right): http://codereview.chromium.org/7824039/diff/1007/chrome/browser/ui/webui/hung... chrome/browser/ui/webui/hung_renderer_dialog.h:72: lint wants a newline at the the end.
http://codereview.chromium.org/7824039/diff/1007/chrome/browser/ui/webui/hung... File chrome/browser/ui/webui/hung_renderer_dialog.cc (right): http://codereview.chromium.org/7824039/diff/1007/chrome/browser/ui/webui/hung... chrome/browser/ui/webui/hung_renderer_dialog.cc:106: DLOG(INFO) << "json does not describe a valid result"; On 2011/09/02 15:57:48, wyck wrote: > This is now reached when the dialog is closed without clicking a button. Since this is an expected case you should remove this log message (or change it to sound less like an error case) and comment this if block to say that it checks if the user chose an action. http://codereview.chromium.org/7824039/diff/1007/chrome/browser/ui/webui/hung... chrome/browser/ui/webui/hung_renderer_dialog.cc:118: if (contents_ && contents_->render_view_host()) When the dialog is closed by HideDialog, won't this path be triggered (by the default response value false), in which case it will try to restart a hang monitor timeout for the already responsive render view host? http://codereview.chromium.org/7824039/diff/1007/chrome/browser/ui/webui/hung... chrome/browser/ui/webui/hung_renderer_dialog.cc:168: } Was lint okay with not having a newline here even though it wanted one in the .h file?
On 2011/09/02 16:39:55, flackr wrote: > http://codereview.chromium.org/7824039/diff/1007/chrome/browser/ui/webui/hung... > File chrome/browser/ui/webui/hung_renderer_dialog.cc (right): > > http://codereview.chromium.org/7824039/diff/1007/chrome/browser/ui/webui/hung... > chrome/browser/ui/webui/hung_renderer_dialog.cc:106: DLOG(INFO) << "json does > not describe a valid result"; > On 2011/09/02 15:57:48, wyck wrote: > > This is now reached when the dialog is closed without clicking a button. > > Since this is an expected case you should remove this log message (or change it > to sound less like an error case) and comment this if block to say that it > checks if the user chose an action. > > http://codereview.chromium.org/7824039/diff/1007/chrome/browser/ui/webui/hung... > chrome/browser/ui/webui/hung_renderer_dialog.cc:118: if (contents_ && > contents_->render_view_host()) > When the dialog is closed by HideDialog, won't this path be triggered (by the > default response value false), in which case it will try to restart a hang > monitor timeout for the already responsive render view host? > > http://codereview.chromium.org/7824039/diff/1007/chrome/browser/ui/webui/hung... > chrome/browser/ui/webui/hung_renderer_dialog.cc:168: } > Was lint okay with not having a newline here even though it wanted one in the .h > file? No, lint is not okay with not having a newline here. With no newline, lint gives the error "Could not find a newline character at the end of the file. [whitespace/ending_newline] [5]" The same is not true for the .h file. The .h file can end without a newline and lint doesn't complain.
http://codereview.chromium.org/7824039/diff/1007/chrome/browser/ui/webui/hung... File chrome/browser/ui/webui/hung_renderer_dialog.cc (right): http://codereview.chromium.org/7824039/diff/1007/chrome/browser/ui/webui/hung... chrome/browser/ui/webui/hung_renderer_dialog.cc:106: DLOG(INFO) << "json does not describe a valid result"; On 2011/09/02 16:39:55, flackr wrote: > On 2011/09/02 15:57:48, wyck wrote: > > This is now reached when the dialog is closed without clicking a button. > > Since this is an expected case you should remove this log message (or change it > to sound less like an error case) and comment this if block to say that it > checks if the user chose an action. Done. http://codereview.chromium.org/7824039/diff/1007/chrome/browser/ui/webui/hung... chrome/browser/ui/webui/hung_renderer_dialog.cc:118: if (contents_ && contents_->render_view_host()) On 2011/09/02 16:39:55, flackr wrote: > When the dialog is closed by HideDialog, won't this path be triggered (by the > default response value false), in which case it will try to restart a hang > monitor timeout for the already responsive render view host? Fixed by setting |contents_| to NULL before closing in HideDialog (line 71-ish) http://codereview.chromium.org/7824039/diff/1007/chrome/browser/ui/webui/hung... chrome/browser/ui/webui/hung_renderer_dialog.cc:168: } On 2011/09/02 16:39:55, flackr wrote: > Was lint okay with not having a newline here even though it wanted one in the .h > file? Ya, lint didn't mind me removing the newline here. weird. http://codereview.chromium.org/7824039/diff/1012/chrome/browser/ui/webui/hung... File chrome/browser/ui/webui/hung_renderer_dialog.cc (right): http://codereview.chromium.org/7824039/diff/1012/chrome/browser/ui/webui/hung... chrome/browser/ui/webui/hung_renderer_dialog.cc:105: ListValue* list = NULL; Decided to initialize this too. http://codereview.chromium.org/7824039/diff/1012/chrome/browser/ui/webui/hung... chrome/browser/ui/webui/hung_renderer_dialog.cc:109: if (root.get() && root->GetAsList(&list) && list && Folded into one statement.
This is looking good. Is this the final CL to bring it to feature parity with the existing hung renderer dialog? http://codereview.chromium.org/7824039/diff/1012/chrome/browser/ui/webui/hung... File chrome/browser/ui/webui/hung_renderer_dialog.cc (right): http://codereview.chromium.org/7824039/diff/1012/chrome/browser/ui/webui/hung... chrome/browser/ui/webui/hung_renderer_dialog.cc:37: NOTIMPLEMENTED() << " ShowHungRendererDialog called twice."; This was the case that you said can occur if another renderer hangs while the dialog is up right? I know the GTK hung renderer dialog didn't handle this but if it can occur you should at least leave a TODO to either support showing more than one hung renderer in one dialog or allow more than one dialog to be shown if other renderers hang. http://codereview.chromium.org/7824039/diff/1012/chrome/browser/ui/webui/hung... chrome/browser/ui/webui/hung_renderer_dialog.cc:68: void HungRendererDialog::HideDialog(TabContents* contents) { You should check that (contents_->GetRenderProcessHost() == contents->GetRenderProcessHost()) to be sure HideDialog call was for this hung renderer. http://codereview.chromium.org/7824039/diff/1012/chrome/browser/ui/webui/hung... chrome/browser/ui/webui/hung_renderer_dialog.cc:110: list->GetBoolean(0, &response) && response) { Why not use the result from list->GetBoolean to know if the user pressed wait or not rather than set contents_ to NULL in HideDialog. i.e. if (root.get() && root->GetAsList(&list) && list && list->GetBoolean(0, &response)) { if (response) // Kill process. else // Restart timer. http://codereview.chromium.org/7824039/diff/1012/chrome/browser/ui/webui/hung... File chrome/browser/ui/webui/hung_renderer_dialog.h (right): http://codereview.chromium.org/7824039/diff/1012/chrome/browser/ui/webui/hung... chrome/browser/ui/webui/hung_renderer_dialog.h:21: HungRendererDialog(); I think you should make the constructor private and have ShowDialog be a public static method. That will provide a strong guarantee that when this class is instantiated the dialog window will also be shown.
http://codereview.chromium.org/7824039/diff/1012/chrome/browser/ui/webui/hung... File chrome/browser/ui/webui/hung_renderer_dialog.cc (right): http://codereview.chromium.org/7824039/diff/1012/chrome/browser/ui/webui/hung... chrome/browser/ui/webui/hung_renderer_dialog.cc:37: NOTIMPLEMENTED() << " ShowHungRendererDialog called twice."; On 2011/09/03 17:51:31, flackr wrote: > This was the case that you said can occur if another renderer hangs while the > dialog is up right? I know the GTK hung renderer dialog didn't handle this but > if it can occur you should at least leave a TODO to either support showing more > than one hung renderer in one dialog or allow more than one dialog to be shown > if other renderers hang. Now that 'hide' is implemented, this doesn't happen. http://codereview.chromium.org/7824039/diff/1012/chrome/browser/ui/webui/hung... chrome/browser/ui/webui/hung_renderer_dialog.cc:68: void HungRendererDialog::HideDialog(TabContents* contents) { On 2011/09/03 17:51:31, flackr wrote: > You should check that (contents_->GetRenderProcessHost() == > contents->GetRenderProcessHost()) to be sure HideDialog call was for this hung > renderer. Done. http://codereview.chromium.org/7824039/diff/1012/chrome/browser/ui/webui/hung... chrome/browser/ui/webui/hung_renderer_dialog.cc:110: list->GetBoolean(0, &response) && response) { On 2011/09/03 17:51:31, flackr wrote: > Why not use the result from list->GetBoolean to know if the user pressed wait or > not rather than set contents_ to NULL in HideDialog. i.e. > if (root.get() && root->GetAsList(&list) && list && > list->GetBoolean(0, &response)) { > if (response) > // Kill process. > else > // Restart timer. Because if you click the X on the dialog, it closes with no response, which is to be interpreted as "do the default action (wait)". But if you kill the dialog, it also closes with no response, which is to be interpreted as "do nothing." So rather than set a flag in the class indicating that it is being closed externally rather than by the user, I clear the contents_ member variable (which serves as such a flag.) That's what the comment on line 69 is getting at. http://codereview.chromium.org/7824039/diff/1012/chrome/browser/ui/webui/hung... File chrome/browser/ui/webui/hung_renderer_dialog.h (right): http://codereview.chromium.org/7824039/diff/1012/chrome/browser/ui/webui/hung... chrome/browser/ui/webui/hung_renderer_dialog.h:21: HungRendererDialog(); On 2011/09/03 17:51:31, flackr wrote: > I think you should make the constructor private and have ShowDialog be a public > static method. That will provide a strong guarantee that when this class is > instantiated the dialog window will also be shown. Done.
LGTM
LGTM
Change committed as 99877 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
