414071 - let escape key close crash reporter dialog

Status: VERIFIED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

Most dialogs in Firefox--including Add-ons, Downloads, Error Console, Page Info, and Preferences/Options--can be closed by hitting the escape key, but the Crash Reporter dialog cannot.  Nevertheless, I keep trying to close it by hitting the escape key, since I'm so used to doing that with other dialogs.

For consistency with the rest of the interface, and so users like me don't have to spend that extra moment readjusting their flow and thinking about how to close the dialog, it would be good to be able to close that dialog with the escape key as well.

Comment 1

[kcarchana77]

I am interested to work on this bug and I would I like to know more details about this bug. I would also like to know where I should get started.

Comment 2

[(not currently active) Ted Mielczarek]

Your first step should be to get the source and build Firefox:
https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

The crashreporter client has code specific to each OS, so fixing this for each of Linux, Mac, Windows will require a separate patch. For the purposes of this bug it's probably fine to just fix it on Linux (I assume that's what myk wanted).

The code that creates the crashreporter dialog for Linux is here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/client/crashreporter_linux.cpp#379

You'd want to figure out how to handle the Escape key in that dialog in GTK and make the application quit.

Comment 3

[kcarchana77]

I am a beginner. I have completed my build, but I have to learn a bit more about GTK for this. I am also looking through the code, finding help in IRC to understand more about this bug.  It would be better if you could assign this bug to me, so that I will be able to concentrate more on this bug.

Comment 4

[(not currently active) Ted Mielczarek]

If you haven't already been there, irc.mozilla.org #introduction is a great place to get help as a beginner.

Comment 5

[kcarchana77]

Attachment: bug414071.patch

Comment 6

[kcarchana77]

Comment on attachment 827319 [details] [diff] [review]
bug414071.patch

Now you can close crashreporter dialogue box by pressing Escape key.

Comment 7

[Sunny [:darkowlzz]]

Comment on attachment 827319 [details] [diff] [review]
bug414071.patch

Review of attachment 827319 [details] [diff] [review]:
-----------------------------------------------------------------

This seems to work in my nightly on linux.

I have pointed out some nits in your patch, please change them and submit it again.

Also, ted has not reviewed it yet, so remove |r=ted.mielczarek| from the patch commit message. You can have it back once he reviews it :)

Thanks for the contribution.

::: toolkit/crashreporter/client/crashreporter_gtk_common.cpp
@@ +227,5 @@
> +                             gpointer userData)
> +{
> +  if (event->keyval == GDK_KEY_Escape) {
> +    gtk_main_quit();
> +    return TRUE; 

Whitespace after the semicolon, get rid of it.

@@ +229,5 @@
> +  if (event->keyval == GDK_KEY_Escape) {
> +    gtk_main_quit();
> +    return TRUE; 
> +  }
> +  return FALSE; 

Same as above.

::: toolkit/crashreporter/client/crashreporter_linux.cpp
@@ +8,5 @@
>  #include <glib.h>
>  #include <gtk/gtk.h>
>  #include <string.h>
>  
> +

Remove this line gap.

@@ +396,5 @@
>    gtk_window_set_position(GTK_WINDOW(gWindow), GTK_WIN_POS_CENTER);
>    gtk_container_set_border_width(GTK_CONTAINER(gWindow), 12);
>    g_signal_connect(gWindow, "delete-event", G_CALLBACK(WindowDeleted), 0);
> +  g_signal_connect(gWindow, "key_press_event", G_CALLBACK(check_escape), NULL);
> +  

Remove this whitespace too :)

@@ +558,5 @@
>    gtk_main();
>  
>    return gDidTrySend;
>  }
> +

Another line gap :)

Comment 8

[kcarchana77]

Attachment: bug_414071.patch

I have removed the whitespaces and line spaces.

Comment 9

[kcarchana77]

Attachment: bug414071.patch

Comment 10

[kcarchana77]

Attachment: bug414071.patch

Comment 11

[Sunny [:darkowlzz]]

Comment on attachment 827375 [details] [diff] [review]
bug414071.patch

Review of attachment 827375 [details] [diff] [review]:
-----------------------------------------------------------------

This looks perfect.

Please add your name at the top of the patch. Refer https://bug920935.bugzilla.mozilla.org/attachment.cgi?id=823939

Thanks again :)

Comment 12

