[Mac] Add field trial tuples to breakpad crash reports.

[Mac] Add field trial tuples to breakpad crash reports.

This is the Mac-side of http://codereview.chromium.org/9432033.

BUG=134741
TEST=Mac crash dumps show experiment chunks.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144929

[Alexei Svitkine (slow), 2012-06-26 21:29:04]

MAD: Please review the chrome/common/metrics/* changes.
Scott: Please review the chrome/common/child_process_logging_mac.mm changes.

[Scott Hess - ex-Googler, 2012-06-27 00:32:41]

http://codereview.chromium.org/10661057/diff/12006/chrome/common/child_proces...
File chrome/common/child_process_logging.h (right):

http://codereview.chromium.org/10661057/diff/12006/chrome/common/child_proces...
chrome/common/child_process_logging.h:35: static const int
kMaxExperimentChunkSize = 64;
Is the plan to share the chunking with other platforms?  Because I think that
OSX Breakpad's limit per value is 256 (or perhaps 255).

If you aren't going to share with other platforms, I think it would be better to
treat the chunking as an implementation detail for the OSX code doing the
storage, since it's not really specific to the location where it's at.

[Alexei Svitkine (slow), 2012-06-27 21:25:57]

On 2012/06/27 00:32:41, shess wrote:
>
http://codereview.chromium.org/10661057/diff/12006/chrome/common/child_proces...
> File chrome/common/child_process_logging.h (right):
> 
>
http://codereview.chromium.org/10661057/diff/12006/chrome/common/child_proces...
> chrome/common/child_process_logging.h:35: static const int
> kMaxExperimentChunkSize = 64;
> Is the plan to share the chunking with other platforms?  Because I think that
> OSX Breakpad's limit per value is 256 (or perhaps 255).
> 
> If you aren't going to share with other platforms, I think it would be better
to
> treat the chunking as an implementation detail for the OSX code doing the
> storage, since it's not really specific to the location where it's at.

We were thinking of keeping the chunking consistent between platforms (at least
initially - we can revisit the decision later if needed).

The reasoning was that we want to minimize the risk of mis-analyzing the data
due to different chunking (especially since there is a limit on the number of
chunks that could result in different number of tuples being uploaded from the
same initial list when chunk sizes are different between platforms).

[MAD, 2012-06-28 13:03:38]

Very well done!

A few comments...

Thanks!

BYE
MAD...

http://codereview.chromium.org/10661057/diff/12006/chrome/common/child_proces...
File chrome/common/child_process_logging.h (right):

http://codereview.chromium.org/10661057/diff/12006/chrome/common/child_proces...
chrome/common/child_process_logging.h:35: static const int
kMaxExperimentChunkSize = 64;
On 2012/06/27 00:32:41, shess wrote:
> Is the plan to share the chunking with other platforms?  Because I think that
> OSX Breakpad's limit per value is 256 (or perhaps 255).
> 
> If you aren't going to share with other platforms, I think it would be better
to
> treat the chunking as an implementation detail for the OSX code doing the
> storage, since it's not really specific to the location where it's at.

Please add a comment explaining why we kept 64.

http://codereview.chromium.org/10661057/diff/12006/chrome/common/child_proces...
File chrome/common/child_process_logging_mac.mm (right):

http://codereview.chromium.org/10661057/diff/12006/chrome/common/child_proces...
chrome/common/child_process_logging_mac.mm:235: if (i < chunks.size()) {
Why not run until the min of kMaxReportedExperimentChunks and chunks.size()?

http://codereview.chromium.org/10661057/diff/12006/chrome/common/child_proces...
chrome/common/child_process_logging_mac.mm:239: ClearCrashKey(key);
If you keep this, break?

[Alexei Svitkine (slow), 2012-06-28 14:56:09]

http://codereview.chromium.org/10661057/diff/12006/chrome/common/child_proces...
File chrome/common/child_process_logging_mac.mm (right):

http://codereview.chromium.org/10661057/diff/12006/chrome/common/child_proces...
chrome/common/child_process_logging_mac.mm:235: if (i < chunks.size()) {
On 2012/06/28 13:03:38, MAD wrote:
> Why not run until the min of kMaxReportedExperimentChunks and chunks.size()?

I originally had it like that, but then I noticed how SetCommandLine(). Since
the state we're setting is persistent, the idea is to clear any entries between
chunks.size() and kMaxReportedExperimentChunks in case they have any data left
over from the last time SetExperimentList() was called. (For example, if a trial
gets disabled between calls of SetExperimentList().)

http://codereview.chromium.org/10661057/diff/12006/chrome/common/child_proces...
chrome/common/child_process_logging_mac.mm:239: ClearCrashKey(key);
On 2012/06/28 13:03:38, MAD wrote:
> If you keep this, break?

See above. (Break does not achieve the result we want - the idea is to clear all
the slots at the end.)

[Alexei Svitkine (slow), 2012-06-28 15:00:21]

address comment

[Alexei Svitkine (slow), 2012-06-28 15:01:32]

https://chromiumcodereview.appspot.com/10661057/diff/12006/chrome/common/chil...
File chrome/common/child_process_logging.h (right):

https://chromiumcodereview.appspot.com/10661057/diff/12006/chrome/common/chil...
chrome/common/child_process_logging.h:35: static const int
kMaxExperimentChunkSize = 64;
On 2012/06/28 13:03:38, MAD wrote:
> On 2012/06/27 00:32:41, shess wrote:
> > Is the plan to share the chunking with other platforms?  Because I think
that
> > OSX Breakpad's limit per value is 256 (or perhaps 255).
> > 
> > If you aren't going to share with other platforms, I think it would be
better
> to
> > treat the chunking as an implementation detail for the OSX code doing the
> > storage, since it's not really specific to the location where it's at.
> 
> Please add a comment explaining why we kept 64.

Done.

[MAD, 2012-06-28 15:05:29]

Got it, thanks...

Now LGTM once you add a comment to the header to explain why we limit the value
to 64.

LGTM

BYE
MAD

[Alexei Svitkine (slow), 2012-06-28 18:43:35]

On 2012/06/28 15:05:29, MAD wrote:
> Now LGTM once you add a comment to the header to explain why we limit the
value
> to 64.

(Comment added.)

Scott, waiting for you to take another glance.

[Scott Hess - ex-Googler, 2012-06-29 00:25:48]

On 2012/06/27 21:25:57, Alexei Svitkine wrote:
> On 2012/06/27 00:32:41, shess wrote:
> >
>
http://codereview.chromium.org/10661057/diff/12006/chrome/common/child_proces...
> > File chrome/common/child_process_logging.h (right):
> > 
> >
>
http://codereview.chromium.org/10661057/diff/12006/chrome/common/child_proces...
> > chrome/common/child_process_logging.h:35: static const int
> > kMaxExperimentChunkSize = 64;
> > Is the plan to share the chunking with other platforms?  Because I think
that
> > OSX Breakpad's limit per value is 256 (or perhaps 255).
> > 
> > If you aren't going to share with other platforms, I think it would be
better
> to
> > treat the chunking as an implementation detail for the OSX code doing the
> > storage, since it's not really specific to the location where it's at.
> 
> We were thinking of keeping the chunking consistent between platforms (at
least
> initially - we can revisit the decision later if needed).
> 
> The reasoning was that we want to minimize the risk of mis-analyzing the data
> due to different chunking (especially since there is a limit on the number of
> chunks that could result in different number of tuples being uploaded from the
> same initial list when chunk sizes are different between platforms).

Sounds reasonable.  Is the incoming vector of experiments sorted?

[Review of the rest coming in a few minutes.]

[Scott Hess - ex-Googler, 2012-06-29 00:37:07]

LGTM.

http://codereview.chromium.org/10661057/diff/12006/chrome/common/child_proces...
File chrome/common/child_process_logging_mac.mm (right):

http://codereview.chromium.org/10661057/diff/12006/chrome/common/child_proces...
chrome/common/child_process_logging_mac.mm:235: if (i < chunks.size()) {
On 2012/06/28 14:56:09, Alexei Svitkine wrote:
> On 2012/06/28 13:03:38, MAD wrote:
> > Why not run until the min of kMaxReportedExperimentChunks and chunks.size()?
> 
> I originally had it like that, but then I noticed how SetCommandLine(). Since
> the state we're setting is persistent, the idea is to clear any entries
between
> chunks.size() and kMaxReportedExperimentChunks in case they have any data left
> over from the last time SetExperimentList() was called. (For example, if a
trial
> gets disabled between calls of SetExperimentList().)

Yes, this is the correct approach to take.

[commit-bot: I haz the power, 2012-06-29 01:20:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10661057/26001

[commit-bot: I haz the power, 2012-06-29 01:39:41]

Try job failure for 10661057-26001 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-06-29 14:41:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10661057/40006

[commit-bot: I haz the power, 2012-06-29 15:06:16]

Try job failure for 10661057-40006 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-06-29 15:18:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10661057/30006

[commit-bot: I haz the power, 2012-06-29 15:59:25]

Try job failure for 10661057-30006 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-06-29 16:49:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10661057/30006

[commit-bot: I haz the power, 2012-06-29 17:54:14]

Change committed as 144929