Add experiments info to crash dumps.

Add experiments info to crash dumps.

BUG=None
TEST=Make sure that the crash dumps are correctly generated and contain experiments information when appropriate.

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

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

[MAD, 2012-03-09 21:15:31]

Let me know if you don't think you are the right person for reviewing this
change...

I'll also need OWNERS approval for:
chrome/browser/metrics/field_trial_synchronizer.*

Thanks!

BYE
MAD
P.S.: Mac / Posix implementations will come in a separate CL.

[Mark Seaborn, 2012-03-12 21:35:06]

I don't really know enough context to be a good person to review this.  Maybe
Carlos Pizano would be a better reviewer?  I'd also suggest setting BUG= or
putting more info into the changeset description.

http://codereview.chromium.org/9432033/diff/35002/chrome/app/breakpad_win_uni...
File chrome/app/breakpad_win_unittest.cc (right):

http://codereview.chromium.org/9432033/diff/35002/chrome/app/breakpad_win_uni...
chrome/app/breakpad_win_unittest.cc:11: // Entry points of breakpad_win.cc not
exposed in any header.
Why not just put these into breakpad_win.h?

[MAD, 2012-03-13 23:37:36]

OK, Thanks Mark...

Carlos, Robert, would one of you be willing to review this change?

BYE
MAD

http://codereview.chromium.org/9432033/diff/35002/chrome/app/breakpad_win_uni...
File chrome/app/breakpad_win_unittest.cc (right):

http://codereview.chromium.org/9432033/diff/35002/chrome/app/breakpad_win_uni...
chrome/app/breakpad_win_unittest.cc:11: // Entry points of breakpad_win.cc not
exposed in any header.
On 2012/03/12 21:35:09, Mark Seaborn wrote:
> Why not just put these into breakpad_win.h?
Yeah, you're right... Will do...
Thanks!

[robertshield, 2012-03-14 13:51:47]

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc
File chrome/app/breakpad_win.cc (right):

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc#...
chrome/app/breakpad_win.cc:87: static size_t g_experiment_chunks_offset;
For clarity I'd prefer these to be initialized explicitly.

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc#...
chrome/app/breakpad_win.cc:179: #if !defined(NACL_WIN64)
Would you consider moving this code to a new file that is not built by 64-bit
NACL instead? This file is already quite long, does the field trial stuff need
to live here?

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc#...
chrome/app/breakpad_win.cc:203: (*g_custom_entries)[g_experiment_chunks_offset +
num_chunks].value,
Not 100% sure, but I believe this will do the wrong thing if UpdateExperiments
is called before GetCustomInfo. If so, this should be guarded against.

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc#...
chrome/app/breakpad_win.cc:239: // TODO(mad): Find a way to just add the new
info instead of starting over.
please clarify that "the new info" is the data stored in |field_trial_name| and
|group_name|.

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win_uni...
File chrome/app/breakpad_win_unittest.cc (right):

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win_uni...
chrome/app/breakpad_win_unittest.cc:39:
custom_info_->entries[num_experiments_index_ + i].name);
is there an off-by-one here and in the two places below, the |i| offset starts
at 1?

http://codereview.chromium.org/9432033/diff/41002/chrome/browser/metrics/fiel...
File chrome/browser/metrics/field_trial_synchronizer.cc (right):

http://codereview.chromium.org/9432033/diff/41002/chrome/browser/metrics/fiel...
chrome/browser/metrics/field_trial_synchronizer.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
2012

http://codereview.chromium.org/9432033/diff/41002/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/9432033/diff/41002/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:2571: ['win_use_allocator_shim==1', {
is this needed for a test executable?

http://codereview.chromium.org/9432033/diff/41002/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:2590: 'app/breakpad_win.cc',
I think source files ending in _win.cc are already only built on Windows via
magical gyp magicky-ness.

http://codereview.chromium.org/9432033/diff/41002/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:2591: 'app/breakpad_win_unittest.cc',
I don't know if the same is true of win_unittest.cc, if it isn't you may want to
rename that file to breakpad_unittest_win.cc.

http://codereview.chromium.org/9432033/diff/41002/chrome/common/child_process...
File chrome/common/child_process_logging_win.cc (right):

http://codereview.chromium.org/9432033/diff/41002/chrome/common/child_process...
chrome/common/child_process_logging_win.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
2012

[cpu_(ooo_6.6-7.5), 2012-03-14 21:55:45]

I respectfully decline. For experiments you want to add an experiments person
--> jar@

If you want I can review a specific section. I don't know about
experiments/trials.

You also might need some owners on some of the files you are touching. check the
owners files.

[MAD, 2012-03-15 22:37:36]

Thanks Robert...

I have a few questions and I'll wait till I have answers before I upload a new
version for you to look at...

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc
File chrome/app/breakpad_win.cc (right):

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc#...
chrome/app/breakpad_win.cc:87: static size_t g_experiment_chunks_offset;
On 2012/03/14 13:51:48, robertshield wrote:
> For clarity I'd prefer these to be initialized explicitly.