[kcarchana77]

Attachment: bug414071.patch

Comment 13

[kcarchana77]

Attachment: bug414071.patch

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 827384 [details] [diff] [review]
bug414071.patch

Review of attachment 827384 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, thanks for the patch! I have some very minor nits I'd like you to fix. Once you've fixed them, you can attach a new patch with "r=ted" at the end of the commit message, and put "checkin-needed" in the keywords field to get someone to land your patch for you.

::: toolkit/crashreporter/client/crashreporter_gtk_common.cpp
@@ +222,5 @@
>    return TRUE;
>  }
>  
> +gboolean check_escape(GtkWidget* window,
> +                             GdkEventKey *event,

nit: your indentation is a little off here, and also the * should go next to the type name, like "GdkEventKey* ".

::: toolkit/crashreporter/client/crashreporter_gtk_common.h
@@ +38,5 @@
>  gpointer SendThread(gpointer args);
>  gboolean WindowDeleted(GtkWidget* window,
>                         GdkEvent* event,
>                         gpointer userData);
> +gboolean check_escape(GtkWidget* window, GdkEventKey *event, gpointer data);

nit: the *s go right next to the type name

::: toolkit/crashreporter/client/crashreporter_linux.cpp
@@ +394,5 @@
>    gtk_window_set_resizable(GTK_WINDOW(gWindow), FALSE);
>    gtk_window_set_position(GTK_WINDOW(gWindow), GTK_WIN_POS_CENTER);
>    gtk_container_set_border_width(GTK_CONTAINER(gWindow), 12);
>    g_signal_connect(gWindow, "delete-event", G_CALLBACK(WindowDeleted), 0);
> +  g_signal_connect(gWindow, "key_press_event", G_CALLBACK(check_escape), NULL);

nit: we use "nullptr" nowadays.

Comment 15

[(not currently active) Ted Mielczarek]

(In reply to Sunny [:darkowlzz] from comment #7)
> I have pointed out some nits in your patch, please change them and submit it
> again.

Thanks for the pre-review here, btw!

Comment 16

[kcarchana77]

Attachment: bug414071.patch

Corrected all the nits. :)

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 829220 [details] [diff] [review]
bug414071.patch

Review of attachment 829220 [details] [diff] [review]:
-----------------------------------------------------------------

This is almost ready for landing, but you accidentally got some tabs in this file. If you can fix that I'll give you a quick r+.

::: toolkit/crashreporter/client/crashreporter_gtk_common.cpp
@@ +214,5 @@
>  }
>  
>  gboolean WindowDeleted(GtkWidget* window,
> +                       GdkEvent* event,
> +                       gpointer userData)

Thanks for fixing this while you're here. :)

@@ +221,5 @@
>    gtk_main_quit();
>    return TRUE;
>  }
>  
> +gboolean check_escape(GtkWidget* window, 

There's a single trailing space here, can you remove it? (The review tool highlights them, you can usually configure your editor to highlight them as well.)

@@ +223,5 @@
>  }
>  
> +gboolean check_escape(GtkWidget* window, 
> +					  GdkEventKey* event,
> +					  gpointer userData)

Argh! You got tabs in this file! (We use spaces for indentation.)

Comment 18

[kcarchana77]

Attachment: bug414071.patch

Comment 19

[kcarchana77]

Attachment: bug414071.patch

Comment 20

[kcarchana77]

Attachment: bug414071.patch

Comment 21

[kcarchana77]

Attachment: bug414071.patch

Comment 22

[Ryan VanderMeulen [:RyanVM]]

This patch needs rebasing and it isn't clear to me that it actually has the proper review it needs to land?

Comment 23

[(not currently active) Ted Mielczarek]

Comment on attachment 829264 [details] [diff] [review]
bug414071.patch

Review of attachment 829264 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 24

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #22)
> This patch needs rebasing

Comment 25

[kcarchana77]

Attachment: bug414071.patch

Comment 26

[Sunny [:darkowlzz]]

Comment on attachment 8356097 [details] [diff] [review]
bug414071.patch

Review of attachment 8356097 [details] [diff] [review]:
-----------------------------------------------------------------

I have pointed out small nits (white-space(s)). Get rid of them and this should be ready.
Tested on my Arch Linux, works fine. Esc closes the crash-reporter.

