let escape key close crash reporter dialog

VERIFIED FIXED in mozilla29

Status

()

Toolkit
Crash Reporting
VERIFIED FIXED
10 years ago
4 years ago

People

(Reporter: myk, Assigned: kcarchana77)

Tracking

unspecified
mozilla29
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [engineering-friend])

Attachments

(2 attachments, 14 obsolete attachments)

3.79 KB, patch
darkowlzz
: feedback+
Details | Diff | Splinter Review
885 bytes, patch
ted
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
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.
(Assignee)

Comment 1

4 years ago
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.
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.
(Assignee)

Comment 3

4 years ago
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.
If you haven't already been there, irc.mozilla.org #introduction is a great place to get help as a beginner.
Assignee: nobody → kcarchana77
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 5

4 years ago
Created attachment 827319 [details] [diff] [review]
bug414071.patch
Attachment #827319 - Flags: review?(ted)
(Assignee)

Comment 6

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

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

Updated

4 years ago
Attachment #827319 - Flags: feedback?(indiasuny000)

Comment 7

4 years ago
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 :)
Attachment #827319 - Flags: feedback?(indiasuny000) → feedback+
(Assignee)

Comment 8

4 years ago
Created attachment 827346 [details] [diff] [review]
bug_414071.patch

I have removed the whitespaces and line spaces.
Attachment #827346 - Flags: review?(ted)
Attachment #827346 - Flags: feedback?(indiasuny000)
(Assignee)

Comment 9

4 years ago
Created attachment 827369 [details] [diff] [review]
bug414071.patch
Attachment #827369 - Flags: review?(ted)
Attachment #827369 - Flags: feedback?(indiasuny000)
(Assignee)

Comment 10

4 years ago
Created attachment 827375 [details] [diff] [review]
bug414071.patch
Attachment #827319 - Attachment is obsolete: true
Attachment #827346 - Attachment is obsolete: true
Attachment #827369 - Attachment is obsolete: true
Attachment #827319 - Flags: review?(ted)
Attachment #827346 - Flags: review?(ted)
Attachment #827346 - Flags: feedback?(indiasuny000)
Attachment #827369 - Flags: review?(ted)
Attachment #827369 - Flags: feedback?(indiasuny000)
Attachment #827375 - Flags: review?(ted)
Attachment #827375 - Flags: feedback?(indiasuny000)

Comment 11

4 years ago
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 :)
Attachment #827375 - Flags: feedback?(indiasuny000) → feedback+
(Assignee)

Comment 12

4 years ago
Created attachment 827380 [details] [diff] [review]
bug414071.patch
Attachment #827375 - Attachment is obsolete: true
Attachment #827375 - Flags: review?(ted)
Attachment #827380 - Flags: review?(ted)
Attachment #827380 - Flags: feedback?(indiasuny000)
(Assignee)

Comment 13

4 years ago
Created attachment 827384 [details] [diff] [review]
bug414071.patch
Attachment #827380 - Attachment is obsolete: true
Attachment #827380 - Flags: review?(ted)
Attachment #827380 - Flags: feedback?(indiasuny000)
Attachment #827384 - Flags: review?(ted)
(Assignee)

Updated

4 years ago
Severity: minor → normal
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.
Attachment #827384 - Flags: review?(ted) → review+
(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!
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 16

4 years ago
Created attachment 829220 [details] [diff] [review]
bug414071.patch

Corrected all the nits. :)
Attachment #827384 - Attachment is obsolete: true
Attachment #829220 - Flags: review?(ted)
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.)
Attachment #829220 - Flags: review?(ted)
(Assignee)

Comment 18

4 years ago
Created attachment 829228 [details] [diff] [review]
bug414071.patch
Attachment #829220 - Attachment is obsolete: true
Attachment #829228 - Flags: review?(ted)
(Assignee)

Comment 19