I don't understand... I just did the same thing as the other variables of the
same usage...

The only thing I see I could do is initialize them to an invalid value and then
DCHECK that the value is valid before using them... Is this what you meant?

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc#...
chrome/app/breakpad_win.cc:179: #if !defined(NACL_WIN64)
On 2012/03/14 13:51:48, robertshield wrote:
> Would you consider moving this code to a new file that is not built by 64-bit
> NACL instead? This file is already quite long, does the field trial stuff need
> to live here? 

It's doable, I guess I could call the function implemented in another file and
pass it the pointers to the global variables.

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc#...
chrome/app/breakpad_win.cc:203: (*g_custom_entries)[g_experiment_chunks_offset +
num_chunks].value,
On 2012/03/14 13:51:48, robertshield wrote:
> Not 100% sure, but I believe this will do the wrong thing if UpdateExperiments
> is called before GetCustomInfo. If so, this should be guarded against.

You're right, I'll add a DCHECK...

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc#...
chrome/app/breakpad_win.cc:239: // TODO(mad): Find a way to just add the new
info instead of starting over.
On 2012/03/14 13:51:48, robertshield wrote:
> please clarify that "the new info" is the data stored in |field_trial_name|
and
> |group_name|.

Done.

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win_uni...
File chrome/app/breakpad_win_unittest.cc (right):

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win_uni...
chrome/app/breakpad_win_unittest.cc:39:
custom_info_->entries[num_experiments_index_ + i].name);
On 2012/03/14 13:51:48, robertshield wrote:
> is there an off-by-one here and in the two places below, the |i| offset starts
> at 1?

No, it's on purpose, we want the string to be 1 base and not 0 based. OK?

http://codereview.chromium.org/9432033/diff/41002/chrome/browser/metrics/fiel...
File chrome/browser/metrics/field_trial_synchronizer.cc (right):

http://codereview.chromium.org/9432033/diff/41002/chrome/browser/metrics/fiel...
chrome/browser/metrics/field_trial_synchronizer.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/03/14 13:51:48, robertshield wrote:
> 2012

Done.

http://codereview.chromium.org/9432033/diff/41002/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/9432033/diff/41002/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:2571: ['win_use_allocator_shim==1', {
On 2012/03/14 13:51:48, robertshield wrote:
> is this needed for a test executable?

Dunno... I just copied that from other tests above... Should I remove it?

http://codereview.chromium.org/9432033/diff/41002/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:2590: 'app/breakpad_win.cc',
On 2012/03/14 13:51:48, robertshield wrote:
> I think source files ending in _win.cc are already only built on Windows via
> magical gyp magicky-ness.

Again, I followed what I saw in other projects... I'll give it a try... Thanks

http://codereview.chromium.org/9432033/diff/41002/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:2591: 'app/breakpad_win_unittest.cc',
On 2012/03/14 13:51:48, robertshield wrote:
> I don't know if the same is true of win_unittest.cc, if it isn't you may want
to
> rename that file to breakpad_unittest_win.cc.

OK, I'll try that too... Thanks!
Although the pattern _win_unittest is much more common than _unittest_win...
Historical?

http://codereview.chromium.org/9432033/diff/41002/chrome/common/child_process...
File chrome/common/child_process_logging_win.cc (right):

http://codereview.chromium.org/9432033/diff/41002/chrome/common/child_process...
chrome/common/child_process_logging_win.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2012/03/14 13:51:48, robertshield wrote:
> 2012

Done.

[MAD, 2012-03-20 19:00:44]

