Closed Bug 378581 Opened 14 years ago Closed 13 years ago

merge platform implementations of crash reporter client

Categories

(Toolkit :: Crash Reporting, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ted, Assigned: dcamp)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
Attached file MainMenu.nib update (obsolete) —
Attached patch first stab (obsolete) — Splinter Review
Here's a first cut.
Attachment #263082 - Flags: review?(ted.mielczarek)
Blocks: 379290
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!
Attachment #263082 - Flags: review?(ted.mielczarek) → review+
Blocks: 358082
Blocks: 380540
Attached file updated MainMenu.nib
Attachment #263081 - Attachment is obsolete: true
Attached patch v2Splinter Review
Attachment #263082 - Attachment is obsolete: true
(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.
Checked in.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
use Carbon function to find the folder.
Attachment #264897 - Flags: review?(ted.mielczarek)
Attachment #264897 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 264897 [details] [diff] [review]
use carbon function to find application support folder

Checked in.
You need to log in before you can comment on or make changes to this bug.