540532 - allow setting report submission preference via XPCOM

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Justin Dolske [:Dolske]]

Over in bug 538910 comment 24, I want to simplify the OOPP crash reporting UI, but to do that I need to expose a checkbox in the browser preferences.

This WIP patch does so (includes /browser prefs stuff, which I might spin off to another bug). I've only implemented this for Windows so far, Linux will be next. I'll need some help doing the OS X flavor, since that needs some ObjC integration.

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (WIP)

Comment 2

[(not currently active) Ted Mielczarek]

You want to tie the plugin submission checkbox to the same pref used by the standalone crash reporter client? (Makes sense, just clarifying.)

Comment 3

[Justin Dolske [:Dolske]]

Yeah. Doesn't seem much benefit to keep them separate, from a user POV at least.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Comment 5

[Justin Dolske [:Dolske]]

Comment on attachment 423115 [details] [diff] [review]
Patch v.2

(r?gavin for the browser parts, specifically)

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 423115 [details] [diff] [review]
Patch v.2

>diff --git a/browser/components/preferences/advanced.js b/browser/components/preferences/advanced.js

>+  initSubmitCrashes: function ()

>+    checkbox.checked = cr.submitReports;

Remember to either get the other platform implementations of "submitReports" done or add a try/catch here (with disabling of the checkbox?) before landing this...

>diff --git a/browser/components/preferences/advanced.xul b/browser/components/preferences/advanced.xul

>-          <groupbox id="systemDefaultsGroup" orient="horizontal">
>+          <groupbox id="systemDefaultsGroup" orient="start">

I don't think that's a valid value for "orient". Did you mean "pack" or "align"? Not sure why you'd need to change this at all...

I take it you've made sure that the en-US entity hardcoding trick will work consistently (will always favor the first reference from the DTD)?

Comment 7

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Fixed gavin's comments.

