378581 - merge platform implementations of crash reporter client

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[(not currently active) Ted Mielczarek]

We should be using a common source file for all crash reporter clients, with OS-specific parts (mostly GUI) in separate files.  This way we don't wind up reimplementing the logic for each one.  The updater code is a good model for this.

Comment 1

[Dave Camp (:dcamp)]

Attachment: MainMenu.nib update

Comment 2

[Dave Camp (:dcamp)]

Attachment: first stab

Here's a first cut.

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 263082 [details] [diff] [review]
first stab

Overall looks really good, I just have a few comments.

>diff -r 41dd8eaf265e toolkit/airbag/client/crashreporter.cpp
>+const char **gArgv;

I am totally picky nowadays about keeping * and & snug to the typename.  It's a little inconsistent in this file (probably just your style vs. my style), so can you double check?

>diff -r 41dd8eaf265e toolkit/airbag/client/crashreporter.h
>+bool CrashReporterSendCompleted(bool success,
>+				const std::string& serverResponse);

It looks like some tabs snuck in here, can you detabify?

>diff -r 41dd8eaf265e toolkit/airbag/client/crashreporter_osx.mm
>+#define N(s) [NSString stringWithUTF8String:(s).c_str()]

This is a bit cryptic, could you call it NSSTR() or something?  I realize the whole point is to make it less verbose, but I think you made it a little too short.  :)

>+void UIError(const string& message)
>+{
>+  // XXX
>+  printf("Error: %s\n", message.c_str());
>+}

This kinda sucks.  Can't you throw up a message box or something?

>diff -r 41dd8eaf265e toolkit/airbag/client/crashreporter_win.cpp
>+bool UIGetIniPath(string& path)
>+{
>+  TCHAR fileName[MAX_PATH];

Might as well just make this wchar_t.

>+  if (GetModuleFileName(NULL, fileName, MAX_PATH)) {
>+    // get crashreporter ini
>+    LPTSTR s = wcsrchr(fileName, '.');

This could also just be wchar_t*.

r=me with those changes.

Overall, love the amount of code removal.  Great job!

Comment 4

[Dave Camp (:dcamp)]

Attachment: updated MainMenu.nib

Comment 5

[Dave Camp (:dcamp)]

Attachment: v2

Comment 6

[Dave Camp (:dcamp)]

(In reply to comment #3)

> I am totally picky nowadays about keeping * and & snug to the typename.  It's a
> little inconsistent in this file (probably just your style vs. my style), so
> can you double check?

Fixed.

> 
> >diff -r 41dd8eaf265e toolkit/airbag/client/crashreporter.h
> >+bool CrashReporterSendCompleted(bool success,
> >+				const std::string& serverResponse);
> 
> It looks like some tabs snuck in here, can you detabify?

Fixed

> >diff -r 41dd8eaf265e toolkit/airbag/client/crashreporter_osx.mm
> >+#define N(s) [NSString stringWithUTF8String:(s).c_str()]
> 
> This is a bit cryptic, could you call it NSSTR() or something?  I realize the
> whole point is to make it less verbose, but I think you made it a little too
> short.  :)
> 

Done.

> >+void UIError(const string& message)
> >+{
> >+  // XXX
> >+  printf("Error: %s\n", message.c_str());
> >+}
> 
> This kinda sucks.  Can't you throw up a message box or something?

So, I was going to save that for a different bug, but I added it to this one.
> Might as well just make this wchar_t.
> This could also just be wchar_t*.

Done.

Comment 7

[(not currently active) Ted Mielczarek]

Checked in.

Comment 8

[Dave Camp (:dcamp)]

Attachment: use carbon function to find application support folder

use Carbon function to find the folder.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 264897 [details] [diff] [review]
use carbon function to find application support folder

Checked in.