crashpad_database_util: Don’t create a database unless explicitly asked

crashpad_database_util: Don’t create a database unless explicitly asked

I’ve accidentally created Crashpad databases when running
crashpad_database_util by mistyping the argument to --database. Typical
users of crashpad_database_util probably don’t want the database to be
created.

This adds a new --create option to crashpad_database_util that is
required to get it to create a database. If not present, a database will
not be created if it does not already exist.

TEST=crashpad_client_test CrashReportDatabaseTest.*
R=rsesek@chromium.org, scottmg@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/553a6434759973788b1913be2457b94aace528a1

[Mark Mentovai, 2015-10-07 18:45:58]

mark@chromium.org changed reviewers:
+ rsesek@chromium.org, scottmg@chromium.org

[Mark Mentovai, 2015-10-07 18:45:58]

https://codereview.chromium.org/1395653002/diff/1/client/crash_report_database.h
File client/crash_report_database.h (right):

https://codereview.chromium.org/1395653002/diff/1/client/crash_report_databas...
client/crash_report_database.h:181: bool create);
This interface is the one thing I’m not sure about. Users of this interface
probably won’t normally mean to specify create=false.

I was thinking of breaking this into Initialize() and
InitializeWithoutCreating() instead. Any preference?

[scottmg, 2015-10-07 19:16:30]

lgtm

https://codereview.chromium.org/1395653002/diff/1/client/crash_report_database.h
File client/crash_report_database.h (right):

https://codereview.chromium.org/1395653002/diff/1/client/crash_report_databas...
client/crash_report_database.h:181: bool create);
On 2015/10/07 18:45:58, Mark Mentovai wrote:
> This interface is the one thing I’m not sure about. Users of this interface
> probably won’t normally mean to specify create=false.
> 
> I was thinking of breaking this into Initialize() and
> InitializeWithoutCreating() instead. Any preference?

I agree, I think a separate interface method for normal usage would be clearer. 

Alternatively, an enum class : bool so that it's more obvious at the callsite
that it might be a bad idea to say `false`.

https://codereview.chromium.org/1395653002/diff/1/handler/main.cc
File handler/main.cc (right):

https://codereview.chromium.org/1395653002/diff/1/handler/main.cc#newcode227
handler/main.cc:227: scoped_ptr<CrashReportDatabase>
database(CrashReportDatabase::Initialize(
Good stuff!

[Robert Sesek, 2015-10-07 19:26:38]

I agree that this should be split into a separate ::Create()

[Mark Mentovai, 2015-10-07 20:44:15]

Updated. I left the behavior of Initialize() alone (it can create a database,
which is what most users will probably want) and added a separate
InitializeWithoutCreating().

[scottmg, 2015-10-07 20:58:18]

lgtm

https://codereview.chromium.org/1395653002/diff/20001/client/crash_report_dat...
File client/crash_report_database_win.cc (right):

https://codereview.chromium.org/1395653002/diff/20001/client/crash_report_dat...
client/crash_report_database_win.cc:565: // Ensure the databse directory exists.
'database' if you didn't already land.

[Mark Mentovai, 2015-10-07 21:03:34]

https://codereview.chromium.org/1395653002/diff/20001/client/crash_report_dat...
File client/crash_report_database_win.cc (right):

https://codereview.chromium.org/1395653002/diff/20001/client/crash_report_dat...
client/crash_report_database_win.cc:565: // Ensure the databse directory exists.
scottmg wrote:
> 'database' if you didn't already land.

Fixed.

Seriously, I think Atom eats keystrokes. Must be because it’s Chromium-based. :(

[Robert Sesek, 2015-10-07 21:04:33]

LGTM

https://codereview.chromium.org/1395653002/diff/20001/client/crash_report_dat...
File client/crash_report_database_win.cc (right):

https://codereview.chromium.org/1395653002/diff/20001/client/crash_report_dat...
client/crash_report_database_win.cc:773: ? database_win.Pass() :
scoped_ptr<CrashReportDatabaseWin>();
nit: shouldn't ? go on the previous line?

[Mark Mentovai, 2015-10-07 21:07:10]

https://codereview.chromium.org/1395653002/diff/20001/client/crash_report_dat...
File client/crash_report_database_win.cc (right):

https://codereview.chromium.org/1395653002/diff/20001/client/crash_report_dat...
client/crash_report_database_win.cc:773: ? database_win.Pass() :
scoped_ptr<CrashReportDatabaseWin>();
Robert Sesek wrote:
> nit: shouldn't ? go on the previous line?

Updated to what clang-format likes here, which is actually a third option.

[Mark Mentovai, 2015-10-08 17:10:06]

Committed patchset #5 (id:80001) manually as
553a6434759973788b1913be2457b94aace528a1 (presubmit successful).