On 2012/03/15 22:37:36, MAD wrote:
> Thanks Robert...
> 
> I have a few questions and I'll wait till I have answers before I upload a new
> version for you to look at...
> 
> http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc
> File chrome/app/breakpad_win.cc (right):
> 
>
http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc#...
> chrome/app/breakpad_win.cc:87: static size_t g_experiment_chunks_offset;
> On 2012/03/14 13:51:48, robertshield wrote:
> > For clarity I'd prefer these to be initialized explicitly.
> 
> I don't understand... I just did the same thing as the other variables of the
> same usage...
> 
> The only thing I see I could do is initialize them to an invalid value and
then
> DCHECK that the value is valid before using them... Is this what you meant?
> 
>
http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc#...
> chrome/app/breakpad_win.cc:179: #if !defined(NACL_WIN64)
> On 2012/03/14 13:51:48, robertshield wrote:
> > Would you consider moving this code to a new file that is not built by
64-bit
> > NACL instead? This file is already quite long, does the field trial stuff
need
> > to live here? 
> 
> It's doable, I guess I could call the function implemented in another file and
> pass it the pointers to the global variables.
> 
>
http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc#...
> chrome/app/breakpad_win.cc:203: (*g_custom_entries)[g_experiment_chunks_offset
+
> num_chunks].value,
> On 2012/03/14 13:51:48, robertshield wrote:
> > Not 100% sure, but I believe this will do the wrong thing if
UpdateExperiments
> > is called before GetCustomInfo. If so, this should be guarded against.
> 
> You're right, I'll add a DCHECK...
> 
>
http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc#...
> chrome/app/breakpad_win.cc:239: // TODO(mad): Find a way to just add the new
> info instead of starting over.
> On 2012/03/14 13:51:48, robertshield wrote:
> > please clarify that "the new info" is the data stored in |field_trial_name|
> and
> > |group_name|.
> 
> Done.
> 
>
http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win_uni...
> File chrome/app/breakpad_win_unittest.cc (right):
> 
>
http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win_uni...
> chrome/app/breakpad_win_unittest.cc:39:
> custom_info_->entries[num_experiments_index_ + i].name);
> On 2012/03/14 13:51:48, robertshield wrote:
> > is there an off-by-one here and in the two places below, the |i| offset
starts
> > at 1?
> 
> No, it's on purpose, we want the string to be 1 base and not 0 based. OK?
> 
>
http://codereview.chromium.org/9432033/diff/41002/chrome/browser/metrics/fiel...
> File chrome/browser/metrics/field_trial_synchronizer.cc (right):
> 
>
http://codereview.chromium.org/9432033/diff/41002/chrome/browser/metrics/fiel...
> chrome/browser/metrics/field_trial_synchronizer.cc:1: // Copyright (c) 2011
The
> Chromium Authors. All rights reserved.
> On 2012/03/14 13:51:48, robertshield wrote:
> > 2012
> 
> Done.
> 
> http://codereview.chromium.org/9432033/diff/41002/chrome/chrome_tests.gypi
> File chrome/chrome_tests.gypi (right):
> 
>
http://codereview.chromium.org/9432033/diff/41002/chrome/chrome_tests.gypi#ne...
> chrome/chrome_tests.gypi:2571: ['win_use_allocator_shim==1', {
> On 2012/03/14 13:51:48, robertshield wrote:
> > is this needed for a test executable?
> 
> Dunno... I just copied that from other tests above... Should I remove it?
> 
>
http://codereview.chromium.org/9432033/diff/41002/chrome/chrome_tests.gypi#ne...
> chrome/chrome_tests.gypi:2590: 'app/breakpad_win.cc',
> On 2012/03/14 13:51:48, robertshield wrote:
> > I think source files ending in _win.cc are already only built on Windows via
> > magical gyp magicky-ness.
> 
> Again, I followed what I saw in other projects... I'll give it a try... Thanks
> 
>
http://codereview.chromium.org/9432033/diff/41002/chrome/chrome_tests.gypi#ne...
> chrome/chrome_tests.gypi:2591: 'app/breakpad_win_unittest.cc',
> On 2012/03/14 13:51:48, robertshield wrote:
> > I don't know if the same is true of win_unittest.cc, if it isn't you may
want
> to
> > rename that file to breakpad_unittest_win.cc.
> 
> OK, I'll try that too... Thanks!
> Although the pattern _win_unittest is much more common than _unittest_win...
> Historical?
> 
>
http://codereview.chromium.org/9432033/diff/41002/chrome/common/child_process...
> File chrome/common/child_process_logging_win.cc (right):
> 
>
http://codereview.chromium.org/9432033/diff/41002/chrome/common/child_process...
> chrome/common/child_process_logging_win.cc:1: // Copyright (c) 2011 The
Chromium
> Authors. All rights reserved.
> On 2012/03/14 13:51:48, robertshield wrote:
> > 2012
> 
> Done.

Ping?

[robertshield, 2012-03-20 19:24:31]

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc
File chrome/app/breakpad_win.cc (right):

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc#...
chrome/app/breakpad_win.cc:87: static size_t g_experiment_chunks_offset;
On 2012/03/15 22:37:36, MAD wrote:
> On 2012/03/14 13:51:48, robertshield wrote:
> > For clarity I'd prefer these to be initialized explicitly.
> 
> I don't understand... I just did the same thing as the other variables of the
> same usage...

Yes, this wasn't a criticism of your code :-)

> 
> The only thing I see I could do is initialize them to an invalid value and
then
> DCHECK that the value is valid before using them... Is this what you meant?

I just mean stick an "= 0;" at the end of each of the above lines. I know it
doesn't make a difference, but it seems more conventional to explicitly
initialize static vars in the Chrome codebase.

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc#...
chrome/app/breakpad_win.cc:179: #if !defined(NACL_WIN64)
On 2012/03/15 22:37:36, MAD wrote:
> On 2012/03/14 13:51:48, robertshield wrote:
> > Would you consider moving this code to a new file that is not built by
64-bit
> > NACL instead? This file is already quite long, does the field trial stuff
need
> > to live here? 
> 
> It's doable, I guess I could call the function implemented in another file and
> pass it the pointers to the global variables.

Yes, that's what I meant. It would be a first step to breaking the tradition of
adding more fairly independent stuff to this file.

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc#...
chrome/app/breakpad_win.cc:203: (*g_custom_entries)[g_experiment_chunks_offset +
num_chunks].value,
On 2012/03/15 22:37:36, MAD wrote:
> On 2012/03/14 13:51:48, robertshield wrote:
> > Not 100% sure, but I believe this will do the wrong thing if
UpdateExperiments
> > is called before GetCustomInfo. If so, this should be guarded against.
> 
> You're right, I'll add a DCHECK...

Sounds good. I suggest also handling this case in release mode.

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc#...
chrome/app/breakpad_win.cc:239: // TODO(mad): Find a way to just add the new
info instead of starting over.
On 2012/03/15 22:37:36, MAD wrote:
> On 2012/03/14 13:51:48, robertshield wrote:
> > please clarify that "the new info" is the data stored in |field_trial_name|
> and
> > |group_name|.
> 
> Done.

Where?

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win_uni...
File chrome/app/breakpad_win_unittest.cc (right):

http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win_uni...
chrome/app/breakpad_win_unittest.cc:39:
custom_info_->entries[num_experiments_index_ + i].name);
On 2012/03/15 22:37:36, MAD wrote:
> On 2012/03/14 13:51:48, robertshield wrote:
> > is there an off-by-one here and in the two places below, the |i| offset
starts
> > at 1?
> 
> No, it's on purpose, we want the string to be 1 base and not 0 based. OK?

That's unconventional enough to merit a comment about the whats and the whys :-)

Also, in breakpad_win.cc, it looks like we iterate over an entries array from 0
to kMaxReportedExperimentChunks - 1, rather than 1 to
kMaxReportedExperimentChunks. Why the discrepancy? (I'm sure all will become
clear with a comment :-))