::: toolkit/crashreporter/client/crashreporter_gtk_common.cpp
@@ +216,5 @@
>  }
>  
> +gboolean WindowDeleted(GtkWidget* window, 
> +                       GdkEvent* event, 
> +                       gpointer userData) 

White-space at the end of all the above 3 lines.

@@ +225,5 @@
>  }
>  
> +gboolean check_escape(GtkWidget* window,
> +                      GdkEventKey* event,
> +                      gpointer userData) 

White-space at the end of above line.

Comment 27

[kcarchana77]

Attachment: bug4.patch

Comment 28

[kcarchana77]

Attachment: bug4.patch

Comment 29

[Sunny [:darkowlzz]]

Comment on attachment 8357134 [details] [diff] [review]
bug4.patch

Review of attachment 8357134 [details] [diff] [review]:
-----------------------------------------------------------------

Okay, it's ready.
Ted has already reviewed it earlier. I don't think it needs another review. It just needed rebasing.
BUT, there seems to be some problem in the way this patch was generated. I don't see the changeset number, author name and patch summary.
Please add them. Refer [1]

[1]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 30

[kcarchana77]

Attachment: bug4.patch

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/796618164ade

Thanks for the patch!

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee31dadd0969

https://tbpl.mozilla.org/php/getParsedLog.php?id=32700909&tree=Mozilla-Inbound

Comment 33

[Sunny [:darkowlzz]]

Could you help in resolving this bustage?
I guess we should add some #ifdef to get this right. But I have no idea about gdk versions.

The patch worked fine on my local build with gdk 2.30.

Comment 34

[(not currently active) Ted Mielczarek]

I can suggest two ways to possibly fix this:
1) It looks like GDK_KEY_Escape is a replacement for GDK_Escape, so maybe:
#ifndef GDK_KEY_Escape
#define GDK_KEY_Escape GDK_Escape
#endif

At the top of the file (after the #includes).

If that doesn't work for some reason, just redefine the constant, like:
#ifndef GDK_KEY_Escape
#define GDK_KEY_Escape 0xff1b
#endif

(I just took that line from the gdkkeysyms.h header file.)

Comment 35

[Karl Tomlinson (:karlt)]

Yes, GDK_KEY_Escape may have been an attempt to make GDK2 more like GDK3.

In other files, we've used GDK_Escape so that things work with other GDK2 versions.
gdk/gdkkeysyms-compat.h would need to be included only for GTK3.
http://hg.mozilla.org/mozilla-central/annotate/f99c42bb080e/widget/gtk/nsGtkKeyUtils.cpp#l16

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #34)
> I can suggest two ways to possibly fix this:
> 1) It looks like GDK_KEY_Escape is a replacement for GDK_Escape, so maybe:
> #ifndef GDK_KEY_Escape
> #define GDK_KEY_Escape GDK_Escape
> #endif

That should work, and is fine if there are not many keysym macros.

Comment 36

[kcarchana77]

Attachment: bug41.patch

Comment 37

[Sunny [:darkowlzz]]

Comment on attachment 8357724 [details] [diff] [review]
bug41.patch

Review of attachment 8357724 [details] [diff] [review]:
-----------------------------------------------------------------

I have combined the previously obsoleted patch and the latest patch and pushed to try.
https://tbpl.mozilla.org/?tree=Try&rev=8e3a5f83360a

Fingers crossed ;)

Comment 38

[(not currently active) Ted Mielczarek]

I'm sorry, what's the needinfo here for? The try push from comment 37 looks fine, you should be able to re-add checkin-needed here.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/08951f068c4c

Comment 40

[(not currently active) Ted Mielczarek]

Thanks for sticking with it and getting the patch fixed up!

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08951f068c4c

Comment 42

[Ioana (away)]

Verified as fixed on Firefox 29.0b1 using Ubuntu 13.04 x86. The breakpad can be closed with Esc both when just opened (case1), and after you clicked to Restart Firefox (case2).

Is this going to get fixed on Windows and Mac any time soon? It's rather peculiar for someone that uses multiple platforms, because this doesn't work at all on Windows, but it does work partially on Mac OS X (case1).

Comment 43

[(not currently active) Ted Mielczarek]

This is a platform-specific UI, so I'd expect it to follow platform guidelines. If someone can show that escape is intended to act similarly on other platforms then we could take a patch to fix those as well.