Expose the ability to supply additional user streams in the windows dump generator.

Expose the ability to supply additional user streams in the windows dump
generator.

BUG=N/A
R=mark@chromium.org

Committed: https://code.google.com/p/google-breakpad/source/detail?r=1258

[Cris Neckar]

cpu@ weaseled his way out of this by apparently not being a breakpad committer
any more.

Mark could you please take a look?

[Mark Mentovai]

I would be happy to make him a committer if that’s what you’re asking. :)

[Mark Mentovai]

LGTM, but the number of overloads here really sucks. I think it’s time we
consider deprecating the old interfaces and let everyone adapt.

[Cris Neckar]

On 2013/12/12 02:16:38, Mark Mentovai wrote:
> LGTM, but the number of overloads here really sucks. I think it’s time we
> consider deprecating the old interfaces and let everyone adapt.

It is a bit silly. I'd be happy to take a stab at that in another CL.

Do you have any specific recommendations on how it might ideally look?

[Mark Mentovai]

We could just get rid of all but the most parametery version of the function,
and force everyone to adapt. We could even maintain this heavy-handed approach
over time. We do that with a bunch of other APIs: when you update to a new
version of the library, if your existing code has become source-incompatible,
you need to update it. We’re generally fine with that approach.

Or we could replace most or all of the parameters with an options struct, and
make the API “zero out this struct and set the fields you care about.” Or we can
make it an options class, and have the API be “call these methods to configure
the behaviors you care about.” Either way, we’d just make WriteMinidump accept a
const ref argument to the struct or class options object.

This is all good as long as we don’t care about binary compatibility. If we do
care about binary compatibility, we can do the same as the above, but we
couldn’t get rid of the old interfaces. We’d have to keep them around and mark
them deprecated.

[Ted Mielczarek]

We haven't been very good about API compat in the past, so I think we should
just declare bankruptcy here and clean this up. We're not shipping any binary
releases of anything, so I can't imagine ABI compat is a thing anyone believes
we provide.

[Cris Neckar]

Cool, I'll take a stab at redesigning this. My inclination is for the
configuration the hang off of the class with required options set in the
constructor and optional things set through calls to public setters.

Then WriteDump() will just be a parameterless method that can be called once
everything is set up.

If that works for you guys I'll land this and start implementing it.

[Mark Mentovai]

Merge the options struct and the class itself. That’s fine, since this is a
one-trick class to begin with. I’m hip to that jive.

[Cris Neckar]

Committed patchset #1 manually as r1258 (presubmit successful).

[cyberparser]

access violation in minidump_generator.cc

https://breakpad.appspot.com/894002/diff/20001/client/windows/crash_generatio...
File client/windows/crash_generation/minidump_generator.cc (right):

https://breakpad.appspot.com/894002/diff/20001/client/windows/crash_generatio...
client/windows/crash_generation/minidump_generator.cc:443: 0]);
I've just got AV (reading at zero) there.

Probably you forgot to use brackets because operator+ will be evaluated before
?:

[Lei Zhang (chromium)]

On 2013/12/13 18:57:26, cyberparser wrote:
> access violation in minidump_generator.cc
> 
>
https://breakpad.appspot.com/894002/diff/20001/client/windows/crash_generatio...
> File client/windows/crash_generation/minidump_generator.cc (right):
> 
>
https://breakpad.appspot.com/894002/diff/20001/client/windows/crash_generatio...
> client/windows/crash_generation/minidump_generator.cc:443: 0]);
> I've just got AV (reading at zero) there.
> 
> Probably you forgot to use brackets because operator+ will be evaluated before
> ?:

https://breakpad.appspot.com/904004/