http://codereview.chromium.org/9432033/diff/41002/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/9432033/diff/41002/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:2571: ['win_use_allocator_shim==1', {
On 2012/03/15 22:37:36, MAD wrote:
> On 2012/03/14 13:51:48, robertshield wrote:
> > is this needed for a test executable?
> 
> Dunno... I just copied that from other tests above... Should I remove it?

Nah, on further investigation, it might be needed. Please ignore my previous
comment :)

http://codereview.chromium.org/9432033/diff/41002/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:2591: 'app/breakpad_win_unittest.cc',
On 2012/03/15 22:37:36, MAD wrote:
> On 2012/03/14 13:51:48, robertshield wrote:
> > I don't know if the same is true of win_unittest.cc, if it isn't you may
want
> to
> > rename that file to breakpad_unittest_win.cc.
> 
> OK, I'll try that too... Thanks!
> Although the pattern _win_unittest is much more common than _unittest_win...
> Historical?

Could be :-) Turns out that win_unittest is handled as well as per
src\build\filename_rules.gypi.

[MAD, 2012-03-20 19:36:00]

Excellent! Thanks...

You should receive an update soon...ish... :-)

On Tue, Mar 20, 2012 at 3:24 PM, <robertshield@chromium.org> wrote:

>
> http://codereview.chromium.**org/9432033/diff/41002/chrome/**
>
app/breakpad_win.cc<http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc>
> File chrome/app/breakpad_win.cc (right):
>
> http://codereview.chromium.**org/9432033/diff/41002/chrome/**
>
app/breakpad_win.cc#newcode87<http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc#newcode87>
> chrome/app/breakpad_win.cc:87: static size_t g_experiment_chunks_offset;
> On 2012/03/15 22:37:36, MAD wrote:
>
>> On 2012/03/14 13:51:48, robertshield wrote:
>> > For clarity I'd prefer these to be initialized explicitly.
>>
>
>  I don't understand... I just did the same thing as the other variables
>>
> of the
>
>> same usage...
>>
>
> Yes, this wasn't a criticism of your code :-)
>
>
>
>  The only thing I see I could do is initialize them to an invalid value
>>
> and then
>
>> DCHECK that the value is valid before using them... Is this what you
>>
> meant?
>
> I just mean stick an "= 0;" at the end of each of the above lines. I
> know it doesn't make a difference, but it seems more conventional to
> explicitly initialize static vars in the Chrome codebase.
>
>
> http://codereview.chromium.**org/9432033/diff/41002/chrome/**
>
app/breakpad_win.cc#newcode179<http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc#newcode179>
> chrome/app/breakpad_win.cc:**179: #if !defined(NACL_WIN64)
> On 2012/03/15 22:37:36, MAD wrote:
>
>> On 2012/03/14 13:51:48, robertshield wrote:
>> > Would you consider moving this code to a new file that is not built
>>
> by 64-bit
>
>> > NACL instead? This file is already quite long, does the field trial
>>
> stuff need
>
>> > to live here?
>>
>
>  It's doable, I guess I could call the function implemented in another
>>
> file and
>
>> pass it the pointers to the global variables.
>>
>
> Yes, that's what I meant. It would be a first step to breaking the
> tradition of adding more fairly independent stuff to this file.
>
>
> http://codereview.chromium.**org/9432033/diff/41002/chrome/**
>
app/breakpad_win.cc#newcode203<http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc#newcode203>
> chrome/app/breakpad_win.cc:**203:
> (*g_custom_entries)[g_**experiment_chunks_offset + num_chunks].value,
> On 2012/03/15 22:37:36, MAD wrote:
>
>> On 2012/03/14 13:51:48, robertshield wrote:
>> > Not 100% sure, but I believe this will do the wrong thing if
>>
> UpdateExperiments
>
>> > is called before GetCustomInfo. If so, this should be guarded
>>
> against.
>
>  You're right, I'll add a DCHECK...
>>
>
> Sounds good. I suggest also handling this case in release mode.
>
>
> http://codereview.chromium.**org/9432033/diff/41002/chrome/**
>
app/breakpad_win.cc#newcode239<http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win.cc#newcode239>
> chrome/app/breakpad_win.cc:**239: // TODO(mad): Find a way to just add the
> new info instead of starting over.
> On 2012/03/15 22:37:36, MAD wrote:
>
>> On 2012/03/14 13:51:48, robertshield wrote:
>> > please clarify that "the new info" is the data stored in
>>
> |field_trial_name|
>
>> and
>> > |group_name|.
>>
>
>  Done.
>>
>
> Where?
>
>
> http://codereview.chromium.**org/9432033/diff/41002/chrome/**
>
app/breakpad_win_unittest.cc<http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win_unittest.cc>
> File chrome/app/breakpad_win_**unittest.cc (right):
>
> http://codereview.chromium.**org/9432033/diff/41002/chrome/**
>
app/breakpad_win_unittest.cc#**newcode39<http://codereview.chromium.org/9432033/diff/41002/chrome/app/breakpad_win_unittest.cc#newcode39>
> chrome/app/breakpad_win_**unittest.cc:39:
> custom_info_->entries[num_**experiments_index_ + i].name);
> On 2012/03/15 22:37:36, MAD wrote:
>
>> On 2012/03/14 13:51:48, robertshield wrote:
>> > is there an off-by-one here and in the two places below, the |i|
>>
> offset starts
>
>> > at 1?
>>
>
>  No, it's on purpose, we want the string to be 1 base and not 0 based.
>>
> OK?
>
> That's unconventional enough to merit a comment about the whats and the
> whys :-)
>
> Also, in breakpad_win.cc, it looks like we iterate over an entries array
> from 0 to kMaxReportedExperimentChunks - 1, rather than 1 to
> kMaxReportedExperimentChunks. Why the discrepancy? (I'm sure all will
> become clear with a comment :-))
>
>
> http://codereview.chromium.**org/9432033/diff/41002/chrome/**
>
chrome_tests.gypi<http://codereview.chromium.org/9432033/diff/41002/chrome/chrome_tests.gypi>
> File chrome/chrome_tests.gypi (right):
>
> http://codereview.chromium.**org/9432033/diff/41002/chrome/**
>
chrome_tests.gypi#newcode2571<http://codereview.chromium.org/9432033/diff/41002/chrome/chrome_tests.gypi#newcode2571>
> chrome/chrome_tests.gypi:2571: ['win_use_allocator_shim==1', {
> On 2012/03/15 22:37:36, MAD wrote:
>
>> On 2012/03/14 13:51:48, robertshield wrote:
>> > is this needed for a test executable?
>>
>
>  Dunno... I just copied that from other tests above... Should I remove
>>
> it?
>
> Nah, on further investigation, it might be needed. Please ignore my
> previous comment :)
>
>
> http://codereview.chromium.**org/9432033/diff/41002/chrome/**
>
chrome_tests.gypi#newcode2591<http://codereview.chromium.org/9432033/diff/41002/chrome/chrome_tests.gypi#newcode2591>
> chrome/chrome_tests.gypi:2591: 'app/breakpad_win_unittest.cc'**,
> On 2012/03/15 22:37:36, MAD wrote:
>
>> On 2012/03/14 13:51:48, robertshield wrote:
>> > I don't know if the same is true of win_unittest.cc, if it isn't you
>>
> may want
>
>> to
>> > rename that file to breakpad_unittest_win.cc.
>>
>
>  OK, I'll try that too... Thanks!
>> Although the pattern _win_unittest is much more common than
>>
> _unittest_win...
>
>> Historical?
>>
>
> Could be :-) Turns out that win_unittest is handled as well as per
> src\build\filename_rules.gypi.
>
>
http://codereview.chromium.**org/9432033/<http://codereview.chromium.org/9432...
>