4 years ago
Created attachment 829234 [details] [diff] [review]
bug414071.patch
Attachment #829228 - Attachment is obsolete: true
Attachment #829228 - Flags: review?(ted)
Attachment #829234 - Flags: review?(ted)
(Assignee)

Comment 20

4 years ago
Created attachment 829237 [details] [diff] [review]
bug414071.patch
Attachment #829234 - Attachment is obsolete: true
Attachment #829234 - Flags: review?(ted)
Attachment #829237 - Flags: review?(ted)
(Assignee)

Comment 21

4 years ago
Created attachment 829264 [details] [diff] [review]
bug414071.patch
Attachment #829237 - Attachment is obsolete: true
Attachment #829237 - Flags: review?(ted)
Attachment #829264 - Flags: review?(ted)
This patch needs rebasing and it isn't clear to me that it actually has the proper review it needs to land?
Keywords: checkin-needed
Comment on attachment 829264 [details] [diff] [review]
bug414071.patch

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

Thanks!
Attachment #829264 - Flags: review?(ted) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #22)
> This patch needs rebasing
Keywords: checkin-needed
(Assignee)

Comment 25

4 years ago
Created attachment 8356097 [details] [diff] [review]
bug414071.patch
Attachment #829264 - Attachment is obsolete: true
Attachment #8356097 - Flags: review?(ted)
Attachment #8356097 - Flags: feedback?(indiasuny000)

Comment 26

4 years ago
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.
Attachment #8356097 - Flags: feedback?(indiasuny000) → feedback+
(Assignee)

Comment 27

4 years ago
Created attachment 8357133 [details] [diff] [review]
bug4.patch
Attachment #8356097 - Attachment is obsolete: true
Attachment #8356097 - Flags: review?(ted)
(Assignee)

Comment 28

4 years ago
Created attachment 8357134 [details] [diff] [review]
bug4.patch
Attachment #8357133 - Attachment is obsolete: true
Attachment #8357134 - Flags: review?(ted)
Attachment #8357134 - Flags: feedback?(indiasuny000)

Comment 29

4 years ago
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
Attachment #8357134 - Flags: feedback?(indiasuny000)
(Assignee)

Comment 30

4 years ago
Created attachment 8357153 [details] [diff] [review]
bug4.patch
Attachment #8357134 - Attachment is obsolete: true
Attachment #8357134 - Flags: review?(ted)
Attachment #8357153 - Flags: review?(ted)
Attachment #8357153 - Flags: feedback?(indiasuny000)

Updated

4 years ago
Attachment #8357153 - Flags: review?(ted)
Attachment #8357153 - Flags: feedback?(indiasuny000)
Attachment #8357153 - Flags: feedback+

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/796618164ade

Thanks for the patch!
Keywords: checkin-needed
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

4 years ago
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.
Flags: needinfo?(roc)
Flags: needinfo?(karlt)
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.)
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.
Flags: needinfo?(roc)
Flags: needinfo?(karlt)
(Assignee)

Comment 36

4 years ago
Created attachment 8357724 [details] [diff] [review]
bug41.patch
Attachment #8357153 - Attachment is obsolete: true
Attachment #8357724 - Flags: review?(ted)
Attachment #8357724 - Flags: feedback?(indiasuny000)
Attachment #8357724 - Flags: review?(ted) → review+

Updated

4 years ago
Attachment #8357153 - Attachment is obsolete: false

Comment 37

4 years ago
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 ;)
Attachment #8357724 - Flags: feedback?(indiasuny000)
(Assignee)

Updated

4 years ago
Flags: needinfo?(ted)
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.
Flags: needinfo?(ted)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/08951f068c4c
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Thanks for sticking with it and getting the patch fixed up!
https://hg.mozilla.org/mozilla-central/rev/08951f068c4c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla29

Updated

4 years ago
Whiteboard: [engineering-friend]

Updated

4 years ago
Keywords: verifyme

Comment 42

4 years ago
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).
Status: RESOLVED → VERIFIED
Keywords: verifyme
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.
You need to log in before you can comment on or make changes to this bug.