Adding support for sampling crashes in Chrome on Windows.

components/crash/content/app/crashpad.cc

Index: components/crash/content/app/crashpad.cc
diff --git a/components/crash/content/app/crashpad.cc b/components/crash/content/app/crashpad.cc
index 85afe46af675145d2b1251ea5a3b9e5989123eb2..63771553df1c4171b8006766926ee96d4d26ed42 100644
--- a/components/crash/content/app/crashpad.cc
+++ b/components/crash/content/app/crashpad.cc
@@ -295,7 +295,8 @@ void InitializeCrashpadWithEmbeddedHandler(bool initial_client,
 void SetUploadsEnabled(bool enable_uploads) {
   if (g_database) {
     crashpad::Settings* settings = g_database->GetSettings();
-    settings->SetUploadsEnabled(enable_uploads);
+    settings->SetUploadsEnabled(
+        enable_uploads && GetCrashReporterClient()->GetCollectStatsInSample());

[Alexei Svitkine (slow), 2016/08/10 21:49:34]

Maybe I didn't follow all the discussion, but what's the reason for having both
GetCollectStatsConsent() and GetCollectStatsInSample() exposed? (as opposed to
something like ShouldUploadCrashes() where the implementation can check both of
those).

If there's a good reason, worth mentioning somewhere in a comment.

[jwd, 2016/08/11 21:41:03]

My understanding is that SetUploadsEnabled expects to see the value of the
checkbox. A ShouldUploadCrashes instead of both a consent and InSample would
mean either (1)making that check in SetUploadsEnabled, probably removing the
bool argument, or (2)doing no check in SetUploadsEnabled and just passing in the
right value.

(2) was explicitly rejected by Mark
(1) seems implicitly rejected by Mark, and would involve complexity w.r.t.
modifying the initialization logic.

In conclusion, I don't have a good reason.

[Alexei Svitkine (slow), 2016/08/11 21:46:34]

Mark, could you clarify your thoughts on this? Would it be simpler to just have
a single ShouldUploadCrashes() crashpad client API or is there a good reason for
having them split?

[Mark Mentovai, 2016/08/11 22:08:36]

SetUploadsEnabled sees the value of the checkbox. It could be turned into
something that sees the value of the checkbox && !SampledOut, but then every
caller would need to do checkbox && !SampledOut, or that logic would need to be
consolidated somewhere else. There are presently two callers, and one of them is
in this file, so this file needs to know something about sampling either way. To
me, that makes this file, and this function in particular, the ideal place to do
the consolidation. In the near future, there will be at least one other caller
into here (the sad tab page), so the need for consolidation of logic will only
increase over time.

I think that the way it's written now provides good consolidation and prevents
misusing the interface by people who don't understand all of the logical bits
that go into the decision we tell Crashpad about. But if you've got an
alternative that addresses logic centralization and API use, I'll entertain it.
I made my recommendations based solely on the Crashpad side of things, but once
UMA is mixed in, what's best might change.

   }
 }
 
@@ -460,6 +461,12 @@ void ReadMainModuleAnnotationsForKasko(
 
 extern "C" {
 
+// This function is used in chrome_metrics_services_manager_client.cc to trigger
+// changes to if crash uploads are enabled.

[Alexei Svitkine (slow), 2016/08/10 21:49:34]

The last bit of this sentence is a bit hard to understand. I think you're
talking specifically about this setting changing at runtime (e.g. if user
toggles their checkbox) - is that right?

What does "if crash uploads are enabled" mean if it also takes a param?

Please rephrase comment and clarify things.

[jwd, 2016/08/11 21:41:03]

Done.

+void __declspec(dllexport) __cdecl SetUploadsEnabledImpl(bool enable_uploads) {
+  crash_reporter::SetUploadsEnabled(enable_uploads);
+}
+
 // NOTE: This function is used by SyzyASAN to annotate crash reports. If you
 // change the name or signature of this function you will break SyzyASAN
 // instrumented releases of Chrome. Please contact syzygy-team@chromium.org