[MAD, 2012-03-22 19:54:29]

Thanks Robert for the comments, I think I addressed them all, and then some...
Can you please take another look?

I needed to make a few more changes since I realized that what I previously had
didn't always work...

Thanks!

BYE
MAD

[robertshield, 2012-03-23 15:42:09]

looking good, few more nits

https://chromiumcodereview.appspot.com/9432033/diff/56014/chrome/app/breakpad...
File chrome/app/breakpad_field_trial_win.cc (right):

https://chromiumcodereview.appspot.com/9432033/diff/56014/chrome/app/breakpad...
chrome/app/breakpad_field_trial_win.cc:18: base::FieldTrialList*
g_field_trial_list = NULL;
Thinking about this a bit more, would a base::LazyInstance be useful here? The
pattern of explicit InitFoo()/TermFoo() calls has proven itself somewhat fragile
in the face of the ever-more complicated Chrome start up and shutdown.

https://chromiumcodereview.appspot.com/9432033/diff/56014/chrome/app/breakpad...
chrome/app/breakpad_field_trial_win.cc:62:
current_chunk.resize(google_breakpad::CustomInfoEntry::kValueMaxLength);
The above can be simplified to:
string16 current_chunk(google_breakpad::CustomInfoEntry::kValueMaxLength, 0);