(In reply to comment #6)

> >-          <groupbox id="systemDefaultsGroup" orient="horizontal">
> >+          <groupbox id="systemDefaultsGroup" orient="start">
> 
> I don't think that's a valid value for "orient". Did you mean "pack" or
> "align"? Not sure why you'd need to change this at all...

Oops. Fixed to be "vertical" -- otherwise the existing checkbox, button, and this new checkbox are all on the same line.
 
> I take it you've made sure that the en-US entity hardcoding trick will work
> consistently (will always favor the first reference from the DTD)?

Yes, though I've gone ahead and removed it for the trunk version of this patch.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 423693 [details] [diff] [review]
Patch v.3

>diff --git a/toolkit/crashreporter/nsExceptionHandler.cpp b/toolkit/crashreporter/nsExceptionHandler.cpp
>--- a/toolkit/crashreporter/nsExceptionHandler.cpp
>+++ b/toolkit/crashreporter/nsExceptionHandler.cpp
>+nsresult GetSubmitReports(PRBool *aSubmitReports)
>+{
>+  nsresult rv;
>+  nsCOMPtr<nsIXULAppInfo> appinfo =
>+    do_GetService("@mozilla.org/xre/app-info;1", &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCAutoString appVendor, appName;
>+  rv = appinfo->GetVendor(appVendor);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = appinfo->GetName(appName);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+// XXX support throttling?

We're not using throttling on 3.6, so I think you can just drop this comment.

>+#if defined(XP_WIN32)
>+  nsCOMPtr<nsIWindowsRegKey> regKey
>+    (do_CreateInstance("@mozilla.org/windows-registry-key;1", &rv));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCAutoString regPath;
>+
>+  regPath.AppendLiteral("Software\\");
>+  if(!appVendor.IsEmpty()) {
>+    regPath.Append(appVendor);
>+    regPath.AppendLiteral("\\");
>+  }
>+  regPath.Append(appName);
>+  regPath.AppendLiteral("\\Crash Reporter");

Please figure out a way to share this code with SetSubmitReports instead of duplicating it in both (similarly, the Linux code). Also, we could probably use a comment noting that this needs to stay in sync with the code in crashreporter_win.cpp. (And a complementary comment in that file pointing here.)

>+
>+  PRUint32 value;
>+  rv = regKey->Open(nsIWindowsRegKey::ROOT_KEY_LOCAL_MACHINE,
>+                    NS_ConvertUTF8toUTF16(regPath),
>+                    nsIWindowsRegKey::ACCESS_QUERY_VALUE);
>+  if (NS_SUCCEEDED(rv)) {
>+    rv = regKey->ReadIntValue(NS_LITERAL_STRING("SubmitCrashReport"), &value);
>+    regKey->Close();
>+    if (NS_SUCCEEDED(rv)) {
>+      *aSubmitReports = !!value;
>+      return NS_OK;
>+    }
>+  }
>+
>+  rv = regKey->Open(nsIWindowsRegKey::ROOT_KEY_CURRENT_USER,
>+                    NS_ConvertUTF8toUTF16(regPath),
>+                    nsIWindowsRegKey::ACCESS_QUERY_VALUE);
>+  if (NS_FAILED(rv)) {
>+    *aSubmitReports = PR_TRUE;
>+    return NS_OK;
>+  }
>+  
>+  rv = regKey->ReadIntValue(NS_LITERAL_STRING("SubmitCrashReport"), &value);
>+  // default to true on failure
>+  if (NS_FAILED(rv)) {
>+    value = 1;
>+    rv = NS_OK;
>+  }
>+  regKey->Close();
>+
>+  *aSubmitReports = !!value;
>+  return NS_OK;
>+#elif defined(XP_MACOSX)
>+  // XXX Yuck. Need ObjC code to use NSUserDefaults
>+  // Skip for now until we support OOPP on OSX? Help from josh et al?
>+  return NS_ERROR_NOT_IMPLEMENTED;

Just put //TODO: bug 542379 here.

>+#elif defined(XP_UNIX)
>+  // Create nsIFile for:
>+  //     $HOME + /. + [ appVendor + / + ] appName +
>+  //             / + "Crash Reports" + / "crashreporter.ini"
>+  //     vendor can be empty (skip), vendor/name should be lowercase

This isn't correct, you want to get "UAppData" from the directory service and append "Crash Reports/crashreporter.ini".
See:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/client/crashreporter.cpp#515
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp#678
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#2871

>+  if (submitReportValue.Equals(NS_LITERAL_CSTRING("0")))

Probably "EqualsASCII("0")" is better.

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

Updated with review comments.

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 425566 [details] [diff] [review]
Patch v.4

>diff --git a/toolkit/crashreporter/nsExceptionHandler.cpp b/toolkit/crashreporter/nsExceptionHandler.cpp
>--- a/toolkit/crashreporter/nsExceptionHandler.cpp
>+++ b/toolkit/crashreporter/nsExceptionHandler.cpp
+ static nsresult PrefSubmitReports(PRBool *aSubmitReports, PRBool writePref)

You can just use 'bool' here instead of PRBool, since this isn't exposed via XPCOM. Also, file style is to snuggle the * next to the type name (please fix this here and in your other function prototypes here and in the header).

>+  nsCOMPtr<nsIFile> reporterINI;
>+  rv = NS_GetSpecialDirectory("UAppData", getter_AddRefs(reporterINI));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  reporterINI->AppendNative(NS_LITERAL_CSTRING("Crash Reports"));
>+  reporterINI->AppendNative(NS_LITERAL_CSTRING("crashreporter.ini"));
>+
>+  PRBool exists;
>+  rv = reporterINI->Exists(&exists);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (!exists) {
>+    if (!writePref) {
>+        // If reading the pref, default to trie if .ini doesn't exist.

spelling: "true:

>+        *aSubmitReports = PR_TRUE;
>+        return NS_OK;
>+    }
>+    // Create the file so the INI processor can write to it.
>+    rv = reporterINI->Create(nsIFile::NORMAL_FILE_TYPE, 0600);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
>+
>+  nsCOMPtr<nsIINIParserFactory> iniFactory =
>+    do_GetService("@mozilla.org/xpcom/ini-processor-factory;1", &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsILocalFile> localFile = do_QueryInterface(reporterINI);
>+  NS_ENSURE_TRUE(localFile, NS_ERROR_FAILURE);
>+  nsCOMPtr<nsIINIParser> iniParser;
>+  rv = iniFactory->CreateINIParser(localFile,
>+                                   getter_AddRefs(iniParser));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // If we're writing the pref, jsut set and we're done.

spelling: "just"

>+  return NS_OK;
>+#else
>+  return NS_ERROR_NOT_IMPLEMENTED;
>+#endif

You can probably just combine this with the OSX section, no need to have two right now.

r=me with those nits fixed.

Comment 11

[Justin Dolske [:Dolske]]

Attachment: Patch v.5

Review comments fixed.

Comment 12

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/b8586e71aa93

Comment 13

[Justin Dolske [:Dolske]]

Bustage fix: http://hg.mozilla.org/mozilla-central/rev/041e0c68e653

Can't actually remove the XP_MACOSX case, since OS X will enter the XP_UNIX case, which we don't want here. Not sure if there are better #ifdef guards to use here, seemed simplest to just go with what the prior patch did.

Comment 14

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/fb91c616d109

Comment 15

[Benjamin Smedberg]

and http://hg.mozilla.org/projects/firefox-lorentz/rev/7e63b3a2db53

Comment 16

[Benjamin Smedberg]

Attachment: Branch-specific interface to avoid UUID changes, rev. 1

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 18

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430