[webkit-reviews] review denied: [Bug 185873] [Curl] Not all Cookie database files are deleted on corruption : [Attachment 341007] Patch

[bugzilla-daemon at webkit.org]

Fujii Hironori <Hironori.Fujii at sony.com> has denied Christopher Reid
<chris.reid at sony.com>'s request for review:
Bug 185873: [Curl] Not all Cookie database files are deleted on corruption
https://bugs.webkit.org/show_bug.cgi?id=185873

Attachment 341007: Patch

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




--- Comment #4 from Fujii Hironori <Hironori.Fujii at sony.com> ---
Comment on attachment 341007
  --> https://bugs.webkit.org/attachment.cgi?id=341007
Patch

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

> Source/WebCore/platform/network/curl/CookieJarDB.cpp:222
> +	   return true;

Should this be a following code?
if (resultCode != SQLITE_OK)
    return false;

> Source/WebCore/platform/network/curl/CookieJarDB.cpp:249
> +    FileSystem::deleteFile(m_databasePath + "-shm");

Why do you need to do 'deleteFile(m_databasePath + "-shm")' twice?

> Source/WebCore/platform/network/curl/CookieJarDB.cpp:510
> +

You use checkSQLiteReturnCode in ASSERT macro even though it has a side effect.
The side effect is effective only in Debug build.
Is this intentional?

checkSQLiteReturnCode should be defined as:

void CookieJarDB::checkSQLiteReturnCode(int actual)
{
    if (!m_detectedDatabaseCorruption) {
	switch (actual) {
	case SQLITE_CORRUPT:
	case SQLITE_SCHEMA:
	case SQLITE_FORMAT:
	case SQLITE_NOTADB:
	    flagDatabaseCorruption();
	    m_detectedDatabaseCorruption = true;
	}
    }
}

This code should be replaced with:

checkSQLiteReturnCode(ret);
if (ret != SQLITE_OK && ret != SQLITE_DONE && ret != SQLITE_ROW &&
!ignoreError)
    LOG_ERROR("Failed to execute %s error: %s", sql,
m_database.lastErrorMsg());

Other ASSERT(checkSQLiteReturnCode(ret, SQLITE_DONE)) should be replaced with a
following code:

checkSQLiteReturnCode(ret);
ASSERT(ret == SQLITE_DONE);