https://chromiumcodereview.appspot.com/9432033/diff/56014/chrome/app/breakpad...
File chrome/app/breakpad_win.cc (right):

https://chromiumcodereview.appspot.com/9432033/diff/56014/chrome/app/breakpad...
chrome/app/breakpad_win.cc:339: breakpad_win::g_num_of_experiments_offset =
g_custom_entries->size();
please add a "using" statement above for g_num_of_experiments_offset to be
consistent with g_custom_entries.

https://chromiumcodereview.appspot.com/9432033/diff/56014/chrome/app/breakpad...
chrome/app/breakpad_win.cc:564: }   // namespace breakpad_win
please move the above breakpad_win namespace block to the top of the file.

https://chromiumcodereview.appspot.com/9432033/diff/56014/chrome/common/child...
File chrome/common/child_process_logging.h (right):

https://chromiumcodereview.appspot.com/9432033/diff/56014/chrome/common/child...
chrome/common/child_process_logging.h:30: static const int
kMaxReportedExperimentChunks = 15;
Is there already an app->common dependency? If there is you should define it
only once, if not then ignore me :-)

[MAD, 2012-03-23 18:24:23]

Thanks...

Should have addressed all your comments... 

PTAL...

BYE
MAD

https://chromiumcodereview.appspot.com/9432033/diff/56014/chrome/app/breakpad...
File chrome/app/breakpad_field_trial_win.cc (right):

https://chromiumcodereview.appspot.com/9432033/diff/56014/chrome/app/breakpad...
chrome/app/breakpad_field_trial_win.cc:18: base::FieldTrialList*
g_field_trial_list = NULL;
On 2012/03/23 15:42:10, robertshield wrote:
> Thinking about this a bit more, would a base::LazyInstance be useful here? The
> pattern of explicit InitFoo()/TermFoo() calls has proven itself somewhat
fragile
> in the face of the ever-more complicated Chrome start up and shutdown.

Done.
Good idea, I didn't think of it...
Now I will never forget... :-)
Thanks!

https://chromiumcodereview.appspot.com/9432033/diff/56014/chrome/app/breakpad...
chrome/app/breakpad_field_trial_win.cc:62:
current_chunk.resize(google_breakpad::CustomInfoEntry::kValueMaxLength);
On 2012/03/23 15:42:10, robertshield wrote:
> The above can be simplified to:
> string16 current_chunk(google_breakpad::CustomInfoEntry::kValueMaxLength, 0);

Done.

https://chromiumcodereview.appspot.com/9432033/diff/56014/chrome/app/breakpad...
File chrome/app/breakpad_win.cc (right):

https://chromiumcodereview.appspot.com/9432033/diff/56014/chrome/app/breakpad...
chrome/app/breakpad_win.cc:339: breakpad_win::g_num_of_experiments_offset =
g_custom_entries->size();
On 2012/03/23 15:42:10, robertshield wrote:
> please add a "using" statement above for g_num_of_experiments_offset to be
> consistent with g_custom_entries.

Done.

https://chromiumcodereview.appspot.com/9432033/diff/56014/chrome/app/breakpad...
chrome/app/breakpad_win.cc:564: }   // namespace breakpad_win
On 2012/03/23 15:42:10, robertshield wrote:
> please move the above breakpad_win namespace block to the top of the file.

Done.

https://chromiumcodereview.appspot.com/9432033/diff/56014/chrome/common/child...
File chrome/common/child_process_logging.h (right):

https://chromiumcodereview.appspot.com/9432033/diff/56014/chrome/common/child...
chrome/common/child_process_logging.h:30: static const int
kMaxReportedExperimentChunks = 15;
On 2012/03/23 15:42:10, robertshield wrote:
> Is there already an app->common dependency? If there is you should define it
> only once, if not then ignore me :-)

It is defined only once... here... :-)

[robertshield, 2012-03-23 18:51:05]

lgtm

