[webkit-reviews] review granted: [Bug 237130] SQLiteDatabase::open should return early if journal mode cannot be set : [Attachment 453774] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 237130: SQLiteDatabase::open should return early if journal mode cannot be
set
https://bugs.webkit.org/show_bug.cgi?id=237130

Attachment 453774: Patch

https://bugs.webkit.org/attachment.cgi?id=453774&action=review




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 453774
  --> https://bugs.webkit.org/attachment.cgi?id=453774
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453774&action=review

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:132
> +	   int result = SQLITE_OK;

Just because there are braces below doesn’t mean we need to initialize this
here. I suppose it’s OK.

> Source/WebCore/platform/sql/SQLiteDatabase.h:102
> +    int checkpoint(CheckpointMode);

Why add this return value? No caller is checking it. Is it for future use?

> Source/WebCore/platform/sql/SQLiteDatabase.h:172
> +    int useWALJournalMode();

Seems like returning an error code as an int with no comment or type to
disambiguate and say what the return value means is not a great pattern.

In this particular case, the caller just needs to know if this succeeded or
failed, so a bool for success would work and I think might be better than
returning the error code.

I suppose it’s not a *disaster* to return an SQLite error code as an int, but
how would I know that’s what this function returns? Elsewhere we use named
types whenever possible for error codes, or enums or special-purpose objects. I
think booleans for success work too.