Closed
Bug 378581
Opened 17 years ago
Closed 17 years ago
merge platform implementations of crash reporter client
Categories
(Toolkit :: Crash Reporting, defect)
Toolkit
Crash Reporting
Tracking
()
RESOLVED
FIXED
People
(Reporter: ted, Assigned: dcamp)
References
Details
Attachments
(3 files, 2 obsolete files)
(deleted),
application/octet-stream
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•17 years ago
|
||
Assignee | ||
Comment 2•17 years ago
|
||
Here's a first cut.
Attachment #263082 -
Flags: review?(ted.mielczarek)
Reporter | ||
Comment 3•17 years ago
|
||
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+
Assignee | ||
Comment 4•17 years ago
|
||
Attachment #263081 -
Attachment is obsolete: true
Assignee | ||
Comment 5•17 years ago
|
||
Attachment #263082 -
Attachment is obsolete: true
Assignee | ||
Comment 6•17 years ago
|
||
(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.
Reporter | ||
Comment 7•17 years ago
|
||
Checked in.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 8•17 years ago
|
||
use Carbon function to find the folder.
Attachment #264897 -
Flags: review?(ted.mielczarek)
Reporter | ||
Updated•17 years ago
|
Attachment #264897 -
Flags: review?(ted.mielczarek) → review+
Reporter | ||
Comment 9•17 years ago
|
||
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.
Description
•