[commit-bot: I haz the power, 2012-03-23 19:29:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/9432033/62003

[commit-bot: I haz the power, 2012-03-23 20:33:26]

Try job failure for 9432033-62003 (previous was lost) (retry) on android for
steps "build, android_gyp".
It's a second try, previously, steps "build, android_gyp" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-03-23 21:19:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/9432033/60011

[commit-bot: I haz the power, 2012-03-24 02:15:17]

Try job failure for 9432033-60011 (previous was lost) (retry) on win_rel for
step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-26 13:07:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/9432033/60011

[MAD, 2012-03-26 13:09:57]

Here are the failing tests, I don't think they are related to my patch,
these were succeeding on previous CQ/tries, so I'll try the CQ yet again...
:-(
http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/13403/...

Two examples:

=====================================================

Note: Google Test filter = PPAPITest.Crypto
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from PPAPITest, where TypeParam =
[ RUN      ] PPAPITest.Crypto
[648:404:0323/190555:2994392:ERROR:network_change_notifier_win.cc(111)]
WSALookupServiceBegin failed with: 1722
[648:4220:0323/190555:2994470:ERROR:network_change_notifier_win.cc(111)]
WSALookupServiceBegin failed with: 1722
[648:4220:0323/190555:2994470:ERROR:network_change_notifier_win.cc(111)]
WSALookupServiceBegin failed with: 1722
[648:4220:0323/190555:2994485:ERROR:network_change_notifier_win.cc(111)]
WSALookupServiceBegin failed with: 1722
[648:4220:0323/190555:2994501:ERROR:network_change_notifier_win.cc(111)]
WSALookupServiceBegin failed with: 1722
[648:4220:0323/190555:2994829:FATAL:browser_process_impl.cc(255)]
Check failed: false.
[648:4220:0323/190555:2994829:FATAL:browser_process_impl.cc(255)]
Check failed: false.
Note: Google Test filter = PPAPITest.Flash_GetProxyForURL
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from PPAPITest, where TypeParam =
[ RUN      ] PPAPITest.Flash_GetProxyForURL
[       OK ] PPAPITest.Flash_GetProxyForURL (208 ms)
[----------] 1 test from PPAPITest (209 ms total)

===========================================================

Note: Google Test filter = HistoryBrowserTest.SavingHistoryDisabledThenEnabled
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from HistoryBrowserTest, where TypeParam =
[ RUN      ] HistoryBrowserTest.SavingHistoryDisabledThenEnabled
[2288:3288:0323/190844:3163544:ERROR:network_change_notifier_win.cc(111)]
WSALookupServiceBegin failed with: 1722
[2288:4920:0323/190844:3163622:ERROR:network_change_notifier_win.cc(111)]
WSALookupServiceBegin failed with: 1722
[2288:4920:0323/190844:3163622:ERROR:network_change_notifier_win.cc(111)]
WSALookupServiceBegin failed with: 1722
[2288:4920:0323/190844:3163637:ERROR:network_change_notifier_win.cc(111)]
WSALookupServiceBegin failed with: 1722
[2288:4920:0323/190844:3163637:ERROR:network_change_notifier_win.cc(111)]
WSALookupServiceBegin failed with: 1722
[2288:4920:0323/190844:3163887:FATAL:browser_process_impl.cc(255)]
Check failed: false.
[2288:4920:0323/190844:3163887:FATAL:browser_process_impl.cc(255)]
Check failed: false.




On Mon, Mar 26, 2012 at 9:07 AM, <commit-bot@chromium.org> wrote:

> CQ is trying da patch. Follow status at
>
https://chromium-status.**appspot.com/cq/mad@chromium.**org/9432033/60011<htt...
>
>
>
http://codereview.chromium.**org/9432033/<http://codereview.chromium.org/9432...
>

[commit-bot: I haz the power, 2012-03-26 14:40:23]

Change committed as 128910

[MAD, 2012-04-04 15:31:59]

Needed to make some changes to support component build...

Can you please take another look?

Thanks!

BYE
MAD

[MAD, 2012-04-04 16:48:43]

On 2012/04/04 15:31:59, MAD wrote:
> Needed to make some changes to support component build...
> 
> Can you please take another look?
> 
> Thanks!
> 
> BYE
> MAD

Wait... I just realized I can't move FieldTrialSynchronizer to common because it
has some content dependencies...

I guess I'll need to create something new in common...

Sorry about that...

[MAD, 2012-04-04 18:42:23]

Should be OK to look at again now...
Again, sorry about that... 

Thanks!

BYE
MAD

[robertshield, 2012-04-05 00:36:41]

LGTM, nits

https://chromiumcodereview.appspot.com/9432033/diff/82008/chrome/browser/metr...
File chrome/browser/metrics/field_trial_synchronizer.cc (right):

https://chromiumcodereview.appspot.com/9432033/diff/82008/chrome/browser/metr...
chrome/browser/metrics/field_trial_synchronizer.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
nit:2012

https://chromiumcodereview.appspot.com/9432033/diff/82008/chrome/chrome_tests...
File chrome/chrome_tests.gypi (right):

https://chromiumcodereview.appspot.com/9432033/diff/82008/chrome/chrome_tests...
chrome/chrome_tests.gypi:2613: # Enough pluses to make get this target prepended
to the target's
*make this target get

https://chromiumcodereview.appspot.com/9432033/diff/82008/chrome/common/child...
File chrome/common/child_process_logging_win.cc (right):

https://chromiumcodereview.appspot.com/9432033/diff/82008/chrome/common/child...
chrome/common/child_process_logging_win.cc:190: static MainSetExperimentList
init_experiment_list = NULL;
Note that this is not thread safe, which probably doesn't matter here (I see the
same pattern is followed throughout this file). Would you consider adding a "NOT
THREAD SAFE" warning to the header file?

[MAD, 2012-04-05 02:31:12]

Thanks... All addressed...

CQ all the way!

Fingers crossed...

BYE
MAD

https://chromiumcodereview.appspot.com/9432033/diff/82008/chrome/browser/metr...
File chrome/browser/metrics/field_trial_synchronizer.cc (right):

https://chromiumcodereview.appspot.com/9432033/diff/82008/chrome/browser/metr...
chrome/browser/metrics/field_trial_synchronizer.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/04/05 00:36:41, robertshield wrote:
> nit:2012

Done.

https://chromiumcodereview.appspot.com/9432033/diff/82008/chrome/chrome_tests...
File chrome/chrome_tests.gypi (right):

https://chromiumcodereview.appspot.com/9432033/diff/82008/chrome/chrome_tests...
chrome/chrome_tests.gypi:2613: # Enough pluses to make get this target prepended
to the target's
On 2012/04/05 00:36:41, robertshield wrote:
> *make this target get

Done.

https://chromiumcodereview.appspot.com/9432033/diff/82008/chrome/common/child...
File chrome/common/child_process_logging_win.cc (right):

https://chromiumcodereview.appspot.com/9432033/diff/82008/chrome/common/child...
chrome/common/child_process_logging_win.cc:190: static MainSetExperimentList
init_experiment_list = NULL;
On 2012/04/05 00:36:41, robertshield wrote:
> Note that this is not thread safe, which probably doesn't matter here (I see
the
> same pattern is followed throughout this file). Would you consider adding a
"NOT
> THREAD SAFE" warning to the header file?

