Add support for invoking the Windows 8 metro style hung renderer dialog box. The dialog box

Add support for invoking the Windows 8 metro style hung renderer dialog box. This functionality is
provided by the newly added class HungRendererDialogMetro. 

The actual dialog box is displayed by the metro driver.dll and the functionality to create and display
this is invoked via the exported functions ShowDialogBox and DismissDialogBox. The handlers to be run when
the user clicks on the kill process/wait buttons are passed into the ShowDialogBox function.

Factored out the platform specific portion of the kill process functionality in the static function
HungRendererDialogView::KillRendererProcess which is conditionally compiled for the platform.

BUG=125672
R=sky
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=148258

[sky, 2012-07-24 04:25:09]

https://chromiumcodereview.appspot.com/10806079/diff/13001/chrome/browser/ui/...
File chrome/browser/ui/views/hung_renderer_view_win.cc (right):

https://chromiumcodereview.appspot.com/10806079/diff/13001/chrome/browser/ui/...
chrome/browser/ui/views/hung_renderer_view_win.cc:46: if
(!base::win::IsMetroProcess()) {
There is no reason for subclassing here. Basically you have a big conditional.
If !metro do one thing, otherwise do something totally different. This
conditional should be up leveled to the create. I'm thinking something like:

void ShowHungRendererDialog(WebContents* contents) {
  if (ShowHungRendererDialogMetro(contents))
    return;
  // create HungRendererDialogView
}

#if !defined(OS_WIN)
bool ShowHungRendererDialogMetro(xxx) {
  return false;
#endif

In hung_renderer_dialog_view_win.cc:

bool ShowHungRendererDialogMetro(xxx) {
  if (IsMetro() && IsFrameActive()) {
    HungRendererDialogViewMetro::Show();
    return true;
  }
  return false;
}

Whatever hung_rendderer_dialog_view_win has in it does not subclass
HungRendererDialogView, it is its own class (or maybe just some functions, not
sure which makes more sense).

[ananta, 2012-07-24 18:43:48]

https://chromiumcodereview.appspot.com/10806079/diff/13001/chrome/browser/ui/...
File chrome/browser/ui/views/hung_renderer_view_win.cc (right):

https://chromiumcodereview.appspot.com/10806079/diff/13001/chrome/browser/ui/...
chrome/browser/ui/views/hung_renderer_view_win.cc:46: if
(!base::win::IsMetroProcess()) {
On 2012/07/24 04:25:09, sky wrote:
> There is no reason for subclassing here. Basically you have a big conditional.
> If !metro do one thing, otherwise do something totally different. This
> conditional should be up leveled to the create. I'm thinking something like:
> 
> void ShowHungRendererDialog(WebContents* contents) {
>   if (ShowHungRendererDialogMetro(contents))
>     return;
>   // create HungRendererDialogView
> }
> 
> #if !defined(OS_WIN)
> bool ShowHungRendererDialogMetro(xxx) {
>   return false;
> #endif
> 
> In hung_renderer_dialog_view_win.cc:
> 
> bool ShowHungRendererDialogMetro(xxx) {
>   if (IsMetro() && IsFrameActive()) {
>     HungRendererDialogViewMetro::Show();
>     return true;
>   }
>   return false;
> }
> 
> Whatever hung_rendderer_dialog_view_win has in it does not subclass
> HungRendererDialogView, it is its own class (or maybe just some functions, not
> sure which makes more sense).

Done. The functionality to override the default hung renderer dialog is provided
via the functions PlatformShowCustomHungRendererDialog and
PlatformHideCustomHungRendererDialog

The functions return false by default to indicate that the default hang dialog
is to be shown. For metro we return true.

The functionality to display the metro dialog is provided by the new class
HungRendererDialogMetro

[sky, 2012-07-24 21:26:04]

https://chromiumcodereview.appspot.com/10806079/diff/10003/chrome/browser/ui/...
File chrome/browser/ui/views/hung_renderer_view_win.cc (right):

https://chromiumcodereview.appspot.com/10806079/diff/10003/chrome/browser/ui/...
chrome/browser/ui/views/hung_renderer_view_win.cc:28: static
HungRendererDialogMetro g_instance;
Static types can't be objects.

https://chromiumcodereview.appspot.com/10806079/diff/10003/chrome/browser/ui/...
chrome/browser/ui/views/hung_renderer_view_win.cc:139:
base::Bind(HungRendererDialogMetro::OnMetroWait));
This is dangerous. How do you know another Show hasn't come in by the time this
is invoked?

https://chromiumcodereview.appspot.com/10806079/diff/10003/chrome/browser/ui/...
File chrome/browser/ui/views/hung_renderer_view_win.h (right):

https://chromiumcodereview.appspot.com/10806079/diff/10003/chrome/browser/ui/...
chrome/browser/ui/views/hung_renderer_view_win.h:12: using content::WebContents;
no using in headers.

https://chromiumcodereview.appspot.com/10806079/diff/10003/chrome/browser/ui/...
chrome/browser/ui/views/hung_renderer_view_win.h:19: virtual
~HungRendererDialogMetro();
no virtual.

https://chromiumcodereview.appspot.com/10806079/diff/10003/chrome/browser/ui/...
chrome/browser/ui/views/hung_renderer_view_win.h:21: virtual void
Show(WebContents* contents);
no virtual on these.

https://chromiumcodereview.appspot.com/10806079/diff/10003/chrome/browser/ui/...
chrome/browser/ui/views/hung_renderer_view_win.h:39: };
DISALLOW_...

[ananta, 2012-07-24 21:36:17]

https://chromiumcodereview.appspot.com/10806079/diff/10003/chrome/browser/ui/...
File chrome/browser/ui/views/hung_renderer_view_win.cc (right):

https://chromiumcodereview.appspot.com/10806079/diff/10003/chrome/browser/ui/...
chrome/browser/ui/views/hung_renderer_view_win.cc:28: static
HungRendererDialogMetro g_instance;
On 2012/07/24 21:26:05, sky wrote:
> Static types can't be objects.

Done.

https://chromiumcodereview.appspot.com/10806079/diff/10003/chrome/browser/ui/...
chrome/browser/ui/views/hung_renderer_view_win.cc:139:
base::Bind(HungRendererDialogMetro::OnMetroWait));
On 2012/07/24 21:26:05, sky wrote:
> This is dangerous. How do you know another Show hasn't come in by the time
this
> is invoked?
The metro_dialog_displayed_ Boolean flag prevents us from seeing multiple Shows.

https://chromiumcodereview.appspot.com/10806079/diff/10003/chrome/browser/ui/...
File chrome/browser/ui/views/hung_renderer_view_win.h (right):

https://chromiumcodereview.appspot.com/10806079/diff/10003/chrome/browser/ui/...
chrome/browser/ui/views/hung_renderer_view_win.h:12: using content::WebContents;
On 2012/07/24 21:26:05, sky wrote:
> no using in headers.

Done.

https://chromiumcodereview.appspot.com/10806079/diff/10003/chrome/browser/ui/...
chrome/browser/ui/views/hung_renderer_view_win.h:19: virtual
~HungRendererDialogMetro();
On 2012/07/24 21:26:05, sky wrote:
> no virtual.

Done.

https://chromiumcodereview.appspot.com/10806079/diff/10003/chrome/browser/ui/...
chrome/browser/ui/views/hung_renderer_view_win.h:21: virtual void
Show(WebContents* contents);
On 2012/07/24 21:26:05, sky wrote:
> no virtual on these.

Artifacts from the previous iteration :(. Removed.

https://chromiumcodereview.appspot.com/10806079/diff/10003/chrome/browser/ui/...
chrome/browser/ui/views/hung_renderer_view_win.h:39: };
On 2012/07/24 21:26:05, sky wrote:
> DISALLOW_...

Done.

[sky, 2012-07-24 22:57:02]

https://chromiumcodereview.appspot.com/10806079/diff/15006/chrome/browser/ui/...
File chrome/browser/ui/views/hung_renderer_view.cc (right):

https://chromiumcodereview.appspot.com/10806079/diff/15006/chrome/browser/ui/...
chrome/browser/ui/views/hung_renderer_view.cc:201: // static
Make position of these two match header.

https://chromiumcodereview.appspot.com/10806079/diff/15006/chrome/browser/ui/...
File chrome/browser/ui/views/hung_renderer_view_win.cc (right):

https://chromiumcodereview.appspot.com/10806079/diff/15006/chrome/browser/ui/...
chrome/browser/ui/views/hung_renderer_view_win.cc:29: HungRendererDialogMetro
g_instance;
You need to make this a pointer.

[ananta, 2012-07-24 23:16:37]

https://chromiumcodereview.appspot.com/10806079/diff/15006/chrome/browser/ui/...
File chrome/browser/ui/views/hung_renderer_view.cc (right):

https://chromiumcodereview.appspot.com/10806079/diff/15006/chrome/browser/ui/...
chrome/browser/ui/views/hung_renderer_view.cc:201: // static
On 2012/07/24 22:57:03, sky wrote:
> Make position of these two match header.

Moved these declaration and impl of these functions after the GetInstance
function in the class.

https://chromiumcodereview.appspot.com/10806079/diff/15006/chrome/browser/ui/...
File chrome/browser/ui/views/hung_renderer_view_win.cc (right):

https://chromiumcodereview.appspot.com/10806079/diff/15006/chrome/browser/ui/...
chrome/browser/ui/views/hung_renderer_view_win.cc:29: HungRendererDialogMetro
g_instance;
On 2012/07/24 22:57:03, sky wrote:
> You need to make this a pointer.

Done. This is now accessed via the statc Create and GetInstance functions in the
HungRendererDialogMetro class

[sky, 2012-07-24 23:40:33]

LGTM

http://codereview.chromium.org/10806079/diff/8007/chrome/browser/ui/views/hun...
File chrome/browser/ui/views/hung_renderer_view_win.cc (right):

http://codereview.chromium.org/10806079/diff/8007/chrome/browser/ui/views/hun...
chrome/browser/ui/views/hung_renderer_view_win.cc:127: g_instance_ = NULL;
move resetting g_instance_ to the destructor (safer). And since you're deleting
you don't really need to reset the other fields.

http://codereview.chromium.org/10806079/diff/8007/chrome/browser/ui/views/hun...
chrome/browser/ui/views/hung_renderer_view_win.cc:141: DCHECK(GetInstance());
I think you should just return here. I could see timing problems triggering
this.

http://codereview.chromium.org/10806079/diff/8007/chrome/browser/ui/views/hun...
chrome/browser/ui/views/hung_renderer_view_win.cc:162: DCHECK(GetInstance());
Same comment here, just return if no instance.

[ananta, 2012-07-24 23:45:15]

https://chromiumcodereview.appspot.com/10806079/diff/8007/chrome/browser/ui/v...
File chrome/browser/ui/views/hung_renderer_view_win.cc (right):

https://chromiumcodereview.appspot.com/10806079/diff/8007/chrome/browser/ui/v...
chrome/browser/ui/views/hung_renderer_view_win.cc:127: g_instance_ = NULL;
On 2012/07/24 23:40:33, sky wrote:
> move resetting g_instance_ to the destructor (safer). And since you're
deleting
> you don't really need to reset the other fields.

Done.

https://chromiumcodereview.appspot.com/10806079/diff/8007/chrome/browser/ui/v...
chrome/browser/ui/views/hung_renderer_view_win.cc:141: DCHECK(GetInstance());
On 2012/07/24 23:40:33, sky wrote:
> I think you should just return here. I could see timing problems triggering
> this.

Done.

https://chromiumcodereview.appspot.com/10806079/diff/8007/chrome/browser/ui/v...
chrome/browser/ui/views/hung_renderer_view_win.cc:162: DCHECK(GetInstance());
On 2012/07/24 23:40:33, sky wrote:
> Same comment here, just return if no instance.

Done.