Done.

[commit-bot: I haz the power, 2012-04-05 02:31:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/9432033/86001

[commit-bot: I haz the power, 2012-04-05 07:23:49]

Change committed as 130858

[Nico, 2012-04-07 16:53:20]

Hi mad,

when you add a new test binary, please mention that in the CL description next
time. This broke some private bot of mine (not your fault at all!), and I
skipped your CL when I was looking for culprits because the CL description
sounded so harmless / unrelated to the issue I was seeing.

[MAD, 2012-04-12 13:34:45]

Sorry about that...

I thought I needed to update the buildbot scripts to take this new target
into account... Are they updated automatically now?

On Sat, Apr 7, 2012 at 12:53 PM, <thakis@chromium.org> wrote:

> Hi mad,
>
> when you add a new test binary, please mention that in the CL description
> next
> time. This broke some private bot of mine (not your fault at all!), and I
> skipped your CL when I was looking for culprits because the CL description
> sounded so harmless / unrelated to the issue I was seeing.
>
>
http://codereview.chromium.**org/9432033/<http://codereview.chromium.org/9432...
>

[sadrul, 2015-08-04 18:03:09]

Hi! Looks like the new test binary added in this CL (chrome_app_unittests)
doesn't actually run on any of the bots? If that is the case, can this be
removed now?

[MAD, 2015-08-04 18:28:21]

Ho! I was supposed to add references to this test in the bot scripts and
never did... :-(

I would be OK to remove it, but would be happier if we would add a
references to this test instead... :-)

Le mar. 4 août 2015 à 14:03, <sadrul@chromium.org> a écrit :

> Hi! Looks like the new test binary added in this CL (chrome_app_unittests)
> doesn't actually run on any of the bots? If that is the case, can this be
> removed now?
>
> https://codereview.chromium.org/9432033/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2015-08-11 22:11:53]

On 2015/08/04 18:28:21, MAD wrote:
> Ho! I was supposed to add references to this test in the bot scripts and
> never did... :-(
> 
> I would be OK to remove it, but would be happier if we would add a
> references to this test instead... :-)

Would it be useful to run them with unit_tests, instead of a separate
test-suite? (looks like the tests still do pass when run with unit_tests in this
hacky cl: https://codereview.chromium.org/1281413002/)

[MAD, 2015-08-13 15:30:53]

Très cool! Thanks... That would make sense to me... I can't remember why it
wasn't done like that initially...


Le mar. 11 août 2015 à 18:11, <sadrul@chromium.org> a écrit :

> On 2015/08/04 18:28:21, MAD wrote:
> > Ho! I was supposed to add references to this test in the bot scripts and
> > never did... :-(
>
> > I would be OK to remove it, but would be happier if we would add a
> > references to this test instead... :-)
>
> Would it be useful to run them with unit_tests, instead of a separate
> test-suite? (looks like the tests still do pass when run with unit_tests in
> this
> hacky cl: https://codereview.chromium.org/1281413002/)
>
> https://codereview.chromium.org/9432033/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.